From 3e7a873f2d1c5170a7f4c88064897e74a149c5d5 Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Thu, 14 Sep 2023 04:31:36 -0700 Subject: [PATCH] Migrate from legacy to new Hermes CDP handler (#39367) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/39367 Updates React Native to use the new CDP handler provided by the Hermes engine instead of the legacy one (`Connection.cpp`) built into React Native. The new Hermes CDP handler has a simpler & safer design, new features (e.g. `console.log` buffering) and is under active development by the Hermes team. NOTE: Both the legacy and modern handlers are Hermes-specific. In future work, React Native will *wrap* the modern Hermes handler in an engine-agnostic CDP layer implementing additional functionality and managing the lifecycle of debugging sessions more correctly. This diff is the first step of this larger migration. Changelog: [General][Changed] Use new Hermes CDP handler implementation for debugging Reviewed By: cipolleschi Differential Revision: D48783980 fbshipit-source-id: 4d2ca3fa04e96e92a38d82c90737cb660ba56655 --- .../hermes/executor/HermesExecutorFactory.cpp | 12 +++++- .../chrome/ConnectionDemux.cpp | 37 +++++++++++++------ .../inspector-modern/chrome/ConnectionDemux.h | 16 ++++++-- .../inspector-modern/chrome/Registration.cpp | 4 ++ .../inspector-modern/chrome/Registration.h | 6 ++- .../react/runtime/hermes/CMakeLists.txt | 8 ++++ .../react/runtime/hermes/HermesInstance.cpp | 2 +- 7 files changed, 67 insertions(+), 18 deletions(-) diff --git a/packages/react-native/ReactCommon/hermes/executor/HermesExecutorFactory.cpp b/packages/react-native/ReactCommon/hermes/executor/HermesExecutorFactory.cpp index 82b0e58ac7a61a..8ae8766f5e3e84 100644 --- a/packages/react-native/ReactCommon/hermes/executor/HermesExecutorFactory.cpp +++ b/packages/react-native/ReactCommon/hermes/executor/HermesExecutorFactory.cpp @@ -14,8 +14,8 @@ #include #include -#include #include +#include #include "JSITracing.h" @@ -26,6 +26,8 @@ namespace facebook::react { namespace { +#ifdef HERMES_ENABLE_DEBUGGER + class HermesExecutorRuntimeAdapter : public facebook::hermes::inspector_modern::RuntimeAdapter { public: @@ -56,6 +58,8 @@ class HermesExecutorRuntimeAdapter std::shared_ptr thread_; }; +#endif // HERMES_ENABLE_DEBUGGER + struct ReentrancyCheck { // This is effectively a very subtle and complex assert, so only // include it in builds which would include asserts. @@ -139,6 +143,7 @@ class DecoratedRuntime : public jsi::WithRuntimeDecorator { const std::string& debuggerName) : jsi::WithRuntimeDecorator(*runtime, reentrancyCheck_), runtime_(std::move(runtime)) { +#ifdef HERMES_ENABLE_DEBUGGER enableDebugger_ = enableDebugger; if (enableDebugger_) { std::shared_ptr rt(runtime_, &hermesRuntime); @@ -147,12 +152,15 @@ class DecoratedRuntime : public jsi::WithRuntimeDecorator { debugToken_ = facebook::hermes::inspector_modern::chrome::enableDebugging( std::move(adapter), debuggerName); } +#endif // HERMES_ENABLE_DEBUGGER } ~DecoratedRuntime() { +#ifdef HERMES_ENABLE_DEBUGGER if (enableDebugger_) { facebook::hermes::inspector_modern::chrome::disableDebugging(debugToken_); } +#endif // HERMES_ENABLE_DEBUGGER } private: @@ -165,8 +173,10 @@ class DecoratedRuntime : public jsi::WithRuntimeDecorator { std::shared_ptr runtime_; ReentrancyCheck reentrancyCheck_; +#ifdef HERMES_ENABLE_DEBUGGER bool enableDebugger_; facebook::hermes::inspector_modern::chrome::DebugSessionToken debugToken_; +#endif // HERMES_ENABLE_DEBUGGER }; } // namespace diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/ConnectionDemux.cpp b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/ConnectionDemux.cpp index 8a7dabf3e15629..5ab0955ecd9888 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/ConnectionDemux.cpp +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/ConnectionDemux.cpp @@ -6,7 +6,11 @@ */ #include "ConnectionDemux.h" -#include "Connection.h" + +#ifdef HERMES_ENABLE_DEBUGGER + +#include +#include #include @@ -24,7 +28,7 @@ namespace { class LocalConnection : public ILocalConnection { public: LocalConnection( - std::shared_ptr conn, + std::shared_ptr conn, std::shared_ptr> inspectedContexts); ~LocalConnection(); @@ -32,12 +36,12 @@ class LocalConnection : public ILocalConnection { void disconnect() override; private: - std::shared_ptr conn_; + std::shared_ptr conn_; std::shared_ptr> inspectedContexts_; }; LocalConnection::LocalConnection( - std::shared_ptr conn, + std::shared_ptr conn, std::shared_ptr> inspectedContexts) : conn_(conn), inspectedContexts_(inspectedContexts) { inspectedContexts_->insert(conn->getTitle()); @@ -46,12 +50,12 @@ LocalConnection::LocalConnection( LocalConnection::~LocalConnection() = default; void LocalConnection::sendMessage(std::string str) { - conn_->sendMessage(std::move(str)); + conn_->handle(std::move(str)); } void LocalConnection::disconnect() { inspectedContexts_->erase(conn_->getTitle()); - conn_->disconnect(); + conn_->unregisterCallbacks(); } } // namespace @@ -87,8 +91,8 @@ DebugSessionToken ConnectionDemux::enableDebugging( auto waitForDebugger = (inspectedContexts_->find(title) != inspectedContexts_->end()); - return addPage( - std::make_shared(std::move(adapter), title, waitForDebugger)); + return addPage(hermes::inspector_modern::chrome::CDPHandler::create( + std::move(adapter), title, waitForDebugger)); } void ConnectionDemux::disableDebugging(DebugSessionToken session) { @@ -99,10 +103,19 @@ void ConnectionDemux::disableDebugging(DebugSessionToken session) { removePage(session); } -int ConnectionDemux::addPage(std::shared_ptr conn) { +int ConnectionDemux::addPage( + std::shared_ptr conn) { auto connectFunc = [conn, this](std::unique_ptr remoteConn) -> std::unique_ptr { - if (!conn->connect(std::move(remoteConn))) { + // This cannot be unique_ptr as std::function is copyable but unique_ptr + // isn't. TODO: Change the CDPHandler API to accommodate this and not + // require a copyable callback? + std::shared_ptr sharedConn = std::move(remoteConn); + if (!conn->registerCallbacks( + [sharedConn](const std::string &message) { + sharedConn->onMessage(message); + }, + [sharedConn]() { sharedConn->onDisconnect(); })) { return nullptr; } @@ -122,7 +135,7 @@ void ConnectionDemux::removePage(int pageId) { auto conn = conns_.at(pageId); std::string title = conn->getTitle(); inspectedContexts_->erase(title); - conn->disconnect(); + conn->unregisterCallbacks(); conns_.erase(pageId); } @@ -130,3 +143,5 @@ void ConnectionDemux::removePage(int pageId) { } // namespace inspector_modern } // namespace hermes } // namespace facebook + +#endif // HERMES_ENABLE_DEBUGGER diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/ConnectionDemux.h b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/ConnectionDemux.h index 8f30c685a45dc6..9ed90410d20f03 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/ConnectionDemux.h +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/ConnectionDemux.h @@ -7,6 +7,8 @@ #pragma once +#ifdef HERMES_ENABLE_DEBUGGER + #include #include #include @@ -14,9 +16,9 @@ #include #include -#include -#include #include +#include +#include #include namespace facebook { @@ -44,13 +46,17 @@ class ConnectionDemux { void disableDebugging(DebugSessionToken session); private: - int addPage(std::shared_ptr conn); + int addPage( + std::shared_ptr conn); void removePage(int pageId); facebook::react::jsinspector_modern::IInspector &globalInspector_; std::mutex mutex_; - std::unordered_map> conns_; + std::unordered_map< + int, + std::shared_ptr> + conns_; std::shared_ptr> inspectedContexts_; }; @@ -58,3 +64,5 @@ class ConnectionDemux { } // namespace inspector_modern } // namespace hermes } // namespace facebook + +#endif // HERMES_ENABLE_DEBUGGER diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/Registration.cpp b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/Registration.cpp index 3a8c99153ef666..3b73b49e7fbcc1 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/Registration.cpp +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/Registration.cpp @@ -8,6 +8,8 @@ #include "Registration.h" #include "ConnectionDemux.h" +#ifdef HERMES_ENABLE_DEBUGGER + namespace facebook { namespace hermes { namespace inspector_modern { @@ -37,3 +39,5 @@ void disableDebugging(DebugSessionToken session) { } // namespace inspector_modern } // namespace hermes } // namespace facebook + +#endif // HERMES_ENABLE_DEBUGGER diff --git a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/Registration.h b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/Registration.h index 222510498f6a18..bda9300e837fe2 100644 --- a/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/Registration.h +++ b/packages/react-native/ReactCommon/hermes/inspector-modern/chrome/Registration.h @@ -7,11 +7,13 @@ #pragma once +#ifdef HERMES_ENABLE_DEBUGGER + #include #include #include -#include +#include namespace facebook { namespace hermes { @@ -41,3 +43,5 @@ extern void disableDebugging(DebugSessionToken session); } // namespace inspector_modern } // namespace hermes } // namespace facebook + +#endif // HERMES_ENABLE_DEBUGGER diff --git a/packages/react-native/ReactCommon/react/runtime/hermes/CMakeLists.txt b/packages/react-native/ReactCommon/react/runtime/hermes/CMakeLists.txt index c307f14afa27a4..668675dbf9ee86 100644 --- a/packages/react-native/ReactCommon/react/runtime/hermes/CMakeLists.txt +++ b/packages/react-native/ReactCommon/react/runtime/hermes/CMakeLists.txt @@ -23,3 +23,11 @@ target_link_libraries(bridgelesshermes jsi hermes_executor_common ) + +if(${CMAKE_BUILD_TYPE} MATCHES Debug) + target_compile_options( + bridgelesshermes + PRIVATE + -DHERMES_ENABLE_DEBUGGER=1 + ) +endif() diff --git a/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.cpp b/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.cpp index 670ec2f1d01a0a..5c3195aec30f5a 100644 --- a/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.cpp +++ b/packages/react-native/ReactCommon/react/runtime/hermes/HermesInstance.cpp @@ -11,8 +11,8 @@ #include #ifdef HERMES_ENABLE_DEBUGGER -#include #include +#include #include #endif