Cancel pending Fabric surface starts (#58977)

Summary:
Pull Request resolved: #58977

Surface startup is deferred to the main queue. A stop request that arrived before the deferred work previously returned without cancelling it, allowing the surface to start after its owner began tearing down.

Track pending starts under the surface mutex so stop can cancel them before registration and startup. Keep attachment and startup serialized, and notify stage changes after releasing the mutex.

Changelog:
[iOS][Fixed] - Prevent Fabric surfaces from starting after an earlier stop request

Reviewed By: christophpurrer

Differential Revision: D124262921

fbshipit-source-id: 59d1f8af2900d204df06106b02b92f68356ab7b2

Author: Abbondanzo

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