From 5e68a98c3dd108b6db858c91e56a864423644b8e Mon Sep 17 00:00:00 2001 From: Ramanpreet Nara Date: Thu, 26 Sep 2019 09:31:45 -0700 Subject: [PATCH] Make TurboModuleProviderFunctionType not depend on TurboModuleManager instance Summary: ## Description The C++ lambda that JS invokes to create TurboModules uses `TurboModuleManager`. It is possible for this lambda to outlive the `TurboModuleManager` instance because we delete `TurboModuleManager` on the JS Thread before we schedule the deletion of `CatalystInstanceImpl` object on a neutral third-party background thread. `CatalystInstanceImpl` owns the JS VM instance that owns the C++ lambda. ## [CatalystInstanceImpl.java](https://fburl.com/diffusion/vt4pwjwa) ``` public void destroy() { // ... getReactQueueConfiguration() .getJSQueueThread() .runOnQueue( new Runnable() { Override public void run() { // We need to destroy the TurboModuleManager on the JS Thread if (turboModuleManager != null) { turboModuleManager.onCatalystInstanceDestroy(); } getReactQueueConfiguration() .getUIQueueThread() .runOnQueue( new Runnable() { Override public void run() { // AsyncTask.execute must be executed from the UI Thread AsyncTask.execute( new Runnable() { Override public void run() { // Kill non-UI threads from neutral third party // potentially expensive, so don't run on UI thread // contextHolder is used as a lock to guard against // other users of the JS VM having the VM destroyed // underneath them, so notify them before we reset // Native mJavaScriptContextHolder.clear(); mHybridData.resetNative(); getReactQueueConfiguration().destroy(); Log.d( ReactConstants.TAG, "CatalystInstanceImpl.destroy() end"); ReactMarker.logMarker( ReactMarkerConstants.DESTROY_CATALYST_INSTANCE_END); } }); } }); } }); } }); // ... } ``` The JS thread is also terminated in the neutral third-party thread. Therefore, it should be possible for JS to request a `TurboModule` after `TurboModuleManager` has been destroyed (i.e: JS can try to access memory that was freed). This is why I think we're getting a segfault in T54298358. ## Fix The fix was to wrap all the member variables of TurboModuleManager we use in `TurboModuleProviderFunctionType` in weak references. This way, we can make sure that the memory is valid before using it. Reviewed By: fkgozali Differential Revision: D17539761 fbshipit-source-id: fe527383458a019a4cb9107ec5c3ddd6295ae41c --- .../jni/ReactCommon/TurboModuleManager.cpp | 73 ++++++++++++------- .../core/jni/ReactCommon/TurboModuleManager.h | 4 +- 2 files changed, 49 insertions(+), 28 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 acaae54701e..616b68b1d84 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 @@ -31,7 +31,8 @@ TurboModuleManager::TurboModuleManager( runtime_(rt), jsCallInvoker_(jsCallInvoker), nativeCallInvoker_(nativeCallInvoker), - delegate_(jni::make_global(delegate)) + delegate_(jni::make_global(delegate)), + turboModuleCache_(std::make_shared()) {} jni::local_ref TurboModuleManager::initHybrid( @@ -58,39 +59,57 @@ void TurboModuleManager::installJSIBindings() { if (!runtime_) { return; // Runtime doesn't exist when attached to Chrome debugger. } + TurboModuleBinding::install(*runtime_, std::make_shared( - [this](const std::string &name) -> std::shared_ptr { - auto turboModuleLookup = turboModuleCache_.find(name); - if (turboModuleLookup != turboModuleCache_.end()) { - return turboModuleLookup->second; - } + [ + 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(); - auto cxxModule = delegate_->cthis()->getTurboModule(name, jsCallInvoker_); - if (cxxModule) { - turboModuleCache_.insert({name, cxxModule}); - return cxxModule; - } + if (!turboModuleCache || !jsCallInvoker || !nativeCallInvoker || !delegate || !javaPart) { + return nullptr; + } - static auto getLegacyCxxModule = delegate_->getClass()->getMethod(const std::string&)>("getLegacyCxxModule"); - auto legacyCxxModule = getLegacyCxxModule(delegate_.get(), name); + auto turboModuleLookup = turboModuleCache->find(name); + if (turboModuleLookup != turboModuleCache->end()) { + return turboModuleLookup->second; + } - if (legacyCxxModule) { - auto turboModule = std::make_shared(legacyCxxModule->cthis()->getModule(), jsCallInvoker_); - turboModuleCache_.insert({name, turboModule}); - return turboModule; - } + auto cxxModule = delegate->cthis()->getTurboModule(name, jsCallInvoker); + if (cxxModule) { + turboModuleCache->insert({name, cxxModule}); + return cxxModule; + } - static auto getJavaModule = javaClassStatic()->getMethod(const std::string&)>("getJavaModule"); - auto moduleInstance = getJavaModule(javaPart_.get(), name); + static auto getLegacyCxxModule = delegate->getClass()->getMethod(const std::string&)>("getLegacyCxxModule"); + auto legacyCxxModule = getLegacyCxxModule(delegate.get(), name); - if (moduleInstance) { - auto turboModule = delegate_->cthis()->getTurboModule(name, moduleInstance, jsCallInvoker_, nativeCallInvoker_); - turboModuleCache_.insert({name, turboModule}); - return turboModule; - } + if (legacyCxxModule) { + auto turboModule = std::make_shared(legacyCxxModule->cthis()->getModule(), jsCallInvoker); + turboModuleCache->insert({name, turboModule}); + return turboModule; + } - return std::shared_ptr(nullptr); - }) + 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; + } + + return nullptr; + }) ); } diff --git a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h index 65927078a48..7d03ec10c82 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h +++ b/ReactAndroid/src/main/java/com/facebook/react/turbomodule/core/jni/ReactCommon/TurboModuleManager.h @@ -40,13 +40,15 @@ private: std::shared_ptr nativeCallInvoker_; jni::global_ref delegate_; + using TurboModuleCache = std::unordered_map>; + /** * TODO(T48018690): * All modules are currently long-lived. * We need to come up with a mechanism to allow modules to specify whether * they want to be long-lived or short-lived. */ - std::unordered_map> turboModuleCache_; + std::shared_ptr turboModuleCache_; void installJSIBindings(); explicit TurboModuleManager(