Pull the stop transaction of a surface in the pull model

In the pull model a commit only notifies the UI thread, and
pullAndExecuteTransaction pulls only surfaces found in the binding's surface
registry. stopSurfaceWithSurfaceHandler removes the surface from the registry
before SurfaceHandler::stop() commits its empty tree, so nothing pulled that
transaction. Mounting override delegates such as LayoutAnimations and C++
Animated never saw its Deletes. The pull cannot happen after stop(): the
shadow tree is destroyed at its end, which revokes the MountingCoordinator.

schedulerShouldRenderTransactions now pulls and executes a commit of an
unregistered surface on the commit thread, as the push model does with
accumulated props. Registered surfaces are pulled on the UI thread as before.

That check reads the registry inside the commit, so stopSurface(int) no longer
holds the registry lock across stop(): it takes the surface out of the
registry first, as stopSurfaceWithSurfaceHandler does.

startSurfaceWithSurfaceHandler (bridgeless) now adds the surface to the
registry before SurfaceHandler::start(), as startSurface and
startSurfaceWithConstraints already do. Otherwise a commit made while the
surface starts would be pulled on the commit thread and queued, and the UI
thread's pull of the next commit would mount ahead of it. Now only a stopping
surface commits outside the registry.

So a registered surface may not be running yet, e.g. a restarted surface, which
keeps its id, with a pull or mount report left from its previous run.
pullAndExecuteTransaction and reportMount now skip a surface that is not
running.

native_startSurfaceWithSurfaceHandler_pullModel_mountsCommitDuringStartInOrder
commits from another thread in an on-surface-start callback while the start
waits, then commits again on the UI thread, and checks the mounted order. It
also passes without this change; it fails, with the views mounted as [42, 43],
if the surface is added to the registry after start().

Author: bartlomiejbloniarz

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

@@ -35,6 +35,33 @@
 
 namespace facebook::react {
 
+namespace {
+
+std::shared_ptr<const MountingCoordinator> getRunningMountingCoordinator(
+    const std::variant<
+        SurfaceHandler,
+        jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>& registryEntry) {
+  const auto* surfaceHandler = std::get_if<SurfaceHandler>(&registryEntry);
+  jni::local_ref<SurfaceHandlerBinding::jhybridobject> javaSurfaceHandler;
+  if (surfaceHandler == nullptr) {
+    javaSurfaceHandler =
+        std::get<jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>(
+            registryEntry)
+            .lockLocal();
+    if (!javaSurfaceHandler) {
+      return nullptr;
+    }
+    surfaceHandler = &javaSurfaceHandler->cthis()->getSurfaceHandler();
+  }
+  // Callers hold the registry lock, and a surface leaves it before it stops.
+  if (surfaceHandler->getStatus() != SurfaceHandler::Status::Running) {
+    return nullptr;
+  }
+  return surfaceHandler->getMountingCoordinator();
+}
+
+} // namespace
+
 void FabricUIManagerBinding::initHybrid(jni::alias_ref<jhybridobject> jobj) {
   setCxxInstance(jobj);
 }
@@ -89,22 +116,10 @@ void FabricUIManagerBinding::reportMount(SurfaceId surfaceId) {
     std::shared_lock lock(surfaceHandlerRegistryMutex_);
     auto iterator = surfaceHandlerRegistry_.find(surfaceId);
     if (iterator != surfaceHandlerRegistry_.end()) {
-      const auto* surfaceHandler =
-          std::get_if<SurfaceHandler>(&iterator->second);
-      if (surfaceHandler == nullptr) {
-        auto javaSurfaceHandler =
-            std::get<jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>(
-                iterator->second)
-                .lockLocal();
-        if (javaSurfaceHandler) {
-          surfaceHandler = &javaSurfaceHandler->cthis()->getSurfaceHandler();
-        }
-      }
-      if (surfaceHandler != nullptr) {
-        auto mountingCoordinator = surfaceHandler->getMountingCoordinator();
-        if (mountingCoordinator != nullptr) {
-          mountingCoordinator->didPerformAsyncTransactions();
-        }
+      auto mountingCoordinator =
+          getRunningMountingCoordinator(iterator->second);
+      if (mountingCoordinator != nullptr) {
+        mountingCoordinator->didPerformAsyncTransactions();
       }
     } else {
       LOG(ERROR) << "FabricUIManagerBinding::reportMount: Surface with id "
@@ -130,20 +145,7 @@ void FabricUIManagerBinding::pullAndExecuteTransaction(SurfaceId surfaceId) {
     if (iterator == surfaceHandlerRegistry_.end()) {
       return;
     }
-    const auto* surfaceHandler = std::get_if<SurfaceHandler>(&iterator->second);
-    jni::local_ref<SurfaceHandlerBinding::jhybridobject> javaSurfaceHandler;
-    if (surfaceHandler == nullptr) {
-      javaSurfaceHandler =
-          std::get<jni::weak_ref<SurfaceHandlerBinding::jhybridobject>>(
-              iterator->second)
-              .lockLocal();
-      if (javaSurfaceHandler) {
-        surfaceHandler = &javaSurfaceHandler->cthis()->getSurfaceHandler();
-      }
-    }
-    if (surfaceHandler != nullptr) {
-      mountingCoordinator = surfaceHandler->getMountingCoordinator();
-    }
+    mountingCoordinator = getRunningMountingCoordinator(iterator->second);
   }
 
   if (mountingCoordinator == nullptr) {
@@ -169,6 +171,11 @@ void FabricUIManagerBinding::pullAndExecuteTransaction(SurfaceId surfaceId) {
   }
 }
 
+bool FabricUIManagerBinding::isSurfaceRegistered(SurfaceId surfaceId) {
+  std::shared_lock lock(surfaceHandlerRegistryMutex_);
+  return surfaceHandlerRegistry_.contains(surfaceId);
+}
+
 #pragma mark - Surface management
 
 // Used by bridgeless
@@ -195,6 +202,13 @@ void FabricUIManagerBinding::startSurfaceWithSurfaceHandler(
         << "FabricUIManagerBinding::startSurfaceWithSurfaceHandler: scheduler disappeared";
     return;
   }
+
+  {
+    std::unique_lock lock(surfaceHandlerRegistryMutex_);
+    surfaceHandlerRegistry_.emplace(
+        surfaceId, jni::make_weak(surfaceHandlerBinding));
+  }
+
   scheduler->registerSurface(surfaceHandler);
 
   auto mountingManager = getMountingManager("startSurfaceWithSurfaceHandler");
@@ -208,12 +222,6 @@ void FabricUIManagerBinding::startSurfaceWithSurfaceHandler(
     surfaceHandler.getMountingCoordinator()->setMountingOverrideDelegate(
         animationDriver_);
   }
-
-  {
-    std::unique_lock lock(surfaceHandlerRegistryMutex_);
-    surfaceHandlerRegistry_.emplace(
-        surfaceId, jni::make_weak(surfaceHandlerBinding));
-  }
 }
 
 // Used by non-bridgeless+Fabric
@@ -459,23 +467,25 @@ void FabricUIManagerBinding::stopSurface(jint surfaceId) {
     return;
   }
 
+  // The surface leaves the registry before it stops: the stop commit reads the
+  // registry on this thread.
+  decltype(surfaceHandlerRegistry_)::node_type registryEntry;
   {
     std::unique_lock lock(surfaceHandlerRegistryMutex_);
-    auto iterator = surfaceHandlerRegistry_.find(surfaceId);
-    if (iterator == surfaceHandlerRegistry_.end()) {
-      LOG(ERROR)
-          << "FabricUIManagerBinding::stopSurface: Surface with given id is not found";
-      return;
-    }
+    registryEntry = surfaceHandlerRegistry_.extract(surfaceId);
+  }
+  if (registryEntry.empty()) {
+    LOG(ERROR)
+        << "FabricUIManagerBinding::stopSurface: Surface with given id is not found";
+    return;
+  }
 
-    auto* surfaceHandler = std::get_if<SurfaceHandler>(&iterator->second);
-    if (surfaceHandler != nullptr) {
-      surfaceHandler->stop();
-      scheduler->unregisterSurface(*surfaceHandler);
-    } else {
-      LOG(ERROR) << "Java-owned SurfaceHandler found in stopSurface";
-    }
-    surfaceHandlerRegistry_.erase(iterator);
+  auto* surfaceHandler = std::get_if<SurfaceHandler>(&registryEntry.mapped());
+  if (surfaceHandler != nullptr) {
+    surfaceHandler->stop();
+    scheduler->unregisterSurface(*surfaceHandler);
+  } else {
+    LOG(ERROR) << "Java-owned SurfaceHandler found in stopSurface";
   }
 
   auto mountingManager = getMountingManager("stopSurface");
@@ -727,7 +737,11 @@ void FabricUIManagerBinding::schedulerShouldRenderTransactions(
     }
   }
 
-  if (ReactNativeFeatureFlags::enableMountingCoordinatorPullModelAndroid()) {
+  // The UI thread pulls only a registered surface, so a commit of a surface
+  // outside the registry, such as the commit that stops it, is pulled below,
+  // on the commit thread.
+  if (ReactNativeFeatureFlags::enableMountingCoordinatorPullModelAndroid() &&
+      isSurfaceRegistered(mountingCoordinator->getSurfaceId())) {
     // 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 calls back into `pullAndExecuteTransaction` at its next mount

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

@@ -137,6 +137,8 @@ class FabricUIManagerBinding : public jni::HybridClass<FabricUIManagerBinding>,
 
   void pullAndExecuteTransaction(SurfaceId surfaceId);
 
+  bool isSurfaceRegistered(SurfaceId surfaceId);
+
   jint findNextFocusableElement(jint parentTag, jint focusedTag, jint direction);
 
   jintArray getRelativeAncestorList(jint rootTag, jint childTag);

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

@@ -18,6 +18,8 @@
 #include <react/renderer/mounting/ShadowView.h>
 #include <react/renderer/scheduler/Scheduler.h>
 
+#include <thread>
+
 namespace facebook::react {
 
 class FabricMountingManagerTestHelper::DeleteRecorder
@@ -148,18 +150,38 @@ void FabricMountingManagerTestHelper::registerViewComponent(
       };
 }
 
+void FabricMountingManagerTestHelper::commitViews(
+    jint surfaceId,
+    jni::alias_ref<jni::JArrayInt> tags) {
+  commitViewTags(surfaceId, toTags(tags));
+}
+
+void FabricMountingManagerTestHelper::commitViewsOnSurfaceStart(
+    jni::alias_ref<jni::JArrayInt> tags) {
+  javaUIManager_->getBinding()
+      ->getScheduler()
+      ->getUIManager()
+      ->addOnSurfaceStartCallback(
+          [this, tags = toTags(tags)](const ShadowTree& shadowTree) {
+            std::thread([&] {
+              jni::ThreadScope::WithClassLoader(
+                  [&] { commitViewTags(shadowTree.getSurfaceId(), tags); });
+            }).join();
+          });
+}
+
 void FabricMountingManagerTestHelper::stopSurfaceHandler(
     jni::alias_ref<SurfaceHandlerBinding::jhybridobject>
         surfaceHandlerBinding) {
   surfaceHandlerBinding->cthis()->getSurfaceHandler().stop();
 }
 
-void FabricMountingManagerTestHelper::commitViews(
-    jint surfaceId,
-    jni::alias_ref<jni::JArrayInt> tags) {
+void FabricMountingManagerTestHelper::commitViewTags(
+    SurfaceId surfaceId,
+    const std::vector<Tag>& tags) {
   auto children =
       std::make_shared<std::vector<std::shared_ptr<const ShadowNode>>>();
-  for (auto tag : toTags(tags)) {
+  for (auto tag : tags) {
     auto props = std::make_shared<ViewProps>();
     props->collapsable = false;
     children->push_back(viewComponentDescriptor_->createShadowNode(
@@ -233,6 +255,9 @@ void FabricMountingManagerTestHelper::registerNatives() {
           FabricMountingManagerTestHelper::registerViewComponent),
       makeNativeMethod(
           "commitViews", FabricMountingManagerTestHelper::commitViews),
+      makeNativeMethod(
+          "commitViewsOnSurfaceStart",
+          FabricMountingManagerTestHelper::commitViewsOnSurfaceStart),
       makeNativeMethod(
           "stopSurfaceHandler",
           FabricMountingManagerTestHelper::stopSurfaceHandler),

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

@@ -43,6 +43,7 @@ class FabricMountingManagerTestHelper : public jni::HybridClass<FabricMountingMa
   void registerViewComponent(ComponentFactory *componentFactory);
 
   void commitViews(jint surfaceId, jni::alias_ref<jni::JArrayInt> tags);
+  void commitViewsOnSurfaceStart(jni::alias_ref<jni::JArrayInt> tags);
   void stopSurfaceHandler(jni::alias_ref<SurfaceHandlerBinding::jhybridobject> surfaceHandlerBinding);
   void recordPulledDeletes(jint surfaceId);
   jni::local_ref<jni::JArrayInt> pulledDeleteTags();
@@ -58,6 +59,8 @@ class FabricMountingManagerTestHelper : public jni::HybridClass<FabricMountingMa
       jni::alias_ref<jhybridobject> jobj,
       jni::alias_ref<JFabricUIManager::javaobject> jFabricUIManager);
 
+  void commitViewTags(SurfaceId surfaceId, const std::vector<Tag> &tags);
+
   jni::global_ref<JFabricUIManager::javaobject> javaUIManager_;
   std::shared_ptr<FabricMountingManager> mountingManager_;
   std::unique_ptr<ViewComponentDescriptor> viewComponentDescriptor_;

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

@@ -8,6 +8,7 @@
 package com.facebook.react.fabric
 
 import android.view.View
+import android.view.ViewGroup
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import androidx.test.platform.app.InstrumentationRegistry
 import com.facebook.react.ReactRootView
@@ -408,6 +409,27 @@ class FabricMountingManagerInstrumentationTest {
     return surfaceHandler
   }
 
+  /**
+   * A bridgeless surface can commit from the JavaScript thread before its start returns. That
+   * revision mounts before the next one.
+   */
+  @Test
+  fun native_startSurfaceWithSurfaceHandler_pullModel_mountsCommitDuringStartInOrder() {
+    overrideCxxFlags(pullModel = true)
+    val fabricUIManager = createFabricUIManager()
+    val helper = registerBinding(fabricUIManager)
+    helper.commitViewsOnSurfaceStart(intArrayOf(42))
+    var mountedTags = emptyList<Int>()
+    runOnUiThread {
+      val surfaceHandler = startSurfaceWithHandler(fabricUIManager)
+      helper.commitViews(surfaceHandler.surfaceId, intArrayOf(43, 42))
+      val rootView = fabricUIManager.resolveView(surfaceHandler.surfaceId) as ViewGroup
+      mountedTags = (0 until rootView.childCount).map { rootView.getChildAt(it).id }
+    }
+
+    assertThat(mountedTags).containsExactly(43, 42)
+  }
+
   /**
    * 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.

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

@@ -49,6 +49,12 @@ private constructor(fabricUIManager: FabricUIManager) : HybridClassBase() {
 
   external fun commitViews(surfaceId: Int, tags: IntArray)
 
+  /**
+   * Commits the views from another thread, as the JavaScript thread would, while a surface starts.
+   * The start waits for the commit.
+   */
+  external fun commitViewsOnSurfaceStart(tags: IntArray)
+
   /**
    * Stops the surface but leaves it in the FabricUIManagerBinding's registry, in the state a
    * registered surface has before it starts.