diff --git a/.github/scatterlab/allowed-tarball-diff.txt b/.github/scatterlab/allowed-tarball-diff.txt index 77ce3ae7f323..bfd0420bf680 100644 --- a/.github/scatterlab/allowed-tarball-diff.txt +++ b/.github/scatterlab/allowed-tarball-diff.txt @@ -55,3 +55,15 @@ ReactAndroid/src/main/java/com/facebook/react/views/text/TextDecorationStyle.kt # Points the consumer's Gradle build at this fork's Android artifacts. Added file, so the # gate sees it as a difference from upstream. See android-prebuilt.md. scripts/android/scatterlab-prebuilt-maven.gradle + +# A second SurfaceHandler::start() on a running surface hands ShadowTreeRegistry::add() +# another tree for the same SurfaceId; the registry drops it and `link_.shadowTree` is +# left dangling, so the next MountingCoordinator lookup crashes (iOS: SIGSEGV at 0x8 in +# MountingCoordinator::setMountingOverrideDelegate). -[RCTFabricSurface start] checks +# the status two async hops before start() runs, so racing callers get there twice. +# start() now ignores a surface that is not Registered, and the iOS surface keeps a +# second start out while one is in flight. Upstream +# https://github.com/react/react-native/pull/57404 (open) covers the unregistered case +# on the iOS side only. C++ reaches Android only through this fork's Android prebuilt. +ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp +React/Fabric/Surface/RCTFabricSurface.mm diff --git a/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm b/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm index 1a96fef72ae2..1593f039505b 100644 --- a/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm +++ b/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm @@ -7,6 +7,7 @@ #import "RCTFabricSurface.h" +#import #import #import @@ -43,6 +44,11 @@ @implementation RCTFabricSurface { // and we need this mutex to prevent races. std::mutex _surfaceMutex; + // `start` returns before `SurfaceHandler::start()` runs (it hops to the main queue, then to a global one), and + // until then the status still reads `Registered`. This keeps a second `start` in that window from attaching the + // root view and starting the surface again. + std::atomic_bool _startInFlight; + // Can be accessed from the main thread only. RCTSurfaceView *_Nullable _view; #if !TARGET_OS_TV @@ -95,7 +101,12 @@ - (void)start { std::lock_guard lock(_surfaceMutex); + // Take the flag before reading the status, so a start that finished in between reads as `Running` here. + if (_startInFlight.exchange(true)) { + return; + } if (_surfaceHandler->getStatus() != SurfaceHandler::Status::Registered) { + _startInFlight = false; return; } @@ -106,9 +117,15 @@ - (void)start surfaceId:self->_surfaceHandler->getSurfaceId()]; dispatch_async(dispatch_get_global_queue(QOS_CLASS_USER_INTERACTIVE, 0), ^{ self->_surfaceHandler->start(); + auto status = self->_surfaceHandler->getStatus(); + self->_startInFlight = false; [self _propagateStageChange]; - [self->_surfacePresenter setupAnimationDriverWithSurfaceHandler:*self->_surfaceHandler]; + // `start()` is a no-op if the surface got unregistered in the meantime (e.g. by an instance teardown), and then + // there is no ShadowTree to take a MountingCoordinator from. + if (status == SurfaceHandler::Status::Running) { + [self->_surfacePresenter setupAnimationDriverWithSurfaceHandler:*self->_surfaceHandler]; + } }); }); } diff --git a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp index 930472f096e9..5c9ed2c2db81 100644 --- a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp +++ b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp @@ -8,6 +8,7 @@ #include "SurfaceHandler.h" #include +#include #include #include @@ -37,8 +38,17 @@ Status SurfaceHandler::getStatus() const noexcept { void SurfaceHandler::start() const noexcept { TraceSection s("SurfaceHandler::start"); std::unique_lock lock(linkMutex_); - react_native_assert( - link_.status == Status::Registered && "Surface must be registered."); + // Callers can race into a second start (-[RCTFabricSurface start] checks the + // status two async hops before this runs). Going on would hand + // ShadowTreeRegistry::add() a second tree for the same SurfaceId, which it + // drops, leaving `link_.shadowTree` dangling; an unregistered surface has no + // `link_.uiManager` at all. So this is a no-op rather than an assert. + if (link_.status != Status::Registered) { + LOG(WARNING) + << "SurfaceHandler::start ignored for a surface that is not in Registered state, surfaceId = " + << getSurfaceId() << ", status = " << static_cast(link_.status); + return; + } react_native_assert( getLayoutConstraints().layoutDirection != LayoutDirection::Undefined && "layoutDirection must be set.");