Allow non-static LongLivedObjectCollections in TurboModuleBinding (fa…

…cebook#42194) Summary: Apps that have multiple concurrently running React instances may suffer from issues where tearing down one instance affects the bindings / LongLivedObjectCollection instance of another due to the use of static getter for LongLivedObjectCollection. This should allow host platforms, e.g., react-native-windows (which still forks the TurboModuleBinding C++ files [here](https://github.com/microsoft/react-native-windows/tree/main/vnext/ReactCommon/TEMP_UntilReactCommonUpdate/react/nativemodule/core/ReactCommon) for the reasons already mentioned) to manage per instance LongLivedObjectCollections. ## Changelog [Internal] Differential Revision: D52581170
rozele · Jan 9, 2024 · 69a7e21 · 69a7e21
1 parent a8aa96c
commit 69a7e21
Show file tree

Hide file tree

Showing 4 changed files with 56 additions and 16 deletions.
diff --git a/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h b/packages/react-native/ReactCommon/react/bridging/CallbackWrapper.h
@@ -23,23 +23,40 @@ class CallbackWrapper : public LongLivedObject {
   CallbackWrapper(
       jsi::Function&& callback,
       jsi::Runtime& runtime,
-      std::shared_ptr<CallInvoker> jsInvoker)
+      std::shared_ptr<CallInvoker> jsInvoker,
+      std::shared_ptr<LongLivedObjectCollection> longLivedObjectCollection =
+          nullptr)
       : callback_(std::move(callback)),
         runtime_(runtime),
-        jsInvoker_(std::move(jsInvoker)) {}
+        jsInvoker_(std::move(jsInvoker)),
+        longLivedObjectCollection_(longLivedObjectCollection) {}
 
   jsi::Function callback_;
   jsi::Runtime& runtime_;
   std::shared_ptr<CallInvoker> jsInvoker_;
 
+  // Use a weak_ptr to avoid a retain cycle: LongLivedObjectCollection owns all
+  // CallbackWrappers. So, CallbackWrapper cannot own its
+  // LongLivedObjectCollection.
+  std::weak_ptr<LongLivedObjectCollection> longLivedObjectCollection_;
+
  public:
   static std::weak_ptr<CallbackWrapper> createWeak(
       jsi::Function&& callback,
       jsi::Runtime& runtime,
-      std::shared_ptr<CallInvoker> jsInvoker) {
+      std::shared_ptr<CallInvoker> jsInvoker,
+      std::shared_ptr<LongLivedObjectCollection> longLivedObjectCollection =
+          nullptr) {
     auto wrapper = std::shared_ptr<CallbackWrapper>(new CallbackWrapper(
-        std::move(callback), runtime, std::move(jsInvoker)));
-    LongLivedObjectCollection::get().add(wrapper);
+        std::move(callback),
+        runtime,
+        std::move(jsInvoker),
+        longLivedObjectCollection));
+    if (longLivedObjectCollection) {
+      longLivedObjectCollection->add(wrapper);
+    } else {
+      LongLivedObjectCollection::get().add(wrapper);
+    }
     return wrapper;
   }
 
@@ -63,6 +80,14 @@ class CallbackWrapper : public LongLivedObject {
   std::shared_ptr<CallInvoker> jsInvokerPtr() noexcept {
     return jsInvoker_;
   }
+
+  void allowRelease() override {
+    if (auto longLivedObjectCollection = longLivedObjectCollection_.lock()) {
+      longLivedObjectCollection->remove(this);
+      return;
+    }
+    LongLivedObject::allowRelease();
+  }
 };
 
 } // namespace facebook::react
diff --git a/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h b/packages/react-native/ReactCommon/react/bridging/LongLivedObject.h
@@ -28,7 +28,7 @@ namespace facebook::react {
  */
 class LongLivedObject {
  public:
-  void allowRelease();
+  virtual void allowRelease();
 
  protected:
   LongLivedObject() = default;
@@ -42,6 +42,7 @@ class LongLivedObjectCollection {
  public:
   static LongLivedObjectCollection& get();
 
+  LongLivedObjectCollection() = default;
   LongLivedObjectCollection(const LongLivedObjectCollection&) = delete;
   void operator=(const LongLivedObjectCollection&) = delete;
 
@@ -51,8 +52,6 @@ class LongLivedObjectCollection {
   size_t size() const;
 
  private:
-  LongLivedObjectCollection() = default;
-
   std::unordered_set<std::shared_ptr<LongLivedObject>> collection_;
   mutable std::mutex collectionMutex_;
 };

diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp
@@ -95,21 +95,25 @@ static void defineReadOnlyGlobal(
  */
 
 TurboModuleBinding::TurboModuleBinding(
-    TurboModuleProviderFunctionType&& moduleProvider)
-    : moduleProvider_(std::move(moduleProvider)) {}
+    TurboModuleProviderFunctionType&& moduleProvider,
+    std::shared_ptr<LongLivedObjectCollection> longLivedObjectCollection)
+    : moduleProvider_(std::move(moduleProvider)),
+      longLivedObjectCollection_(std::move(longLivedObjectCollection)) {}
 
 void TurboModuleBinding::install(
     jsi::Runtime& runtime,
     TurboModuleProviderFunctionType&& moduleProvider,
-    TurboModuleProviderFunctionType&& legacyModuleProvider) {
+    TurboModuleProviderFunctionType&& legacyModuleProvider,
+    std::shared_ptr<LongLivedObjectCollection> longLivedObjectCollection) {
   runtime.global().setProperty(
       runtime,
       "__turboModuleProxy",
       jsi::Function::createFromHostFunction(
           runtime,
           jsi::PropNameID::forAscii(runtime, "__turboModuleProxy"),
           1,
-          [binding = TurboModuleBinding(std::move(moduleProvider))](
+          [binding = TurboModuleBinding(
+               std::move(moduleProvider), longLivedObjectCollection)](
               jsi::Runtime& rt,
               const jsi::Value& thisVal,
               const jsi::Value* args,
@@ -132,7 +136,8 @@ void TurboModuleBinding::install(
               runtime,
               std::make_shared<BridgelessNativeModuleProxy>(
                   std::make_unique<TurboModuleBinding>(
-                      std::move(legacyModuleProvider)))));
+                      std::move(legacyModuleProvider),
+                      longLivedObjectCollection))));
     } else {
       defineReadOnlyGlobal(
           runtime,
@@ -144,7 +149,11 @@ void TurboModuleBinding::install(
 }
 
 TurboModuleBinding::~TurboModuleBinding() {
-  LongLivedObjectCollection::get().clear();
+  if (longLivedObjectCollection_) {
+    longLivedObjectCollection_->clear();
+  } else {
+    LongLivedObjectCollection::get().clear();
+  }
 }
 
 jsi::Value TurboModuleBinding::getModule(

diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h
@@ -11,6 +11,7 @@
 
 #include <ReactCommon/TurboModule.h>
 #include <jsi/jsi.h>
+#include <react/bridging/LongLivedObject.h>
 
 namespace facebook::react {
 
@@ -28,9 +29,14 @@ class TurboModuleBinding {
   static void install(
       jsi::Runtime& runtime,
       TurboModuleProviderFunctionType&& moduleProvider,
-      TurboModuleProviderFunctionType&& legacyModuleProvider = nullptr);
+      TurboModuleProviderFunctionType&& legacyModuleProvider = nullptr,
+      std::shared_ptr<LongLivedObjectCollection> longLivedObjectCollection =
+          nullptr);
+
+  TurboModuleBinding(
+      TurboModuleProviderFunctionType&& moduleProvider,
+      std::shared_ptr<LongLivedObjectCollection> longLivedObjectCollection);
 
-  TurboModuleBinding(TurboModuleProviderFunctionType&& moduleProvider);
   virtual ~TurboModuleBinding();
 
  private:
@@ -44,6 +50,7 @@ class TurboModuleBinding {
       const;
 
   TurboModuleProviderFunctionType moduleProvider_;
+  std::shared_ptr<LongLivedObjectCollection> longLivedObjectCollection_;
 };
 
 } // namespace facebook::react