From 76ce7890141f22ba75cf62806762179be3c8cfa9 Mon Sep 17 00:00:00 2001 From: Ruslan Shestopalyuk Date: Fri, 8 Mar 2024 10:28:45 -0800 Subject: [PATCH] Pass jsi::Runtime reference into CallInvoker::invoke* callbacks (#43375) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/43375 ## Changelog: [Internal] - As discussed with the team, it makes more sense to pass the reference to the correct `jsi::Runtime` object as an argument to the ` CallInvoker::invoke*` callbacks, that are provided by the user. There are various use cases when user would like to get a hold of the `jsi::Runtime` in the callback, and it makes sense, since it is guaranteed to run on the JS thread. So far people have been coming up with all kinds of workarounds for that, none of them safe enough. Reviewed By: rubennorte Differential Revision: D54643171 fbshipit-source-id: 2f6015426a9e29cb9fcf5a9a3e2f6f33ff692538 --- .../React/CxxBridge/RCTCxxBridge.mm | 2 +- .../callinvoker/ReactCommon/CallInvoker.h | 24 ++++++++++++-- .../ReactCommon/cxxreact/Instance.cpp | 11 +++---- .../ReactCommon/cxxreact/Instance.h | 8 ++--- .../ReactCommon/cxxreact/NativeToJsBridge.cpp | 4 +-- .../ReactCommon/react/bridging/Function.h | 10 +++--- .../react/bridging/tests/BridgingTest.h | 8 ++--- .../core/ReactCommon/TurboCxxModule.cpp | 31 +++++++++---------- .../core/ReactCommon/TurboModule.cpp | 16 +++++----- .../RuntimeSchedulerCallInvoker.cpp | 6 ++-- .../react/runtime/BridgelessJSCallInvoker.cpp | 8 ++--- .../react/runtime/BridgelessJSCallInvoker.h | 4 +-- .../BridgelessNativeMethodCallInvoker.cpp | 4 +-- .../BridgelessNativeMethodCallInvoker.h | 4 +-- 14 files changed, 77 insertions(+), 63 deletions(-) diff --git a/packages/react-native/React/CxxBridge/RCTCxxBridge.mm b/packages/react-native/React/CxxBridge/RCTCxxBridge.mm index 0af826104e4..23257666977 100644 --- a/packages/react-native/React/CxxBridge/RCTCxxBridge.mm +++ b/packages/react-native/React/CxxBridge/RCTCxxBridge.mm @@ -1566,7 +1566,7 @@ RCT_NOT_IMPLEMENTED(-(instancetype)initWithBundleURL return _reactInstance->getJavaScriptContext(); } -- (void)invokeAsync:(std::function &&)func +- (void)invokeAsync:(CallFunc &&)func { __block auto retainedFunc = std::move(func); __weak __typeof(self) weakSelf = self; diff --git a/packages/react-native/ReactCommon/callinvoker/ReactCommon/CallInvoker.h b/packages/react-native/ReactCommon/callinvoker/ReactCommon/CallInvoker.h index 85d9aa8bc48..e5b4167264d 100644 --- a/packages/react-native/ReactCommon/callinvoker/ReactCommon/CallInvoker.h +++ b/packages/react-native/ReactCommon/callinvoker/ReactCommon/CallInvoker.h @@ -12,9 +12,13 @@ #include "SchedulerPriority.h" +namespace facebook::jsi { +class Runtime; +} + namespace facebook::react { -using CallFunc = std::function; +using CallFunc = std::function; /** * An interface for a generic native-to-JS call invoker. See BridgeJSCallInvoker @@ -31,15 +35,29 @@ class CallInvoker { invokeAsync(std::move(func)); } virtual void invokeSync(CallFunc&& func) = 0; + + // Backward compatibility only, prefer the CallFunc methods instead + virtual void invokeAsync(std::function&& func) noexcept { + invokeAsync([func](jsi::Runtime&) { func(); }); + } + + virtual void invokeSync(std::function&& func) { + invokeSync([func](jsi::Runtime&) { func(); }); + } + virtual ~CallInvoker() {} }; +using NativeMethodCallFunc = std::function; + class NativeMethodCallInvoker { public: virtual void invokeAsync( const std::string& methodName, - CallFunc&& func) noexcept = 0; - virtual void invokeSync(const std::string& methodName, CallFunc&& func) = 0; + NativeMethodCallFunc&& func) noexcept = 0; + virtual void invokeSync( + const std::string& methodName, + NativeMethodCallFunc&& func) = 0; virtual ~NativeMethodCallInvoker() {} }; diff --git a/packages/react-native/ReactCommon/cxxreact/Instance.cpp b/packages/react-native/ReactCommon/cxxreact/Instance.cpp index 28721278219..ed0a2443efc 100644 --- a/packages/react-native/ReactCommon/cxxreact/Instance.cpp +++ b/packages/react-native/ReactCommon/cxxreact/Instance.cpp @@ -279,14 +279,13 @@ void Instance::JSCallInvoker::setNativeToJsBridgeAndFlushCalls( } } -void Instance::JSCallInvoker::invokeSync(std::function&& work) { +void Instance::JSCallInvoker::invokeSync(CallFunc&& /*work*/) { // TODO: Replace JS Callinvoker with RuntimeExecutor. throw std::runtime_error( "Synchronous native -> JS calls are currently not supported."); } -void Instance::JSCallInvoker::invokeAsync( - std::function&& work) noexcept { +void Instance::JSCallInvoker::invokeAsync(CallFunc&& work) noexcept { std::scoped_lock guard(m_mutex); /** @@ -311,12 +310,12 @@ void Instance::JSCallInvoker::invokeAsync( scheduleAsync(std::move(work)); } -void Instance::JSCallInvoker::scheduleAsync( - std::function&& work) noexcept { +void Instance::JSCallInvoker::scheduleAsync(CallFunc&& work) noexcept { if (auto strongNativeToJsBridge = m_nativeToJsBridge.lock()) { strongNativeToJsBridge->runOnExecutorQueue( [work = std::move(work)](JSExecutor* executor) { - work(); + auto* runtime = (jsi::Runtime*)executor->getJavaScriptContext(); + work(*runtime); executor->flush(); }); } diff --git a/packages/react-native/ReactCommon/cxxreact/Instance.h b/packages/react-native/ReactCommon/cxxreact/Instance.h index 630c7946d2e..547c971dfe9 100644 --- a/packages/react-native/ReactCommon/cxxreact/Instance.h +++ b/packages/react-native/ReactCommon/cxxreact/Instance.h @@ -164,15 +164,15 @@ class RN_EXPORT Instance : private jsinspector_modern::InstanceTargetDelegate { std::weak_ptr m_nativeToJsBridge; std::mutex m_mutex; bool m_shouldBuffer = true; - std::list> m_workBuffer; + std::list m_workBuffer; - void scheduleAsync(std::function&& work) noexcept; + void scheduleAsync(CallFunc&& work) noexcept; public: void setNativeToJsBridgeAndFlushCalls( std::weak_ptr nativeToJsBridge); - void invokeAsync(std::function&& work) noexcept override; - void invokeSync(std::function&& work) override; + void invokeAsync(CallFunc&& work) noexcept override; + void invokeSync(CallFunc&& work) override; }; std::shared_ptr jsCallInvoker_ = diff --git a/packages/react-native/ReactCommon/cxxreact/NativeToJsBridge.cpp b/packages/react-native/ReactCommon/cxxreact/NativeToJsBridge.cpp index f23ce412fae..6595550001f 100644 --- a/packages/react-native/ReactCommon/cxxreact/NativeToJsBridge.cpp +++ b/packages/react-native/ReactCommon/cxxreact/NativeToJsBridge.cpp @@ -326,14 +326,14 @@ NativeToJsBridge::getDecoratedNativeMethodCallInvoker( void invokeAsync( const std::string& methodName, - std::function&& func) noexcept override { + NativeMethodCallFunc&& func) noexcept override { if (auto strongJsToNativeBridge = m_jsToNativeBridge.lock()) { strongJsToNativeBridge->recordTurboModuleAsyncMethodCall(); } m_nativeInvoker->invokeAsync(methodName, std::move(func)); } - void invokeSync(const std::string& methodName, std::function&& func) + void invokeSync(const std::string& methodName, NativeMethodCallFunc&& func) override { m_nativeInvoker->invokeSync(methodName, std::move(func)); } diff --git a/packages/react-native/ReactCommon/react/bridging/Function.h b/packages/react-native/ReactCommon/react/bridging/Function.h index 0760e6abbc4..fc124bc0a7f 100644 --- a/packages/react-native/ReactCommon/react/bridging/Function.h +++ b/packages/react-native/ReactCommon/react/bridging/Function.h @@ -64,9 +64,8 @@ class AsyncCallback { if (auto wrapper = callback_->wrapper_.lock()) { auto fn = [callback = callback_, argsPtr = std::make_shared>( - std::make_tuple(std::forward(args)...))] { - callback->apply(std::move(*argsPtr)); - }; + std::make_tuple(std::forward(args)...))]( + jsi::Runtime&) { callback->apply(std::move(*argsPtr)); }; auto& jsInvoker = wrapper->jsInvoker(); if (priority) { @@ -85,9 +84,10 @@ class AsyncCallback { // Capture callback_ and not wrapper_. If callback_ is deallocated or the // JSVM is shutdown before the async task is scheduled, the underlying // function will have been deallocated. - auto fn = [callback = callback_, callImpl = std::move(callImpl)]() { + auto fn = [callback = callback_, + callImpl = std::move(callImpl)](jsi::Runtime& rt) { if (auto wrapper2 = callback->wrapper_.lock()) { - callImpl(wrapper2->runtime(), wrapper2->callback()); + callImpl(rt, wrapper2->callback()); } }; diff --git a/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.h b/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.h index 9b60ec82f69..ffc4b49d67b 100644 --- a/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.h +++ b/packages/react-native/ReactCommon/react/bridging/tests/BridgingTest.h @@ -17,18 +17,18 @@ namespace facebook::react { class TestCallInvoker : public CallInvoker { public: - void invokeAsync(std::function&& fn) noexcept override { + void invokeAsync(CallFunc&& fn) noexcept override { queue_.push_back(std::move(fn)); } - void invokeSync(std::function&&) override { + void invokeSync(CallFunc&&) override { FAIL() << "JSCallInvoker does not support invokeSync()"; } private: friend class BridgingTest; - std::list> queue_; + std::list queue_; }; class BridgingTest : public ::testing::Test { @@ -63,7 +63,7 @@ class BridgingTest : public ::testing::Test { void flushQueue() { while (!invoker->queue_.empty()) { - invoker->queue_.front()(); + invoker->queue_.front()(*runtime); invoker->queue_.pop_front(); rt.drainMicrotasks(); // Run microtasks every cycle. } diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.cpp b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.cpp index d1704b57b9f..f0de6f80d20 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.cpp +++ b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboCxxModule.cpp @@ -33,24 +33,23 @@ CxxModule::Callback makeTurboCxxModuleCallback( return; } - strongWrapper->jsInvoker().invokeAsync([weakWrapper, args]() { - auto strongWrapper2 = weakWrapper.lock(); - if (!strongWrapper2) { - return; - } + strongWrapper->jsInvoker().invokeAsync( + [weakWrapper, args](jsi::Runtime& rt) { + auto strongWrapper2 = weakWrapper.lock(); + if (!strongWrapper2) { + return; + } - std::vector innerArgs; - for (auto& a : args) { - innerArgs.push_back( - jsi::valueFromDynamic(strongWrapper2->runtime(), a)); - } - strongWrapper2->callback().call( - strongWrapper2->runtime(), - (const jsi::Value*)innerArgs.data(), - innerArgs.size()); + std::vector innerArgs; + innerArgs.reserve(args.size()); + for (auto& a : args) { + innerArgs.push_back(jsi::valueFromDynamic(rt, a)); + } + strongWrapper2->callback().call( + rt, (const jsi::Value*)innerArgs.data(), innerArgs.size()); - strongWrapper2->destroy(); - }); + strongWrapper2->destroy(); + }); wrapperWasCalled = true; }; diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp index b0924be26e1..cf165e30e12 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp +++ b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp @@ -43,22 +43,20 @@ void TurboModule::emitDeviceEvent( jsi::Runtime& runtime, const std::string& eventName, ArgFactory argFactory) { - jsInvoker_->invokeAsync([&runtime, eventName, argFactory]() { - jsi::Value emitter = - runtime.global().getProperty(runtime, "__rctDeviceEventEmitter"); + jsInvoker_->invokeAsync([eventName, argFactory](jsi::Runtime& rt) { + jsi::Value emitter = rt.global().getProperty(rt, "__rctDeviceEventEmitter"); if (!emitter.isUndefined()) { - jsi::Object emitterObject = emitter.asObject(runtime); + jsi::Object emitterObject = emitter.asObject(rt); // TODO: consider caching these jsi::Function emitFunction = - emitterObject.getPropertyAsFunction(runtime, "emit"); + emitterObject.getPropertyAsFunction(rt, "emit"); std::vector args; - args.emplace_back( - jsi::String::createFromAscii(runtime, eventName.c_str())); + args.emplace_back(jsi::String::createFromAscii(rt, eventName.c_str())); if (argFactory) { - argFactory(runtime, args); + argFactory(rt, args); } emitFunction.callWithThis( - runtime, emitterObject, (const jsi::Value*)args.data(), args.size()); + rt, emitterObject, (const jsi::Value*)args.data(), args.size()); } }); } diff --git a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeSchedulerCallInvoker.cpp b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeSchedulerCallInvoker.cpp index 081118f2c31..10bd57157f7 100644 --- a/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeSchedulerCallInvoker.cpp +++ b/packages/react-native/ReactCommon/react/renderer/runtimescheduler/RuntimeSchedulerCallInvoker.cpp @@ -19,14 +19,14 @@ RuntimeSchedulerCallInvoker::RuntimeSchedulerCallInvoker( void RuntimeSchedulerCallInvoker::invokeAsync(CallFunc&& func) noexcept { if (auto runtimeScheduler = runtimeScheduler_.lock()) { runtimeScheduler->scheduleWork( - [func = std::move(func)](jsi::Runtime&) { func(); }); + [func = std::move(func)](jsi::Runtime& rt) { func(rt); }); } } void RuntimeSchedulerCallInvoker::invokeSync(CallFunc&& func) { if (auto runtimeScheduler = runtimeScheduler_.lock()) { runtimeScheduler->executeNowOnTheSameThread( - [func = std::move(func)](jsi::Runtime&) { func(); }); + [func = std::move(func)](jsi::Runtime& rt) { func(rt); }); } } @@ -35,7 +35,7 @@ void RuntimeSchedulerCallInvoker::invokeAsync( CallFunc&& func) noexcept { if (auto runtimeScheduler = runtimeScheduler_.lock()) { runtimeScheduler->scheduleTask( - priority, [func = std::move(func)](jsi::Runtime&) { func(); }); + priority, [func = std::move(func)](jsi::Runtime& rt) { func(rt); }); } } diff --git a/packages/react-native/ReactCommon/react/runtime/BridgelessJSCallInvoker.cpp b/packages/react-native/ReactCommon/react/runtime/BridgelessJSCallInvoker.cpp index 33b151a1e1f..20519e6a3b4 100644 --- a/packages/react-native/ReactCommon/react/runtime/BridgelessJSCallInvoker.cpp +++ b/packages/react-native/ReactCommon/react/runtime/BridgelessJSCallInvoker.cpp @@ -15,12 +15,12 @@ BridgelessJSCallInvoker::BridgelessJSCallInvoker( RuntimeExecutor runtimeExecutor) : runtimeExecutor_(std::move(runtimeExecutor)) {} -void BridgelessJSCallInvoker::invokeAsync( - std::function&& func) noexcept { - runtimeExecutor_([func = std::move(func)](jsi::Runtime& runtime) { func(); }); +void BridgelessJSCallInvoker::invokeAsync(CallFunc&& func) noexcept { + runtimeExecutor_( + [func = std::move(func)](jsi::Runtime& runtime) { func(runtime); }); } -void BridgelessJSCallInvoker::invokeSync(std::function&& func) { +void BridgelessJSCallInvoker::invokeSync(CallFunc&& /*func*/) { // TODO: Implement this method. The TurboModule infra doesn't call invokeSync. throw std::runtime_error( "Synchronous native -> JS calls are currently not supported."); diff --git a/packages/react-native/ReactCommon/react/runtime/BridgelessJSCallInvoker.h b/packages/react-native/ReactCommon/react/runtime/BridgelessJSCallInvoker.h index b6308c4169c..a411c5e8a03 100644 --- a/packages/react-native/ReactCommon/react/runtime/BridgelessJSCallInvoker.h +++ b/packages/react-native/ReactCommon/react/runtime/BridgelessJSCallInvoker.h @@ -20,8 +20,8 @@ namespace facebook::react { class BridgelessJSCallInvoker : public CallInvoker { public: explicit BridgelessJSCallInvoker(RuntimeExecutor runtimeExecutor); - void invokeAsync(std::function&& func) noexcept override; - void invokeSync(std::function&& func) override; + void invokeAsync(CallFunc&& func) noexcept override; + void invokeSync(CallFunc&& func) override; private: RuntimeExecutor runtimeExecutor_; diff --git a/packages/react-native/ReactCommon/react/runtime/BridgelessNativeMethodCallInvoker.cpp b/packages/react-native/ReactCommon/react/runtime/BridgelessNativeMethodCallInvoker.cpp index e5f3e6813dc..f81cba0e9e1 100644 --- a/packages/react-native/ReactCommon/react/runtime/BridgelessNativeMethodCallInvoker.cpp +++ b/packages/react-native/ReactCommon/react/runtime/BridgelessNativeMethodCallInvoker.cpp @@ -15,13 +15,13 @@ BridgelessNativeMethodCallInvoker::BridgelessNativeMethodCallInvoker( void BridgelessNativeMethodCallInvoker::invokeAsync( const std::string& methodName, - std::function&& func) noexcept { + NativeMethodCallFunc&& func) noexcept { messageQueueThread_->runOnQueue(std::move(func)); } void BridgelessNativeMethodCallInvoker::invokeSync( const std::string& methodName, - std::function&& func) { + NativeMethodCallFunc&& func) { messageQueueThread_->runOnQueueSync(std::move(func)); } diff --git a/packages/react-native/ReactCommon/react/runtime/BridgelessNativeMethodCallInvoker.h b/packages/react-native/ReactCommon/react/runtime/BridgelessNativeMethodCallInvoker.h index 952a48427ef..60509b09867 100644 --- a/packages/react-native/ReactCommon/react/runtime/BridgelessNativeMethodCallInvoker.h +++ b/packages/react-native/ReactCommon/react/runtime/BridgelessNativeMethodCallInvoker.h @@ -20,8 +20,8 @@ class BridgelessNativeMethodCallInvoker : public NativeMethodCallInvoker { std::shared_ptr messageQueueThread); void invokeAsync( const std::string& methodName, - std::function&& func) noexcept override; - void invokeSync(const std::string& methodName, std::function&& func) + NativeMethodCallFunc&& func) noexcept override; + void invokeSync(const std::string& methodName, NativeMethodCallFunc&& func) override; private: