Cancel pending Fabric surface starts

packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm

@@ -37,11 +37,8 @@ @implementation RCTFabricSurface {
   // hence we wrap a value into `optional` to workaround it.
   std::optional<SurfaceHandler> _surfaceHandler;
 
-  // Protects Surface's start and stop processes.
-  // Even though SurfaceHandler is tread-safe, it will crash if we try to stop a surface that is not running.
-  // To make the API easy to use, we check the status of the surface before calling `start` or `stop`,
-  // and we need this mutex to prevent races.
   std::mutex _surfaceMutex;
+  BOOL _startPending;
 
   // Can be accessed from the main thread only.
   RCTSurfaceView *_Nullable _view;
@@ -93,36 +90,45 @@ - (void)dealloc
 
 - (void)start
 {
-  std::lock_guard<std::mutex> lock(_surfaceMutex);
+  {
+    std::lock_guard<std::mutex> lock(_surfaceMutex);
 
-  if (_surfaceHandler->getStatus() != SurfaceHandler::Status::Registered) {
-    return;
+    if (_startPending || _surfaceHandler->getStatus() != SurfaceHandler::Status::Registered) {
+      return;
+    }
+
+    _startPending = YES;
   }
 
-  // We need to register a root view component here synchronously because right after
-  // we start a surface, it can initiate an update that can query the root component.
   RCTExecuteOnMainQueue(^{
-    [self->_surfacePresenter.mountingManager attachSurfaceToView:self.view
-                                                       surfaceId:self->_surfaceHandler->getSurfaceId()];
-    dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INTERACTIVE, 0), ^{
-      // This runs two async hops later; a concurrent instance teardown/reload may have unregistered
-      // the surface since the check in -start. Without this re-check SurfaceHandler::start()
-      // dereferences a now-null uiManager.
-      if (self->_surfaceHandler->getStatus() != SurfaceHandler::Status::Registered) {
+    {
+      std::lock_guard<std::mutex> lock(self->_surfaceMutex);
+
+      if (!self->_startPending || self->_surfaceHandler->getStatus() != SurfaceHandler::Status::Registered) {
+        self->_startPending = NO;
         return;
       }
+
+      // We need to register a root view component here synchronously because right after
+      // we start a surface, it can initiate an update that can query the root component.
+      [self->_surfacePresenter.mountingManager attachSurfaceToView:self.view
+                                                         surfaceId:self->_surfaceHandler->getSurfaceId()];
       self->_surfaceHandler->start();
-      [self _propagateStageChange];
+      self->_startPending = NO;
 
       [self->_surfacePresenter setupAnimationDriverWithSurfaceHandler:*self->_surfaceHandler];
-    });
+    }
+
+    [self _propagateStageChange];
   });
 }
 
 - (void)stop
 {
   std::lock_guard<std::mutex> lock(_surfaceMutex);
 
+  _startPending = NO;
+
   if (_surfaceHandler->getStatus() != SurfaceHandler::Status::Running) {
     return;
   }

packages/react-native/React/Tests/Mounting/RCTFabricSurfaceLifecycleTests.mm

@@ -0,0 +1,82 @@
+/*
+ * 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.
+ */
+
+#import <React/RCTFabricSurface.h>
+#import <React/RCTSurfacePresenter.h>
+#import <React/RCTSurfaceView.h>
+#import <XCTest/XCTest.h>
+#import <react/renderer/runtimescheduler/RuntimeScheduler.h>
+#import <react/utils/ContextContainer.h>
+
+using facebook::react::ContextContainer;
+using facebook::react::RuntimeExecutor;
+using facebook::react::RuntimeScheduler;
+using facebook::react::RuntimeSchedulerKey;
+using facebook::react::SurfaceHandler;
+
+@interface RCTFabricSurfaceLifecycleTests : XCTestCase
+@end
+
+@implementation RCTFabricSurfaceLifecycleTests {
+  std::shared_ptr<RuntimeScheduler> _runtimeScheduler;
+  RCTSurfacePresenter *_surfacePresenter;
+}
+
+- (void)setUp
+{
+  [super setUp];
+
+  RuntimeExecutor runtimeExecutor = [](std::function<void(facebook::jsi::Runtime &)> &&) {};
+  _runtimeScheduler = std::make_shared<RuntimeScheduler>(runtimeExecutor);
+
+  auto contextContainer = std::make_shared<ContextContainer>();
+  contextContainer->insert(RuntimeSchedulerKey, std::weak_ptr<RuntimeScheduler>(_runtimeScheduler));
+
+  _surfacePresenter = [[RCTSurfacePresenter alloc] initWithContextContainer:contextContainer
+                                                            runtimeExecutor:runtimeExecutor
+                                                 bridgelessBindingsExecutor:std::nullopt];
+}
+
+- (void)tearDown
+{
+  [_surfacePresenter suspend];
+  _surfacePresenter = nil;
+  _runtimeScheduler.reset();
+
+  [super tearDown];
+}
+
+- (void)testStopCancelsPendingStart
+{
+  RCTFabricSurface *surface = [[RCTFabricSurface alloc] initWithSurfacePresenter:_surfacePresenter
+                                                                      moduleName:@""
+                                                               initialProperties:@{}];
+  XCTestExpectation *lifecycleSettled = [self expectationWithDescription:@"Deferred surface start settled"];
+  dispatch_queue_t lifecycleQueue = dispatch_queue_create("RCTFabricSurfaceLifecycleTests", DISPATCH_QUEUE_SERIAL);
+
+  XCTAssertTrue([NSThread isMainThread]);
+  dispatch_sync(lifecycleQueue, ^{
+    [surface start];
+    [surface stop];
+  });
+  dispatch_async(dispatch_get_main_queue(), ^{
+    [lifecycleSettled fulfill];
+  });
+
+  [self waitForExpectations:@[ lifecycleSettled ] timeout:2];
+
+  XCTAssertEqual(surface.surfaceHandler.getStatus(), SurfaceHandler::Status::Registered);
+  XCTAssertEqual(surface.view.subviews.count, 0u);
+
+  [surface start];
+  XCTAssertEqual(surface.surfaceHandler.getStatus(), SurfaceHandler::Status::Running);
+
+  [surface stop];
+  XCTAssertEqual(surface.surfaceHandler.getStatus(), SurfaceHandler::Status::Registered);
+}
+
+@end