Clear all held jsi::Functions when jsi::Runtime is deleted

Summary: ## Description You're not supposed to hold on to JSI objects (ex: `jsi::Function`) past the point where their `jsi::Runtime` is deleted. Otherwise, we get a dangling pointer crash, like this: T60262810! Historically, this cleanup problem has always been really tricky to get right. With this diff, I hope to fix that problem once and for all by deleting all `jsi::Function`s when we delete the global `__turboModuleProxy` function. ## Current Setup - The TurboModules infra uses weak references to `CallbackWrapper`s to hold on to the `jsi::Function`s passed from JS to ObjC. - The LongLivedObjectCollection holds on to strong references to `CallbackWrapper`s. This ensures that the `jsi::Function`s aren't deleted prematurely. This also means that we can use `LongLivedObjectCollection` to delete all `CallbackWrappers`. - `TurboModuleBinding` is the abstraction we use to install the global `__turboModuleProxy` function. It is owned by `TurboModuleManager`, and `TurboModuleManager` uses it to clear all references to `jsi::Function`s, when we delete all NativeModules. ## Solution 1. Transfer ownership of `TurboModuleBinding` from `TurboModuleManager` to the `__turboModuleProxy` function. 2. Clear the `LongLivedObjectCollection` when `TurboModuleBinding` is deleted. Changelog: [iOS][Fixed] - Clear all held jsi::Functions when jsi::Runtime is deleted Reviewed By: JoshuaGross Differential Revision: D19565499 fbshipit-source-id: e3510ea04e72f6bda363a8fc3ee2be60303b70a6
facebook · Jan 30, 2020 · 9ae9558 · 9ae9558
1 parent 7001dc3
commit 9ae9558
Show file tree

Hide file tree

Showing 5 changed files with 94 additions and 116 deletions.
diff --git a/.../src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp b/.../src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp
@@ -65,65 +65,62 @@ void TurboModuleManager::installJSIBindings() {
 
   TurboModuleBinding::install(
       *runtime_,
-      std::make_shared<TurboModuleBinding>(
-          [turboModuleCache_ =
-               std::weak_ptr<TurboModuleCache>(turboModuleCache_),
-           jsCallInvoker_ = std::weak_ptr<CallInvoker>(jsCallInvoker_),
-           nativeCallInvoker_ = std::weak_ptr<CallInvoker>(nativeCallInvoker_),
-           delegate_ = jni::make_weak(delegate_),
-           javaPart_ = jni::make_weak(javaPart_)](
-              const std::string &name) -> std::shared_ptr<TurboModule> {
-            auto turboModuleCache = turboModuleCache_.lock();
-            auto jsCallInvoker = jsCallInvoker_.lock();
-            auto nativeCallInvoker = nativeCallInvoker_.lock();
-            auto delegate = delegate_.lockLocal();
-            auto javaPart = javaPart_.lockLocal();
-
-            if (!turboModuleCache || !jsCallInvoker || !nativeCallInvoker ||
-                !delegate || !javaPart) {
-              return nullptr;
-            }
-
-            auto turboModuleLookup = turboModuleCache->find(name);
-            if (turboModuleLookup != turboModuleCache->end()) {
-              return turboModuleLookup->second;
-            }
-
-            auto cxxModule =
-                delegate->cthis()->getTurboModule(name, jsCallInvoker);
-            if (cxxModule) {
-              turboModuleCache->insert({name, cxxModule});
-              return cxxModule;
-            }
-
-            static auto getLegacyCxxModule =
-                delegate->getClass()
-                    ->getMethod<jni::alias_ref<CxxModuleWrapper::javaobject>(
-                        const std::string &)>("getLegacyCxxModule");
-            auto legacyCxxModule = getLegacyCxxModule(delegate.get(), name);
-
-            if (legacyCxxModule) {
-              auto turboModule = std::make_shared<react::TurboCxxModule>(
-                  legacyCxxModule->cthis()->getModule(), jsCallInvoker);
-              turboModuleCache->insert({name, turboModule});
-              return turboModule;
-            }
-
-            static auto getJavaModule =
-                javaPart->getClass()
-                    ->getMethod<jni::alias_ref<JTurboModule>(
-                        const std::string &)>("getJavaModule");
-            auto moduleInstance = getJavaModule(javaPart.get(), name);
-
-            if (moduleInstance) {
-              auto turboModule = delegate->cthis()->getTurboModule(
-                  name, moduleInstance, jsCallInvoker, nativeCallInvoker);
-              turboModuleCache->insert({name, turboModule});
-              return turboModule;
-            }
-
-            return nullptr;
-          }));
+      [turboModuleCache_ = std::weak_ptr<TurboModuleCache>(turboModuleCache_),
+       jsCallInvoker_ = std::weak_ptr<CallInvoker>(jsCallInvoker_),
+       nativeCallInvoker_ = std::weak_ptr<CallInvoker>(nativeCallInvoker_),
+       delegate_ = jni::make_weak(delegate_),
+       javaPart_ = jni::make_weak(javaPart_)](
+          const std::string &name) -> std::shared_ptr<TurboModule> {
+        auto turboModuleCache = turboModuleCache_.lock();
+        auto jsCallInvoker = jsCallInvoker_.lock();
+        auto nativeCallInvoker = nativeCallInvoker_.lock();
+        auto delegate = delegate_.lockLocal();
+        auto javaPart = javaPart_.lockLocal();
+
+        if (!turboModuleCache || !jsCallInvoker || !nativeCallInvoker ||
+            !delegate || !javaPart) {
+          return nullptr;
+        }
+
+        auto turboModuleLookup = turboModuleCache->find(name);
+        if (turboModuleLookup != turboModuleCache->end()) {
+          return turboModuleLookup->second;
+        }
+
+        auto cxxModule = delegate->cthis()->getTurboModule(name, jsCallInvoker);
+        if (cxxModule) {
+          turboModuleCache->insert({name, cxxModule});
+          return cxxModule;
+        }
+
+        static auto getLegacyCxxModule =
+            delegate->getClass()
+                ->getMethod<jni::alias_ref<CxxModuleWrapper::javaobject>(
+                    const std::string &)>("getLegacyCxxModule");
+        auto legacyCxxModule = getLegacyCxxModule(delegate.get(), name);
+
+        if (legacyCxxModule) {
+          auto turboModule = std::make_shared<react::TurboCxxModule>(
+              legacyCxxModule->cthis()->getModule(), jsCallInvoker);
+          turboModuleCache->insert({name, turboModule});
+          return turboModule;
+        }
+
+        static auto getJavaModule =
+            javaPart->getClass()
+                ->getMethod<jni::alias_ref<JTurboModule>(const std::string &)>(
+                    "getJavaModule");
+        auto moduleInstance = getJavaModule(javaPart.get(), name);
+
+        if (moduleInstance) {
+          auto turboModule = delegate->cthis()->getTurboModule(
+              name, moduleInstance, jsCallInvoker, nativeCallInvoker);
+          turboModuleCache->insert({name, turboModule});
+          return turboModule;
+        }
+
+        return nullptr;
+      });
 }
 
 } // namespace react

diff --git a/ReactCommon/turbomodule/core/TurboModuleBinding.cpp b/ReactCommon/turbomodule/core/TurboModuleBinding.cpp
@@ -21,20 +21,21 @@ namespace react {
  * Public API to install the TurboModule system.
  */
 TurboModuleBinding::TurboModuleBinding(
-    const TurboModuleProviderFunctionType &moduleProvider)
-    : moduleProvider_(moduleProvider) {}
+    const TurboModuleProviderFunctionType &&moduleProvider)
+    : moduleProvider_(std::move(moduleProvider)) {}
 
 void TurboModuleBinding::install(
     jsi::Runtime &runtime,
-    std::shared_ptr<TurboModuleBinding> binding) {
+    const TurboModuleProviderFunctionType &&moduleProvider) {
   runtime.global().setProperty(
       runtime,
       "__turboModuleProxy",
       jsi::Function::createFromHostFunction(
           runtime,
           jsi::PropNameID::forAscii(runtime, "__turboModuleProxy"),
           1,
-          [binding](
+          [binding =
+               std::make_shared<TurboModuleBinding>(std::move(moduleProvider))](
               jsi::Runtime &rt,
               const jsi::Value &thisVal,
               const jsi::Value *args,
@@ -43,7 +44,7 @@ void TurboModuleBinding::install(
           }));
 }
 
-void TurboModuleBinding::invalidate() const {
+TurboModuleBinding::~TurboModuleBinding() {
   LongLivedObjectCollection::get().clear();
 }
 

diff --git a/ReactCommon/turbomodule/core/TurboModuleBinding.h b/ReactCommon/turbomodule/core/TurboModuleBinding.h
@@ -28,15 +28,10 @@ class TurboModuleBinding {
    */
   static void install(
       jsi::Runtime &runtime,
-      std::shared_ptr<TurboModuleBinding> binding);
+      const TurboModuleProviderFunctionType &&moduleProvider);
 
-  TurboModuleBinding(const TurboModuleProviderFunctionType &moduleProvider);
-
-  /*
-   * Invalidates the binding.
-   * Can be called in any thread.
-   */
-  void invalidate() const;
+  TurboModuleBinding(const TurboModuleProviderFunctionType &&moduleProvider);
+  virtual ~TurboModuleBinding();
 
   /**
    * Get an TurboModule instance for the given module name.

diff --git a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.h b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.h
@@ -44,8 +44,6 @@
 
 - (void)installJSBindingWithRuntime:(facebook::jsi::Runtime *)runtime;
 
-- (std::shared_ptr<facebook::react::TurboModule>)getModule:(const std::string &)name;
-
 - (void)invalidate;
 
 @end
diff --git a/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm b/ReactCommon/turbomodule/core/platform/ios/RCTTurboModuleManager.mm
@@ -36,7 +36,6 @@ static Class getFallbackClassFromName(const char *name)
 @implementation RCTTurboModuleManager {
   jsi::Runtime *_runtime;
   std::shared_ptr<facebook::react::CallInvoker> _jsInvoker;
-  std::shared_ptr<react::TurboModuleBinding> _binding;
   __weak id<RCTTurboModuleManagerDelegate> _delegate;
   __weak RCTBridge *_bridge;
   /**
@@ -83,38 +82,6 @@ - (instancetype)initWithBridge:(RCTBridge *)bridge
                                              selector:@selector(bridgeDidInvalidateModules:)
                                                  name:RCTBridgeDidInvalidateModulesNotification
                                                object:_bridge.parentBridge];
-
-    __weak __typeof(self) weakSelf = self;
-
-    auto moduleProvider = [weakSelf](const std::string &name) -> std::shared_ptr<react::TurboModule> {
-      if (!weakSelf) {
-        return nullptr;
-      }
-
-      __strong __typeof(self) strongSelf = weakSelf;
-
-      auto moduleName = name.c_str();
-      auto moduleWasNotInitialized = ![strongSelf moduleIsInitialized:moduleName];
-      if (moduleWasNotInitialized) {
-        [strongSelf->_bridge.performanceLogger markStartForTag:RCTPLTurboModuleSetup];
-      }
-
-      /**
-       * By default, all TurboModules are long-lived.
-       * Additionally, if a TurboModule with the name `name` isn't found, then we
-       * trigger an assertion failure.
-       */
-      auto turboModule = [strongSelf provideTurboModule:moduleName];
-
-      if (moduleWasNotInitialized && [strongSelf moduleIsInitialized:moduleName]) {
-        [strongSelf->_bridge.performanceLogger markStopForTag:RCTPLTurboModuleSetup];
-        [strongSelf notifyAboutTurboModuleSetup:moduleName];
-      }
-
-      return turboModule;
-    };
-
-    _binding = std::make_shared<react::TurboModuleBinding>(moduleProvider);
   }
   return self;
 }
@@ -376,12 +343,36 @@ - (void)installJSBindingWithRuntime:(jsi::Runtime *)runtime
     return;
   }
 
-  react::TurboModuleBinding::install(*_runtime, _binding);
-}
+  __weak __typeof(self) weakSelf = self;
 
-- (std::shared_ptr<facebook::react::TurboModule>)getModule:(const std::string &)name
-{
-  return _binding->getModule(name);
+  react::TurboModuleBinding::install(
+      *_runtime, [weakSelf](const std::string &name) -> std::shared_ptr<react::TurboModule> {
+        if (!weakSelf) {
+          return nullptr;
+        }
+
+        __strong __typeof(self) strongSelf = weakSelf;
+
+        auto moduleName = name.c_str();
+        auto moduleWasNotInitialized = ![strongSelf moduleIsInitialized:moduleName];
+        if (moduleWasNotInitialized) {
+          [strongSelf->_bridge.performanceLogger markStartForTag:RCTPLTurboModuleSetup];
+        }
+
+        /**
+         * By default, all TurboModules are long-lived.
+         * Additionally, if a TurboModule with the name `name` isn't found, then we
+         * trigger an assertion failure.
+         */
+        auto turboModule = [strongSelf provideTurboModule:moduleName];
+
+        if (moduleWasNotInitialized && [strongSelf moduleIsInitialized:moduleName]) {
+          [strongSelf->_bridge.performanceLogger markStopForTag:RCTPLTurboModuleSetup];
+          [strongSelf notifyAboutTurboModuleSetup:moduleName];
+        }
+
+        return turboModule;
+      });
 }
 
 #pragma mark RCTTurboModuleLookupDelegate
@@ -465,8 +456,6 @@ - (void)bridgeDidInvalidateModules:(NSNotification *)notification
   }
 
   _turboModuleCache.clear();
-
-  _binding->invalidate();
 }
 
 - (void)invalidate
@@ -491,8 +480,6 @@ - (void)invalidate
   }
 
   _turboModuleCache.clear();
-
-  _binding->invalidate();
 }
 
 @end