From 6fcfe2e1b35e9bdf319bcdc1647c8c0d997b58c7 Mon Sep 17 00:00:00 2001 From: Matt Blagden Date: Fri, 12 Aug 2022 06:43:46 -0700 Subject: [PATCH] Identify debug sessions with a token Summary: The `RuntimeAdapter` may be used after `disableDebugging` has been called. To ensure the `RuntimeAdapter` is alive long enough for this use, the Inspector should continue to control `RuntimeAdapter`'s lifetime (which is currently done via a `unique_ptr` that's moved into the Inspector). Callers need a way to identify the `RuntimeAdapter` after it has been moved into the Inspector so that debugging can be disabled via `disableDebugging`. This could be done by switching from a `unique_ptr` to a `shared_ptr` (so the caller can keep a copy), but consumers don't really have a reason to hang onto the `RuntimeAdapter` instance. Instead, leave the `RuntimeAdapter` inside a `unique_ptr`, and have consumers use a token to identify instances. Update all consumers of this API to use this new token interface. Changelog: [Internal] Reviewed By: jpporto Differential Revision: D38513256 fbshipit-source-id: 33580747cd8365d25dbddbe289f0c41141e3bc6a --- .../hermes/executor/HermesExecutorFactory.cpp | 13 ++++++------- .../inspector/chrome/ConnectionDemux.cpp | 19 +++++-------------- .../hermes/inspector/chrome/ConnectionDemux.h | 5 +++-- .../hermes/inspector/chrome/Registration.cpp | 8 ++++---- .../hermes/inspector/chrome/Registration.h | 14 +++++++++----- .../chrome/tests/ConnectionDemuxTests.cpp | 2 +- 6 files changed, 28 insertions(+), 33 deletions(-) diff --git a/ReactCommon/hermes/executor/HermesExecutorFactory.cpp b/ReactCommon/hermes/executor/HermesExecutorFactory.cpp index 3bb4e0c7b08..53a454f8822 100644 --- a/ReactCommon/hermes/executor/HermesExecutorFactory.cpp +++ b/ReactCommon/hermes/executor/HermesExecutorFactory.cpp @@ -149,21 +149,18 @@ class DecoratedRuntime : public jsi::WithRuntimeDecorator { HermesRuntime &hermesRuntime, std::shared_ptr jsQueue) : jsi::WithRuntimeDecorator(*runtime, reentrancyCheck_), - runtime_(std::move(runtime)), - hermesRuntime_(hermesRuntime) { + runtime_(std::move(runtime)) { #ifdef HERMES_ENABLE_DEBUGGER std::shared_ptr rt(runtime_, &hermesRuntime); auto adapter = std::make_unique(rt, jsQueue); - facebook::hermes::inspector::chrome::enableDebugging( + debugToken_ = facebook::hermes::inspector::chrome::enableDebugging( std::move(adapter), "Hermes React Native"); -#else - (void)hermesRuntime_; #endif } ~DecoratedRuntime() { #ifdef HERMES_ENABLE_DEBUGGER - facebook::hermes::inspector::chrome::disableDebugging(hermesRuntime_); + facebook::hermes::inspector::chrome::disableDebugging(debugToken_); #endif } @@ -177,7 +174,9 @@ class DecoratedRuntime : public jsi::WithRuntimeDecorator { std::shared_ptr runtime_; ReentrancyCheck reentrancyCheck_; - HermesRuntime &hermesRuntime_; +#ifdef HERMES_ENABLE_DEBUGGER + facebook::hermes::inspector::chrome::DebugSessionToken debugToken_; +#endif }; } // namespace diff --git a/ReactCommon/hermes/inspector/chrome/ConnectionDemux.cpp b/ReactCommon/hermes/inspector/chrome/ConnectionDemux.cpp index 31694decf11..e8b81786ac2 100644 --- a/ReactCommon/hermes/inspector/chrome/ConnectionDemux.cpp +++ b/ReactCommon/hermes/inspector/chrome/ConnectionDemux.cpp @@ -63,7 +63,7 @@ ConnectionDemux::ConnectionDemux(facebook::react::IInspector &inspector) ConnectionDemux::~ConnectionDemux() = default; -int ConnectionDemux::enableDebugging( +DebugSessionToken ConnectionDemux::enableDebugging( std::unique_ptr adapter, const std::string &title) { std::lock_guard lock(mutex_); @@ -94,21 +94,12 @@ int ConnectionDemux::enableDebugging( std::make_shared(std::move(adapter), title, waitForDebugger)); } -void ConnectionDemux::disableDebugging(HermesRuntime &runtime) { +void ConnectionDemux::disableDebugging(DebugSessionToken session) { std::lock_guard lock(mutex_); - - for (auto &it : conns_) { - int pageId = it.first; - auto &conn = it.second; - - if (&(conn->getRuntime()) == &runtime) { - removePage(pageId); - - // must break here. removePage mutates conns_, so range-for iterator is - // now invalid. - break; - } + if (conns_.find(session) == conns_.end()) { + return; } + removePage(session); } int ConnectionDemux::addPage(std::shared_ptr conn) { diff --git a/ReactCommon/hermes/inspector/chrome/ConnectionDemux.h b/ReactCommon/hermes/inspector/chrome/ConnectionDemux.h index d7697bc6b4f..e9510b4b060 100644 --- a/ReactCommon/hermes/inspector/chrome/ConnectionDemux.h +++ b/ReactCommon/hermes/inspector/chrome/ConnectionDemux.h @@ -16,6 +16,7 @@ #include #include #include +#include #include namespace facebook { @@ -36,10 +37,10 @@ class ConnectionDemux { ConnectionDemux(const ConnectionDemux &) = delete; ConnectionDemux &operator=(const ConnectionDemux &) = delete; - int enableDebugging( + DebugSessionToken enableDebugging( std::unique_ptr adapter, const std::string &title); - void disableDebugging(HermesRuntime &runtime); + void disableDebugging(DebugSessionToken session); private: int addPage(std::shared_ptr conn); diff --git a/ReactCommon/hermes/inspector/chrome/Registration.cpp b/ReactCommon/hermes/inspector/chrome/Registration.cpp index d64a08ebbfc..4560b94950c 100644 --- a/ReactCommon/hermes/inspector/chrome/Registration.cpp +++ b/ReactCommon/hermes/inspector/chrome/Registration.cpp @@ -22,14 +22,14 @@ ConnectionDemux &demux() { } // namespace -void enableDebugging( +DebugSessionToken enableDebugging( std::unique_ptr adapter, const std::string &title) { - demux().enableDebugging(std::move(adapter), title); + return demux().enableDebugging(std::move(adapter), title); } -void disableDebugging(HermesRuntime &runtime) { - demux().disableDebugging(runtime); +void disableDebugging(DebugSessionToken session) { + demux().disableDebugging(session); } } // namespace chrome diff --git a/ReactCommon/hermes/inspector/chrome/Registration.h b/ReactCommon/hermes/inspector/chrome/Registration.h index 9acff574df8..d4a5ef57892 100644 --- a/ReactCommon/hermes/inspector/chrome/Registration.h +++ b/ReactCommon/hermes/inspector/chrome/Registration.h @@ -18,20 +18,24 @@ namespace hermes { namespace inspector { namespace chrome { +using DebugSessionToken = int; + /* * enableDebugging adds this runtime to the list of debuggable JS targets - * (called "pages" in the higher-leavel React Native API) in this process. It - * should be called before any JS runs in the runtime. + * (called "pages" in the higher-level React Native API) in this process. It + * should be called before any JS runs in the runtime. The returned token + * can be used to disable debugging for this runtime. */ -extern void enableDebugging( +extern DebugSessionToken enableDebugging( std::unique_ptr adapter, const std::string &title); /* * disableDebugging removes this runtime from the list of debuggable JS targets - * in this process. + * in this process. The runtime to remove is identified by the token returned + * from enableDebugging. */ -extern void disableDebugging(HermesRuntime &runtime); +extern void disableDebugging(DebugSessionToken session); } // namespace chrome } // namespace inspector diff --git a/ReactCommon/hermes/inspector/chrome/tests/ConnectionDemuxTests.cpp b/ReactCommon/hermes/inspector/chrome/tests/ConnectionDemuxTests.cpp index 08c86996dc9..02f38c085ce 100644 --- a/ReactCommon/hermes/inspector/chrome/tests/ConnectionDemuxTests.cpp +++ b/ReactCommon/hermes/inspector/chrome/tests/ConnectionDemuxTests.cpp @@ -124,7 +124,7 @@ TEST(ConnectionDemuxTests, TestEnableDisable) { // Disable debugging on runtime2. This should remove its page from the list // and call onDisconnect on its remoteConn - demux.disableDebugging(*runtime2); + demux.disableDebugging(id2); expectPages(*inspector, {{id1, "page1"}}); remoteData2->expectDisconnected();