From 31c25a4abdfaeabdcc08b0dd858f0c5e6757a2e1 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Wed, 14 Feb 2024 05:42:59 -0800 Subject: [PATCH 1/2] [skip ci] RuntimeTarget refactor - Add RuntimeExecutor to RuntimeTarget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Changelog: [Internal] I'm refactoring the way the Runtime concept works in the modern CDP backend to bring it in line with the Page/Instance concepts. Overall, this will let us: * Integrate with engines that require us to instantiate a shared Target-like object (e.g. Hermes AsyncDebuggingAPI) in addition to an per-session Agent-like object. * Access JSI in a CDP context (both at target setup/teardown time and during a CDP session) to implement our own engine-agnostic functionality (`console` interception, `Runtime.addBinding`, etc). * Manage CDP execution contexts natively in RN, and (down the line) enable first-class debugging support for multiple Runtimes in an Instance. The core diffs in this stack will: * ~~Introduce a `RuntimeTarget` class similar to `{Page,Instance}Target`. ~~ * ~~Make runtime registration explicit (`InstanceTarget::registerRuntime` similar to `PageTarget::registerInstance`). ~~ * ~~Rename the existing `RuntimeAgent` interface to `RuntimeAgentDelegate`.~~ * ~~Create a new concrete `RuntimeAgent` class similar to `{Page,Instance}Agent`.~~ * Provide `RuntimeTarget` and `RuntimeAgent` with primitives for safe JSI access, namely a `RuntimeExecutor` for scheduling work on the JS thread. *← This diff* * We'll likely develop a similar mechanism for scheduling work on the "main" thread from the JS thread, for when we need to do more than just send a CDP message (which we can already do with the thread-safe `FrontendChannel`) in response to a JS event. ## Architecture diagrams Before this stack: https://pxl.cl/4h7m0 After this stack: https://pxl.cl/4h7m7 Reviewed By: hoxyq Differential Revision: D53266710 --- .../ReactCommon/cxxreact/Instance.cpp | 4 ++- .../jsinspector-modern/CMakeLists.txt | 1 + .../jsinspector-modern/InstanceTarget.cpp | 5 ++-- .../jsinspector-modern/InstanceTarget.h | 4 ++- .../React-jsinspector.podspec | 1 + .../jsinspector-modern/RuntimeTarget.cpp | 7 +++-- .../jsinspector-modern/RuntimeTarget.h | 8 +++++- .../tests/PageTargetTest.cpp | 28 ++++++++++++------- .../react/runtime/ReactInstance.cpp | 10 ++++--- 9 files changed, 47 insertions(+), 21 deletions(-) diff --git a/packages/react-native/ReactCommon/cxxreact/Instance.cpp b/packages/react-native/ReactCommon/cxxreact/Instance.cpp index 702ed3a88c84..01c03ee97854 100644 --- a/packages/react-native/ReactCommon/cxxreact/Instance.cpp +++ b/packages/react-native/ReactCommon/cxxreact/Instance.cpp @@ -67,8 +67,10 @@ void Instance::initializeBridge( if (parentInspectorTarget) { inspectorTarget_ = &parentInspectorTarget->registerInstance(*this); + RuntimeExecutor runtimeExecutorIfJsi = getRuntimeExecutor(); runtimeInspectorTarget_ = &inspectorTarget_->registerRuntime( - nativeToJsBridge_->getInspectorTargetDelegate()); + nativeToJsBridge_->getInspectorTargetDelegate(), + runtimeExecutorIfJsi ? runtimeExecutorIfJsi : [](auto) {}); } /** diff --git a/packages/react-native/ReactCommon/jsinspector-modern/CMakeLists.txt b/packages/react-native/ReactCommon/jsinspector-modern/CMakeLists.txt index ee8290b07439..cbbc44f6dd01 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/CMakeLists.txt +++ b/packages/react-native/ReactCommon/jsinspector-modern/CMakeLists.txt @@ -22,4 +22,5 @@ target_link_libraries(jsinspector folly_runtime glog react_featureflags + runtimeexecutor ) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp index 920973fa027f..7736862fb4cb 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp @@ -46,9 +46,10 @@ InstanceTarget::~InstanceTarget() { } RuntimeTarget& InstanceTarget::registerRuntime( - RuntimeTargetDelegate& delegate) { + RuntimeTargetDelegate& delegate, + RuntimeExecutor executor) { assert(!currentRuntime_ && "Only one Runtime allowed"); - currentRuntime_.emplace(delegate); + currentRuntime_.emplace(delegate, executor); forEachAgent([currentRuntime = &*currentRuntime_](InstanceAgent& agent) { agent.setCurrentRuntime(currentRuntime); }); diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h index 5be5e97b7c50..87fca391e38a 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h @@ -59,7 +59,9 @@ class InstanceTarget final { FrontendChannel channel, SessionState& sessionState); - RuntimeTarget& registerRuntime(RuntimeTargetDelegate& delegate); + RuntimeTarget& registerRuntime( + RuntimeTargetDelegate& delegate, + RuntimeExecutor executor); void unregisterRuntime(RuntimeTarget& runtime); private: diff --git a/packages/react-native/ReactCommon/jsinspector-modern/React-jsinspector.podspec b/packages/react-native/ReactCommon/jsinspector-modern/React-jsinspector.podspec index 53089c6ca95c..12c619ccd9e8 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/React-jsinspector.podspec +++ b/packages/react-native/ReactCommon/jsinspector-modern/React-jsinspector.podspec @@ -51,4 +51,5 @@ Pod::Spec.new do |s| s.dependency "RCT-Folly", folly_version s.dependency "React-featureflags" s.dependency "DoubleConversion" + s.dependency "React-runtimeexecutor", version end diff --git a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp index c56003dc958c..ea260acec027 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp @@ -8,8 +8,11 @@ #include namespace facebook::react::jsinspector_modern { -RuntimeTarget::RuntimeTarget(RuntimeTargetDelegate& delegate) - : delegate_(delegate) {} + +RuntimeTarget::RuntimeTarget( + RuntimeTargetDelegate& delegate, + RuntimeExecutor executor) + : delegate_(delegate), executor_(executor) {} std::shared_ptr RuntimeTarget::createAgent( FrontendChannel channel, diff --git a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h index a067ed10e70b..f83f3a66b680 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h @@ -7,6 +7,7 @@ #pragma once +#include #include "InspectorInterfaces.h" #include "RuntimeAgent.h" #include "SessionState.h" @@ -53,8 +54,12 @@ class JSINSPECTOR_EXPORT RuntimeTarget final { * \param delegate The object that will receive events from this target. * The caller is responsible for ensuring that the delegate outlives this * object. + * \param executor A RuntimeExecutor that can be used to schedule work on + * the JS runtime's thread. The executor's queue should be empty when + * RuntimeTarget is constructed (i.e. anything scheduled during the + * constructor should be executed before any user code is run). */ - explicit RuntimeTarget(RuntimeTargetDelegate& delegate); + RuntimeTarget(RuntimeTargetDelegate& delegate, RuntimeExecutor executor); RuntimeTarget(const RuntimeTarget&) = delete; RuntimeTarget(RuntimeTarget&&) = delete; @@ -77,6 +82,7 @@ class JSINSPECTOR_EXPORT RuntimeTarget final { private: RuntimeTargetDelegate& delegate_; + RuntimeExecutor executor_; std::list> agents_; /** diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/PageTargetTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/PageTargetTest.cpp index 9f68661833e5..205b8bc6b3f7 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/PageTargetTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/PageTargetTest.cpp @@ -56,6 +56,9 @@ class PageTargetTest : public Test { MockInstanceTargetDelegate instanceTargetDelegate_; MockRuntimeTargetDelegate runtimeTargetDelegate_; + // We don't have access to a jsi::Runtime in these tests, so just use an + // executor that never runs the scheduled callbacks. + RuntimeExecutor runtimeExecutor_ = [](auto) {}; UniquePtrFactory> runtimeAgentDelegates_; @@ -252,7 +255,8 @@ TEST_F(PageTargetTest, ConnectToAlreadyRegisteredInstanceWithEvents) { TEST_F(PageTargetTest, ConnectToAlreadyRegisteredRuntimeWithEvents) { auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); - auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_); + auto& runtimeTarget = + instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); connect(); @@ -287,8 +291,8 @@ TEST_F(PageTargetTest, ConnectToAlreadyRegisteredRuntimeWithEvents) { TEST_F(PageTargetProtocolTest, RuntimeAgentDelegateLifecycle) { { auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); - auto& runtimeTarget = - instanceTarget.registerRuntime(runtimeTargetDelegate_); + auto& runtimeTarget = instanceTarget.registerRuntime( + runtimeTargetDelegate_, runtimeExecutor_); EXPECT_TRUE(runtimeAgentDelegates_[0]); @@ -300,8 +304,8 @@ TEST_F(PageTargetProtocolTest, RuntimeAgentDelegateLifecycle) { { auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); - auto& runtimeTarget = - instanceTarget.registerRuntime(runtimeTargetDelegate_); + auto& runtimeTarget = instanceTarget.registerRuntime( + runtimeTargetDelegate_, runtimeExecutor_); EXPECT_TRUE(runtimeAgentDelegates_[1]); @@ -316,7 +320,8 @@ TEST_F(PageTargetProtocolTest, MethodNotHandledByRuntimeAgentDelegate) { InSequence s; auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); - auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_); + auto& runtimeTarget = + instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); ASSERT_TRUE(runtimeAgentDelegates_[0]); EXPECT_CALL(*runtimeAgentDelegates_[0], handleRequest(_)) @@ -341,7 +346,8 @@ TEST_F(PageTargetProtocolTest, MethodHandledByRuntimeAgentDelegate) { InSequence s; auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); - auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_); + auto& runtimeTarget = + instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); ASSERT_TRUE(runtimeAgentDelegates_[0]); EXPECT_CALL(*runtimeAgentDelegates_[0], handleRequest(_)) @@ -384,7 +390,8 @@ TEST_F(PageTargetProtocolTest, MessageRoutingWhileNoRuntimeAgentDelegate) { })"); auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); - auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_); + auto& runtimeTarget = + instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); ASSERT_TRUE(runtimeAgentDelegates_[0]); EXPECT_CALL(*runtimeAgentDelegates_[0], handleRequest(_)) @@ -432,7 +439,8 @@ TEST_F(PageTargetProtocolTest, InstanceWithNullRuntimeAgentDelegate) { .WillRepeatedly(ReturnNull()); auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); - auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_); + auto& runtimeTarget = + instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); EXPECT_FALSE(runtimeAgentDelegates_[0]); @@ -466,7 +474,7 @@ TEST_F(PageTargetProtocolTest, RuntimeAgentDelegateHasAccessToSessionState) { })"); auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); - instanceTarget.registerRuntime(runtimeTargetDelegate_); + instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); ASSERT_TRUE(runtimeAgentDelegates_[0]); EXPECT_TRUE(runtimeAgentDelegates_[0]->sessionState.isRuntimeDomainEnabled); diff --git a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp index 0f5fca84bab8..afefd87605c4 100644 --- a/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp +++ b/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp @@ -37,10 +37,6 @@ ReactInstance::ReactInstance( jsErrorHandler_(jsErrorHandlingFunc), hasFatalJsError_(std::make_shared(false)), parentInspectorTarget_(parentInspectorTarget) { - if (parentInspectorTarget_) { - inspectorTarget_ = &parentInspectorTarget_->registerInstance(*this); - runtimeInspectorTarget_ = &inspectorTarget_->registerRuntime(*runtime_); - } auto runtimeExecutor = [weakRuntime = std::weak_ptr(runtime_), weakTimerManager = std::weak_ptr(timerManager_), @@ -88,6 +84,12 @@ ReactInstance::ReactInstance( } }; + if (parentInspectorTarget_) { + inspectorTarget_ = &parentInspectorTarget_->registerInstance(*this); + runtimeInspectorTarget_ = + &inspectorTarget_->registerRuntime(*runtime_, runtimeExecutor); + } + runtimeScheduler_ = std::make_shared(std::move(runtimeExecutor)); From 98fcbb6144093a7b0d36f5b9c7e39f55d1787094 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Wed, 14 Feb 2024 05:42:59 -0800 Subject: [PATCH 2/2] RuntimeTarget refactor - Add `this`-scoped async executors to all Targets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Changelog: [Internal] # This diff 1. Provides all Targets with an `executorFromThis()` method, which can be used from within a Target to access a *`this`-scoped main thread executor* = a `std::function` that will execute a callback asynchronously iff the current Target isn't destroyed first. 2. Refactors how (all) Target objects are constructed and retained, from a plain constructor to `static shared_ptr create()`. This is because `executorFromThis()` relies internally on `enable_shared_from_this` plus two-phase construction to populate the executor. 3. Creates utilities for deriving scoped executors from other executors and `shared_ptr`s. The concept is very much like `RuntimeExecutor` in reverse: the #1 use case is moving from the JS thread back to the main thread - where "main thread" is defined loosely as "anywhere it's legal to call methods on Target/Agent objects, access session state, etc". The actual dispatching mechanism is left up to the owner of each `PageTarget` object; for now we only have an iOS integration, where we use `RCTExecuteOnMainQueue`. Coupling the ownership/lifetime semantics with task scheduling is helpful, because it avoids the footgun of accidentally/nondeterministically moving `shared_ptr`s (and destructors!) to a different thread/queue . # This stack I'm refactoring the way the Runtime concept works in the modern CDP backend to bring it in line with the Page/Instance concepts. Overall, this will let us: * Integrate with engines that require us to instantiate a shared Target-like object (e.g. Hermes AsyncDebuggingAPI) in addition to an per-session Agent-like object. * Access JSI in a CDP context (both at target setup/teardown time and during a CDP session) to implement our own engine-agnostic functionality (`console` interception, `Runtime.addBinding`, etc). * Manage CDP execution contexts natively in RN, and (down the line) enable first-class debugging support for multiple Runtimes in an Instance. The core diffs in this stack: * ~~Introduce a `RuntimeTarget` class similar to `{Page,Instance}Target`. ~~ * ~~Make runtime registration explicit (`InstanceTarget::registerRuntime` similar to `PageTarget::registerInstance`). ~~ * ~~Rename the existing `RuntimeAgent` interface to `RuntimeAgentDelegate`.~~ * ~~Create a new concrete `RuntimeAgent` class similar to `{Page,Instance}Agent`.~~ * ~~Provide `RuntimeTarget` and `RuntimeAgent` with primitives for safe JSI access, namely a `RuntimeExecutor` for scheduling work on the JS thread.~~ * Provide RuntimeTarget with mechanism for scheduling work on the "main" thread from the JS thread, for when we need to do more than just send a CDP message (which we can already do with the thread-safe `FrontendChannel`) in response to a JS event. *← This diff* ## Architecture diagrams Before this stack: https://pxl.cl/4h7m0 After this stack: https://pxl.cl/4h7m7 Reviewed By: hoxyq Differential Revision: D53356953 --- packages/react-native/React/Base/RCTBridge.mm | 9 +- .../jsinspector-modern/InstanceTarget.cpp | 19 +++- .../jsinspector-modern/InstanceTarget.h | 22 ++++- .../jsinspector-modern/PageTarget.cpp | 15 ++- .../jsinspector-modern/PageTarget.h | 32 ++++-- .../ReactCommon/jsinspector-modern/ReactCdp.h | 1 + .../jsinspector-modern/RuntimeTarget.cpp | 14 ++- .../jsinspector-modern/RuntimeTarget.h | 31 +++++- .../jsinspector-modern/ScopedExecutor.h | 97 +++++++++++++++++++ .../tests/PageTargetTest.cpp | 58 ++++++----- .../platform/ios/ReactCommon/RCTHost.mm | 9 +- 11 files changed, 253 insertions(+), 54 deletions(-) create mode 100644 packages/react-native/ReactCommon/jsinspector-modern/ScopedExecutor.h diff --git a/packages/react-native/React/Base/RCTBridge.mm b/packages/react-native/React/Base/RCTBridge.mm index 3d5520b9cb91..65d827e163be 100644 --- a/packages/react-native/React/Base/RCTBridge.mm +++ b/packages/react-native/React/Base/RCTBridge.mm @@ -209,7 +209,7 @@ @implementation RCTBridge { NSURL *_delegateBundleURL; std::unique_ptr _inspectorPageDelegate; - std::unique_ptr _inspectorTarget; + std::shared_ptr _inspectorTarget; std::optional _inspectorPageId; } @@ -412,7 +412,12 @@ - (void)setUp auto &inspectorFlags = facebook::react::jsinspector_modern::InspectorFlags::getInstance(); if (inspectorFlags.getEnableModernCDPRegistry() && !_inspectorPageId.has_value()) { - _inspectorTarget = std::make_unique(*_inspectorPageDelegate); + _inspectorTarget = + facebook::react::jsinspector_modern::PageTarget::create(*_inspectorPageDelegate, [](auto callback) { + RCTExecuteOnMainQueue(^{ + callback(); + }); + }); __weak RCTBridge *weakSelf = self; _inspectorPageId = facebook::react::jsinspector_modern::getInspectorInstance().addPage( "React Native Bridge (Experimental)", diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp index 7736862fb4cb..060045c27032 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.cpp @@ -12,6 +12,14 @@ namespace facebook::react::jsinspector_modern { +std::shared_ptr InstanceTarget::create( + InstanceTargetDelegate& delegate, + VoidExecutor executor) { + std::shared_ptr instanceTarget{new InstanceTarget(delegate)}; + instanceTarget->setExecutor(executor); + return instanceTarget; +} + InstanceTarget::InstanceTarget(InstanceTargetDelegate& delegate) : delegate_(delegate) { (void)delegate_; @@ -24,8 +32,7 @@ std::shared_ptr InstanceTarget::createAgent( SessionState& sessionState) { auto instanceAgent = std::make_shared(channel, *this, sessionState); - instanceAgent->setCurrentRuntime( - currentRuntime_.has_value() ? &*currentRuntime_ : nullptr); + instanceAgent->setCurrentRuntime(currentRuntime_.get()); agents_.push_back(instanceAgent); return instanceAgent; } @@ -47,9 +54,11 @@ InstanceTarget::~InstanceTarget() { RuntimeTarget& InstanceTarget::registerRuntime( RuntimeTargetDelegate& delegate, - RuntimeExecutor executor) { + RuntimeExecutor jsExecutor) { assert(!currentRuntime_ && "Only one Runtime allowed"); - currentRuntime_.emplace(delegate, executor); + currentRuntime_ = RuntimeTarget::create( + delegate, jsExecutor, makeVoidExecutor(executorFromThis())); + forEachAgent([currentRuntime = &*currentRuntime_](InstanceAgent& agent) { agent.setCurrentRuntime(currentRuntime); }); @@ -58,7 +67,7 @@ RuntimeTarget& InstanceTarget::registerRuntime( void InstanceTarget::unregisterRuntime(RuntimeTarget& Runtime) { assert( - currentRuntime_.has_value() && ¤tRuntime_.value() == &Runtime && + currentRuntime_ && currentRuntime_.get() == &Runtime && "Invalid unregistration"); forEachAgent([](InstanceAgent& agent) { agent.setCurrentRuntime(nullptr); }); currentRuntime_.reset(); diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h index 87fca391e38a..266dc5dec873 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InstanceTarget.h @@ -8,6 +8,7 @@ #pragma once #include "RuntimeTarget.h" +#include "ScopedExecutor.h" #include "SessionState.h" #include @@ -40,14 +41,20 @@ class InstanceTargetDelegate { /** * A Target that represents a single instance of React Native. */ -class InstanceTarget final { +class InstanceTarget : public EnableExecutorFromThis { public: /** + * Constructs a new InstanceTarget. * \param delegate The object that will receive events from this target. * The caller is responsible for ensuring that the delegate outlives this * object. + * \param executor An executor that may be used to call methods on this + * InstanceTarget while it exists. \c create additionally guarantees that the + * executor will not be called after the InstanceTarget is destroyed. */ - explicit InstanceTarget(InstanceTargetDelegate& delegate); + static std::shared_ptr create( + InstanceTargetDelegate& delegate, + VoidExecutor executor); InstanceTarget(const InstanceTarget&) = delete; InstanceTarget(InstanceTarget&&) = delete; @@ -65,8 +72,17 @@ class InstanceTarget final { void unregisterRuntime(RuntimeTarget& runtime); private: + /** + * Constructs a new InstanceTarget. The caller must call setExecutor + * immediately afterwards. + * \param delegate The object that will receive events from this target. + * The caller is responsible for ensuring that the delegate outlives this + * object. + */ + InstanceTarget(InstanceTargetDelegate& delegate); + InstanceTargetDelegate& delegate_; - std::optional currentRuntime_{std::nullopt}; + std::shared_ptr currentRuntime_{nullptr}; std::list> agents_; /** diff --git a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp index 54f263e2342e..2b3d32fd0ab8 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.cpp @@ -99,6 +99,14 @@ class PageTargetSession { SessionState state_; }; +std::shared_ptr PageTarget::create( + PageTargetDelegate& delegate, + VoidExecutor executor) { + std::shared_ptr pageTarget{new PageTarget(delegate)}; + pageTarget->setExecutor(executor); + return pageTarget; +} + PageTarget::PageTarget(PageTargetDelegate& delegate) : delegate_(delegate) {} std::unique_ptr PageTarget::connect( @@ -106,7 +114,7 @@ std::unique_ptr PageTarget::connect( SessionMetadata sessionMetadata) { auto session = std::make_shared( std::move(connectionToFrontend), controller_, std::move(sessionMetadata)); - session->setCurrentInstance(currentInstance_ ? &*currentInstance_ : nullptr); + session->setCurrentInstance(currentInstance_.get()); sessions_.push_back(std::weak_ptr(session)); return std::make_unique( [session](std::string message) { (*session)(message); }); @@ -131,7 +139,8 @@ PageTargetDelegate::~PageTargetDelegate() {} InstanceTarget& PageTarget::registerInstance(InstanceTargetDelegate& delegate) { assert(!currentInstance_ && "Only one instance allowed"); - currentInstance_.emplace(delegate); + currentInstance_ = + InstanceTarget::create(delegate, makeVoidExecutor(executorFromThis())); forEachSession( [currentInstance = &*currentInstance_](PageTargetSession& session) { session.setCurrentInstance(currentInstance); @@ -141,7 +150,7 @@ InstanceTarget& PageTarget::registerInstance(InstanceTargetDelegate& delegate) { void PageTarget::unregisterInstance(InstanceTarget& instance) { assert( - currentInstance_.has_value() && ¤tInstance_.value() == &instance && + currentInstance_ && currentInstance_.get() == &instance && "Invalid unregistration"); forEachSession( [](PageTargetSession& session) { session.setCurrentInstance(nullptr); }); diff --git a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h index a3e592650ebc..c1d874a68c31 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/PageTarget.h @@ -7,6 +7,8 @@ #pragma once +#include "ScopedExecutor.h" + #include #include @@ -95,7 +97,8 @@ class PageTargetController final { * "Host" in React Native's architecture - the entity that manages the * lifecycle of a React Instance. */ -class JSINSPECTOR_EXPORT PageTarget final { +class JSINSPECTOR_EXPORT PageTarget + : public EnableExecutorFromThis { public: struct SessionMetadata { std::optional integrationName; @@ -103,11 +106,16 @@ class JSINSPECTOR_EXPORT PageTarget final { /** * Constructs a new PageTarget. - * \param delegate The PageTargetDelegate that will receive events from - * this PageTarget. The caller is responsible for ensuring that the - * PageTargetDelegate outlives this object. + * \param delegate The PageTargetDelegate that will + * receive events from this PageTarget. The caller is responsible for ensuring + * that the PageTargetDelegate outlives this object. + * \param executor An executor that may be used to call methods on this + * PageTarget while it exists. \c create additionally guarantees that the + * executor will not be called after the PageTarget is destroyed. */ - explicit PageTarget(PageTargetDelegate& delegate); + static std::shared_ptr create( + PageTargetDelegate& delegate, + VoidExecutor executor); PageTarget(const PageTarget&) = delete; PageTarget(PageTarget&&) = delete; @@ -146,11 +154,19 @@ class JSINSPECTOR_EXPORT PageTarget final { void unregisterInstance(InstanceTarget& instance); private: - std::list> sessions_; + /** + * Constructs a new PageTarget. + * The caller must call setExecutor immediately afterwards. + * \param delegate The PageTargetDelegate that will + * receive events from this PageTarget. The caller is responsible for ensuring + * that the PageTargetDelegate outlives this object. + */ + PageTarget(PageTargetDelegate& delegate); PageTargetDelegate& delegate_; + std::list> sessions_; PageTargetController controller_{*this}; - std::optional currentInstance_{std::nullopt}; + std::shared_ptr currentInstance_{nullptr}; /** * Call the given function for every active session, and clean up any @@ -175,7 +191,7 @@ class JSINSPECTOR_EXPORT PageTarget final { } inline bool hasInstance() const { - return currentInstance_.has_value(); + return currentInstance_ != nullptr; } // Necessary to allow PageAgent to access PageTarget's internals in a diff --git a/packages/react-native/ReactCommon/jsinspector-modern/ReactCdp.h b/packages/react-native/ReactCommon/jsinspector-modern/ReactCdp.h index 78524402152c..d81d695867e9 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/ReactCdp.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/ReactCdp.h @@ -11,4 +11,5 @@ #include #include #include +#include #include diff --git a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp index ea260acec027..677351365746 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.cpp @@ -9,10 +9,20 @@ namespace facebook::react::jsinspector_modern { +std::shared_ptr RuntimeTarget::create( + RuntimeTargetDelegate& delegate, + RuntimeExecutor jsExecutor, + VoidExecutor selfExecutor) { + std::shared_ptr runtimeTarget{ + new RuntimeTarget(delegate, jsExecutor)}; + runtimeTarget->setExecutor(selfExecutor); + return runtimeTarget; +} + RuntimeTarget::RuntimeTarget( RuntimeTargetDelegate& delegate, - RuntimeExecutor executor) - : delegate_(delegate), executor_(executor) {} + RuntimeExecutor jsExecutor) + : delegate_(delegate), jsExecutor_(jsExecutor) {} std::shared_ptr RuntimeTarget::createAgent( FrontendChannel channel, diff --git a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h index f83f3a66b680..6a954a05f1d8 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/RuntimeTarget.h @@ -10,6 +10,7 @@ #include #include "InspectorInterfaces.h" #include "RuntimeAgent.h" +#include "ScopedExecutor.h" #include "SessionState.h" #include @@ -48,18 +49,27 @@ class RuntimeTargetDelegate { /** * A Target corresponding to a JavaScript runtime. */ -class JSINSPECTOR_EXPORT RuntimeTarget final { +class JSINSPECTOR_EXPORT RuntimeTarget + : public EnableExecutorFromThis { public: /** + * Constructs a new RuntimeTarget. The caller must call setExecutor + * immediately afterwards. * \param delegate The object that will receive events from this target. * The caller is responsible for ensuring that the delegate outlives this * object. - * \param executor A RuntimeExecutor that can be used to schedule work on + * \param jsExecutor A RuntimeExecutor that can be used to schedule work on * the JS runtime's thread. The executor's queue should be empty when * RuntimeTarget is constructed (i.e. anything scheduled during the * constructor should be executed before any user code is run). + * \param selfExecutor An executor that may be used to call methods on this + * RuntimeTarget while it exists. \c create additionally guarantees that the + * executor will not be called after the RuntimeTarget is destroyed. */ - RuntimeTarget(RuntimeTargetDelegate& delegate, RuntimeExecutor executor); + static std::shared_ptr create( + RuntimeTargetDelegate& delegate, + RuntimeExecutor jsExecutor, + VoidExecutor selfExecutor); RuntimeTarget(const RuntimeTarget&) = delete; RuntimeTarget(RuntimeTarget&&) = delete; @@ -81,8 +91,21 @@ class JSINSPECTOR_EXPORT RuntimeTarget final { SessionState& sessionState); private: + /** + * Constructs a new RuntimeTarget. The caller must call setExecutor + * immediately afterwards. + * \param delegate The object that will receive events from this target. + * The caller is responsible for ensuring that the delegate outlives this + * object. + * \param jsExecutor A RuntimeExecutor that can be used to schedule work on + * the JS runtime's thread. The executor's queue should be empty when + * RuntimeTarget is constructed (i.e. anything scheduled during the + * constructor should be executed before any user code is run). + */ + RuntimeTarget(RuntimeTargetDelegate& delegate, RuntimeExecutor jsExecutor); + RuntimeTargetDelegate& delegate_; - RuntimeExecutor executor_; + RuntimeExecutor jsExecutor_; std::list> agents_; /** diff --git a/packages/react-native/ReactCommon/jsinspector-modern/ScopedExecutor.h b/packages/react-native/ReactCommon/jsinspector-modern/ScopedExecutor.h new file mode 100644 index 000000000000..6ede8b356d9d --- /dev/null +++ b/packages/react-native/ReactCommon/jsinspector-modern/ScopedExecutor.h @@ -0,0 +1,97 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include +#include +#include + +namespace facebook::react::jsinspector_modern { + +/** + * Takes a function and calls it when it is safe to access the Self& + * parameter without locking. The function is not called if + * the underlying Self object is destroyed while the function is pending. + */ +template +using ScopedExecutor = + std::function&& callback)>; + +using VoidExecutor = std::function&& callback)>; + +/** + * Creates a ScopedExecutor from a shared (weak) pointer to Self plus some + * base executor for a "parent" object of Self. The resulting executor will call + * the callback with a valid reference to Self iff Self is still alive. + */ +template +ScopedExecutor makeScopedExecutor( + std::shared_ptr self, + ScopedExecutor executor) { + return makeScopedExecutor(self, makeVoidExecutor(executor)); +} + +/** + * Creates a ScopedExecutor from a shared (weak) pointer to Self plus some + * base executor. The resulting executor will call the callback with a valid + * reference to Self iff Self is still alive. + */ +template +ScopedExecutor makeScopedExecutor( + std::shared_ptr self, + VoidExecutor executor) { + return [self = std::weak_ptr(self), executor](auto&& callback) { + executor([self, callback = std::move(callback)]() { + auto lockedSelf = self.lock(); + if (!lockedSelf) { + return; + } + callback(*lockedSelf); + }); + }; +} + +/** + * Creates a VoidExecutor from a ScopedExecutor by ignoring the Self& + * parameter. + */ +template +VoidExecutor makeVoidExecutor(ScopedExecutor executor) { + return [executor](auto&& callback) { + executor([callback = std::move(callback)](Self&) { callback(); }); + }; +} + +template +class EnableExecutorFromThis : public std::enable_shared_from_this { + public: + /** + * Returns an executor that can be used to safely invoke methods on Self. + * Must not be called during the constructor of Self. + */ + ScopedExecutor executorFromThis() { + assert(baseExecutor_); + return makeScopedExecutor(this->shared_from_this(), baseExecutor_); + } + + template + void setExecutor(ScopedExecutor executor) { + setExecutor(makeVoidExecutor(executor)); + } + + void setExecutor(VoidExecutor executor) { + assert(executor); + assert(!baseExecutor_); + baseExecutor_ = std::move(executor); + } + + private: + VoidExecutor baseExecutor_; +}; + +}; // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/PageTargetTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/PageTargetTest.cpp index 205b8bc6b3f7..efddba194720 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/PageTargetTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/PageTargetTest.cpp @@ -6,6 +6,7 @@ */ #include +#include #include #include #include @@ -26,6 +27,8 @@ namespace facebook::react::jsinspector_modern { namespace { class PageTargetTest : public Test { + folly::QueuedImmediateExecutor immediateExecutor_; + protected: PageTargetTest() { EXPECT_CALL(runtimeTargetDelegate_, createAgentDelegate(_, _)) @@ -36,7 +39,7 @@ class PageTargetTest : public Test { void connect() { ASSERT_FALSE(toPage_) << "Can only connect once in a PageTargetTest."; - toPage_ = page_.connect( + toPage_ = page_->connect( remoteConnections_.make_unique(), {.integrationName = "PageTargetTest"}); @@ -52,7 +55,12 @@ class PageTargetTest : public Test { return *remoteConnections_[0]; } - PageTarget page_{pageTargetDelegate_}; + VoidExecutor inspectorExecutor_ = [this](auto callback) { + immediateExecutor_.add(callback); + }; + + std::shared_ptr page_ = + PageTarget::create(pageTargetDelegate_, inspectorExecutor_); MockInstanceTargetDelegate instanceTargetDelegate_; MockRuntimeTargetDelegate runtimeTargetDelegate_; @@ -197,17 +205,17 @@ TEST_F(PageTargetProtocolTest, PageReloadMethod) { } TEST_F(PageTargetProtocolTest, RegisterUnregisterInstanceWithoutEvents) { - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } TEST_F(PageTargetTest, ConnectToAlreadyRegisteredInstanceWithoutEvents) { - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); connect(); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } TEST_F(PageTargetProtocolTest, RegisterUnregisterInstanceWithEvents) { @@ -222,16 +230,16 @@ TEST_F(PageTargetProtocolTest, RegisterUnregisterInstanceWithEvents) { "method": "Runtime.enable" })"); - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); EXPECT_CALL(fromPage(), onMessage(JsonEq(R"({ "method": "Runtime.executionContextsCleared" })"))); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } TEST_F(PageTargetTest, ConnectToAlreadyRegisteredInstanceWithEvents) { - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); connect(); @@ -250,11 +258,11 @@ TEST_F(PageTargetTest, ConnectToAlreadyRegisteredInstanceWithEvents) { EXPECT_CALL(fromPage(), onMessage(JsonEq(R"({ "method": "Runtime.executionContextsCleared" })"))); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } TEST_F(PageTargetTest, ConnectToAlreadyRegisteredRuntimeWithEvents) { - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); @@ -285,32 +293,32 @@ TEST_F(PageTargetTest, ConnectToAlreadyRegisteredRuntimeWithEvents) { runtimeAgentDelegates_[0]->frontendChannel(kFooResponse); instanceTarget.unregisterRuntime(runtimeTarget); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } TEST_F(PageTargetProtocolTest, RuntimeAgentDelegateLifecycle) { { - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); auto& runtimeTarget = instanceTarget.registerRuntime( runtimeTargetDelegate_, runtimeExecutor_); EXPECT_TRUE(runtimeAgentDelegates_[0]); instanceTarget.unregisterRuntime(runtimeTarget); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } EXPECT_FALSE(runtimeAgentDelegates_[0]); { - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); auto& runtimeTarget = instanceTarget.registerRuntime( runtimeTargetDelegate_, runtimeExecutor_); EXPECT_TRUE(runtimeAgentDelegates_[1]); instanceTarget.unregisterRuntime(runtimeTarget); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } EXPECT_FALSE(runtimeAgentDelegates_[1]); @@ -319,7 +327,7 @@ TEST_F(PageTargetProtocolTest, RuntimeAgentDelegateLifecycle) { TEST_F(PageTargetProtocolTest, MethodNotHandledByRuntimeAgentDelegate) { InSequence s; - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); @@ -339,13 +347,13 @@ TEST_F(PageTargetProtocolTest, MethodNotHandledByRuntimeAgentDelegate) { })"); instanceTarget.unregisterRuntime(runtimeTarget); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } TEST_F(PageTargetProtocolTest, MethodHandledByRuntimeAgentDelegate) { InSequence s; - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); @@ -372,7 +380,7 @@ TEST_F(PageTargetProtocolTest, MethodHandledByRuntimeAgentDelegate) { runtimeAgentDelegates_[0]->frontendChannel(kFooResponse); instanceTarget.unregisterRuntime(runtimeTarget); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } TEST_F(PageTargetProtocolTest, MessageRoutingWhileNoRuntimeAgentDelegate) { @@ -389,7 +397,7 @@ TEST_F(PageTargetProtocolTest, MessageRoutingWhileNoRuntimeAgentDelegate) { } })"); - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); @@ -416,7 +424,7 @@ TEST_F(PageTargetProtocolTest, MessageRoutingWhileNoRuntimeAgentDelegate) { runtimeAgentDelegates_[0]->frontendChannel(kFooResponse); instanceTarget.unregisterRuntime(runtimeTarget); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); EXPECT_FALSE(runtimeAgentDelegates_[0]); @@ -438,7 +446,7 @@ TEST_F(PageTargetProtocolTest, InstanceWithNullRuntimeAgentDelegate) { EXPECT_CALL(runtimeTargetDelegate_, createAgentDelegate(_, _)) .WillRepeatedly(ReturnNull()); - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); auto& runtimeTarget = instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); @@ -456,7 +464,7 @@ TEST_F(PageTargetProtocolTest, InstanceWithNullRuntimeAgentDelegate) { })"); instanceTarget.unregisterRuntime(runtimeTarget); - page_.unregisterInstance(instanceTarget); + page_->unregisterInstance(instanceTarget); } TEST_F(PageTargetProtocolTest, RuntimeAgentDelegateHasAccessToSessionState) { @@ -473,7 +481,7 @@ TEST_F(PageTargetProtocolTest, RuntimeAgentDelegateHasAccessToSessionState) { "method": "Runtime.enable" })"); - auto& instanceTarget = page_.registerInstance(instanceTargetDelegate_); + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); ASSERT_TRUE(runtimeAgentDelegates_[0]); diff --git a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm index 01c1b499ff7a..85602c4c8a6f 100644 --- a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm +++ b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm @@ -68,7 +68,7 @@ @implementation RCTHost { RCTModuleRegistry *_moduleRegistry; std::unique_ptr _inspectorPageDelegate; - std::unique_ptr _inspectorTarget; + std::shared_ptr _inspectorTarget; std::optional _inspectorPageId; } @@ -168,7 +168,12 @@ - (void)start { auto &inspectorFlags = jsinspector_modern::InspectorFlags::getInstance(); if (inspectorFlags.getEnableModernCDPRegistry() && !_inspectorPageId.has_value()) { - _inspectorTarget = std::make_unique(*_inspectorPageDelegate); + _inspectorTarget = + facebook::react::jsinspector_modern::PageTarget::create(*_inspectorPageDelegate, [](auto callback) { + RCTExecuteOnMainQueue(^{ + callback(); + }); + }); __weak RCTHost *weakSelf = self; _inspectorPageId = facebook::react::jsinspector_modern::getInspectorInstance().addPage( "React Native Bridgeless (Experimental)",