From b145a829643bfce083dac3f15a2fbc17dbe81bab Mon Sep 17 00:00:00 2001 From: Valentin Shergin Date: Thu, 2 Apr 2020 11:12:22 -0700 Subject: [PATCH] Fixed crash in JSIExecutor::NativeModuleProxy Summary: JSIExecutor::NativeModuleProxy is an object created by JSIExecutor and essentially representing that in JavaScript world. Before this change, JSIExecutor::NativeModuleProxy had a raw reference to JSIExecutor which (I believe) caused a crash because JSIExecutor can be deallocated before JSIExecutor::NativeModuleProxy. Now, instead of storing a pointer to JSIExecutor, we store a weak pointer to JSINativeModules which we can safely validate before calling on it. Changelog: [Internal] Fixed crash in JSIExecutor Now the configuration looks like this: ``` + - - - - - - - - - - - - - - - - - - - - - Something else | | shared_ptr runtime --+ | | + - - - - - - - - - - - - - - - - - - - - - | | | +------------------------------------------+ | | | | | JSExecutorFactory | | +--------------------------------+-------------------------------+ | | +-----------------------+ | | | +------------------------------------------+ | | v | | | +------------------------------------------+ | +--------------------------+ | | | | | | | | ModuleRegistry | | v | | | | | +------------------------------------------+ | | +------------------------------------------+ | | HermesRuntimeImpl | | | | | | (jsi::Runtime) |--+ | | +->+------------------------------------------+ | | | | | | | |std::unordered_map | | +------------------------------------------+ | | | | |modulesByName_ | | | | | | | | | | | | | +------------------------------------------+ | | | | +->+------------------------------------------+ | +-----------------------+ | | |std::vector>| | | | | |modules_ | | | | | | | | v | | +------------------------------------------+ | +------------------------------------------+ | | | | | | | | | JSIExecutor::NativeModuleProxy | | | | | | | | | +------------------------------------------+ | | | +------------------------------------------+ | | | | | | +->+------------------------------------------+ | | | | NativeToJsBridge | |shared_ptr | | | | | | |nativeModules_ | | | | +------------------------------------------+ +------------------------------------------+--+-----+------------------------------------+ | | | | | | +->+------------------------------------------+ | | | | | |unique_ptr | | | | | | |m_executor | | | | | | |(`::destroy()` resets it.) | | | | | | +------------------------------------------+--------------------------------+ | | | | +->+------------------------------------------+ | | | | | | |shared_ptr | | | | | | | |m_delegate | | | | | | | +------------------------------------------+--+ v | | | | +->+------------------------------------------+ | +------------------------------------------+ | | | | |shared_ptr | | | | | | | | |m_executorMessageQueueThread | | | HermesExecutor: JSIExecutor: JSExecutor | | | | | +------------------------------------------+ | | | | | | | | +------------------------------------------+ | | | | | | | | | | | +->+------------------------------------------+ | | | | | | |shared_ptr | | | | | | | |runtime_ | | | | | | | +------------------------------------------+--+ | | | | +->+------------------------------------------+ | | | | | |shared_ptr | | | | | | |nativeModules_ | | | | | | +------------------------------------------+--------+------------------------------------+ | +--------------------------+ +->+------------------------------------------+ | | | | |std::shared_ptr | | v | | |delegate_ | | +------------------------------------------+ | | +------------------------------------------+--+ | | | | | | | | JSINativeModules | | | | | | | | | | | +------------------------------------------+ | | | | | | | | | +-->+------------------------------------------+ | +-----------------------------------------------------------------------------------+ | |m_moduleRegistry | | | | |(shared_ptr) | | | | +------------------------------------------+--+ | | | | v | +------------------------------------------+ | | | | | JsToNativeBridge: ExecutorDelegate | | | | | +------------------------------------------+ | | | +->+------------------------------------------+ | |shared_ptr | | |m_registry | | +------------------------------------------+-----------------------------------------------------------------+ ``` Reviewed By: RSNara Differential Revision: D20817257 fbshipit-source-id: 9ae378dbe880aaabfef7ae783dae2f94ee4b0af5 --- .../jsiexecutor/jsireact/JSIExecutor.cpp | 17 ++++++++++++----- ReactCommon/jsiexecutor/jsireact/JSIExecutor.h | 2 +- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp index 360918d672c..0f1370a7eeb 100644 --- a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp +++ b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp @@ -27,14 +27,20 @@ namespace react { class JSIExecutor::NativeModuleProxy : public jsi::HostObject { public: - NativeModuleProxy(JSIExecutor &executor) : executor_(executor) {} + NativeModuleProxy(std::shared_ptr nativeModules) + : weakNativeModules_(nativeModules) {} Value get(Runtime &rt, const PropNameID &name) override { if (name.utf8(rt) == "name") { return jsi::String::createFromAscii(rt, "NativeModules"); } - return executor_.nativeModules_.getModule(rt, name); + auto nativeModules = weakNativeModules_.lock(); + if (!nativeModules) { + return nullptr; + } + + return nativeModules->getModule(rt, name); } void set(Runtime &, const PropNameID &, const Value &) override { @@ -43,7 +49,7 @@ class JSIExecutor::NativeModuleProxy : public jsi::HostObject { } private: - JSIExecutor &executor_; + std::weak_ptr weakNativeModules_; }; namespace { @@ -63,7 +69,8 @@ JSIExecutor::JSIExecutor( RuntimeInstaller runtimeInstaller) : runtime_(runtime), delegate_(delegate), - nativeModules_(delegate ? delegate->getModuleRegistry() : nullptr), + nativeModules_(std::make_shared( + delegate ? delegate->getModuleRegistry() : nullptr)), scopedTimeoutInvoker_(scopedTimeoutInvoker), runtimeInstaller_(runtimeInstaller) { runtime_->global().setProperty( @@ -76,7 +83,7 @@ void JSIExecutor::initializeRuntime() { *runtime_, "nativeModuleProxy", Object::createFromHostObject( - *runtime_, std::make_shared(*this))); + *runtime_, std::make_shared(nativeModules_))); runtime_->global().setProperty( *runtime_, diff --git a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h index a8a1042ab87..2cd3776baf5 100644 --- a/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h +++ b/ReactCommon/jsiexecutor/jsireact/JSIExecutor.h @@ -122,7 +122,7 @@ class JSIExecutor : public JSExecutor { std::shared_ptr runtime_; std::shared_ptr delegate_; - JSINativeModules nativeModules_; + std::shared_ptr nativeModules_; std::once_flag bindFlag_; std::unique_ptr bundleRegistry_; JSIScopedTimeoutInvoker scopedTimeoutInvoker_;