Use RuntimeAdapter to disable debugging

Summary:
Make the interface to enable/disable debugging symmetrical; both enabling and disabling are done by passing in a reference to the RuntimeAdapter.

Doing so requires moving ownership of the RuntimeAdapter, so each caller (java2js, arfx engine, React Native, react-native-github, and venice) has been updated to own their adapter.

Additionally, the two implementations of the Inspector connection (react-native-github and arfx engine) have been updated to expect and use the new argument.

`Connection::getRuntime` could be removed and replaced with calls to `Connection::getRuntimeAdapter::getRuntime`. I've left that choice to a followup diff, as this one is getting messy enough as it is.

Changelog: [Internal]

Reviewed By: jpporto

Differential Revision: D38242964

fbshipit-source-id: f2a3354f9d424b203a76d2c15122a6515a0926f2

ReactCommon/hermes/executor/HermesExecutorFactory.cpp

@@ -149,35 +149,38 @@ class DecoratedRuntime : public jsi::WithRuntimeDecorator<ReentrancyCheck> {
       HermesRuntime &hermesRuntime,
       std::shared_ptr<MessageQueueThread> jsQueue)
       : jsi::WithRuntimeDecorator<ReentrancyCheck>(*runtime, reentrancyCheck_),
-        runtime_(std::move(runtime)),
-        hermesRuntime_(hermesRuntime) {
+        runtime_(std::move(runtime))
+#ifdef HERMES_ENABLE_DEBUGGER
+        ,
+        adapter_(
+            std::shared_ptr<HermesRuntime>(runtime_, &hermesRuntime),
+            jsQueue)
+#endif
+  {
 #ifdef HERMES_ENABLE_DEBUGGER
-    std::shared_ptr<HermesRuntime> rt(runtime_, &hermesRuntime);
-    auto adapter = std::make_unique<HermesExecutorRuntimeAdapter>(rt, jsQueue);
     facebook::hermes::inspector::chrome::enableDebugging(
-        std::move(adapter), "Hermes React Native");
-#else
-    (void)hermesRuntime_;
+        adapter_, "Hermes React Native");
 #endif
   }
 
   ~DecoratedRuntime() {
 #ifdef HERMES_ENABLE_DEBUGGER
-    facebook::hermes::inspector::chrome::disableDebugging(hermesRuntime_);
+    facebook::hermes::inspector::chrome::disableDebugging(adapter_);
 #endif
   }
 
  private:
   // runtime_ is a potentially decorated Runtime.
-  // hermesRuntime is a reference to a HermesRuntime managed by runtime_.
   //
   // HermesExecutorRuntimeAdapter requirements are kept, because the
   // dtor will disable debugging on the HermesRuntime before the
   // member managing it is destroyed.
 
   std::shared_ptr<Runtime> runtime_;
+#ifdef HERMES_ENABLE_DEBUGGER
+  HermesExecutorRuntimeAdapter adapter_;
+#endif
   ReentrancyCheck reentrancyCheck_;
-  HermesRuntime &hermesRuntime_;
 };
 
 } // namespace

ReactCommon/hermes/inspector/Inspector.cpp

@@ -104,16 +104,16 @@ static constexpr bool kShouldLog = false;
   } while (0)
 
 Inspector::Inspector(
-    std::shared_ptr<RuntimeAdapter> adapter,
+    RuntimeAdapter &adapter,
     InspectorObserver &observer,
     bool pauseOnFirstStatement)
     : adapter_(adapter),
-      debugger_(adapter->getRuntime().getDebugger()),
+      debugger_(adapter.getRuntime().getDebugger()),
       observer_(observer),
       executor_(std::make_unique<detail::SerialExecutor>("hermes-inspector")) {
   // TODO (t26491391): make tickleJs a real Hermes runtime API
   std::string src = "function __tickleJs() { return Math.random(); }";
-  adapter->getRuntime().evaluateJavaScript(
+  adapter.getRuntime().evaluateJavaScript(
       std::make_shared<jsi::StringBuffer>(src), "__tickleJsHackUrl");
 
   {
@@ -163,7 +163,7 @@ void Inspector::installConsoleFunction(
     std::shared_ptr<jsi::Object> &originalConsole,
     const std::string &name,
     const std::string &chromeTypeDefault = "") {
-  jsi::Runtime &rt = adapter_->getRuntime();
+  jsi::Runtime &rt = adapter_.getRuntime();
   auto chromeType = chromeTypeDefault == "" ? name : chromeTypeDefault;
   auto nameID = jsi::PropNameID::forUtf8(rt, name);
   auto weakInspector = std::weak_ptr<Inspector>(shared_from_this());
@@ -222,7 +222,7 @@ void Inspector::installConsoleFunction(
 }
 
 void Inspector::installLogHandler() {
-  jsi::Runtime &rt = adapter_->getRuntime();
+  jsi::Runtime &rt = adapter_.getRuntime();
   auto console = jsi::Object(rt);
   auto val = rt.global().getProperty(rt, "console");
   std::shared_ptr<jsi::Object> originalConsole;
@@ -261,9 +261,8 @@ void Inspector::triggerAsyncPause(bool andTickle) {
   if (andTickle) {
     // We run the dummy JS on a background thread to avoid any reentrancy issues
     // in case this thread is called with the inspector mutex held.
-    std::shared_ptr<RuntimeAdapter> adapter = adapter_;
     detail::Thread tickleJsLater(
-        "inspectorTickleJs", [adapter]() { adapter->tickleJs(); });
+        "inspectorTickleJs", [&adapter = adapter_]() { adapter.tickleJs(); });
     tickleJsLater.detach();
   }
 }
@@ -706,8 +705,8 @@ void Inspector::alertIfPausedInSupersededFile() {
         "=true to "
         "suppress this warning. Filename: " +
         info.fileName + ").";
-    jsi::Array jsiArray(adapter_->getRuntime(), 1);
-    jsiArray.setValueAtIndex(adapter_->getRuntime(), 0, warning);
+    jsi::Array jsiArray(adapter_.getRuntime(), 1);
+    jsiArray.setValueAtIndex(adapter_.getRuntime(), 0, warning);
 
     ConsoleMessageInfo logMessage("warning", std::move(jsiArray));
     observer_.onMessageAdded(*this, logMessage);
@@ -715,7 +714,7 @@ void Inspector::alertIfPausedInSupersededFile() {
 }
 
 bool Inspector::shouldSuppressAlertAboutSupersededFiles() {
-  jsi::Runtime &rt = adapter_->getRuntime();
+  jsi::Runtime &rt = adapter_.getRuntime();
   jsi::Value setting = rt.global().getProperty(rt, kSuppressionVariable);
 
   if (setting.isUndefined() || !setting.isBool())

ReactCommon/hermes/inspector/Inspector.h

@@ -106,7 +106,7 @@ class Inspector : public facebook::hermes::debugger::EventObserver,
    * provided runtime before any JS executes in the runtime.
    */
   Inspector(
-      std::shared_ptr<RuntimeAdapter> adapter,
+      RuntimeAdapter &adapter,
       InspectorObserver &observer,
       bool pauseOnFirstStatement);
   ~Inspector();
@@ -317,7 +317,7 @@ class Inspector : public facebook::hermes::debugger::EventObserver,
       const std::string &name,
       const std::string &chromeType);
 
-  std::shared_ptr<RuntimeAdapter> adapter_;
+  RuntimeAdapter &adapter_;
   facebook::hermes::debugger::Debugger &debugger_;
   InspectorObserver &observer_;
 

ReactCommon/hermes/inspector/chrome/Connection.cpp

@@ -48,13 +48,11 @@ static const char *const kBeforeScriptWithSourceMapExecution =
 class Connection::Impl : public inspector::InspectorObserver,
                          public message::RequestHandler {
  public:
-  Impl(
-      std::unique_ptr<RuntimeAdapter> adapter,
-      const std::string &title,
-      bool waitForDebugger);
+  Impl(RuntimeAdapter &adapter, const std::string &title, bool waitForDebugger);
   ~Impl();
 
   jsi::Runtime &getRuntime();
+  RuntimeAdapter &getRuntimeAdapter();
   std::string getTitle() const;
 
   bool connect(std::unique_ptr<IRemoteConnection> remoteConn);
@@ -143,7 +141,7 @@ class Connection::Impl : public inspector::InspectorObserver,
     folly::via(executor_.get(), [cb = std::move(callback)]() { cb(); });
   }
 
-  std::shared_ptr<RuntimeAdapter> runtimeAdapter_;
+  RuntimeAdapter &runtimeAdapter_;
   std::string title_;
 
   // connected_ is protected by connectionMutex_.
@@ -185,10 +183,10 @@ class Connection::Impl : public inspector::InspectorObserver,
 };
 
 Connection::Impl::Impl(
-    std::unique_ptr<RuntimeAdapter> adapter,
+    RuntimeAdapter &adapter,
     const std::string &title,
     bool waitForDebugger)
-    : runtimeAdapter_(std::move(adapter)),
+    : runtimeAdapter_(adapter),
       title_(title),
       connected_(false),
       executor_(std::make_unique<inspector::detail::SerialExecutor>(
@@ -204,7 +202,11 @@ Connection::Impl::Impl(
 Connection::Impl::~Impl() = default;
 
 jsi::Runtime &Connection::Impl::getRuntime() {
-  return runtimeAdapter_->getRuntime();
+  return runtimeAdapter_.getRuntime();
+}
+
+RuntimeAdapter &Connection::Impl::getRuntimeAdapter() {
+  return runtimeAdapter_;
 }
 
 std::string Connection::Impl::getTitle() const {
@@ -1571,11 +1573,10 @@ void Connection::Impl::sendNotificationToClientViaExecutor(
  * Connection
  */
 Connection::Connection(
-    std::unique_ptr<RuntimeAdapter> adapter,
+    RuntimeAdapter &adapter,
     const std::string &title,
     bool waitForDebugger)
-    : impl_(
-          std::make_unique<Impl>(std::move(adapter), title, waitForDebugger)) {}
+    : impl_(std::make_unique<Impl>(adapter, title, waitForDebugger)) {}
 
 Connection::~Connection() = default;
 
@@ -1583,6 +1584,10 @@ jsi::Runtime &Connection::getRuntime() {
   return impl_->getRuntime();
 }
 
+RuntimeAdapter &Connection::getRuntimeAdapter() {
+  return impl_->getRuntimeAdapter();
+}
+
 std::string Connection::getTitle() const {
   return impl_->getTitle();
 }

ReactCommon/hermes/inspector/chrome/Connection.h

@@ -29,7 +29,7 @@ class INSPECTOR_EXPORT Connection {
   /// Connection constructor enables the debugger on the provided runtime. This
   /// should generally called before you start running any JS in the runtime.
   Connection(
-      std::unique_ptr<RuntimeAdapter> adapter,
+      RuntimeAdapter &adapter,
       const std::string &title,
       bool waitForDebugger = false);
   ~Connection();
@@ -37,6 +37,9 @@ class INSPECTOR_EXPORT Connection {
   /// getRuntime returns the underlying runtime being debugged.
   jsi::Runtime &getRuntime();
 
+  /// getRuntimeAdapter returns the runtime adapter being debugged.
+  RuntimeAdapter &getRuntimeAdapter();
+
   /// getTitle returns the name of the friendly name of the runtime that's shown
   /// to users in Nuclide.
   std::string getTitle() const;

ReactCommon/hermes/inspector/chrome/ConnectionDemux.cpp

@@ -64,7 +64,7 @@ ConnectionDemux::ConnectionDemux(facebook::react::IInspector &inspector)
 ConnectionDemux::~ConnectionDemux() = default;
 
 int ConnectionDemux::enableDebugging(
-    std::unique_ptr<RuntimeAdapter> adapter,
+    RuntimeAdapter &adapter,
     const std::string &title) {
   std::lock_guard<std::mutex> lock(mutex_);
 
@@ -90,18 +90,17 @@ int ConnectionDemux::enableDebugging(
       (inspectedContexts_->find(title) != inspectedContexts_->end()) ||
       isNetworkInspected(title, "app_name", "device_name");
 
-  return addPage(
-      std::make_shared<Connection>(std::move(adapter), title, waitForDebugger));
+  return addPage(std::make_shared<Connection>(adapter, title, waitForDebugger));
 }
 
-void ConnectionDemux::disableDebugging(HermesRuntime &runtime) {
+void ConnectionDemux::disableDebugging(RuntimeAdapter &adapter) {
   std::lock_guard<std::mutex> lock(mutex_);
 
   for (auto &it : conns_) {
     int pageId = it.first;
     auto &conn = it.second;
 
-    if (&(conn->getRuntime()) == &runtime) {
+    if (&(conn->getRuntimeAdapter()) == &adapter) {
       removePage(pageId);
 
       // must break here. removePage mutates conns_, so range-for iterator is

ReactCommon/hermes/inspector/chrome/ConnectionDemux.h

@@ -36,10 +36,8 @@ class ConnectionDemux {
   ConnectionDemux(const ConnectionDemux &) = delete;
   ConnectionDemux &operator=(const ConnectionDemux &) = delete;
 
-  int enableDebugging(
-      std::unique_ptr<RuntimeAdapter> adapter,
-      const std::string &title);
-  void disableDebugging(HermesRuntime &runtime);
+  int enableDebugging(RuntimeAdapter &adapter, const std::string &title);
+  void disableDebugging(RuntimeAdapter &adapter);
 
  private:
   int addPage(std::shared_ptr<Connection> conn);

ReactCommon/hermes/inspector/chrome/Registration.cpp

@@ -22,14 +22,12 @@ ConnectionDemux &demux() {
 
 } // namespace
 
-void enableDebugging(
-    std::unique_ptr<RuntimeAdapter> adapter,
-    const std::string &title) {
-  demux().enableDebugging(std::move(adapter), title);
+void enableDebugging(RuntimeAdapter &adapter, const std::string &title) {
+  demux().enableDebugging(adapter, title);
 }
 
-void disableDebugging(HermesRuntime &runtime) {
-  demux().disableDebugging(runtime);
+void disableDebugging(RuntimeAdapter &adapter) {
+  demux().disableDebugging(adapter);
 }
 
 } // namespace chrome

ReactCommon/hermes/inspector/chrome/Registration.h

@@ -23,15 +23,13 @@ namespace chrome {
  * (called "pages" in the higher-leavel React Native API) in this process. It
  * should be called before any JS runs in the runtime.
  */
-extern void enableDebugging(
-    std::unique_ptr<RuntimeAdapter> adapter,
-    const std::string &title);
+extern void enableDebugging(RuntimeAdapter &adapter, const std::string &title);
 
 /*
  * disableDebugging removes this runtime from the list of debuggable JS targets
  * in this process.
  */
-extern void disableDebugging(HermesRuntime &runtime);
+extern void disableDebugging(RuntimeAdapter &adapter);
 
 } // namespace chrome
 } // namespace inspector

ReactCommon/hermes/inspector/chrome/cli/main.cpp

@@ -214,10 +214,9 @@ static void runDebuggerLoop(
 static void runScript(const std::string &scriptSource, const std::string &url) {
   std::shared_ptr<fbhermes::HermesRuntime> runtime(
       fbhermes::makeHermesRuntime());
-  auto adapter =
-      std::make_unique<fbhermes::inspector::SharedRuntimeAdapter>(runtime);
+  fbhermes::inspector::SharedRuntimeAdapter adapter(runtime);
   fbhermes::inspector::chrome::Connection conn(
-      std::move(adapter), "hermes-chrome-debug-server");
+      adapter, "hermes-chrome-debug-server");
   std::thread debuggerLoop(runDebuggerLoop, std::ref(conn), scriptSource);
 
   fbhermes::HermesRuntime::DebugFlags flags{};

ReactCommon/hermes/inspector/chrome/tests/ConnectionDemuxTests.cpp

@@ -97,10 +97,10 @@ TEST(ConnectionDemuxTests, TestEnableDisable) {
 
   ConnectionDemux demux{*inspector};
 
-  int id1 = demux.enableDebugging(
-      std::make_unique<SharedRuntimeAdapter>(runtime1), "page1");
-  int id2 = demux.enableDebugging(
-      std::make_unique<SharedRuntimeAdapter>(runtime2), "page2");
+  SharedRuntimeAdapter adapter1(runtime1);
+  int id1 = demux.enableDebugging(adapter1, "page1");
+  SharedRuntimeAdapter adapter2(runtime2);
+  int id2 = demux.enableDebugging(adapter2, "page2");
 
   expectPages(*inspector, {{id1, "page1"}, {id2, "page2"}});
 
@@ -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(adapter2);
   expectPages(*inspector, {{id1, "page1"}});
   remoteData2->expectDisconnected();
 

ReactCommon/hermes/inspector/chrome/tests/SyncConnection.cpp

@@ -55,10 +55,8 @@ class SyncConnection::RemoteConnnection : public IRemoteConnection {
 SyncConnection::SyncConnection(
     std::shared_ptr<HermesRuntime> runtime,
     bool waitForDebugger)
-    : connection_(
-          std::make_unique<SharedRuntimeAdapter>(runtime),
-          "testConn",
-          waitForDebugger) {
+    : runtimeAdapter_(runtime),
+      connection_(runtimeAdapter_, "testConn", waitForDebugger) {
   connection_.connect(std::make_unique<RemoteConnnection>(*this));
 }
 

ReactCommon/hermes/inspector/chrome/tests/SyncConnection.h

@@ -54,6 +54,7 @@ class SyncConnection {
 
   void onReply(const std::string &message);
 
+  SharedRuntimeAdapter runtimeAdapter_;
   Connection connection_;
 
   std::mutex mutex_;

ReactCommon/hermes/inspector/tests/InspectorTests.cpp

@@ -92,10 +92,8 @@ struct HermesDebugContext {
       InspectorObserver &observer,
       folly::Future<Unit> &&finished)
       : runtime(makeHermesRuntime()),
-        inspector(
-            std::make_shared<SharedRuntimeAdapter>(runtime),
-            observer,
-            false),
+        adapter(runtime),
+        inspector(adapter, observer, false),
         stopFlag(false),
         finished(std::move(finished)) {
     runtime->global().setProperty(
@@ -124,6 +122,7 @@ struct HermesDebugContext {
   }
 
   std::shared_ptr<HermesRuntime> runtime;
+  SharedRuntimeAdapter adapter;
   Inspector inspector;
   std::atomic<bool> stopFlag{};
   folly::Future<Unit> finished;