From 2cf9b5c8913b3a92df39860613b7ef3a451e401e Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Thu, 9 Oct 2025 12:19:40 -0700 Subject: [PATCH] Remove move constructor from SurfaceHandler Summary: Removed SurfaceHandler's move constructor, to avoid any potential issues with referencing moved-from instances. Changelog: [Internal] Reviewed By: mdvacca Differential Revision: D83977790 --- .../React/Fabric/Surface/RCTFabricSurface.mm | 2 +- .../react/fabric/FabricUIManagerBinding.cpp | 62 +++++++++++-------- .../renderer/scheduler/SurfaceHandler.cpp | 20 ------ .../react/renderer/scheduler/SurfaceHandler.h | 6 +- .../renderer/scheduler/SurfaceManager.cpp | 5 +- 5 files changed, 42 insertions(+), 53 deletions(-) diff --git a/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm b/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm index 1382f13e9ac1bb..cf932b52bf00c5 100644 --- a/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm +++ b/packages/react-native/React/Fabric/Surface/RCTFabricSurface.mm @@ -57,7 +57,7 @@ - (instancetype)initWithSurfacePresenter:(RCTSurfacePresenter *)surfacePresenter if (self = [super init]) { _surfacePresenter = surfacePresenter; - _surfaceHandler = SurfaceHandler{RCTStringFromNSString(moduleName), getNextRootViewTag()}; + _surfaceHandler.emplace(RCTStringFromNSString(moduleName), getNextRootViewTag()); _surfaceHandler->setProps(convertIdToFollyDynamic(initialProperties)); [_surfacePresenter registerSurface:self]; diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp index 801c8f1fd6017a..cf813835bc954e 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp @@ -166,34 +166,39 @@ void FabricUIManagerBinding::startSurface( return; } - auto layoutContext = LayoutContext{}; - layoutContext.pointScaleFactor = pointScaleFactor_; + SurfaceHandler* surfaceHandler = nullptr; + { + std::unique_lock lock(surfaceHandlerRegistryMutex_); + auto [it, _] = surfaceHandlerRegistry_.try_emplace( + surfaceId, + std::in_place_index<0>, + moduleName->toStdString(), + surfaceId); + surfaceHandler = &std::get(it->second); + } - auto surfaceHandler = SurfaceHandler{moduleName->toStdString(), surfaceId}; - surfaceHandler.setContextContainer(scheduler->getContextContainer()); + surfaceHandler->setContextContainer(scheduler->getContextContainer()); if (initialProps != nullptr) { - surfaceHandler.setProps(initialProps->consume()); + surfaceHandler->setProps(initialProps->consume()); } - surfaceHandler.constraintLayout({}, layoutContext); - scheduler->registerSurface(surfaceHandler); + auto layoutContext = LayoutContext{}; + layoutContext.pointScaleFactor = pointScaleFactor_; + surfaceHandler->constraintLayout({}, layoutContext); + + scheduler->registerSurface(*surfaceHandler); auto mountingManager = getMountingManager("startSurface"); if (mountingManager != nullptr) { mountingManager->onSurfaceStart(surfaceId); } - surfaceHandler.start(); + surfaceHandler->start(); if (ReactNativeFeatureFlags::enableLayoutAnimationsOnAndroid()) { - surfaceHandler.getMountingCoordinator()->setMountingOverrideDelegate( + surfaceHandler->getMountingCoordinator()->setMountingOverrideDelegate( animationDriver_); } - - { - std::unique_lock lock(surfaceHandlerRegistryMutex_); - surfaceHandlerRegistry_.emplace(surfaceId, std::move(surfaceHandler)); - } } jint FabricUIManagerBinding::findNextFocusableElement( @@ -338,31 +343,36 @@ void FabricUIManagerBinding::startSurfaceWithConstraints( constraints.layoutDirection = isRTL != 0 ? LayoutDirection::RightToLeft : LayoutDirection::LeftToRight; - auto surfaceHandler = SurfaceHandler{moduleName->toStdString(), surfaceId}; - surfaceHandler.setContextContainer(scheduler->getContextContainer()); + SurfaceHandler* surfaceHandler = nullptr; + { + std::unique_lock lock(surfaceHandlerRegistryMutex_); + auto [it, _] = surfaceHandlerRegistry_.try_emplace( + surfaceId, + std::in_place_index<0>, + moduleName->toStdString(), + surfaceId); + surfaceHandler = &std::get(it->second); + } + + surfaceHandler->setContextContainer(scheduler->getContextContainer()); if (initialProps != nullptr) { - surfaceHandler.setProps(initialProps->consume()); + surfaceHandler->setProps(initialProps->consume()); } - surfaceHandler.constraintLayout(constraints, context); + surfaceHandler->constraintLayout(constraints, context); - scheduler->registerSurface(surfaceHandler); + scheduler->registerSurface(*surfaceHandler); auto mountingManager = getMountingManager("startSurfaceWithConstraints"); if (mountingManager != nullptr) { mountingManager->onSurfaceStart(surfaceId); } - surfaceHandler.start(); + surfaceHandler->start(); if (ReactNativeFeatureFlags::enableLayoutAnimationsOnAndroid()) { - surfaceHandler.getMountingCoordinator()->setMountingOverrideDelegate( + surfaceHandler->getMountingCoordinator()->setMountingOverrideDelegate( animationDriver_); } - - { - std::unique_lock lock(surfaceHandlerRegistryMutex_); - surfaceHandlerRegistry_.emplace(surfaceId, std::move(surfaceHandler)); - } } // Used by non-bridgeless+Fabric diff --git a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp index 976454fb66589c..56505218703099 100644 --- a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp +++ b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.cpp @@ -23,26 +23,6 @@ SurfaceHandler::SurfaceHandler( parameters_.surfaceId = surfaceId; } -SurfaceHandler::SurfaceHandler(SurfaceHandler&& other) noexcept { - operator=(std::move(other)); -} - -SurfaceHandler& SurfaceHandler::operator=(SurfaceHandler&& other) noexcept { - std::unique_lock lock1(linkMutex_, std::defer_lock); - std::unique_lock lock2(parametersMutex_, std::defer_lock); - std::unique_lock lock3(other.linkMutex_, std::defer_lock); - std::unique_lock lock4(other.parametersMutex_, std::defer_lock); - std::lock(lock1, lock2, lock3, lock4); - - link_ = other.link_; - parameters_ = other.parameters_; - - other.link_ = Link{}; - other.parameters_ = Parameters{}; - other.parameters_.contextContainer = parameters_.contextContainer; - return *this; -} - #pragma mark - Surface Life-Cycle Management void SurfaceHandler::setContextContainer( diff --git a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.h b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.h index 9a4f0195eb0580..c22abaea8827d1 100644 --- a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.h +++ b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceHandler.h @@ -67,11 +67,11 @@ class SurfaceHandler { virtual ~SurfaceHandler() noexcept; /* - * Movable-only. + * Not moveable or copyable */ - SurfaceHandler(SurfaceHandler&& other) noexcept; + SurfaceHandler(SurfaceHandler&& other) noexcept = delete; SurfaceHandler(const SurfaceHandler& SurfaceHandler) noexcept = delete; - SurfaceHandler& operator=(SurfaceHandler&& other) noexcept; + SurfaceHandler& operator=(SurfaceHandler&& other) noexcept = delete; SurfaceHandler& operator=(const SurfaceHandler& other) noexcept = delete; #pragma mark - Surface Life-Cycle Management diff --git a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceManager.cpp b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceManager.cpp index 52e11a7c5fa566..8547f401bd4386 100644 --- a/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceManager.cpp +++ b/packages/react-native/ReactCommon/react/renderer/scheduler/SurfaceManager.cpp @@ -29,9 +29,8 @@ void SurfaceManager::startSurface( const LayoutContext& layoutContext) noexcept { { std::unique_lock lock(mutex_); - auto surfaceHandler = SurfaceHandler{moduleName, surfaceId}; - surfaceHandler.setContextContainer(scheduler_.getContextContainer()); - registry_.emplace(surfaceId, std::move(surfaceHandler)); + auto [it, _] = registry_.try_emplace(surfaceId, moduleName, surfaceId); + it->second.setContextContainer(scheduler_.getContextContainer()); } visit(surfaceId, [&](const SurfaceHandler& surfaceHandler) {