From 54074030dc70ca6fd3527fa57c239cabd9fed045 Mon Sep 17 00:00:00 2001 From: Bartlomiej Bloniarz Date: Wed, 26 Aug 2026 07:43:46 -0700 Subject: [PATCH] Defer the pull model's synchronous mount batch until the root view is attached Summary: Two crashes can occur when a synchronous mount batch runs before its root view is attached. Both need the same bad state: a `ViewState` for a tag is present in `tagToViewState`, but its `view` field is null. ## Cause A surface can render before its root view is attached. While the root view is not attached, `MountItemDispatcher.executeOrEnqueue` defers every mount item into `SurfaceMountingManager.onViewAttachMountItems`. The pull model does not defer one of them: `FabricUIManager.scheduleMountItem(synchronous = true)` calls `mountItem.execute()` directly. That gives this sequence for a tag `T`: 1. **C++ claims the tag first.** `preallocateShadowView` puts `T` into `allocatedViewRegistry_`. It does this before it calls Java. 2. **Java does not create the view.** The `PreAllocateViewMountItem` for `T` is deferred, because `isWaitingForViewAttach` is true. No `ViewState` exists for `T`. 3. **C++ omits the Create instruction.** `executeMount` finds `T` in `allocatedViewTags`, so it does not add a Create for `T`. 4. **The mount batch runs too early.** The batch is not deferred, so it runs while the root view is still not attached. It has no Create for `T`, but it has an `UpdateEventEmitter`. `updateEventEmitter` calls `tagToViewState.getOrPut(T) { ViewState(T) }`, which makes a `ViewState` with a null `view`. 5. **The preallocation is cancelled.** The root view attaches and the deferred `PreAllocateViewMountItem` runs. `preallocateView` finds a `ViewState` for `T` and returns. `T` now has no view, and no Create will come. The next `updateState` or `updateOverflowInset` for `T` throws. ## Fix Apply the same attach barrier to the synchronous batch that every other mount item already obeys. If the root view is not attached, put the batch in the dispatcher queue instead of running it inline. The preallocations then run first, and the batch runs after the root view is attached. Only `pullAndExecuteTransaction` passes `synchronous = true`, so the push model never reaches this path. Differential Revision: D117519782 --- .../react/fabric/FabricUIManager.java | 12 +++- .../fabric/FabricUIManagerPullModelTest.kt | 67 +++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java index 7479f9cb53e8..1ba1a11fabd2 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/FabricUIManager.java @@ -971,9 +971,15 @@ private void scheduleMountItem( if (shouldSchedule) { Assertions.assertNotNull(mountItem, "MountItem is null"); if (synchronous) { - // Pull model: we are already on the UI thread, inside the dispatcher's loop executing - // a PullTransactionMountItem. We don't schedule the item, we execute it directly. - mountItem.execute(mMountingManager); + if (mMountingManager.isWaitingForViewAttach(mountItem.getSurfaceId())) { + // Regular mount items are still being deferred into the surface's attach queue. + // Executing this batch inline would run it ahead of the preallocations queued there. + mMountItemDispatcher.addMountItem(mountItem); + } else { + // Pull model: we are already on the UI thread, inside the dispatcher's loop executing + // a PullTransactionMountItem. We don't schedule the item, we execute it directly. + mountItem.execute(mMountingManager); + } } else { mMountItemDispatcher.addMountItem(mountItem); if (UiThreadUtil.isOnUiThread()) { diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricUIManagerPullModelTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricUIManagerPullModelTest.kt index cd1e7be80176..f3bea15d3c84 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricUIManagerPullModelTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricUIManagerPullModelTest.kt @@ -9,9 +9,15 @@ package com.facebook.react.fabric +import com.facebook.react.ReactRootView import com.facebook.react.bridge.ReactApplicationContext import com.facebook.react.bridge.ReactTestHelper +import com.facebook.react.fabric.mounting.MountingManager +import com.facebook.react.fabric.mounting.mountitems.MountItem +import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags +import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsDefaults import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests +import com.facebook.react.uimanager.ThemedReactContext import com.facebook.react.uimanager.ViewManagerRegistry import com.facebook.testutils.fakes.FakeBatchEventDispatchedListener import com.facebook.testutils.shadows.ShadowFabricUIManagerBinding @@ -19,12 +25,14 @@ import com.facebook.testutils.shadows.ShadowNativeLoader import com.facebook.testutils.shadows.ShadowPerformanceTracer import com.facebook.testutils.shadows.ShadowSoLoader import org.assertj.core.api.Assertions.assertThat +import org.junit.After import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.robolectric.RobolectricTestRunner import org.robolectric.annotation.Config import org.robolectric.shadow.api.Shadow +import org.robolectric.util.ReflectionHelpers /** * Tests for the pull-model notification path: [FabricUIManager.onTransactionAvailable] enqueues a @@ -55,6 +63,11 @@ class FabricUIManagerPullModelTest { @Before fun setup() { ReactNativeFeatureFlagsForTests.setUp() + ReactNativeFeatureFlags.override( + object : ReactNativeFeatureFlagsDefaults() { + override fun enableMountingCoordinatorPullModelAndroid(): Boolean = true + }, + ) reactContext = ReactTestHelper.createCatalystContextForTest() underTest = FabricUIManager( @@ -67,6 +80,11 @@ class FabricUIManagerPullModelTest { underTest.setBinding(binding) } + @After + fun tearDown() { + ReactNativeFeatureFlags.dangerouslyReset() + } + private fun runOnBackgroundThread(block: () -> Unit) { var error: Throwable? = null val thread = Thread { @@ -81,6 +99,28 @@ class FabricUIManagerPullModelTest { error?.let { throw it } } + private fun scheduleSynchronously(mountItem: MountItem) { + val method = + FabricUIManager::class + .java + .getDeclaredMethod( + "scheduleMountItem", + MountItem::class.java, + Integer.TYPE, + java.lang.Long.TYPE, + java.lang.Long.TYPE, + java.lang.Long.TYPE, + java.lang.Long.TYPE, + java.lang.Long.TYPE, + java.lang.Long.TYPE, + java.lang.Long.TYPE, + Integer.TYPE, + java.lang.Boolean.TYPE, + ) + method.isAccessible = true + method.invoke(underTest, mountItem, 0, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0, true) + } + @Test fun onTransactionAvailable_onUiThread_pullsSynchronously() { underTest.onTransactionAvailable(1) @@ -105,4 +145,31 @@ class FabricUIManagerPullModelTest { assertThat(shadowBinding.pulledSurfaceIds).containsExactly(1, 2, 1, 3) } + + @Test + fun synchronousBatch_waitingForRootAttach_isDeferred() { + val surfaceId = 12 + val themedReactContext = ThemedReactContext(reactContext, reactContext, "TestModule", surfaceId) + val mountingManager = ReflectionHelpers.getField(underTest, "mMountingManager") + mountingManager.startSurface(surfaceId, themedReactContext, null) + + var executionCount = 0 + val mountItem = + object : MountItem { + override fun execute(mountingManager: MountingManager) { + executionCount++ + } + + override fun getSurfaceId(): Int = surfaceId + } + + scheduleSynchronously(mountItem) + + assertThat(executionCount).isZero() + + mountingManager.attachRootView(surfaceId, ReactRootView(reactContext), themedReactContext) + underTest.onTransactionAvailable(surfaceId) + + assertThat(executionCount).isEqualTo(1) + } }