[repro] Pull model: the stop transaction is never pulled, and a pull of a registered surface that is not running crashes

FabricMountingManagerInstrumentationTest gains tests that drive a real
FabricUIManagerBinding: the test helper supplies a RuntimeScheduler that never
runs JavaScript, a View-only component registry, and commits views to a
surface's shadow tree itself.

native_stopSurfaceWithSurfaceHandler_pullModel_pullsDeletes fails: the
transaction that stops a surface is never pulled, so mounting override
delegates (LayoutAnimations, C++ Animated) never see its Deletes. The surface
leaves the binding's registry before stop(), so the UI-thread pull finds
nothing. The push-model twin passes.

native_registeredSurfaceThatIsNotRunning_pullModel_skipsPullAndMountReport
crashes the process: pullAndExecuteTransaction and reportMount take the
MountingCoordinator of any registered surface, and a SurfaceHandler that is not
running has no shadow tree (SIGSEGV in ShadowTree::getMountingCoordinator()).
The helper's stopSurfaceHandler stops a handler that stays in the registry, the
state a registered surface has before it starts. The Kotlin helper class is
internal so that it can take a SurfaceHandlerBinding.

Author: bartlomiejbloniarz

packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.cpp

@@ -7,14 +7,54 @@
 
 #include "FabricMountingManagerTestHelper.h"
 
+#include <react/fabric/FabricUIManagerBinding.h>
 #include <react/renderer/animationbackend/AnimatedProps.h>
+#include <react/renderer/componentregistry/ComponentDescriptorProviderRegistry.h>
+#include <react/renderer/components/root/RootShadowNode.h>
 #include <react/renderer/components/view/ViewComponentDescriptor.h>
 #include <react/renderer/components/view/ViewProps.h>
 #include <react/renderer/core/ComponentDescriptor.h>
+#include <react/renderer/mounting/MountingOverrideDelegate.h>
 #include <react/renderer/mounting/ShadowView.h>
+#include <react/renderer/scheduler/Scheduler.h>
 
 namespace facebook::react {
 
+class FabricMountingManagerTestHelper::DeleteRecorder
+    : public MountingOverrideDelegate {
+ public:
+  bool shouldOverridePullTransaction() const override {
+    return true;
+  }
+
+  std::optional<MountingTransaction> 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<Tag> deletedTags;
+};
+
+namespace {
+
+void runNoJavaScript(std::function<void(jsi::Runtime& runtime)>&& /*unused*/) {}
+
+std::vector<Tag> toTags(jni::alias_ref<jni::JArrayInt> tags) {
+  auto region = tags->getRegion(0, static_cast<jsize>(tags->size()));
+  return {region.get(), region.get() + tags->size()};
+}
+
+} // namespace
+
 FabricMountingManagerTestHelper::FabricMountingManagerTestHelper(
     jni::alias_ref<JFabricUIManager::javaobject> jFabricUIManager)
     : javaUIManager_(jni::make_global(jFabricUIManager)) {
@@ -81,6 +121,89 @@ void FabricMountingManagerTestHelper::synchronouslyUpdateAnimatedProps(
   mountingManager_->synchronouslyUpdateAnimatedProps(updates);
 }
 
+jni::local_ref<JRuntimeExecutor::jhybridobject>
+FabricMountingManagerTestHelper::createRuntimeExecutor() {
+  return JRuntimeExecutor::newObjectCxxArgs(runNoJavaScript);
+}
+
+jni::local_ref<JRuntimeScheduler::jhybridobject>
+FabricMountingManagerTestHelper::createRuntimeScheduler() {
+  runtimeScheduler_ = std::make_shared<RuntimeScheduler>(runNoJavaScript);
+  return JRuntimeScheduler::newObjectCxxArgs(runtimeScheduler_);
+}
+
+void FabricMountingManagerTestHelper::registerViewComponent(
+    ComponentFactory* componentFactory) {
+  componentFactory->buildRegistryFunction =
+      [](const EventDispatcher::Weak& eventDispatcher,
+         const std::shared_ptr<const ContextContainer>& contextContainer) {
+        auto providerRegistry =
+            std::make_shared<ComponentDescriptorProviderRegistry>();
+        providerRegistry->add(
+            concreteComponentDescriptorProvider<ViewComponentDescriptor>());
+        return providerRegistry->createComponentDescriptorRegistry(
+            {.eventDispatcher = eventDispatcher,
+             .contextContainer = contextContainer,
+             .flavor = nullptr});
+      };
+}
+
+void FabricMountingManagerTestHelper::stopSurfaceHandler(
+    jni::alias_ref<SurfaceHandlerBinding::jhybridobject>
+        surfaceHandlerBinding) {
+  surfaceHandlerBinding->cthis()->getSurfaceHandler().stop();
+}
+
+void FabricMountingManagerTestHelper::commitViews(
+    jint surfaceId,
+    jni::alias_ref<jni::JArrayInt> tags) {
+  auto children =
+      std::make_shared<std::vector<std::shared_ptr<const ShadowNode>>>();
+  for (auto tag : toTags(tags)) {
+    auto props = std::make_shared<ViewProps>();
+    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<RootShadowNode>(
+                  oldRootShadowNode,
+                  ShadowNodeFragment{
+                      .props = ShadowNodeFragment::propsPlaceholder(),
+                      .children = children});
+            },
+            {});
+      });
+}
+
+void FabricMountingManagerTestHelper::recordPulledDeletes(jint surfaceId) {
+  deleteRecorder_ = std::make_shared<DeleteRecorder>();
+  javaUIManager_->getBinding()
+      ->getScheduler()
+      ->getUIManager()
+      ->getShadowTreeRegistry()
+      .visit(surfaceId, [&](const ShadowTree& shadowTree) {
+        shadowTree.getMountingCoordinator()->setMountingOverrideDelegate(
+            deleteRecorder_);
+      });
+}
+
+jni::local_ref<jni::JArrayInt>
+FabricMountingManagerTestHelper::pulledDeleteTags() {
+  const auto& tags = deleteRecorder_->deletedTags;
+  auto array = jni::JArrayInt::newArray(tags.size());
+  array->setRegion(0, static_cast<jsize>(tags.size()), tags.data());
+  return array;
+}
+
 void FabricMountingManagerTestHelper::registerNatives() {
   registerHybrid({
       makeNativeMethod(
@@ -99,6 +222,26 @@ void FabricMountingManagerTestHelper::registerNatives() {
       makeNativeMethod(
           "synchronouslyUpdateAnimatedProps",
           FabricMountingManagerTestHelper::synchronouslyUpdateAnimatedProps),
+      makeNativeMethod(
+          "createRuntimeExecutor",
+          FabricMountingManagerTestHelper::createRuntimeExecutor),
+      makeNativeMethod(
+          "createRuntimeScheduler",
+          FabricMountingManagerTestHelper::createRuntimeScheduler),
+      makeNativeMethod(
+          "registerViewComponent",
+          FabricMountingManagerTestHelper::registerViewComponent),
+      makeNativeMethod(
+          "commitViews", FabricMountingManagerTestHelper::commitViews),
+      makeNativeMethod(
+          "stopSurfaceHandler",
+          FabricMountingManagerTestHelper::stopSurfaceHandler),
+      makeNativeMethod(
+          "recordPulledDeletes",
+          FabricMountingManagerTestHelper::recordPulledDeletes),
+      makeNativeMethod(
+          "pulledDeleteTags",
+          FabricMountingManagerTestHelper::pulledDeleteTags),
   });
 }
 

packages/react-native/ReactAndroid/src/main/jni/react/fabric/test_helper/FabricMountingManagerTestHelper.h

@@ -8,10 +8,14 @@
 #pragma once
 
 #include <fbjni/fbjni.h>
+#include <react/jni/JRuntimeExecutor.h>
+#include <react/jni/JRuntimeScheduler.h>
 #include <react/jni/NativeMap.h>
 #include <react/renderer/components/view/ViewComponentDescriptor.h>
 
+#include <react/fabric/ComponentFactory.h>
 #include <react/fabric/FabricMountingManager.h>
+#include <react/fabric/SurfaceHandlerBinding.h>
 
 namespace facebook::react {
 
@@ -31,6 +35,18 @@ class FabricMountingManagerTestHelper : public jni::HybridClass<FabricMountingMa
       jni::alias_ref<jni::JArrayInt> tags,
       jni::alias_ref<jni::JArrayClass<NativeMap::javaobject>> props);
 
+  // A FabricUIManagerBinding registered on the helper's FabricUIManager with
+  // these never runs JavaScript: its surfaces mount only what the helper
+  // commits.
+  jni::local_ref<JRuntimeExecutor::jhybridobject> createRuntimeExecutor();
+  jni::local_ref<JRuntimeScheduler::jhybridobject> createRuntimeScheduler();
+  void registerViewComponent(ComponentFactory *componentFactory);
+
+  void commitViews(jint surfaceId, jni::alias_ref<jni::JArrayInt> tags);
+  void stopSurfaceHandler(jni::alias_ref<SurfaceHandlerBinding::jhybridobject> surfaceHandlerBinding);
+  void recordPulledDeletes(jint surfaceId);
+  jni::local_ref<jni::JArrayInt> pulledDeleteTags();
+
   static void registerNatives();
 
  private:
@@ -45,6 +61,9 @@ class FabricMountingManagerTestHelper : public jni::HybridClass<FabricMountingMa
   jni::global_ref<JFabricUIManager::javaobject> javaUIManager_;
   std::shared_ptr<FabricMountingManager> mountingManager_;
   std::unique_ptr<ViewComponentDescriptor> viewComponentDescriptor_;
+  std::shared_ptr<RuntimeScheduler> runtimeScheduler_;
+  class DeleteRecorder;
+  std::shared_ptr<DeleteRecorder> deleteRecorder_;
 };
 
 } // namespace facebook::react

packages/react-native/ReactAndroid/src/test/java/com/facebook/react/fabric/FabricMountingManagerInstrumentationTest.kt

@@ -7,6 +7,7 @@
 
 package com.facebook.react.fabric
 
+import android.view.View
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import androidx.test.platform.app.InstrumentationRegistry
 import com.facebook.react.ReactRootView
@@ -16,11 +17,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
@@ -76,6 +81,7 @@ class FabricMountingManagerInstrumentationTest {
   @After
   fun tearDown() {
     ReactNativeFeatureFlags.dangerouslyReset()
+    ReactNativeFeatureFlagsCxxInterop.dangerouslyReset()
   }
 
   private fun startSurface() {
@@ -335,4 +341,121 @@ class FabricMountingManagerInstrumentationTest {
     assertThat(fallbackView.alpha).isEqualTo(0.5f)
     assertThat(fallbackView.tag).isEqualTo("animated-view")
   }
+
+  // ---- 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
+        }
+    )
+  }
+
+  private fun registerBinding(
+      fabricUIManager: FabricUIManager,
+      binding: FabricUIManagerBinding = FabricUIManagerBinding(),
+  ): FabricMountingManagerTestHelper {
+    val helper = FabricMountingManagerTestHelper.create(fabricUIManager)
+    val componentFactory = ComponentFactory()
+    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
+  }
+
+  /**
+   * A restarted bridgeless surface keeps its id and is registered before it starts, so a pull or a
+   * mount report left from its previous run can reach it before it runs. Both skip it.
+   */
+  @Test
+  fun native_registeredSurfaceThatIsNotRunning_pullModel_skipsPullAndMountReport() {
+    overrideCxxFlags(pullModel = true)
+    val fabricUIManager = createFabricUIManager()
+    val binding = FabricUIManagerBinding()
+    val helper = registerBinding(fabricUIManager, binding)
+    runOnUiThread {
+      val surfaceHandler = startSurfaceWithHandler(fabricUIManager)
+      helper.stopSurfaceHandler(surfaceHandler)
+      binding.pullAndExecuteTransaction(surfaceHandler.surfaceId)
+      binding.reportMount(surfaceHandler.surfaceId)
+    }
+  }
+
+  @Test
+  fun native_stopSurfaceWithSurfaceHandler_pullModel_pullsDeletes() {
+    assertStoppingSurfacePullsDeletes(pullModel = true)
+  }
+
+  @Test
+  fun native_stopSurfaceWithSurfaceHandler_pushModel_pullsDeletes() {
+    assertStoppingSurfacePullsDeletes(pullModel = false)
+  }
+
+  /**
+   * 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) {
+    overrideCxxFlags(pullModel)
+    val fabricUIManager = createFabricUIManager()
+    val helper = registerBinding(fabricUIManager)
+    lateinit var surfaceHandler: SurfaceHandlerBinding
+    var mounted = false
+    runOnUiThread {
+      surfaceHandler = startSurfaceWithHandler(fabricUIManager)
+      helper.recordPulledDeletes(surfaceHandler.surfaceId)
+      helper.commitViews(surfaceHandler.surfaceId, intArrayOf(42, 43))
+      mounted = fabricUIManager.resolveView(42) != null
+    }
+    assertThat(mounted).isTrue()
+
+    runOnUiThread { fabricUIManager.stopSurface(surfaceHandler) }
+
+    assertThat(helper.pulledDeleteTags()).containsExactlyInAnyOrder(42, 43)
+  }
 }