From 52153d131f4fbd114af9551395c9820da7359a48 Mon Sep 17 00:00:00 2001 From: Peter Abbondanzo Date: Fri, 9 Oct 2026 15:06:47 -0700 Subject: [PATCH] Cancel pending Fabric surface starts Summary: 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 Differential Revision: D124262921 --- .../React/Fabric/Surface/RCTFabricSurface.mm | 42 ++++++---- .../RCTFabricSurfaceLifecycleTests.mm | 82 +++++++++++++++++++ 2 files changed, 106 insertions(+), 18 deletions(-) create mode 100644 packages/react-native/React/Tests/Mounting/RCTFabricSurfaceLifecycleTests.mm diff --git a/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm b/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm index 22e2275f9c8e..b0bdfed944eb 100644 --- a/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm +++ b/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; - // 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,29 +90,36 @@ - (void)dealloc - (void)start { - std::lock_guard lock(_surfaceMutex); + { + std::lock_guard 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 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]; }); } @@ -123,6 +127,8 @@ - (void)stop { std::lock_guard lock(_surfaceMutex); + _startPending = NO; + if (_surfaceHandler->getStatus() != SurfaceHandler::Status::Running) { return; } diff --git a/packages/react-native/React/Tests/Mounting/RCTFabricSurfaceLifecycleTests.mm b/packages/react-native/React/Tests/Mounting/RCTFabricSurfaceLifecycleTests.mm new file mode 100644 index 000000000000..006453c1b6fd --- /dev/null +++ b/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 +#import +#import +#import +#import +#import + +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; + RCTSurfacePresenter *_surfacePresenter; +} + +- (void)setUp +{ + [super setUp]; + + RuntimeExecutor runtimeExecutor = [](std::function &&) {}; + _runtimeScheduler = std::make_shared(runtimeExecutor); + + auto contextContainer = std::make_shared(); + contextContainer->insert(RuntimeSchedulerKey, std::weak_ptr(_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