Skip to content

Commit

Permalink
fix(Android,Fabric,bridge-mode): patch crash with context detached fr…
Browse files Browse the repository at this point in the history
…om activity (software-mansion#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
  • Loading branch information
kkafar authored and ja1ns committed Oct 9, 2024
1 parent 191c585 commit 1967093
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 17 deletions.
2 changes: 1 addition & 1 deletion FabricExample/android/settings.gradle
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
pluginManagement { includeBuild("../node_modules/@react-native/gradle-plugin") }
plugins { id("com.facebook.react.settings") }
extensions.configure(com.facebook.react.ReactSettingsExtension){ ex -> ex.autolinkLibrariesFromCommand() }
extensions.configure(com.facebook.react.ReactSettingsExtension) { ex -> ex.autolinkLibrariesFromCommand() }
rootProject.name = 'FabricExample'
include ':app'
includeBuild('../node_modules/@react-native/gradle-plugin')
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import android.util.Log
import android.view.View
import androidx.appcompat.widget.Toolbar
import androidx.coordinatorlayout.widget.CoordinatorLayout
import com.facebook.react.bridge.LifecycleEventListener
import com.facebook.react.bridge.ReactApplicationContext
import com.facebook.react.uimanager.PixelUtil
import com.google.android.material.appbar.AppBarLayout
Expand All @@ -19,7 +20,7 @@ import java.lang.ref.WeakReference
*/
internal class ScreenDummyLayoutHelper(
reactContext: ReactApplicationContext,
) {
) : LifecycleEventListener {
// The state required to compute header dimensions. We want this on instance rather than on class
// for context access & being tied to instance lifetime.
private lateinit var coordinatorLayout: CoordinatorLayout
Expand All @@ -39,7 +40,6 @@ internal class ScreenDummyLayoutHelper(
WeakReference(reactContext)

init {

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

weakInstance = WeakReference(this)
ensureDummyLayoutWithHeader(reactContext)

if (!(reactContext.hasCurrentActivity() && maybeInitDummyLayoutWithHeader(reactContext))) {
reactContext.addLifecycleEventListener(this)
}
}

/**
* Initializes dummy view hierarchy with CoordinatorLayout, AppBarLayout and dummy View.
* We utilize this to compute header height (app bar layout height) from C++ layer when its needed.
*
* @return boolean whether the layout was initialised or not
*/
private fun ensureDummyLayoutWithHeader(reactContext: ReactApplicationContext) {
if (::coordinatorLayout.isInitialized) {
return
private fun maybeInitDummyLayoutWithHeader(reactContext: ReactApplicationContext): Boolean {
if (isLayoutInitialized) {
return true
}

if (!reactContext.hasCurrentActivity()) {
return false
}

// We need to use activity here, as react context does not have theme attributes required by
Expand Down Expand Up @@ -108,6 +117,9 @@ internal class ScreenDummyLayoutHelper(
addView(appBarLayout)
addView(dummyContentView)
}

isLayoutInitialized = true
return true
}

/**
Expand All @@ -121,12 +133,18 @@ internal class ScreenDummyLayoutHelper(
fontSize: Int,
isTitleEmpty: Boolean,
): Float {
if (!::coordinatorLayout.isInitialized) {
Log.e(
TAG,
"[RNScreens] Attempt to access dummy view hierarchy before it is initialized",
)
return 0.0f
if (!isLayoutInitialized) {
val reactContext =
requireReactContext { "[RNScreens] Context was null-ed before dummy layout was initialized" }
if (!maybeInitDummyLayoutWithHeader(reactContext)) {
// This theoretically might happen at Fabric + "bridgefull" combination, due to race condition where `reactContext.currentActivity`
// is still null at this execution point. We don't wanna crash in such case, thus returning zeroed height.
Log.e(
TAG,
"[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"
)
return 0.0f
}
}

if (cache.hasKey(CacheKey(fontSize, isTitleEmpty))) {
Expand Down Expand Up @@ -168,10 +186,10 @@ internal class ScreenDummyLayoutHelper(
return headerHeight
}

private fun requireReactContext(): ReactApplicationContext =
requireNotNull(reactContextRef.get()) {
"[RNScreens] Attempt to require missing react context"
}
private fun requireReactContext(lazyMessage: (() -> Any)? = null): ReactApplicationContext =
requireNotNull(
reactContextRef.get(),
lazyMessage ?: { "[RNScreens] Attempt to require missing react context" })

private fun requireActivity(): Activity =
requireNotNull(requireReactContext().currentActivity) {
Expand All @@ -195,6 +213,19 @@ internal class ScreenDummyLayoutHelper(
@JvmStatic
fun getInstance(): ScreenDummyLayoutHelper? = weakInstance.get()
}

private var isLayoutInitialized = false

override fun onHostResume() {
// This is the earliest we have guarantee that the context has a reference to an activity.
val reactContext = requireReactContext { "[RNScreens] ReactContext missing in onHostResume! This should not happen." }
check(maybeInitDummyLayoutWithHeader(reactContext)) { "[RNScreens] Failed to initialise dummy layout in onHostResume. This is not expected."}
reactContext.removeLifecycleEventListener(this)
}

override fun onHostPause() = Unit

override fun onHostDestroy() = Unit
}

private data class CacheKey(
Expand Down

0 comments on commit 1967093

Please sign in to comment.