From 3bb7b413cde18cbfdcdb698edfad09d2ce59811b Mon Sep 17 00:00:00 2001 From: Ruslan Shestopalyuk Date: Thu, 7 Mar 2024 15:18:06 -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 | 19 ++++++------ .../ReactCommon/cxxreact/Instance.h | 8 ++--- .../ReactCommon/cxxreact/NativeToJsBridge.cpp | 4 +-- .../ReactCommon/react/bridging/Function.h | 11 +++---- .../react/bridging/tests/BridgingTest.h | 8 ++--- .../core/ReactCommon/TurboCxxModule.cpp | 30 +++++++++---------- .../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, 81 insertions(+), 67 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..ee2f07f27da551 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) noexcept { + 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..acc9898cfb47c7 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,14 +310,14 @@ 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(); - executor->flush(); - }); + strongNativeToJsBridge->runOnExecutorQueue([work = std::move(work)]( + JSExecutor* executor) { + jsi::Runtime* 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..656f56d6b2a311 100644 --- a/packages/react-native/ReactCommon/react/bridging/Function.h +++ b/packages/react-native/ReactCommon/react/bridging/Function.h @@ -11,6 +11,7 @@ #include #include +#include "jsi/jsi/jsi.h" namespace facebook::react { @@ -64,9 +65,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 +85,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..00011c3d745d49 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,22 @@ 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; + 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..daa5ab7fd63b7f 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: