From 4bc41206ff39c70412a480021fa91d5368a7b9d5 Mon Sep 17 00:00:00 2001 From: blonibartlomiej Date: Wed, 14 Oct 2026 05:18:32 -0700 Subject: [PATCH 1/2] Generated from a GitHub Pull Request. Run 'jf sync' on this diff to load the correct commit data. Differential Revision: D124966304 --- .../react/fabric/FabricUIManager.java | 59 +-- .../fabric/mounting/MountItemDispatcher.kt | 78 +++- .../mountitems/PullTransactionMountItem.kt | 33 -- .../featureflags/ReactNativeFeatureFlags.kt | 4 +- .../jni/react/fabric/FabricMountingManager.h | 2 +- .../react/fabric/FabricUIManagerBinding.cpp | 4 +- .../fabric/FabricUIManagerPullModelTest.kt | 356 ++++++++++++++++-- .../shadows/ShadowFabricUIManagerBinding.kt | 11 + .../featureflags/ReactNativeFeatureFlags.h | 4 +- .../ReactNativePublicFeatureFlags.h | 4 +- .../ReactNativeFeatureFlags.config.js | 2 +- .../featureflags/ReactNativeFeatureFlags.js | 4 +- 12 files changed, 445 insertions(+), 116 deletions(-) delete mode 100644 packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PullTransactionMountItem.kt 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 97de9935b8cf..1dd1c9460bc9 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 @@ -67,7 +67,6 @@ import com.facebook.react.fabric.mounting.mountitems.MountItem; import com.facebook.react.fabric.mounting.mountitems.MountItemFactory; import com.facebook.react.fabric.mounting.mountitems.PrefetchResourcesMountItem; -import com.facebook.react.fabric.mounting.mountitems.PullTransactionMountItem; import com.facebook.react.fabric.mounting.mountitems.SynchronousMountItem; import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags; import com.facebook.react.internal.featureflags.ReactNativeNewArchitectureFeatureFlags; @@ -237,6 +236,11 @@ public void executeItems(Queue items) { // This executor can be technically accessed before the dispatcher is created, // but if that happens, something is terribly wrong mMountItemDispatcher.dispatchMountItems(items); + // Only called when a surface's root view attaches, before the surface counts as attached, + // so the dispatch above skips its pull. Pull model: the next frame pulls it. + if (mMountItemDispatcher.hasSurfaceIdsToPull()) { + mDispatchUIFrameCallback.schedule(); + } } }; @@ -1015,15 +1019,9 @@ private void scheduleMountItem( if (shouldSchedule) { Assertions.assertNotNull(mountItem, "MountItem is null"); if (synchronous) { - 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); - } + // Pull model: we are already on the UI thread, inside the dispatcher's pull of a surface + // whose root view is attached. We don't schedule the item, we execute it directly. + mountItem.execute(mMountingManager); } else { mMountItemDispatcher.addMountItem(mountItem); if (UiThreadUtil.isOnUiThread()) { @@ -1073,19 +1071,18 @@ public void runGuarded() { /** * Pull model: called from C++ via JNI (usually on the commit thread) to signal that a transaction - * is available for {@code surfaceId}. Enqueues a PullTransactionMountItem so the UI thread pulls - * and applies the transaction itself, preserving mount-item ordering. + * is available for {@code surfaceId}. The next dispatch on the UI thread pulls and applies it + * before the queued mount items. */ @SuppressWarnings("unused") @AnyThread @ThreadConfined(ANY) @VisibleForTesting void onTransactionAvailable(int surfaceId) { - FabricUIManagerBinding binding = mBinding; - if (binding == null) { + if (mBinding == null) { return; } - mMountItemDispatcher.addMountItem(new PullTransactionMountItem(surfaceId, binding)); + mMountItemDispatcher.addSurfaceIdToPull(surfaceId); if (UiThreadUtil.isOnUiThread()) { mMountItemDispatcher.tryDispatchMountItems(); } @@ -1550,16 +1547,14 @@ public void didMountItems(@Nullable List mountItems) { listener.didMountItems(FabricUIManager.this); } - if (mountItems == null || mountItems.isEmpty()) { - return; - } - - // Collect surface IDs for all the mount items - for (MountItem mountItem : mountItems) { - if (mountItem != null - && mountItem.getSurfaceId() != View.NO_ID - && !mSurfaceIdsWithPendingMountNotification.contains(mountItem.getSurfaceId())) { - mSurfaceIdsWithPendingMountNotification.add(mountItem.getSurfaceId()); + if (mountItems != null) { + // Collect surface IDs for all the mount items + for (MountItem mountItem : mountItems) { + if (mountItem != null + && mountItem.getSurfaceId() != View.NO_ID + && !mSurfaceIdsWithPendingMountNotification.contains(mountItem.getSurfaceId())) { + mSurfaceIdsWithPendingMountNotification.add(mountItem.getSurfaceId()); + } } } @@ -1590,6 +1585,20 @@ public void didMountItems(@Nullable List mountItems) { } } + @UiThread + @ThreadConfined(UI) + @Override + public void pullTransaction(int surfaceId) { + FabricUIManagerBinding binding = mBinding; + if (binding == null) { + return; + } + binding.pullAndExecuteTransaction(surfaceId); + if (!mSurfaceIdsWithPendingMountNotification.contains(surfaceId)) { + mSurfaceIdsWithPendingMountNotification.add(surfaceId); + } + } + @Override public void didDispatchMountItems() { for (UIManagerListener listener : mListeners) { diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountItemDispatcher.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountItemDispatcher.kt index 72efd7c42058..f74bb987636f 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountItemDispatcher.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/MountItemDispatcher.kt @@ -24,6 +24,7 @@ import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags import com.facebook.react.internal.tracing.PerformanceTracer import com.facebook.systrace.Systrace import java.util.Queue +import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.ConcurrentLinkedQueue internal class MountItemDispatcher( @@ -33,10 +34,20 @@ internal class MountItemDispatcher( private val viewCommandMountItems: Queue = ConcurrentLinkedQueue() private val mountItems: Queue = ConcurrentLinkedQueue() private val preMountItems: Queue = ConcurrentLinkedQueue() + private val surfaceIdsToPull: MutableSet = ConcurrentHashMap.newKeySet() - /** @return true if any mount items, pre-mount items or view commands are still pending */ + /** + * @return true if any mount items, pre-mount items, view commands or pulls of surfaces with an + * attached root view are still pending. The attach schedules the frame that pulls its surface. + */ fun hasPendingItems(): Boolean = - !viewCommandMountItems.isEmpty() || !mountItems.isEmpty() || !preMountItems.isEmpty() + !viewCommandMountItems.isEmpty() || + !mountItems.isEmpty() || + !preMountItems.isEmpty() || + surfaceIdsToPull.any { !mountingManager.isWaitingForViewAttach(it) } + + /** Pull model: @return true if any surface has a transaction waiting to be pulled. */ + fun hasSurfaceIdsToPull(): Boolean = surfaceIdsToPull.isNotEmpty() // Items can be queued from any thread while the DISPATCH_UI frame callback is disarmed at // idle (see FabricUIManager's doFrameGuarded); the listener re-arms it. @@ -69,6 +80,12 @@ internal class MountItemDispatcher( notifyItemsQueued() } + /** Pull model: a pull mounts the surface's latest revision, so it is pending at most once. */ + fun addSurfaceIdToPull(surfaceId: Int) { + surfaceIdsToPull.add(surfaceId) + notifyItemsQueued() + } + fun addPreAllocateMountItem(mountItem: MountItem) { // We do this check only for PreAllocateViewMountItem - and not DispatchMountItem or regular // MountItem - because PreAllocateViewMountItem are not batched, and is relatively more @@ -147,9 +164,10 @@ internal class MountItemDispatcher( * Executes view commands, pre mount items and mount items in the respective order: * 1. View commands. * 2. Pre mount items. - * 3. Regular mount items. + * 3. Pulls of surfaces with an available transaction and an attached root view (pull model). + * 4. Regular mount items. * - * Does nothing if `viewCommandMountItemsToDispatch` and `mountItemsToDispatch` are empty. + * Does nothing if there are no view commands, mount items or surfaces to pull. * Nothing should call this directly except for `tryDispatchMountItems`. */ @UiThread @@ -160,8 +178,13 @@ internal class MountItemDispatcher( val viewCommandMountItemsToDispatch = getAndResetViewCommandMountItems() val mountItemsToDispatch = getAndResetMountItems() + val attachedSurfaceIdsToPull = getAttachedSurfaceIdsToPull() - if (mountItemsToDispatch == null && viewCommandMountItemsToDispatch == null) { + if ( + mountItemsToDispatch == null && + viewCommandMountItemsToDispatch == null && + attachedSurfaceIdsToPull == null + ) { return } @@ -256,7 +279,8 @@ internal class MountItemDispatcher( Systrace.endSection(Systrace.TRACE_TAG_REACT) } - mountItemsToDispatch?.let { items -> + if (mountItemsToDispatch != null || attachedSurfaceIdsToPull != null) { + val items = mountItemsToDispatch.orEmpty() Systrace.beginSection( Systrace.TRACE_TAG_REACT, "MountItemDispatcher::mountViews mountItems to execute", @@ -269,6 +293,16 @@ internal class MountItemDispatcher( { -> val batchedExecutionStartTime = SystemClock.uptimeMillis() + for (surfaceId in attachedSurfaceIdsToPull.orEmpty()) { + surfaceIdsToPull.remove(surfaceId) + try { + itemDispatchListener.pullTransaction(surfaceId) + } catch (e: Throwable) { + FLog.e(TAG, "dispatchMountItems: caught exception pulling surface $surfaceId", e) + logOrRethrowMountingException(e, surfaceId) + } + } + for (mountItem in items) { if (ReactNativeFeatureFlags.enableFabricLogs()) { printMountItem(mountItem, "dispatchMountItems: Executing mountItem") @@ -298,15 +332,7 @@ internal class MountItemDispatcher( } } - if (mountItem.getSurfaceId() != View.NO_ID) { - mountingManager.getSurfaceManager(mountItem.getSurfaceId())?.printSurfaceState() - } - - if (ReactIgnorableMountingException.isIgnorable(e)) { - ReactSoftExceptionLogger.logSoftException(TAG, e) - } else { - throw e - } + logOrRethrowMountingException(e, mountItem.getSurfaceId()) } } batchedExecutionTime += SystemClock.uptimeMillis() - batchedExecutionStartTime @@ -375,6 +401,18 @@ internal class MountItemDispatcher( Systrace.endSection(Systrace.TRACE_TAG_REACT) } + private fun logOrRethrowMountingException(e: Throwable, surfaceId: Int) { + if (surfaceId != View.NO_ID) { + mountingManager.getSurfaceManager(surfaceId)?.printSurfaceState() + } + + if (ReactIgnorableMountingException.isIgnorable(e)) { + ReactSoftExceptionLogger.logSoftException(TAG, e) + } else { + throw e + } + } + private fun executeOrEnqueue(item: MountItem) { if (mountingManager.isWaitingForViewAttach(item.getSurfaceId())) { if (ReactNativeFeatureFlags.enableFabricLogs()) { @@ -408,6 +446,13 @@ internal class MountItemDispatcher( @UiThread private fun getAndResetPreMountItems(): List? = drainConcurrentItemQueue(preMountItems) + /** A surface waiting for its root view keeps its pull until the frame after the attach. */ + @ThreadConfined(UI) + @UiThread + private fun getAttachedSurfaceIdsToPull(): List? = + if (surfaceIdsToPull.isEmpty()) null + else surfaceIdsToPull.filterNot(mountingManager::isWaitingForViewAttach).ifEmpty { null } + interface ItemDispatchListener { fun willMountItems(mountItems: List?) @@ -415,6 +460,9 @@ internal class MountItemDispatcher( fun didDispatchMountItems() + /** Pull model: pulls the surface's latest transaction and mounts it synchronously. */ + fun pullTransaction(surfaceId: Int) + /** Called (from any thread) whenever new items are queued into the dispatcher. */ fun onItemsQueued() } diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PullTransactionMountItem.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PullTransactionMountItem.kt deleted file mode 100644 index 68563cdf4ec0..000000000000 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/fabric/mounting/mountitems/PullTransactionMountItem.kt +++ /dev/null @@ -1,33 +0,0 @@ -/* - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -package com.facebook.react.fabric.mounting.mountitems - -import com.facebook.proguard.annotations.DoNotStripAny -import com.facebook.react.fabric.FabricUIManagerBinding -import com.facebook.react.fabric.mounting.MountingManager - -/** - * Pull model mount item. Enqueued (on the commit thread) when C++ notifies that a transaction is - * available for a surface. When it executes on the UI thread it asks C++ to pull the surface's - * pending transaction and apply it synchronously, so the diff + batch construction happens on the - * UI thread instead of the commit thread (matching iOS). - */ -@DoNotStripAny -internal class PullTransactionMountItem( - private val surfaceId: Int, - private val binding: FabricUIManagerBinding, -) : MountItem { - - override fun execute(mountingManager: MountingManager) { - binding.pullAndExecuteTransaction(surfaceId) - } - - override fun getSurfaceId(): Int = surfaceId - - override fun toString(): String = "PullTransactionMountItem [surfaceId: $surfaceId]" -} diff --git a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlags.kt b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlags.kt index 0342cf267f2f..b7ba0e3a5bf6 100644 --- a/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlags.kt +++ b/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/internal/featureflags/ReactNativeFeatureFlags.kt @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<> + * @generated SignedSource<<7fbc80af8ddd69f5986f2c955272c334>> */ /** @@ -289,7 +289,7 @@ public object ReactNativeFeatureFlags { public fun enableModuleArgumentNSNullConversionIOS(): Boolean = accessor.enableModuleArgumentNSNullConversionIOS() /** - * When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself via a PullTransactionMountItem enqueued in the MountItemDispatcher, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`. + * When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself at its next mount dispatch, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`. */ @JvmStatic public fun enableMountingCoordinatorPullModelAndroid(): Boolean = accessor.enableMountingCoordinatorPullModelAndroid() diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h index 5a7fbc5ff001..9d0aac95c823 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h @@ -69,7 +69,7 @@ class FabricMountingManager final { /* * Pull model: notify Java that a transaction is available for `surfaceId` so - * the UI thread can pull it via a PullTransactionMountItem. + * the UI thread pulls it at its next mount dispatch. */ void onTransactionAvailable(SurfaceId surfaceId); diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp index d5c154f1e6f1..6621b05fa821 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp @@ -730,8 +730,8 @@ void FabricUIManagerBinding::schedulerShouldRenderTransactions( if (ReactNativeFeatureFlags::enableMountingCoordinatorPullModelAndroid()) { // Pull model: do NOT pull the transaction or build the batch here (on the // commit thread). Just notify Java that a transaction is available; the UI - // thread will pull it via a PullTransactionMountItem and call back into - // `pullAndExecuteTransaction`. + // thread calls back into `pullAndExecuteTransaction` at its next mount + // dispatch. mountingManager->onTransactionAvailable( mountingCoordinator->getSurfaceId()); } else if (ReactNativeFeatureFlags:: 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 f3bea15d3c84..e0796289dfc3 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,14 +9,22 @@ package com.facebook.react.fabric +import android.os.Looper import com.facebook.react.ReactRootView import com.facebook.react.bridge.ReactApplicationContext import com.facebook.react.bridge.ReactTestHelper +import com.facebook.react.bridge.UIManager +import com.facebook.react.bridge.UIManagerListener +import com.facebook.react.common.annotations.UnstableReactNativeAPI +import com.facebook.react.fabric.mounting.MountItemDispatcher import com.facebook.react.fabric.mounting.MountingManager +import com.facebook.react.fabric.mounting.mountitems.DispatchCommandMountItem 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.modules.core.ReactChoreographer +import com.facebook.react.uimanager.GuardedFrameCallback import com.facebook.react.uimanager.ThemedReactContext import com.facebook.react.uimanager.ViewManagerRegistry import com.facebook.testutils.fakes.FakeBatchEventDispatchedListener @@ -29,20 +37,23 @@ import org.junit.After import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.mockito.kotlin.mock import org.robolectric.RobolectricTestRunner +import org.robolectric.Shadows.shadowOf 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 - * PullTransactionMountItem that asks the (shadowed, JNI-backed) [FabricUIManagerBinding] to pull - * and apply the surface's pending transaction. + * Tests for the pull-model notification path: [FabricUIManager.onTransactionAvailable] marks the + * surface as having a transaction, and the next dispatch asks the (shadowed, JNI-backed) + * [FabricUIManagerBinding] to pull and apply it. * - * The Robolectric test thread is the UI thread, so a notification posted from it dispatches the - * queued mount items synchronously; notifications posted from a background thread stay queued until - * the next UI-thread dispatch (in production, the dispatcher's frame callback). + * The Robolectric test thread is the UI thread, so a notification posted from it dispatches + * synchronously; notifications posted from a background thread wait for the next UI-thread dispatch + * (in production, the dispatcher's frame callback). */ +@OptIn(UnstableReactNativeAPI::class) @RunWith(RobolectricTestRunner::class) @Config( shadows = @@ -59,15 +70,12 @@ class FabricUIManagerPullModelTest { private lateinit var underTest: FabricUIManager private lateinit var binding: FabricUIManagerBinding private lateinit var shadowBinding: ShadowFabricUIManagerBinding + private var didMountItemsCount = 0 @Before fun setup() { ReactNativeFeatureFlagsForTests.setUp() - ReactNativeFeatureFlags.override( - object : ReactNativeFeatureFlagsDefaults() { - override fun enableMountingCoordinatorPullModelAndroid(): Boolean = true - }, - ) + overrideFeatureFlags() reactContext = ReactTestHelper.createCatalystContextForTest() underTest = FabricUIManager( @@ -78,6 +86,21 @@ class FabricUIManagerPullModelTest { binding = FabricUIManagerBinding() shadowBinding = Shadow.extract(binding) underTest.setBinding(binding) + underTest.addUIManagerEventListener( + object : UIManagerListener { + override fun willDispatchViewUpdates(uiManager: UIManager) = Unit + + override fun willMountItems(uiManager: UIManager) = Unit + + override fun didMountItems(uiManager: UIManager) { + didMountItemsCount++ + } + + override fun didDispatchMountItems(uiManager: UIManager) = Unit + + override fun didScheduleMountItems(uiManager: UIManager) = Unit + } + ) } @After @@ -85,6 +108,50 @@ class FabricUIManagerPullModelTest { ReactNativeFeatureFlags.dangerouslyReset() } + private fun overrideFeatureFlags( + disableIdleMountItemFrameCallbackRearm: Boolean = false, + disableEarlyViewCommandExecution: Boolean = false, + ) { + ReactNativeFeatureFlags.dangerouslyReset() + ReactNativeFeatureFlags.override( + object : ReactNativeFeatureFlagsDefaults() { + override fun enableMountingCoordinatorPullModelAndroid(): Boolean = true + + override fun disableIdleMountItemFrameCallbackRearmAndroid(): Boolean = + disableIdleMountItemFrameCallbackRearm + + override fun disableEarlyViewCommandExecution(): Boolean = + disableEarlyViewCommandExecution + }, + ) + } + + /** + * Runs [block] with the frame callback that disarms at idle. The test drives its frames; the + * mocked Choreographer only records that one is scheduled. + */ + private fun withIdleFrameCallback(block: (frameCallback: GuardedFrameCallback) -> Unit) { + overrideFeatureFlags(disableIdleMountItemFrameCallbackRearm = true) + val originalChoreographer = ReactChoreographer.overrideInstanceForTest(mock()) + try { + val frameCallback: GuardedFrameCallback = + ReflectionHelpers.getField(underTest, "mDispatchUIFrameCallback") + ReflectionHelpers.setField(frameCallback, "mShouldSchedule", true) + block(frameCallback) + } finally { + ReactChoreographer.overrideInstanceForTest(originalChoreographer) + } + } + + private fun isFrameScheduled(frameCallback: GuardedFrameCallback): Boolean = + ReflectionHelpers.getField(frameCallback, "mIsScheduled") + + private val mountingManager: MountingManager + get() = ReflectionHelpers.getField(underTest, "mMountingManager") + + private val dispatcher: MountItemDispatcher + get() = ReflectionHelpers.getField(underTest, "mMountItemDispatcher") + private fun runOnBackgroundThread(block: () -> Unit) { var error: Throwable? = null val thread = Thread { @@ -121,6 +188,60 @@ class FabricUIManagerPullModelTest { method.invoke(underTest, mountItem, 0, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0, true) } + private fun recordingMountItem(surfaceId: Int, name: String, log: MutableList) = + object : MountItem { + override fun execute(mountingManager: MountingManager) { + log.add(name) + } + + override fun getSurfaceId(): Int = surfaceId + } + + private fun recordingCommand(surfaceId: Int, name: String, log: MutableList) = + object : DispatchCommandMountItem() { + override fun execute(mountingManager: MountingManager) { + log.add(name) + } + + override fun getSurfaceId(): Int = surfaceId + } + + /** + * Stands in for a surface's MountingCoordinator: a commit notifies from a background (commit) + * thread, and a pull mounts every revision committed since the previous pull as one synchronous + * batch named "->", or mounts nothing when there is no new revision. + */ + private inner class PulledSurface(val surfaceId: Int, val log: MutableList) { + private var committedRevision = 0 + private var pulledRevision = 0 + + fun commit() { + committedRevision++ + runOnBackgroundThread { underTest.onTransactionAvailable(surfaceId) } + } + + fun commitOnUiThread() { + committedRevision++ + underTest.onTransactionAvailable(surfaceId) + } + + fun pull() { + if (committedRevision == pulledRevision) { + return + } + val batch = "$pulledRevision->$committedRevision" + pulledRevision = committedRevision + scheduleSynchronously(recordingMountItem(surfaceId, batch, log)) + } + } + + /** The revisions that the "->" batches mounted, in mount order. */ + private fun mountedRevisions(batches: List): List = + batches.flatMap { batch -> + val (from, to) = batch.split("->").map(String::toInt) + (from + 1..to).toList() + } + @Test fun onTransactionAvailable_onUiThread_pullsSynchronously() { underTest.onTransactionAvailable(1) @@ -129,47 +250,220 @@ class FabricUIManagerPullModelTest { } @Test - fun onTransactionAvailable_enqueuesOnePullPerNotification() { - // Mirrors iOS: one transaction block per commit, in notification order. Redundant pulls - // (revisions already consumed by an earlier accumulating pull) no-op in C++, not here. + fun onTransactionAvailable_pullsEachPendingSurfaceOnce() { + // A pull mounts every revision committed since the previous one, so further notifications for + // a surface that is already pending add nothing. runOnBackgroundThread { underTest.onTransactionAvailable(1) underTest.onTransactionAvailable(2) underTest.onTransactionAvailable(1) } - // Notifications from a background (commit) thread are queued, not dispatched. + // Notifications from a background (commit) thread are not dispatched. assertThat(shadowBinding.pulledSurfaceIds).isEmpty() - // The next UI-thread dispatch drains them in order. + // The next UI-thread dispatch pulls every pending surface. underTest.onTransactionAvailable(3) - assertThat(shadowBinding.pulledSurfaceIds).containsExactly(1, 2, 1, 3) + assertThat(shadowBinding.pulledSurfaceIds).containsExactlyInAnyOrder(1, 2, 3) } @Test - fun synchronousBatch_waitingForRootAttach_isDeferred() { + fun pullDeferredByRootAttach_isAppliedBeforeLaterPulls() { val surfaceId = 12 val themedReactContext = ThemedReactContext(reactContext, reactContext, "TestModule", surfaceId) - val mountingManager = ReflectionHelpers.getField(underTest, "mMountingManager") mountingManager.startSurface(surfaceId, themedReactContext, null) + val appliedBatches = mutableListOf() + val surface = PulledSurface(surfaceId, appliedBatches) + shadowBinding.onPull = { surface.pull() } - var executionCount = 0 - val mountItem = - object : MountItem { - override fun execute(mountingManager: MountingManager) { - executionCount++ - } + surface.commit() + dispatcher.tryDispatchMountItems() // the root view is not attached, so nothing mounts yet + surface.commit() + mountingManager.attachRootView(surfaceId, ReactRootView(reactContext), themedReactContext) + surface.commit() + dispatcher.tryDispatchMountItems() + + assertThat(mountedRevisions(appliedBatches)).containsExactly(1, 2, 3) + } + + @Test + fun preallocationQueuedBeforeDispatch_runsBeforeThePull() { + val log = mutableListOf() + val surface = PulledSurface(11, log) + shadowBinding.onPull = { surface.pull() } + + surface.commit() + runOnBackgroundThread { + dispatcher.addPreAllocateMountItem(recordingMountItem(11, "preallocate", log)) + } + dispatcher.tryDispatchMountItems() + + assertThat(log).containsExactly("preallocate", "0->1") + } - override fun getSurfaceId(): Int = surfaceId + @Test + fun mountItemQueuedBeforeCommit_runsAfterThePull() { + // Pending pulls run before every queued mount item, whatever their notification order. + val log = mutableListOf() + val surface = PulledSurface(11, log) + shadowBinding.onPull = { surface.pull() } + + runOnBackgroundThread { dispatcher.addMountItem(recordingMountItem(11, "event", log)) } + surface.commit() + dispatcher.tryDispatchMountItems() + + assertThat(log).containsExactly("0->1", "event") + } + + @Test + fun pendingPullOfStoppedSurface_runsOnceAndIsDropped() { + // The C++ pull of a stopped surface finds no surface and mounts nothing. + val surfaceId = 12 + val themedReactContext = ThemedReactContext(reactContext, reactContext, "TestModule", surfaceId) + mountingManager.startSurface(surfaceId, themedReactContext, null) + runOnBackgroundThread { underTest.onTransactionAvailable(surfaceId) } + dispatcher.tryDispatchMountItems() // waits for the root view + + mountingManager.stopSurface(surfaceId) + dispatcher.tryDispatchMountItems() + dispatcher.tryDispatchMountItems() + + assertThat(shadowBinding.pulledSurfaceIds).containsExactly(surfaceId) + } + + @Test + fun pull_runsMountHooks() { + val surface = PulledSurface(11, mutableListOf()) + shadowBinding.onPull = { surface.pull() } + + surface.commit() + dispatcher.tryDispatchMountItems() + shadowOf(Looper.getMainLooper()).idle() + + assertThat(didMountItemsCount).isEqualTo(1) + assertThat(shadowBinding.reportedMountSurfaceIds).containsExactly(11) + } + + @Test + fun pullWaitingForRootAttach_isMountedByTheFrameAfterTheAttach() { + // With the idle frame callback, nothing may keep frames running while the root is detached, so + // the attach must schedule the frame that mounts the surface. + withIdleFrameCallback { frameCallback -> + val surfaceId = 12 + val themedReactContext = + ThemedReactContext(reactContext, reactContext, "TestModule", surfaceId) + mountingManager.startSurface(surfaceId, themedReactContext, null) + val log = mutableListOf() + val surface = PulledSurface(surfaceId, log) + shadowBinding.onPull = { surface.pull() } + + surface.commit() + runOnBackgroundThread { + dispatcher.addPreAllocateMountItem(recordingMountItem(surfaceId, "preallocate", log)) + } + frameCallback.doFrame(System.nanoTime()) + + assertThat(log).isEmpty() + assertThat(isFrameScheduled(frameCallback)).isFalse() + + mountingManager.attachRootView(surfaceId, ReactRootView(reactContext), themedReactContext) + + assertThat(isFrameScheduled(frameCallback)).isTrue() + + frameCallback.doFrame(System.nanoTime()) + + assertThat(log).containsExactly("preallocate", "0->1") + } + } + + @Test + fun rootAttachWithoutPendingPull_doesNotScheduleFrame() { + withIdleFrameCallback { frameCallback -> + val surfaceId = 12 + val themedReactContext = + ThemedReactContext(reactContext, reactContext, "TestModule", surfaceId) + mountingManager.startSurface(surfaceId, themedReactContext, null) + + mountingManager.attachRootView(surfaceId, ReactRootView(reactContext), themedReactContext) + + assertThat(isFrameScheduled(frameCallback)).isFalse() + } + } + + @Test + fun pendingPull_keepsTheIdleFrameCallbackArmed() { + withIdleFrameCallback { frameCallback -> + val firstLog = mutableListOf() + val secondLog = mutableListOf() + val surfaces = mapOf(11 to PulledSurface(11, firstLog), 12 to PulledSurface(12, secondLog)) + shadowBinding.onPull = { surfaceId -> + surfaces.getValue(surfaceId).pull() + if (surfaceId == 11) { + // The commit thread notifies while the frame mounts surface 11. + surfaces.getValue(12).commit() } + } - scheduleSynchronously(mountItem) + surfaces.getValue(11).commit() + frameCallback.doFrame(System.nanoTime()) - assertThat(executionCount).isZero() + assertThat(secondLog).isEmpty() + assertThat(isFrameScheduled(frameCallback)).isTrue() - mountingManager.attachRootView(surfaceId, ReactRootView(reactContext), themedReactContext) - underTest.onTransactionAvailable(surfaceId) + frameCallback.doFrame(System.nanoTime()) + + assertThat(firstLog).containsExactly("0->1") + assertThat(secondLog).containsExactly("0->1") + assertThat(isFrameScheduled(frameCallback)).isFalse() + } + } + + @Test + fun commitFromUiThreadDuringPull_isMountedByTheNextFrame() { + // The nested dispatch returns early, so the pending pull alone must schedule the next frame. + withIdleFrameCallback { frameCallback -> + val log = mutableListOf() + val surface = PulledSurface(11, log) + var committedDuringPull = false + shadowBinding.onPull = { + surface.pull() + if (!committedDuringPull) { + committedDuringPull = true + surface.commitOnUiThread() + } + } + + surface.commit() + frameCallback.doFrame(System.nanoTime()) + + assertThat(log).containsExactly("0->1") + assertThat(isFrameScheduled(frameCallback)).isTrue() + + frameCallback.doFrame(System.nanoTime()) + + assertThat(log).containsExactly("0->1", "1->2") + assertThat(isFrameScheduled(frameCallback)).isFalse() + } + } + + @Test + fun viewCommandsInMountItemOrder_runAfterThePull() { + // With disableEarlyViewCommandExecution, view commands go through the mount item queue. A + // command dispatched before a commit's notification runs after that commit mounts. + overrideFeatureFlags(disableEarlyViewCommandExecution = true) + val log = mutableListOf() + val surface = PulledSurface(11, log) + shadowBinding.onPull = { surface.pull() } + + runOnBackgroundThread { + dispatcher.addViewCommandMountItem(recordingCommand(11, "commandBeforeCommit", log)) + } + surface.commit() + runOnBackgroundThread { + dispatcher.addViewCommandMountItem(recordingCommand(11, "commandAfterCommit", log)) + } + dispatcher.tryDispatchMountItems() - assertThat(executionCount).isEqualTo(1) + assertThat(log).containsExactly("0->1", "commandBeforeCommit", "commandAfterCommit") } } diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/testutils/shadows/ShadowFabricUIManagerBinding.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/testutils/shadows/ShadowFabricUIManagerBinding.kt index a9fbbf95e9f8..ffade439c192 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/testutils/shadows/ShadowFabricUIManagerBinding.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/testutils/shadows/ShadowFabricUIManagerBinding.kt @@ -18,9 +18,20 @@ import org.robolectric.annotation.Implements @Implements(className = "com.facebook.react.fabric.FabricUIManagerBinding") class ShadowFabricUIManagerBinding { val pulledSurfaceIds: MutableList = mutableListOf() + val reportedMountSurfaceIds: MutableList = mutableListOf() + /** Stands in for the C++ pull and mount. */ + var onPull: (surfaceId: Int) -> Unit = {} @Implementation fun pullAndExecuteTransaction(surfaceId: Int) { pulledSurfaceIds.add(surfaceId) + onPull(surfaceId) } + + @Implementation + fun reportMount(surfaceId: Int) { + reportedMountSurfaceIds.add(surfaceId) + } + + @Implementation fun drainPreallocateViewsQueue() = Unit } diff --git a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h index c5195f19bd59..a1c7ddec6434 100644 --- a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h +++ b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<<1a9a8a587b477b6aa0d3aa57e30272c2>> + * @generated SignedSource<> */ /** @@ -257,7 +257,7 @@ class ReactNativeFeatureFlags { RN_EXPORT static bool enableModuleArgumentNSNullConversionIOS(); /** - * When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself via a PullTransactionMountItem enqueued in the MountItemDispatcher, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`. + * When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself at its next mount dispatch, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`. */ RN_EXPORT static bool enableMountingCoordinatorPullModelAndroid(); diff --git a/packages/react-native/ReactCommon/react/featureflags/ReactNativePublicFeatureFlags.h b/packages/react-native/ReactCommon/react/featureflags/ReactNativePublicFeatureFlags.h index 542d15a59baa..7f688dfe3552 100644 --- a/packages/react-native/ReactCommon/react/featureflags/ReactNativePublicFeatureFlags.h +++ b/packages/react-native/ReactCommon/react/featureflags/ReactNativePublicFeatureFlags.h @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<> + * @generated SignedSource<<571d4f35aacf5086c088bdc8dd1abd33>> */ /** @@ -48,7 +48,7 @@ class ReactNativeFeatureFlags_DO_NOT_USE { RN_EXPORT static bool enableExclusivePropsUpdateAndroid(); /** - * When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself via a PullTransactionMountItem enqueued in the MountItemDispatcher, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`. + * When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself at its next mount dispatch, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`. */ RN_EXPORT static bool enableMountingCoordinatorPullModelAndroid(); diff --git a/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js b/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js index 08f37bd4f127..0d8a098bf665 100644 --- a/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js +++ b/packages/react-native/scripts/featureflags/ReactNativeFeatureFlags.config.js @@ -517,7 +517,7 @@ const definitions: FeatureFlagDefinitions = { metadata: { dateAdded: '2026-07-14', description: - 'When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself via a PullTransactionMountItem enqueued in the MountItemDispatcher, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`.', + 'When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself at its next mount dispatch, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`.', expectedReleaseValue: true, purpose: 'experimentation', }, diff --git a/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js b/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js index d8626a635e6b..a34f7fb00644 100644 --- a/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js +++ b/packages/react-native/src/private/featureflags/ReactNativeFeatureFlags.js @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<> + * @generated SignedSource<> * @flow strict * @noformat */ @@ -397,7 +397,7 @@ export const enableLayoutAnimationsOnIOS: Getter = createNativeFlagGett */ export const enableModuleArgumentNSNullConversionIOS: Getter = createNativeFlagGetter('enableModuleArgumentNSNullConversionIOS', false); /** - * When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself via a PullTransactionMountItem enqueued in the MountItemDispatcher, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`. + * When enabled, Android mounts transactions with the pull model (like iOS): the commit thread no longer pulls and builds the mount batch in schedulerShouldRenderTransactions. Instead the UI thread pulls the transaction itself at its next mount dispatch, builds the IntBufferBatchMountItem, and applies it synchronously. Requires `enableAccumulatedUpdatesInRawPropsAndroid` to be enabled as well, since a single pull may collapse several commits into one diff and therefore needs complete accumulated rawProps; when used together with Props 2.0, also enable `enableExclusivePropsUpdateAndroid` and `enablePropsUpdateReconciliationAndroid`. */ export const enableMountingCoordinatorPullModelAndroid: Getter = createNativeFlagGetter('enableMountingCoordinatorPullModelAndroid', false); /** From 0839a3c4eb870d1749ee5c55ae128ae542d659c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20B=C5=82oniarz?= Date: Wed, 14 Oct 2026 06:21:57 -0700 Subject: [PATCH 2/2] Pull model: mount the commits of starting and stopping surfaces (#59030) Summary: With `enableMountingCoordinatorPullModelAndroid`, a commit tells Java only its surface id. At the next mount dispatch, `FabricUIManagerBinding::pullAndExecuteTransaction` looks the surface's `MountingCoordinator` up in the binding's surface registry. A surface is not in the registry while it starts or stops: - The transaction that stops a surface is never pulled, because `stopSurfaceWithSurfaceHandler` removes the surface from the registry before `SurfaceHandler::stop()` commits the empty tree. Mounting override delegates such as LayoutAnimations and C++ Animated never see those Deletes. With `enableLayoutAnimationsOnAndroid`, a layout animation that runs when its surface stops stays in flight, and `FabricUIManager` calls `driveCxxAnimations` on every frame until another surface pulls a transaction. - A commit that a bridgeless surface makes before `startSurfaceWithSurfaceHandler` registers it is not mounted until the surface commits again. - The legacy `stopSurface(int)` holds the registry lock across `stop()`. On the UI thread, the stop commit pulls inline and blocks on the same lock, so the app freezes. iOS does not look the surface up. `RCTMountingManager` captures the `MountingCoordinator` that reported the commit in the block it dispatches to the main queue, and pulls from it. This PR does the same. `schedulerShouldRenderTransactions` keeps the reporting `MountingCoordinator` until the UI thread pulls, and `pullAndExecuteTransaction` pulls from it. Java still receives only the surface id. The surface registry, the start and stop functions, `reportMount` and the push model are unchanged. As on iOS, a surface stopped on the UI thread has its stop transaction pulled during the stop commit. When it stops on another thread, its shadow tree is destroyed and its coordinator revoked before the UI thread pulls, so that transaction is not mounted. A pull of a stopped surface can log "Executing commit after surface N was stopped!" when a mounting override delegate still returns a transaction; iOS pulls these the same way. Only one coordinator per surface id is kept. Stopping and starting the same `ReactSurface` on the same host reuses its id. If the stopped surface's commit has not been pulled yet, the restarted surface's coordinator replaces it and a warning is logged. The replaced coordinator is revoked and would pull nothing. Java does not mount such a restarted surface anyway: `MountingManager.startSurface` logs "Called startSurface more than once" and keeps the stopped surface's manager. ## Changelog: [ANDROID] [FIXED] - Pull model: mount the commits of starting and stopping surfaces, and fix a freeze when the legacy `stopSurface(int)` runs on the UI thread Pull Request resolved: https://github.com/react/react-native/pull/59030 Test Plan: New `FabricMountingManagerInstrumentationTest` cases drive a real `FabricUIManagerBinding`. The test helper runs no JavaScript and commits views to the surface's shadow tree itself. The pull-model cases run with the Props 2.0 flags and `enablePreallocatedPropsDiffOnInsertAndroid`. | Test | Without the fix | With the fix | | --- | --- | --- | | `native_stopSurfaceWithSurfaceHandler_pullModel_pullsDeletes` | fails: no Deletes are pulled | passes | | `native_stopSurface_pullModel_pullsDeletes` | hangs | passes | | `native_stopSurfaceWithSurfaceHandler_pushModel_pullsDeletes` | passes | passes | The push case is a control. The hang stops the run, so the bridgeless stop test was also run without the legacy one. With the fix, the whole class passes. A one-off check, not committed, covered the start: a bridgeless surface started on a background thread commits a view, and the start is held before `startSurfaceWithSurfaceHandler` registers the surface. On https://github.com/react/react-native/issues/59029 alone the view is not mounted while the start is held. With this PR it is mounted by the next frame. The test class has no Gradle target in the open-source repository, so I ran it on an Android emulator through a local androidTest source set: ``` ./gradlew :packages:react-native:ReactAndroid:assembleReleaseAndroidTest -PreactNativeArchitectures=arm64-v8a adb install -r -t packages/react-native/ReactAndroid/build/outputs/apk/androidTest/release/ReactAndroid-release-androidTest.apk adb shell am instrument -w -e class com.facebook.react.fabric.FabricMountingManagerInstrumentationTest \ com.facebook.react.tests.gradle/androidx.test.runner.AndroidJUnitRunner ``` I also checked the effect in RNTester with a temporary hook, not part of this PR. The Playground starts a 60 s `LayoutAnimation`, then `ReactDelegate.unloadApp()` stops the surface on the UI thread while the activity stays resumed. Props 2.0 and `enableLayoutAnimationsOnAndroid` were on: | Run | After the stop | | --- | --- | | Pull, without this PR | no `onAllAnimationsComplete`; `driveCxxAnimations` keeps running on every frame | | Pull, with this PR | `onAllAnimationsComplete` right after the stop, then no more driven frames | | Push, with this PR | `onAllAnimationsComplete` right after the stop, as before | Pulled By: bartlomiejbloniarz bartlomiejbloniarz Differential Revision: D124966432 --- .../react/fabric/FabricUIManagerBinding.cpp | 40 ++--- .../jni/react/fabric/FabricUIManagerBinding.h | 6 + .../FabricMountingManagerTestHelper.cpp | 136 +++++++++++++++ .../FabricMountingManagerTestHelper.h | 17 ++ ...abricMountingManagerInstrumentationTest.kt | 165 +++++++++++++++++- .../fabric/FabricMountingManagerTestHelper.kt | 21 ++- 6 files changed, 358 insertions(+), 27 deletions(-) diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp index 6621b05fa821..f5a2247bc4bd 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp @@ -125,29 +125,12 @@ void FabricUIManagerBinding::pullAndExecuteTransaction(SurfaceId surfaceId) { std::shared_ptr mountingCoordinator; { - std::shared_lock lock(surfaceHandlerRegistryMutex_); - auto iterator = surfaceHandlerRegistry_.find(surfaceId); - if (iterator == surfaceHandlerRegistry_.end()) { + std::scoped_lock lock(pullableMountingCoordinatorsMutex_); + auto pullable = pullableMountingCoordinators_.extract(surfaceId); + if (pullable.empty()) { return; } - const auto* surfaceHandler = std::get_if(&iterator->second); - jni::local_ref javaSurfaceHandler; - if (surfaceHandler == nullptr) { - javaSurfaceHandler = - std::get>( - iterator->second) - .lockLocal(); - if (javaSurfaceHandler) { - surfaceHandler = &javaSurfaceHandler->cthis()->getSurfaceHandler(); - } - } - if (surfaceHandler != nullptr) { - mountingCoordinator = surfaceHandler->getMountingCoordinator(); - } - } - - if (mountingCoordinator == nullptr) { - return; + mountingCoordinator = std::move(pullable.mapped()); } auto mountingManager = getMountingManager("pullAndExecuteTransaction"); @@ -732,8 +715,19 @@ void FabricUIManagerBinding::schedulerShouldRenderTransactions( // commit thread). Just notify Java that a transaction is available; the UI // thread calls back into `pullAndExecuteTransaction` at its next mount // dispatch. - mountingManager->onTransactionAvailable( - mountingCoordinator->getSurfaceId()); + auto surfaceId = mountingCoordinator->getSurfaceId(); + { + std::scoped_lock lock(pullableMountingCoordinatorsMutex_); + auto& pullable = pullableMountingCoordinators_[surfaceId]; + if (pullable != nullptr && pullable != mountingCoordinator) { + LOG(WARNING) + << "FabricUIManagerBinding::schedulerShouldRenderTransactions: " + << "surface " << surfaceId + << " was restarted while the previous surface's commit waits to be pulled"; + } + pullable = mountingCoordinator; + } + mountingManager->onTransactionAvailable(surfaceId); } else if (ReactNativeFeatureFlags:: enableAccumulatedUpdatesInRawPropsAndroid()) { auto mountingTransaction = mountingCoordinator->pullTransaction( diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.h b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.h index 97229f08fdcf..68c47706ba7b 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.h @@ -167,6 +167,12 @@ class FabricUIManagerBinding : public jni::HybridClass, std::mutex pendingTransactionsMutex_; std::vector pendingTransactions_; + // Pull model: keeps the coordinator that reported a surface's commit alive + // until the UI thread handles the pull, as iOS does by capturing it in the + // block it dispatches to the main queue. Java only gets the surface id. + std::mutex pullableMountingCoordinatorsMutex_; + std::unordered_map> pullableMountingCoordinators_; + float pointScaleFactor_ = 1; bool enableFabricLogs_{false}; diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.cpp index 8a72bfa3ab71..892896c847d6 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.cpp @@ -7,15 +7,20 @@ #include "FabricMountingManagerTestHelper.h" +#include #include #include #include +#include +#include #include #include #include #include #include +#include #include +#include namespace facebook::react { @@ -29,6 +34,41 @@ static LayoutConstraints makeLayoutConstraints( .layoutDirection = LayoutDirection::LeftToRight}; } +class FabricMountingManagerTestHelper::DeleteRecorder + : public MountingOverrideDelegate { + public: + bool shouldOverridePullTransaction() const override { + return true; + } + + std::optional pullTransaction( + SurfaceId surfaceId, + MountingTransaction::Number number, + const TransactionTelemetry& telemetry, + ShadowViewMutationList mutations) const override { + for (const auto& mutation : mutations) { + if (mutation.type == ShadowViewMutation::Delete) { + deletedTags.push_back(mutation.oldChildShadowView.tag); + } + } + return MountingTransaction{ + surfaceId, number, std::move(mutations), telemetry}; + } + + mutable std::vector deletedTags; +}; + +namespace { + +void runNoJavaScript(std::function&& /*unused*/) {} + +std::vector toTags(jni::alias_ref tags) { + auto region = tags->getRegion(0, static_cast(tags->size())); + return {region.get(), region.get() + tags->size()}; +} + +} // namespace + FabricMountingManagerTestHelper::FabricMountingManagerTestHelper( jni::alias_ref jFabricUIManager) : javaUIManager_(jni::make_global(jFabricUIManager)) { @@ -223,6 +263,85 @@ void FabricMountingManagerTestHelper::mountPendingTransaction() { } } +jni::local_ref +FabricMountingManagerTestHelper::createRuntimeExecutor() { + return JRuntimeExecutor::newObjectCxxArgs(runNoJavaScript); +} + +jni::local_ref +FabricMountingManagerTestHelper::createRuntimeScheduler() { + // Work scheduled here never runs and stays queued. A queued task holds the + // UIManager, which holds this scheduler, so both outlive the test. + runtimeScheduler_ = std::make_shared(runNoJavaScript); + return JRuntimeScheduler::newObjectCxxArgs(runtimeScheduler_); +} + +void FabricMountingManagerTestHelper::registerViewComponent( + ComponentFactory* componentFactory) { + componentFactory->buildRegistryFunction = + [](const EventDispatcher::Weak& eventDispatcher, + const std::shared_ptr& contextContainer) { + auto providerRegistry = + std::make_shared(); + providerRegistry->add( + concreteComponentDescriptorProvider()); + return providerRegistry->createComponentDescriptorRegistry( + {.eventDispatcher = eventDispatcher, + .contextContainer = contextContainer, + .flavor = nullptr}); + }; +} + +void FabricMountingManagerTestHelper::commitViews( + jint surfaceId, + jni::alias_ref tags) { + auto children = + std::make_shared>>(); + for (auto tag : toTags(tags)) { + auto props = std::make_shared(); + props->collapsable = false; + children->push_back(viewComponentDescriptor_->createShadowNode( + {.props = props}, + viewComponentDescriptor_->createFamily( + {.tag = tag, .surfaceId = surfaceId, .instanceHandle = nullptr}))); + } + javaUIManager_->getBinding() + ->getScheduler() + ->getUIManager() + ->getShadowTreeRegistry() + .visit(surfaceId, [&](const ShadowTree& shadowTree) { + shadowTree.commit( + [&](const RootShadowNode& oldRootShadowNode) { + return std::make_shared( + oldRootShadowNode, + ShadowNodeFragment{ + .props = ShadowNodeFragment::propsPlaceholder(), + .children = children}); + }, + {}); + }); +} + +void FabricMountingManagerTestHelper::recordPulledDeletes(jint surfaceId) { + deleteRecorder_ = std::make_shared(); + javaUIManager_->getBinding() + ->getScheduler() + ->getUIManager() + ->getShadowTreeRegistry() + .visit(surfaceId, [&](const ShadowTree& shadowTree) { + shadowTree.getMountingCoordinator()->setMountingOverrideDelegate( + deleteRecorder_); + }); +} + +jni::local_ref +FabricMountingManagerTestHelper::pulledDeleteTags() { + const auto& tags = deleteRecorder_->deletedTags; + auto array = jni::JArrayInt::newArray(tags.size()); + array->setRegion(0, static_cast(tags.size()), tags.data()); + return array; +} + void FabricMountingManagerTestHelper::registerNatives() { registerHybrid({ makeNativeMethod( @@ -256,6 +375,23 @@ void FabricMountingManagerTestHelper::registerNatives() { makeNativeMethod( "resetFeatureFlags", FabricMountingManagerTestHelper::resetFeatureFlags), + makeNativeMethod( + "createRuntimeExecutor", + FabricMountingManagerTestHelper::createRuntimeExecutor), + makeNativeMethod( + "createRuntimeScheduler", + FabricMountingManagerTestHelper::createRuntimeScheduler), + makeNativeMethod( + "registerViewComponent", + FabricMountingManagerTestHelper::registerViewComponent), + makeNativeMethod( + "commitViews", FabricMountingManagerTestHelper::commitViews), + makeNativeMethod( + "recordPulledDeletes", + FabricMountingManagerTestHelper::recordPulledDeletes), + makeNativeMethod( + "pulledDeleteTags", + FabricMountingManagerTestHelper::pulledDeleteTags), }); } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.h b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.h index 2e2b0192eca6..4348f72a7c68 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.h @@ -8,11 +8,14 @@ #pragma once #include +#include +#include #include #include #include #include +#include #include namespace facebook::react { @@ -38,6 +41,17 @@ class FabricMountingManagerTestHelper : public jni::HybridClass createRuntimeExecutor(); + jni::local_ref createRuntimeScheduler(); + void registerViewComponent(ComponentFactory *componentFactory); + + void commitViews(jint surfaceId, jni::alias_ref tags); + void recordPulledDeletes(jint surfaceId); + jni::local_ref pulledDeleteTags(); + static void registerNatives(); private: @@ -57,6 +71,9 @@ class FabricMountingManagerTestHelper : public jni::HybridClass viewComponentDescriptor_; std::unique_ptr shadowTreeDelegate_; std::unique_ptr shadowTree_; + std::shared_ptr runtimeScheduler_; + class DeleteRecorder; + std::shared_ptr deleteRecorder_; }; } // namespace facebook::react diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerInstrumentationTest.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerInstrumentationTest.kt index 9f05e5f880b1..64962a757483 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerInstrumentationTest.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerInstrumentationTest.kt @@ -8,6 +8,7 @@ package com.facebook.react.fabric import android.graphics.Rect +import android.view.View import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.platform.app.InstrumentationRegistry import com.facebook.react.ReactRootView @@ -17,11 +18,15 @@ import com.facebook.react.bridge.ReactApplicationContext import com.facebook.react.bridge.ReadableType import com.facebook.react.bridge.WritableNativeArray import com.facebook.react.bridge.WritableNativeMap +import com.facebook.react.fabric.events.EventBeatManager import com.facebook.react.fabric.mounting.MountingManager import com.facebook.react.fabric.mounting.MountingManager.MountItemExecutor import com.facebook.react.fabric.mounting.mountitems.IntBufferBatchMountItem import com.facebook.react.internal.featureflags.ReactNativeFeatureFlags +import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsCxxInterop +import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsDefaults import com.facebook.react.internal.featureflags.ReactNativeFeatureFlagsForTests +import com.facebook.react.uimanager.DisplayMetricsHolder import com.facebook.react.uimanager.PixelUtil import com.facebook.react.uimanager.ThemedReactContext import com.facebook.react.uimanager.ViewManager @@ -52,6 +57,7 @@ import org.mockito.Mockito.`when` class FabricMountingManagerInstrumentationTest { private lateinit var mountingManager: MountingManager private lateinit var themedReactContext: ThemedReactContext + private val surfaceId = 1 private var nativeFeatureFlagsHelper: FabricMountingManagerTestHelper? = null @@ -78,8 +84,16 @@ class FabricMountingManagerInstrumentationTest { @After fun tearDown() { - nativeFeatureFlagsHelper?.resetFeatureFlags() - ReactNativeFeatureFlags.dangerouslyReset() + try { + while (cleanups.isNotEmpty()) { + cleanups.removeLast()() + } + } finally { + bindingDependencies.clear() + nativeFeatureFlagsHelper?.resetFeatureFlags() + ReactNativeFeatureFlags.dangerouslyReset() + ReactNativeFeatureFlagsCxxInterop.dangerouslyReset() + } } private fun startSurface() { @@ -396,4 +410,151 @@ class FabricMountingManagerInstrumentationTest { view.setPadding(left, top, right, bottom) } } + + // ---- Surface lifecycle through a real FabricUIManagerBinding ---- + + /** + * The pull model is tested with Props 2.0 (accumulated, exclusive and reconciled props, and props + * diffed on insert); the push model with all of these flags off. + */ + private fun overrideCxxFlags(pullModel: Boolean) { + ReactNativeFeatureFlagsCxxInterop.dangerouslyForceOverride( + object : ReactNativeFeatureFlagsDefaults() { + override fun enableMountingCoordinatorPullModelAndroid(): Boolean = pullModel + + override fun enableAccumulatedUpdatesInRawPropsAndroid(): Boolean = pullModel + + override fun enableExclusivePropsUpdateAndroid(): Boolean = pullModel + + override fun enablePropsUpdateReconciliationAndroid(): Boolean = pullModel + + override fun enablePreallocatedPropsDiffOnInsertAndroid(): Boolean = pullModel + } + ) + } + + /** + * Run in reverse order after each test: stop the surfaces a test leaves running, then unregister + * the bindings. + */ + private val cleanups = ArrayDeque<() -> Unit>() + + /** + * Kept alive until the bindings are unregistered, as a React instance keeps them: the native + * binding's event beat uses the event beat manager, and the helper owns the view component + * descriptor of the committed views. + */ + private val bindingDependencies = mutableListOf() + + private fun registerBinding(fabricUIManager: FabricUIManager): FabricMountingManagerTestHelper { + val binding = FabricUIManagerBinding() + val helper = FabricMountingManagerTestHelper.create(fabricUIManager) + cleanups.addLast { runOnUiThread { binding.unregister() } } + val componentFactory = ComponentFactory() + val eventBeatManager = EventBeatManager() + bindingDependencies.addAll(listOf(helper, eventBeatManager)) + helper.registerViewComponent(componentFactory) + DisplayMetricsHolder.initDisplayMetricsIfNotInitialized(themedReactContext) + binding.register( + helper.createRuntimeExecutor(), + helper.createRuntimeScheduler(), + fabricUIManager, + eventBeatManager, + componentFactory, + ) + return helper + } + + private fun runOnUiThread(block: () -> Unit) { + InstrumentationRegistry.getInstrumentation().runOnMainSync(block) + } + + private val rootMeasureSpec = View.MeasureSpec.makeMeasureSpec(100, View.MeasureSpec.EXACTLY) + + private fun startSurfaceWithHandler(fabricUIManager: FabricUIManager): SurfaceHandlerBinding { + val surfaceHandler = SurfaceHandlerBinding("TestModule") + surfaceHandler.setLayoutConstraints( + rootMeasureSpec, + rootMeasureSpec, + 0, + 0, + false, + false, + 1f, + 1f, + ) + fabricUIManager.startSurface( + surfaceHandler, + themedReactContext, + ReactRootView(themedReactContext), + ) + return surfaceHandler + } + + /** Starts a surface through the bridgeless path and returns its id and a function stopping it. */ + private fun startBridgelessSurface(fabricUIManager: FabricUIManager): Pair Unit> { + val surfaceHandler = startSurfaceWithHandler(fabricUIManager) + return surfaceHandler.surfaceId to { fabricUIManager.stopSurface(surfaceHandler) } + } + + /** Starts a surface through the legacy path and returns its id and a function stopping it. */ + private fun startLegacySurface(fabricUIManager: FabricUIManager): Pair Unit> { + val surfaceId = + fabricUIManager.startSurface( + ReactRootView(themedReactContext), + "TestModule", + null, + rootMeasureSpec, + rootMeasureSpec, + ) + return surfaceId to { fabricUIManager.stopSurface(surfaceId) } + } + + @Test + fun native_stopSurfaceWithSurfaceHandler_pullModel_pullsDeletes() { + assertStoppingSurfacePullsDeletes(pullModel = true, ::startBridgelessSurface) + } + + @Test + fun native_stopSurfaceWithSurfaceHandler_pushModel_pullsDeletes() { + assertStoppingSurfacePullsDeletes(pullModel = false, ::startBridgelessSurface) + } + + /** The legacy stop also runs on the UI thread, where its stop commit pulls inline. */ + @Test + fun native_stopSurface_pullModel_pullsDeletes() { + assertStoppingSurfacePullsDeletes(pullModel = true, ::startLegacySurface) + } + + /** + * The transaction that stops a surface deletes its views. Mounting override delegates, such as + * LayoutAnimations and C++ Animated, must see it pulled. + */ + private fun assertStoppingSurfacePullsDeletes( + pullModel: Boolean, + startSurface: (FabricUIManager) -> Pair Unit>, + ) { + overrideCxxFlags(pullModel) + val fabricUIManager = createFabricUIManager() + val helper = registerBinding(fabricUIManager) + lateinit var stopSurface: () -> Unit + var running = true + var mounted = false + runOnUiThread { + val (surfaceId, stop) = startSurface(fabricUIManager) + stopSurface = stop + cleanups.addLast { if (running) runOnUiThread(stop) } + helper.recordPulledDeletes(surfaceId) + helper.commitViews(surfaceId, intArrayOf(42, 43)) + mounted = fabricUIManager.resolveView(42) != null + } + assertThat(mounted).isTrue() + + runOnUiThread { + running = false + stopSurface() + } + + assertThat(helper.pulledDeleteTags()).containsExactlyInAnyOrder(42, 43) + } } diff --git a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerTestHelper.kt b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerTestHelper.kt index ce133c1d0a67..957e3e8c5c0b 100644 --- a/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerTestHelper.kt +++ b/packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerTestHelper.kt @@ -9,6 +9,8 @@ package com.facebook.react.fabric import com.facebook.jni.HybridClassBase import com.facebook.react.bridge.NativeMap +import com.facebook.react.bridge.RuntimeExecutor +import com.facebook.react.bridge.RuntimeScheduler import com.facebook.soloader.SoLoader /** @@ -16,8 +18,8 @@ import com.facebook.soloader.SoLoader * around preallocateShadowView, destroyUnmountedShadowNode, and the allocatedViewRegistry_ * lifecycle. */ -class FabricMountingManagerTestHelper private constructor(fabricUIManager: FabricUIManager) : - HybridClassBase() { +internal class FabricMountingManagerTestHelper +private constructor(fabricUIManager: FabricUIManager) : HybridClassBase() { init { initHybrid(fabricUIManager) @@ -57,6 +59,21 @@ class FabricMountingManagerTestHelper private constructor(fabricUIManager: Fabri external fun resetFeatureFlags() + // A FabricUIManagerBinding registered on the helper's FabricUIManager with these never runs + // JavaScript: its surfaces mount only what the helper commits. + external fun createRuntimeExecutor(): RuntimeExecutor + + external fun createRuntimeScheduler(): RuntimeScheduler + + external fun registerViewComponent(componentFactory: ComponentFactory) + + external fun commitViews(surfaceId: Int, tags: IntArray) + + /** Records the Delete mutations of every transaction pulled for the surface from now on. */ + external fun recordPulledDeletes(surfaceId: Int) + + external fun pulledDeleteTags(): IntArray + companion object { init { SoLoader.loadLibrary("fabricjni")