From 87e0ec6b9be02c6ff1a4b1cc7b6cd34d9a8c0df2 Mon Sep 17 00:00:00 2001 From: Ruslan Shestopalyuk Date: Thu, 7 Mar 2024 18:23:14 -0800 Subject: [PATCH 1/2] 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 | 10 +++---- .../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, 80 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..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..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..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..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: From 2c88b993416594ed34c338f6da54d3a29a330bda Mon Sep 17 00:00:00 2001 From: Ruslan Shestopalyuk Date: Thu, 7 Mar 2024 18:23:14 -0800 Subject: [PATCH 2/2] TurboModule::emitDeviceEvent doesn't require jsi::Runtime argument anymore (#43376) Summary: ## Changelog: [Internal] - Make it possible to call `emitDeviceEvent` from C++ TurboModules without the need to explicitly provide the reference to `jsi::Runtime`, as in some contexts (when we call e.g. not from the JS thread itself) it may be hard to get hold of. Reviewed By: rubennorte Differential Revision: D54643903 --- .../react/nativemodule/core/ReactCommon/TurboModule.cpp | 1 - .../react/nativemodule/core/ReactCommon/TurboModule.h | 9 ++++++++- .../NativeCxxModuleExample/NativeCxxModuleExample.cpp | 1 - 3 files changed, 8 insertions(+), 3 deletions(-) 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 cf165e30e1215f..fe2411974d20cd 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp +++ b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.cpp @@ -40,7 +40,6 @@ TurboModule::TurboModule( : name_(std::move(name)), jsInvoker_(std::move(jsInvoker)) {} void TurboModule::emitDeviceEvent( - jsi::Runtime& runtime, const std::string& eventName, ArgFactory argFactory) { jsInvoker_->invokeAsync([eventName, argFactory](jsi::Runtime& rt) { diff --git a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.h b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.h index ccd9d1f85968ef..08f4fdbc5cef75 100644 --- a/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.h +++ b/packages/react-native/ReactCommon/react/nativemodule/core/ReactCommon/TurboModule.h @@ -106,10 +106,17 @@ class JSI_EXPORT TurboModule : public facebook::jsi::HostObject { * }); */ void emitDeviceEvent( - jsi::Runtime& runtime, const std::string& eventName, ArgFactory argFactory = nullptr); + // Backwards compatibility version + void emitDeviceEvent( + jsi::Runtime& /*runtime*/, + const std::string& eventName, + ArgFactory argFactory = nullptr) { + emitDeviceEvent(eventName, std::move(argFactory)); + } + virtual jsi::Value create( jsi::Runtime& runtime, const jsi::PropNameID& propName) { diff --git a/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.cpp b/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.cpp index abec005d02e00b..0bbc831d838156 100644 --- a/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.cpp +++ b/packages/rn-tester/NativeCxxModuleExample/NativeCxxModuleExample.cpp @@ -184,7 +184,6 @@ void NativeCxxModuleExample::emitCustomDeviceEvent( // Test emitting device events (RCTDeviceEventEmitter.emit) from C++ // TurboModule with arbitrary arguments emitDeviceEvent( - rt, eventName.utf8(rt).c_str(), [](jsi::Runtime& rt, std::vector& args) { args.emplace_back(jsi::Value(true));