From 58ddd74202bede2238768def01898741606ec64f Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Wed, 17 Apr 2024 00:58:02 -0700 Subject: [PATCH] Add HostCommands for resuming and stepping debugger (#44080) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/44080 ## Design Adds a new public `HostTarget::sendCommand` method, enabling integrators to send simple imperative commands to the target. As an implementation detail, the commands are translated internally to CDP and sent over a dedicated `HostTargetSession` (encapsulated in `HostCommandSender`). Any response from the underlying Agent is ignored. From the caller's perspective, these commands don't occur in the context of a session at all, and from the frontend's perspective, only the *effects* of the commands (if any) are seen. ## Use case HostCommands are specifically useful when we want to resume/step execution in response to a UI action. The commands map directly to the `Debugger.resume` and `Debugger.stepOver` CDP methods. NOTE: This is inspired by Chrome/V8's existing support for multiple concurrent CDP sessions. Any CDP client can successfully send `Debugger.resume` and `Debugger.stepOver` (without even subscribing to debugger events using `Debugger.enable`) and affect the state of other ongoing debugging sessions. Changelog: [Internal] Reviewed By: hoxyq Differential Revision: D56098083 fbshipit-source-id: 013ab748b360f700c453cf1447fb82d6d0d77c6f --- .../jsinspector-modern/CdpJson.cpp | 11 +++ .../ReactCommon/jsinspector-modern/CdpJson.h | 18 ++++- .../jsinspector-modern/HostCommand.h | 19 ++++++ .../jsinspector-modern/HostTarget.cpp | 47 +++++++++++++ .../jsinspector-modern/HostTarget.h | 14 +++- .../jsinspector-modern/InspectorUtilities.h | 8 +++ .../tests/HostTargetTest.cpp | 68 +++++++++++++++++++ 7 files changed, 180 insertions(+), 5 deletions(-) create mode 100644 packages/react-native/ReactCommon/jsinspector-modern/HostCommand.h diff --git a/packages/react-native/ReactCommon/jsinspector-modern/CdpJson.cpp b/packages/react-native/ReactCommon/jsinspector-modern/CdpJson.cpp index f9a0a2fc9dc..b5efccb43f7 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/CdpJson.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/CdpJson.cpp @@ -58,4 +58,15 @@ std::string jsonNotification( return folly::toJson(std::move(dynamicNotification)); } +std::string jsonRequest( + RequestId id, + std::string_view method, + std::optional params) { + auto dynamicRequest = folly::dynamic::object("id", id)("method", method); + if (params) { + dynamicRequest("params", *params); + } + return folly::toJson(std::move(dynamicRequest)); +} + } // namespace facebook::react::jsinspector_modern::cdp diff --git a/packages/react-native/ReactCommon/jsinspector-modern/CdpJson.h b/packages/react-native/ReactCommon/jsinspector-modern/CdpJson.h index b77e40f7ef7..dba85f39bec 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/CdpJson.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/CdpJson.h @@ -110,15 +110,29 @@ std::string jsonResult( const folly::dynamic& result = folly::dynamic::object()); /** - * Returns a JSON-formatted string representing a unilateral notifcation. + * Returns a JSON-formatted string representing a unilateral notification. * * {"method": , "params": } * * \param method Notification (aka "event") method. - * \param params Optional payload pbject. + * \param params Optional payload object. */ std::string jsonNotification( std::string_view method, std::optional params = std::nullopt); +/** + * Returns a JSON-formatted string representing a request. + * + * {"id": , "method": , "params": } + * + * \param id Request ID. + * \param method Requested method. + * \param params Optional payload object. + */ +std::string jsonRequest( + RequestId id, + std::string_view method, + std::optional params = std::nullopt); + } // namespace facebook::react::jsinspector_modern::cdp diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostCommand.h b/packages/react-native/ReactCommon/jsinspector-modern/HostCommand.h new file mode 100644 index 00000000000..e064f49114c --- /dev/null +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostCommand.h @@ -0,0 +1,19 @@ +/* + * 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 + +namespace facebook::react::jsinspector_modern { + +enum class HostCommand { + /** Resumes JavaScript execution. */ + DebuggerResume, + /** Steps over the statement. */ + DebuggerStepOver +}; + +} // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp index d880dc6fd27..2e682460a94 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp @@ -98,6 +98,41 @@ class HostTargetSession { HostAgent hostAgent_; }; +/** + * Converts HostCommands to CDP method calls and sends them over a private + * connection to the HostTarget. + */ +class HostCommandSender { + public: + explicit HostCommandSender(HostTarget& target) + : connection_(target.connect(std::make_unique())) {} + + /** + * Send a \c HostCommand to the HostTarget. + */ + void sendCommand(HostCommand command) { + cdp::RequestId id = makeRequestId(); + switch (command) { + case HostCommand::DebuggerResume: + connection_->sendMessage(cdp::jsonRequest(id, "Debugger.resume")); + break; + case HostCommand::DebuggerStepOver: + connection_->sendMessage(cdp::jsonRequest(id, "Debugger.stepOver")); + break; + default: + assert(false && "unknown HostCommand"); + } + } + + private: + cdp::RequestId makeRequestId() { + return nextRequestId_++; + } + + cdp::RequestId nextRequestId_{1}; + std::unique_ptr connection_; +}; + std::shared_ptr HostTarget::create( HostTargetDelegate& delegate, VoidExecutor executor) { @@ -122,6 +157,9 @@ std::unique_ptr HostTarget::connect( } HostTarget::~HostTarget() { + // HostCommandSender owns a session, so we must release it for the assertion + // below to be valid. + commandSender_.reset(); // Sessions are owned by InspectorPackagerConnection, not by HostTarget, but // they hold a HostTarget& that we must guarantee is valid. assert( @@ -151,6 +189,15 @@ void HostTarget::unregisterInstance(InstanceTarget& instance) { currentInstance_.reset(); } +void HostTarget::sendCommand(HostCommand command) { + executorFromThis()([command](HostTarget& self) { + if (!self.commandSender_) { + self.commandSender_ = std::make_unique(self); + } + self.commandSender_->sendCommand(command); + }); +} + HostTargetController::HostTargetController(HostTarget& target) : target_(target) {} diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h index da0f95a1e4b..05fe80b97bc 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h @@ -8,12 +8,12 @@ #pragma once #include "ExecutionContextManager.h" +#include "HostCommand.h" +#include "InspectorInterfaces.h" +#include "InstanceTarget.h" #include "ScopedExecutor.h" #include "WeakList.h" -#include -#include - #include #include @@ -33,6 +33,7 @@ namespace facebook::react::jsinspector_modern { class HostTargetSession; class HostAgent; +class HostCommandSender; class HostTarget; /** @@ -202,6 +203,12 @@ class JSINSPECTOR_EXPORT HostTarget */ void unregisterInstance(InstanceTarget& instance); + /** + * Sends an imperative command to the HostTarget. May be called from any + * thread. + */ + void sendCommand(HostCommand command); + private: /** * Constructs a new HostTarget. @@ -220,6 +227,7 @@ class JSINSPECTOR_EXPORT HostTarget // briefly outliving the HostTarget, which it generally shouldn't). std::shared_ptr executionContextManager_; std::shared_ptr currentInstance_{nullptr}; + std::unique_ptr commandSender_; inline HostTargetDelegate& getDelegate() { return delegate_; diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorUtilities.h b/packages/react-native/ReactCommon/jsinspector-modern/InspectorUtilities.h index 41b6f7c84e3..9c14d1be980 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorUtilities.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorUtilities.h @@ -49,4 +49,12 @@ class RAIIRemoteConnection { std::unique_ptr remote_; }; +/** + * An \c IRemoteConnection that does nothing. + */ +class NullRemoteConnection : public IRemoteConnection { + inline void onMessage(std::string /*message*/) override {} + inline void onDisconnect() override {} +}; + } // namespace facebook::react::jsinspector_modern diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/HostTargetTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/HostTargetTest.cpp index 2f76ace49f3..d9a38c120d6 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/HostTargetTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/HostTargetTest.cpp @@ -632,4 +632,72 @@ TEST_F(HostTargetProtocolTest, RuntimeAgentDelegateHasAccessToSessionState) { EXPECT_FALSE(runtimeAgentDelegates_[0]->sessionState.isRuntimeDomainEnabled); } +TEST_F(HostTargetTest, HostCommands) { + // Set up expectations for the RuntimeAgentDelegate that will be created + // as part of the private session inside HostCommandSender. + EXPECT_CALL(runtimeTargetDelegate_, createAgentDelegate(_, _, _, _, _)) + .WillOnce([this]( + FrontendChannel frontendChannel, + SessionState& sessionState, + std::unique_ptr + exportedState, + const ExecutionContextDescription& context, + RuntimeExecutor runtimeExecutor) { + auto delegate = runtimeAgentDelegates_.make_unique( + std::move(frontendChannel), + sessionState, + std::move(exportedState), + context, + std::move(runtimeExecutor)); + InSequence s; + EXPECT_CALL( + *delegate, + handleRequest( + Field(&cdp::PreparsedRequest::method, "Debugger.resume"))) + .WillOnce(Return(false)) + .RetiresOnSaturation(); + EXPECT_CALL( + *delegate, + handleRequest( + Field(&cdp::PreparsedRequest::method, "Debugger.stepOver"))) + .WillOnce(Return(false)) + .RetiresOnSaturation(); + return delegate; + }) + .RetiresOnSaturation(); + + // No RuntimeAgent yet; this command is simply ignored. + page_->sendCommand(HostCommand::DebuggerStepOver); + EXPECT_FALSE(runtimeAgentDelegates_[0]); + + auto& instanceTarget = page_->registerInstance(instanceTargetDelegate_); + auto& runtimeTarget = + instanceTarget.registerRuntime(runtimeTargetDelegate_, runtimeExecutor_); + + page_->sendCommand(HostCommand::DebuggerResume); + page_->sendCommand(HostCommand::DebuggerStepOver); + ASSERT_TRUE(runtimeAgentDelegates_[0]); + + connect(); + + // This is part of the HostCommandSender session. + ASSERT_TRUE(runtimeAgentDelegates_[0]); + // This is part of the session we just connect()ed to above. + EXPECT_TRUE(runtimeAgentDelegates_[1]); + // We can still send commands. + EXPECT_CALL( + *runtimeAgentDelegates_[0], + handleRequest(Field(&cdp::PreparsedRequest::method, "Debugger.stepOver"))) + .WillOnce(Return(false)) + .RetiresOnSaturation(); + page_->sendCommand(HostCommand::DebuggerStepOver); + + // NOTE: Our use of StrictMock ensures that the session doesn't receive any + // noise resulting from the sendCommand call ( = no + // runtimeAgentDelegates_[1]->handleRequest, no fromPage()->onMessage, etc). + + instanceTarget.unregisterRuntime(runtimeTarget); + page_->unregisterInstance(instanceTarget); +} + } // namespace facebook::react::jsinspector_modern