From b73dd6726d88ca4d8ffe71f69cd7a7f668582f21 Mon Sep 17 00:00:00 2001 From: Pieter De Baets Date: Tue, 7 Mar 2023 01:17:44 -0800 Subject: [PATCH] Use WeakObject for new turbomodule binding mechanism Summary: A previous version of this experiment saw crashes on iOS once rolled out. Switching to WeakObject to make sure we're not accessing invalid references, and setting up the ability to experiment with this on iOS. Changelog: [Internal] Reviewed By: cipolleschi Differential Revision: D41552667 fbshipit-source-id: dc0c54edc2ad18c1947941119ffd50038a47c5f6 --- .../core/ReactCommon/TurboCxxModule.cpp | 7 ++- .../core/ReactCommon/TurboModule.cpp | 3 +- .../core/ReactCommon/TurboModule.h | 2 +- .../core/ReactCommon/TurboModuleBinding.cpp | 46 +++++++++++-------- .../core/ReactCommon/TurboModuleBinding.h | 2 +- .../ios/ReactCommon/RCTTurboModuleManager.h | 6 ++- .../ios/ReactCommon/RCTTurboModuleManager.mm | 13 ++++-- 7 files changed, 51 insertions(+), 28 deletions(-) diff --git a/ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.cpp b/ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.cpp index 82d87674643..1e5a1f6b6a6 100644 --- a/ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.cpp +++ b/ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.cpp @@ -111,7 +111,12 @@ jsi::Value TurboCxxModule::get( // If we have a JS wrapper, cache the result of this lookup if (jsRepresentation_) { - jsRepresentation_->setProperty(runtime, propName, result); + auto jsRepresentation = jsRepresentation_->lock(runtime); + if (!jsRepresentation.isUndefined()) { + std::move(jsRepresentation) + .asObject(runtime) + .setProperty(runtime, propName, result); + } } return result; diff --git a/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp b/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp index 87e0eabcd55..0eb26e7b625 100644 --- a/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp +++ b/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp @@ -31,7 +31,8 @@ jsi::Value TurboModule::get( // If we have a JS wrapper, cache the result of this lookup // We don't cache misses, to allow for methodMap_ to dynamically be extended if (jsRepresentation_) { - jsRepresentation_->setProperty(runtime, propName, result); + jsRepresentation_->lock(runtime).asObject(runtime).setProperty( + runtime, propName, result); } return result; } diff --git a/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.h b/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.h index 6a8bba7b5fa..5491e80eee3 100644 --- a/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.h +++ b/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.h @@ -106,7 +106,7 @@ class JSI_EXPORT TurboModule : public facebook::jsi::HostObject { private: friend class TurboCxxModule; friend class TurboModuleBinding; - std::unique_ptr jsRepresentation_; + std::unique_ptr jsRepresentation_; facebook::jsi::Value get( facebook::jsi::Runtime &runtime, diff --git a/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp b/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp index 8d4b6a50556..b3f1dbdf2c4 100644 --- a/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp +++ b/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.cpp @@ -43,7 +43,7 @@ void TurboModuleBinding::install( jsi::Runtime &rt, const jsi::Value &thisVal, const jsi::Value *args, - size_t count) mutable { + size_t count) { return binding.getModule(rt, thisVal, args, count); })); } @@ -56,7 +56,7 @@ jsi::Value TurboModuleBinding::getModule( jsi::Runtime &runtime, const jsi::Value &thisVal, const jsi::Value *args, - size_t count) { + size_t count) const { if (count < 1) { throw std::invalid_argument( "__turboModuleProxy must be called with at least 1 argument"); @@ -75,25 +75,33 @@ jsi::Value TurboModuleBinding::getModule( return jsi::Object::createFromHostObject(runtime, std::move(module)); } - auto &jsRepresentation = module->jsRepresentation_; - if (!jsRepresentation) { - jsRepresentation = std::make_unique(runtime); - if (bindingMode_ == TurboModuleBindingMode::Prototype) { - // Option 1: create plain object, with it's prototype mapped back to the - // hostobject. Any properties accessed are stored on the plain object - auto hostObject = - jsi::Object::createFromHostObject(runtime, std::move(module)); - jsRepresentation->setProperty( - runtime, "__proto__", std::move(hostObject)); - } else { - // Option 2: eagerly install all hostfunctions at this point, avoids - // prototype - for (auto &propName : module->getPropertyNames(runtime)) { - module->get(runtime, propName); - } + auto &weakJsRepresentation = module->jsRepresentation_; + if (weakJsRepresentation) { + auto jsRepresentation = weakJsRepresentation->lock(runtime); + if (!jsRepresentation.isUndefined()) { + return jsRepresentation; } } - return jsi::Value(runtime, *jsRepresentation); + + // No JS representation found, or object has been collected + jsi::Object jsRepresentation(runtime); + weakJsRepresentation = + std::make_unique(runtime, jsRepresentation); + + if (bindingMode_ == TurboModuleBindingMode::Prototype) { + // Option 1: create plain object, with it's prototype mapped back to the + // hostobject. Any properties accessed are stored on the plain object + auto hostObject = + jsi::Object::createFromHostObject(runtime, std::move(module)); + jsRepresentation.setProperty(runtime, "__proto__", std::move(hostObject)); + } else { + // Option 2: eagerly install all hostfunctions at this point, avoids + // prototype + for (auto &propName : module->getPropertyNames(runtime)) { + module->get(runtime, propName); + } + } + return jsRepresentation; } else { return jsi::Value::null(); } diff --git a/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h b/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h index 7a0e558dfb9..c573b4262f9 100644 --- a/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h +++ b/ReactCommon/react/nativemodule/core/ReactCommon/TurboModuleBinding.h @@ -50,7 +50,7 @@ class TurboModuleBinding { jsi::Runtime &runtime, const jsi::Value &thisVal, const jsi::Value *args, - size_t count); + size_t count) const; TurboModuleProviderFunctionType moduleProvider_; TurboModuleBindingMode bindingMode_; diff --git a/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.h b/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.h index 016d38225ae..2124e05ac01 100644 --- a/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.h +++ b/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.h @@ -9,10 +9,14 @@ #import +#import #import #import +#import #import "RCTTurboModule.h" +RCT_EXTERN void RCTTurboModuleSetBindingMode(facebook::react::TurboModuleBindingMode bindingMode); + @protocol RCTTurboModuleManagerDelegate @optional @@ -44,7 +48,7 @@ delegate:(id)delegate jsInvoker:(std::shared_ptr)jsInvoker; -- (void)installJSBindingWithRuntimeExecutor:(facebook::react::RuntimeExecutor)runtimeExecutor; +- (void)installJSBindingWithRuntimeExecutor:(facebook::react::RuntimeExecutor &)runtimeExecutor; - (void)invalidate; diff --git a/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.mm b/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.mm index 5b2881bc305..522247f1f7c 100644 --- a/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.mm +++ b/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModuleManager.mm @@ -25,13 +25,18 @@ #import #import #import -#import #import #import using namespace facebook; using namespace facebook::react; +static TurboModuleBindingMode sTurboModuleBindingMode = TurboModuleBindingMode::HostObject; +void RCTTurboModuleSetBindingMode(TurboModuleBindingMode bindingMode) +{ + sTurboModuleBindingMode = bindingMode; +} + /** * A global variable whose address we use to associate method queues to id objects. */ @@ -181,7 +186,7 @@ static Class getFallbackClassFromName(const char *name) jsInvoker:(std::shared_ptr)jsInvoker { if (self = [super init]) { - _jsInvoker = jsInvoker; + _jsInvoker = std::move(jsInvoker); _delegate = delegate; _bridge = bridge; _invalidating = false; @@ -691,7 +696,7 @@ static Class getFallbackClassFromName(const char *name) return requiresMainQueueSetup; } -- (void)installJSBindingWithRuntimeExecutor:(facebook::react::RuntimeExecutor)runtimeExecutor +- (void)installJSBindingWithRuntimeExecutor:(facebook::react::RuntimeExecutor &)runtimeExecutor { if (!runtimeExecutor) { // jsi::Runtime doesn't exist when attached to Chrome debugger. @@ -736,7 +741,7 @@ static Class getFallbackClassFromName(const char *name) }; runtimeExecutor([turboModuleProvider = std::move(turboModuleProvider)](jsi::Runtime &runtime) { - TurboModuleBinding::install(runtime, std::move(turboModuleProvider), TurboModuleBindingMode::HostObject); + TurboModuleBinding::install(runtime, std::move(turboModuleProvider), sTurboModuleBindingMode); }); }