From 9ae95582e792a3dca4487bdce9080c6d874c7dd7 Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Thu, 30 Jan 2020 15:11:46 -0800 Subject: [PATCH] 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 --- .../jni/ReactCommon/TurboModuleManager.cpp | 99 +++++++++---------- .../turbomodule/core/TurboModuleBinding.cpp | 11 ++- .../turbomodule/core/TurboModuleBinding.h | 11 +-- .../core/platform/ios/RCTTurboModuleManager.h | 2 - .../platform/ios/RCTTurboModuleManager.mm | 71 ++++++------- 5 files changed, 86 insertions(+), 108 deletions(-) diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp index db8c26ad075..73cfd4f994b 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.cpp +++ b/ReactAndroid/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( - [turboModuleCache_ = - std::weak_ptr(turboModuleCache_), - jsCallInvoker_ = std::weak_ptr(jsCallInvoker_), - nativeCallInvoker_ = std::weak_ptr(nativeCallInvoker_), - delegate_ = jni::make_weak(delegate_), - javaPart_ = jni::make_weak(javaPart_)]( - const std::string &name) -> std::shared_ptr { - auto turboModuleCache = turboModuleCache_.lock(); - auto jsCallInvoker = jsCallInvoker_.lock(); - auto nativeCallInvoker = nativeCallInvoker_.lock(); - auto delegate = delegate_.lockLocal(); - auto javaPart = javaPart_.lockLocal(); + [turboModuleCache_ = std::weak_ptr(turboModuleCache_), + jsCallInvoker_ = std::weak_ptr(jsCallInvoker_), + nativeCallInvoker_ = std::weak_ptr(nativeCallInvoker_), + delegate_ = jni::make_weak(delegate_), + javaPart_ = jni::make_weak(javaPart_)]( + const std::string &name) -> std::shared_ptr { + 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; - } + if (!turboModuleCache || !jsCallInvoker || !nativeCallInvoker || + !delegate || !javaPart) { + return nullptr; + } - auto turboModuleLookup = turboModuleCache->find(name); - if (turboModuleLookup != turboModuleCache->end()) { - return turboModuleLookup->second; - } + 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; - } + auto cxxModule = delegate->cthis()->getTurboModule(name, jsCallInvoker); + if (cxxModule) { + turboModuleCache->insert({name, cxxModule}); + return cxxModule; + } - static auto getLegacyCxxModule = - delegate->getClass() - ->getMethod( - const std::string &)>("getLegacyCxxModule"); - auto legacyCxxModule = getLegacyCxxModule(delegate.get(), name); + static auto getLegacyCxxModule = + delegate->getClass() + ->getMethod( + const std::string &)>("getLegacyCxxModule"); + auto legacyCxxModule = getLegacyCxxModule(delegate.get(), name); - if (legacyCxxModule) { - auto turboModule = std::make_shared( - legacyCxxModule->cthis()->getModule(), jsCallInvoker); - turboModuleCache->insert({name, turboModule}); - return turboModule; - } + if (legacyCxxModule) { + auto turboModule = std::make_shared( + legacyCxxModule->cthis()->getModule(), jsCallInvoker); + turboModuleCache->insert({name, turboModule}); + return turboModule; + } - static auto getJavaModule = - javaPart->getClass() - ->getMethod( - const std::string &)>("getJavaModule"); - auto moduleInstance = getJavaModule(javaPart.get(), name); + static auto getJavaModule = + javaPart->getClass() + ->getMethod(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; - } + if (moduleInstance) { + auto turboModule = delegate->cthis()->getTurboModule( + name, moduleInstance, jsCallInvoker, nativeCallInvoker); + turboModuleCache->insert({name, turboModule}); + return turboModule; + } - return nullptr; - })); + return nullptr; + }); } } // namespace react diff --git a/ReactCommon/turbomodule/core/TurboModuleBinding.cpp b/ReactCommon/turbomodule/core/TurboModuleBinding.cpp index 549e0df0607..cfb9ecd3e89 100644 --- a/ReactCommon/turbomodule/core/TurboModuleBinding.cpp +++ b/ReactCommon/turbomodule/core/TurboModuleBinding.cpp @@ -21,12 +21,12 @@ 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 binding) { + const TurboModuleProviderFunctionType &&moduleProvider) { runtime.global().setProperty( runtime, "__turboModuleProxy", @@ -34,7 +34,8 @@ void TurboModuleBinding::install( runtime, jsi::PropNameID::forAscii(runtime, "__turboModuleProxy"), 1, - [binding]( + [binding = + std::make_shared(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 index c98113da2ad..96de89bed47 100644 --- 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 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 index 7fdb9fd017b..2adeb6bbbc3 100644 --- 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)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 index 004aa7113cf..c5a46e004d8 100644 --- 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 _jsInvoker; - std::shared_ptr _binding; __weak id _delegate; __weak RCTBridge *_bridge; /** @@ -83,38 +82,6 @@ static Class getFallbackClassFromName(const char *name) selector:@selector(bridgeDidInvalidateModules:) name:RCTBridgeDidInvalidateModulesNotification object:_bridge.parentBridge]; - - __weak __typeof(self) weakSelf = self; - - auto moduleProvider = [weakSelf](const std::string &name) -> std::shared_ptr { - 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(moduleProvider); } return self; } @@ -376,12 +343,36 @@ static Class getFallbackClassFromName(const char *name) return; } - react::TurboModuleBinding::install(*_runtime, _binding); -} + __weak __typeof(self) weakSelf = self; -- (std::shared_ptr)getModule:(const std::string &)name -{ - return _binding->getModule(name); + react::TurboModuleBinding::install( + *_runtime, [weakSelf](const std::string &name) -> std::shared_ptr { + 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 @@ static Class getFallbackClassFromName(const char *name) } _turboModuleCache.clear(); - - _binding->invalidate(); } - (void)invalidate @@ -491,8 +480,6 @@ static Class getFallbackClassFromName(const char *name) } _turboModuleCache.clear(); - - _binding->invalidate(); } @end