Revert changes to getters in Binding (#37589)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/37589

In D44221018, I changed this to return a ref to the smart pointer, and even made it a raw pointer in D45948987, but this is not thread-safe: the uninstallFabricUIManager happens on a different thread from the accesses to these pointers on the JS thread, so we actually need to use the shared pointer here to be safe.

Changelog: [Internal]

Reviewed By: sammy-SC

Differential Revision: D46221511

fbshipit-source-id: d8080042d6f1dbf5550a09b77f1090f78ec82455

packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.cpp

@@ -45,15 +45,17 @@ jni::local_ref<Binding::jhybriddata> Binding::initHybrid(
 }
 
 // Thread-safe getter
-const std::shared_ptr<Scheduler> &Binding::getScheduler() {
+std::shared_ptr<Scheduler> Binding::getScheduler() {
   std::shared_lock lock(installMutex_);
+  // Need to return a copy of the shared_ptr to make sure this is safe if called
+  // concurrently with uninstallFabricUIManager
   return scheduler_;
 }
 
 jni::local_ref<ReadableNativeMap::jhybridobject>
 Binding::getInspectorDataForInstance(
     jni::alias_ref<EventEmitterWrapper::javaobject> eventEmitterWrapper) {
-  auto &scheduler = getScheduler();
+  auto scheduler = getScheduler();
   if (!scheduler) {
     LOG(ERROR) << "Binding::startSurface: scheduler disappeared";
     return ReadableNativeMap::newObjectCxxArgs(folly::dynamic::object());
@@ -103,7 +105,7 @@ void Binding::startSurface(
     NativeMap *initialProps) {
   SystraceSection s("FabricUIManagerBinding::startSurface");
 
-  auto &scheduler = getScheduler();
+  auto scheduler = getScheduler();
   if (!scheduler) {
     LOG(ERROR) << "Binding::startSurface: scheduler disappeared";
     return;
@@ -131,7 +133,7 @@ void Binding::startSurface(
     surfaceHandlerRegistry_.emplace(surfaceId, std::move(surfaceHandler));
   }
 
-  auto *mountingManager = getMountingManager("startSurface");
+  auto mountingManager = getMountingManager("startSurface");
   if (!mountingManager) {
     return;
   }
@@ -158,7 +160,7 @@ void Binding::startSurfaceWithConstraints(
         << this << ", surfaceId: " << surfaceId << ").";
   }
 
-  auto &scheduler = getScheduler();
+  auto scheduler = getScheduler();
   if (!scheduler) {
     LOG(ERROR) << "Binding::startSurfaceWithConstraints: scheduler disappeared";
     return;
@@ -201,7 +203,7 @@ void Binding::startSurfaceWithConstraints(
     surfaceHandlerRegistry_.emplace(surfaceId, std::move(surfaceHandler));
   }
 
-  auto *mountingManager = getMountingManager("startSurfaceWithConstraints");
+  auto mountingManager = getMountingManager("startSurfaceWithConstraints");
   if (!mountingManager) {
     return;
   }
@@ -211,7 +213,7 @@ void Binding::startSurfaceWithConstraints(
 void Binding::renderTemplateToSurface(jint surfaceId, jstring uiTemplate) {
   SystraceSection s("FabricUIManagerBinding::renderTemplateToSurface");
 
-  auto &scheduler = getScheduler();
+  auto scheduler = getScheduler();
   if (!scheduler) {
     LOG(ERROR) << "Binding::renderTemplateToSurface: scheduler disappeared";
     return;
@@ -231,7 +233,7 @@ void Binding::stopSurface(jint surfaceId) {
                  << ", surfaceId: " << surfaceId << ").";
   }
 
-  auto &scheduler = getScheduler();
+  auto scheduler = getScheduler();
   if (!scheduler) {
     LOG(ERROR) << "Binding::stopSurface: scheduler disappeared";
     return;
@@ -253,7 +255,7 @@ void Binding::stopSurface(jint surfaceId) {
     scheduler->unregisterSurface(surfaceHandler);
   }
 
-  auto *mountingManager = getMountingManager("stopSurface");
+  auto mountingManager = getMountingManager("stopSurface");
   if (!mountingManager) {
     return;
   }
@@ -269,7 +271,7 @@ void Binding::registerSurface(SurfaceHandlerBinding *surfaceHandlerBinding) {
   }
   scheduler->registerSurface(surfaceHandler);
 
-  auto *mountingManager = getMountingManager("registerSurface");
+  auto mountingManager = getMountingManager("registerSurface");
   if (!mountingManager) {
     return;
   }
@@ -285,7 +287,7 @@ void Binding::unregisterSurface(SurfaceHandlerBinding *surfaceHandlerBinding) {
   }
   scheduler->unregisterSurface(surfaceHandler);
 
-  auto *mountingManager = getMountingManager("unregisterSurface");
+  auto mountingManager = getMountingManager("unregisterSurface");
   if (!mountingManager) {
     return;
   }
@@ -304,7 +306,7 @@ void Binding::setConstraints(
     jboolean doLeftAndRightSwapInRTL) {
   SystraceSection s("FabricUIManagerBinding::setConstraints");
 
-  auto &scheduler = getScheduler();
+  auto scheduler = getScheduler();
   if (!scheduler) {
     LOG(ERROR) << "Binding::setConstraints: scheduler disappeared";
     return;
@@ -368,7 +370,7 @@ void Binding::installFabricUIManager(
 
   auto globalJavaUiManager = make_global(javaUIManager);
   mountingManager_ =
-      std::make_unique<FabricMountingManager>(config, globalJavaUiManager);
+      std::make_shared<FabricMountingManager>(config, globalJavaUiManager);
 
   ContextContainer::Shared contextContainer =
       std::make_shared<ContextContainer>();
@@ -462,18 +464,21 @@ void Binding::uninstallFabricUIManager() {
   reactNativeConfig_ = nullptr;
 }
 
-FabricMountingManager *Binding::getMountingManager(const char *locationHint) {
+std::shared_ptr<FabricMountingManager> Binding::getMountingManager(
+    const char *locationHint) {
   std::shared_lock lock(installMutex_);
   if (!mountingManager_) {
     LOG(ERROR) << "FabricMountingManager::" << locationHint
                << " mounting manager disappeared";
   }
-  return mountingManager_.get();
+  // Need to return a copy of the shared_ptr to make sure this is safe if called
+  // concurrently with uninstallFabricUIManager
+  return mountingManager_;
 }
 
 void Binding::schedulerDidFinishTransaction(
     const MountingCoordinator::Shared &mountingCoordinator) {
-  auto *mountingManager = getMountingManager("schedulerDidFinishTransaction");
+  auto mountingManager = getMountingManager("schedulerDidFinishTransaction");
   if (!mountingManager) {
     return;
   }
@@ -492,7 +497,7 @@ void Binding::schedulerDidRequestPreliminaryViewAllocation(
     return;
   }
 
-  auto *mountingManager = getMountingManager("preallocateView");
+  auto mountingManager = getMountingManager("preallocateView");
   if (!mountingManager) {
     return;
   }
@@ -503,7 +508,7 @@ void Binding::schedulerDidDispatchCommand(
     const ShadowView &shadowView,
     std::string const &commandName,
     folly::dynamic const &args) {
-  auto *mountingManager = getMountingManager("schedulerDidDispatchCommand");
+  auto mountingManager = getMountingManager("schedulerDidDispatchCommand");
   if (!mountingManager) {
     return;
   }
@@ -513,7 +518,7 @@ void Binding::schedulerDidDispatchCommand(
 void Binding::schedulerDidSendAccessibilityEvent(
     const ShadowView &shadowView,
     std::string const &eventType) {
-  auto *mountingManager =
+  auto mountingManager =
       getMountingManager("schedulerDidSendAccessibilityEvent");
   if (!mountingManager) {
     return;
@@ -525,7 +530,7 @@ void Binding::schedulerDidSetIsJSResponder(
     ShadowView const &shadowView,
     bool isJSResponder,
     bool blockNativeResponder) {
-  auto *mountingManager = getMountingManager("schedulerDidSetIsJSResponder");
+  auto mountingManager = getMountingManager("schedulerDidSetIsJSResponder");
   if (!mountingManager) {
     return;
   }
@@ -534,7 +539,7 @@ void Binding::schedulerDidSetIsJSResponder(
 }
 
 void Binding::onAnimationStarted() {
-  auto *mountingManager = getMountingManager("onAnimationStarted");
+  auto mountingManager = getMountingManager("onAnimationStarted");
   if (!mountingManager) {
     return;
   }
@@ -542,7 +547,7 @@ void Binding::onAnimationStarted() {
 }
 
 void Binding::onAllAnimationsComplete() {
-  auto *mountingManager = getMountingManager("onAnimationComplete");
+  auto mountingManager = getMountingManager("onAnimationComplete");
   if (!mountingManager) {
     return;
   }

packages/react-native/ReactAndroid/src/main/jni/react/fabric/Binding.h

@@ -42,7 +42,7 @@ class Binding : public jni::HybridClass<Binding>,
 
   static void registerNatives();
 
-  const std::shared_ptr<Scheduler> &getScheduler();
+  std::shared_ptr<Scheduler> getScheduler();
 
  private:
   void setConstraints(
@@ -124,10 +124,11 @@ class Binding : public jni::HybridClass<Binding>,
 
   // Private member variables
   std::shared_mutex installMutex_;
-  std::unique_ptr<FabricMountingManager> mountingManager_;
+  std::shared_ptr<FabricMountingManager> mountingManager_;
   std::shared_ptr<Scheduler> scheduler_;
 
-  FabricMountingManager *getMountingManager(const char *locationHint);
+  std::shared_ptr<FabricMountingManager> getMountingManager(
+      const char *locationHint);
 
   // LayoutAnimations
   void onAnimationStarted() override;