Skip to content

Commit 1713e8f

Browse files
authored
fix(Android,Fabric,bridge-mode): patch crash with context detached from activity (#2276)
## Description When running on Fabric + "bridgefull" combination, react packages are initialised very early in application lifetime (much earlier than in Fabric + bridgeless), when there is no activity injected to `ReactContext` yet. Thus when creating packages they do not have access to the activity and we relied on this access up to this moment to setup `ScreenDummyLayoutHelper` object, throwing exception if activity was not available. ## Changes This diff delays initialisation of the dummy layout in case there is no access to activity when creating the object up to the point of invocation of `onHostResume` callbacks of `ReactContext` (this is the very moment activity is injected into the context), avoiding the crash. There is also fallback mechanism added in `computeDummyLayout` method, so that when accessed from C++ layer it has another chance of initialising the dummy layout, before performing computations. If it fails (see below for reasons why it might fail) `0f` is returned as header height causing content jump, but not crashing the application. > [!important] **Most likely** there is a race condition in this code. `onHostResume` is called from UI thread at the execution point, when JS thread & first native render & thus commit & layout is already in progress on background thread. In case JS thread proceeds to layout & `computeDummyLayout` is called from our Screen component descriptor before `onHostResume` is called on UI thread & the dummy layout is initialised, we hit the case when computing header height will not be possible due to uninitialised dummy layout. I failed to trigger this behaviour even once for ~30 min of testing with trying to put UI thread to sleep etc., however I've also failed to find a proof that it won't happen because of some synchronisation / execution order. What's also important is that there is no good way to synchronise these threads, because it is not the matter of exclusive access to some critical section, but rather a order of access between UI & background thread. Some barrier mechanism would be required here, however we do not have thread references accessible from our code. > [!note] One possible solution would be to synchronise access to `maybeInitDummyLayoutWithHeader` method & in case the background thread hits it before UI, we could wait for UI in some kind of loop ("slow spinlock"). This could guarantee correctness, however it is crazy bad, because we would impede whole render process for possibly long time. Despite the flaws of this approach this might be something to consider for the future. > [!caution] One more thing to note is that I rely on [JVM atomicity guarantees](https://docs.oracle.com/javase/tutorial/essential/concurrency/atomic.html) when reading / writing `isDummyLayoutInitialized` variable. There is danger that both threads hit the layout initialisation code at the same time and thus leading to corruption. TODO: try to handle this case more gracefully. A lock for initialisation code should be enough. ## Test code and steps to reproduce Run `FabricExample` with `load(bridgelessEnabled = false)` set in `MainApplication` and see that it now works. ## Checklist - [x] Ensured that CI passes
1 parent 032ec01 commit 1713e8f

File tree

2 files changed

+48
-17
lines changed

2 files changed

+48
-17
lines changed

FabricExample/android/settings.gradle

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
pluginManagement { includeBuild("../node_modules/@react-native/gradle-plugin") }
22
plugins { id("com.facebook.react.settings") }
3-
extensions.configure(com.facebook.react.ReactSettingsExtension){ ex -> ex.autolinkLibrariesFromCommand() }
3+
extensions.configure(com.facebook.react.ReactSettingsExtension) { ex -> ex.autolinkLibrariesFromCommand() }
44
rootProject.name = 'FabricExample'
55
include ':app'
66
includeBuild('../node_modules/@react-native/gradle-plugin')

android/src/main/java/com/swmansion/rnscreens/utils/ScreenDummyLayoutHelper.kt

Lines changed: 47 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import android.util.Log
55
import android.view.View
66
import androidx.appcompat.widget.Toolbar
77
import androidx.coordinatorlayout.widget.CoordinatorLayout
8+
import com.facebook.react.bridge.LifecycleEventListener
89
import com.facebook.react.bridge.ReactApplicationContext
910
import com.facebook.react.uimanager.PixelUtil
1011
import com.google.android.material.appbar.AppBarLayout
@@ -19,7 +20,7 @@ import java.lang.ref.WeakReference
1920
*/
2021
internal class ScreenDummyLayoutHelper(
2122
reactContext: ReactApplicationContext,
22-
) {
23+
) : LifecycleEventListener {
2324
// The state required to compute header dimensions. We want this on instance rather than on class
2425
// for context access & being tied to instance lifetime.
2526
private lateinit var coordinatorLayout: CoordinatorLayout
@@ -39,7 +40,6 @@ internal class ScreenDummyLayoutHelper(
3940
WeakReference(reactContext)
4041

4142
init {
42-
4343
// We load the library so that we are able to communicate with our C++ code (descriptor & shadow nodes).
4444
// Basically we leak this object to C++, as its lifecycle should span throughout whole application
4545
// lifecycle anyway.
@@ -50,16 +50,25 @@ internal class ScreenDummyLayoutHelper(
5050
}
5151

5252
weakInstance = WeakReference(this)
53-
ensureDummyLayoutWithHeader(reactContext)
53+
54+
if (!(reactContext.hasCurrentActivity() && maybeInitDummyLayoutWithHeader(reactContext))) {
55+
reactContext.addLifecycleEventListener(this)
56+
}
5457
}
5558

5659
/**
5760
* Initializes dummy view hierarchy with CoordinatorLayout, AppBarLayout and dummy View.
5861
* We utilize this to compute header height (app bar layout height) from C++ layer when its needed.
62+
*
63+
* @return boolean whether the layout was initialised or not
5964
*/
60-
private fun ensureDummyLayoutWithHeader(reactContext: ReactApplicationContext) {
61-
if (::coordinatorLayout.isInitialized) {
62-
return
65+
private fun maybeInitDummyLayoutWithHeader(reactContext: ReactApplicationContext): Boolean {
66+
if (isLayoutInitialized) {
67+
return true
68+
}
69+
70+
if (!reactContext.hasCurrentActivity()) {
71+
return false
6372
}
6473

6574
// We need to use activity here, as react context does not have theme attributes required by
@@ -108,6 +117,9 @@ internal class ScreenDummyLayoutHelper(
108117
addView(appBarLayout)
109118
addView(dummyContentView)
110119
}
120+
121+
isLayoutInitialized = true
122+
return true
111123
}
112124

113125
/**
@@ -121,12 +133,18 @@ internal class ScreenDummyLayoutHelper(
121133
fontSize: Int,
122134
isTitleEmpty: Boolean,
123135
): Float {
124-
if (!::coordinatorLayout.isInitialized) {
125-
Log.e(
126-
TAG,
127-
"[RNScreens] Attempt to access dummy view hierarchy before it is initialized",
128-
)
129-
return 0.0f
136+
if (!isLayoutInitialized) {
137+
val reactContext =
138+
requireReactContext { "[RNScreens] Context was null-ed before dummy layout was initialized" }
139+
if (!maybeInitDummyLayoutWithHeader(reactContext)) {
140+
// This theoretically might happen at Fabric + "bridgefull" combination, due to race condition where `reactContext.currentActivity`
141+
// is still null at this execution point. We don't wanna crash in such case, thus returning zeroed height.
142+
Log.e(
143+
TAG,
144+
"[RNScreens] Failed to late-init layout while computing header height. This is a race-condition-bug in react-native-screens, please file an issue at https://github.com/software-mansion/react-native-screens/issues"
145+
)
146+
return 0.0f
147+
}
130148
}
131149

132150
if (cache.hasKey(CacheKey(fontSize, isTitleEmpty))) {
@@ -168,10 +186,10 @@ internal class ScreenDummyLayoutHelper(
168186
return headerHeight
169187
}
170188

171-
private fun requireReactContext(): ReactApplicationContext =
172-
requireNotNull(reactContextRef.get()) {
173-
"[RNScreens] Attempt to require missing react context"
174-
}
189+
private fun requireReactContext(lazyMessage: (() -> Any)? = null): ReactApplicationContext =
190+
requireNotNull(
191+
reactContextRef.get(),
192+
lazyMessage ?: { "[RNScreens] Attempt to require missing react context" })
175193

176194
private fun requireActivity(): Activity =
177195
requireNotNull(requireReactContext().currentActivity) {
@@ -195,6 +213,19 @@ internal class ScreenDummyLayoutHelper(
195213
@JvmStatic
196214
fun getInstance(): ScreenDummyLayoutHelper? = weakInstance.get()
197215
}
216+
217+
private var isLayoutInitialized = false
218+
219+
override fun onHostResume() {
220+
// This is the earliest we have guarantee that the context has a reference to an activity.
221+
val reactContext = requireReactContext { "[RNScreens] ReactContext missing in onHostResume! This should not happen." }
222+
check(maybeInitDummyLayoutWithHeader(reactContext)) { "[RNScreens] Failed to initialise dummy layout in onHostResume. This is not expected."}
223+
reactContext.removeLifecycleEventListener(this)
224+
}
225+
226+
override fun onHostPause() = Unit
227+
228+
override fun onHostDestroy() = Unit
198229
}
199230

200231
private data class CacheKey(

0 commit comments

Comments
 (0)