From 1c8f2232fbf9595e126b06944015047ba5cc51df Mon Sep 17 00:00:00 2001 From: Ruslan Shestopalyuk Date: Fri, 8 Mar 2024 06:57:25 -0800 Subject: [PATCH] Pass jsi::Runtime reference into CallInvoker::invoke* callbacks (#43375) Summary: ## 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 --- .../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 0af826104e4898..232576669771be 100644 --- a/packages/react-native/React/CxxBridge/RCTCxxBridge.mm +++ b/packages/react-native/React/CxxBridge/RCTCxxBridge.mm @@ -1566,7 +1566,7 @@ - (void *)runtime 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 85d9aa8bc4812b..e5b4167264d7b9 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 287212782191ef..ed0a2443efcf5d 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 630c7946d2eab5..547c971dfe98c6 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 f23ce412fae178..6595550001faed 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 0760e6abbc4cdc..fc124bc0a7f266 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 9b60ec82f691ed..ffc4b49d67be9a 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 d1704b57b9f53e..f0de6f80d20c97 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 b0924be26e1ab8..cf165e30e1215f 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 081118f2c31ed5..10bd57157f747e 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 33b151a1e1fd5f..20519e6a3b4c88 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 b6308c4169c4f8..a411c5e8a03aca 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 e5f3e6813dc923..f81cba0e9e1356 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 952a48427ef325..60509b098678e4 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: