Revert D38242964: Multisect successfully blamed D38242964 for test or build failures

Summary:
This diff is reverting D38242964 (https://github.com/facebook/react-native/commit/d19cee34927c116b27b1513267bdf3ad20f1ea8d)
D38242964 (https://github.com/facebook/react-native/commit/d19cee34927c116b27b1513267bdf3ad20f1ea8d) has been identified to be causing the following test or build failures:
Tests affected:
- https://www.internalfb.com/intern/test/281475044166194/
- https://www.internalfb.com/intern/test/562950020887755/

Here's the Multisect link:
https://www.internalfb.com/intern/testinfra/multisect/1117138
Here are the tasks that are relevant to this breakage:
T101319872: 14 tests started failing for oncall ar_engine in the last 2 weeks
We're generating a revert to back out the changes in this diff, please note the backout may land if someone accepts it.

Changelog: [Internal]

Reviewed By: jpporto

Differential Revision: D38452063

fbshipit-source-id: 8d89fe742f04b60fcc110ab3163874ad5eb2af9a

ReactCommon/hermes/executor/HermesExecutorFactory.cpp

@@ -149,38 +149,35 @@ class DecoratedRuntime : public jsi::WithRuntimeDecorator<ReentrancyCheck> {
       HermesRuntime &hermesRuntime,
       std::shared_ptr<MessageQueueThread> jsQueue)
       : jsi::WithRuntimeDecorator<ReentrancyCheck>(*runtime, reentrancyCheck_),
-        runtime_(std::move(runtime))
-#ifdef HERMES_ENABLE_DEBUGGER
-        ,
-        adapter_(
-            std::shared_ptr<HermesRuntime>(runtime_, &hermesRuntime),
-            jsQueue)
-#endif
-  {
+        runtime_(std::move(runtime)),
+        hermesRuntime_(hermesRuntime) {
 #ifdef HERMES_ENABLE_DEBUGGER
+    std::shared_ptr<HermesRuntime> rt(runtime_, &hermesRuntime);
+    auto adapter = std::make_unique<HermesExecutorRuntimeAdapter>(rt, jsQueue);
     facebook::hermes::inspector::chrome::enableDebugging(
-        adapter_, "Hermes React Native");
+        std::move(adapter), "Hermes React Native");
+#else
+    (void)hermesRuntime_;
 #endif
   }
 
   ~DecoratedRuntime() {
 #ifdef HERMES_ENABLE_DEBUGGER
-    facebook::hermes::inspector::chrome::disableDebugging(adapter_);
+    facebook::hermes::inspector::chrome::disableDebugging(hermesRuntime_);
 #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(
-    RuntimeAdapter &adapter,
+    std::shared_ptr<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,8 +261,9 @@ 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_]() { adapter.tickleJs(); });
+        "inspectorTickleJs", [adapter]() { adapter->tickleJs(); });
     tickleJsLater.detach();
   }
 }
@@ -705,8 +706,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);
@@ -714,7 +715,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(
-      RuntimeAdapter &adapter,
+      std::shared_ptr<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);
 
-  RuntimeAdapter &adapter_;
+  std::shared_ptr<RuntimeAdapter> adapter_;
   facebook::hermes::debugger::Debugger &debugger_;
   InspectorObserver &observer_;
 

ReactCommon/hermes/inspector/chrome/Connection.cpp

@@ -48,11 +48,13 @@ static const char *const kBeforeScriptWithSourceMapExecution =
 class Connection::Impl : public inspector::InspectorObserver,
                          public message::RequestHandler {
  public:
-  Impl(RuntimeAdapter &adapter, const std::string &title, bool waitForDebugger);
+  Impl(
+      std::unique_ptr<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);
@@ -141,7 +143,7 @@ class Connection::Impl : public inspector::InspectorObserver,
     folly::via(executor_.get(), [cb = std::move(callback)]() { cb(); });
   }
 
-  RuntimeAdapter &runtimeAdapter_;
+  std::shared_ptr<RuntimeAdapter> runtimeAdapter_;
   std::string title_;
 
   // connected_ is protected by connectionMutex_.
@@ -183,10 +185,10 @@ class Connection::Impl : public inspector::InspectorObserver,
 };
 
 Connection::Impl::Impl(
-    RuntimeAdapter &adapter,
+    std::unique_ptr<RuntimeAdapter> adapter,
     const std::string &title,
     bool waitForDebugger)
-    : runtimeAdapter_(adapter),
+    : runtimeAdapter_(std::move(adapter)),
       title_(title),
       connected_(false),
       executor_(std::make_unique<inspector::detail::SerialExecutor>(
@@ -202,11 +204,7 @@ Connection::Impl::Impl(
 Connection::Impl::~Impl() = default;
 
 jsi::Runtime &Connection::Impl::getRuntime() {
-  return runtimeAdapter_.getRuntime();
-}
-
-RuntimeAdapter &Connection::Impl::getRuntimeAdapter() {
-  return runtimeAdapter_;
+  return runtimeAdapter_->getRuntime();
 }
 
 std::string Connection::Impl::getTitle() const {
@@ -1573,10 +1571,11 @@ void Connection::Impl::sendNotificationToClientViaExecutor(
  * Connection
  */
 Connection::Connection(
-    RuntimeAdapter &adapter,
+    std::unique_ptr<RuntimeAdapter> adapter,
     const std::string &title,
     bool waitForDebugger)
-    : impl_(std::make_unique<Impl>(adapter, title, waitForDebugger)) {}
+    : impl_(
+          std::make_unique<Impl>(std::move(adapter), title, waitForDebugger)) {}
 
 Connection::~Connection() = default;
 
@@ -1584,10 +1583,6 @@ 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(
-      RuntimeAdapter &adapter,
+      std::unique_ptr<RuntimeAdapter> adapter,
       const std::string &title,
       bool waitForDebugger = false);
   ~Connection();
@@ -37,9 +37,6 @@ 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(
-    RuntimeAdapter &adapter,
+    std::unique_ptr<RuntimeAdapter> adapter,
     const std::string &title) {
   std::lock_guard<std::mutex> lock(mutex_);
 
@@ -90,17 +90,18 @@ int ConnectionDemux::enableDebugging(
       (inspectedContexts_->find(title) != inspectedContexts_->end()) ||
       isNetworkInspected(title, "app_name", "device_name");
 
-  return addPage(std::make_shared<Connection>(adapter, title, waitForDebugger));
+  return addPage(
+      std::make_shared<Connection>(std::move(adapter), title, waitForDebugger));
 }
 
-void ConnectionDemux::disableDebugging(RuntimeAdapter &adapter) {
+void ConnectionDemux::disableDebugging(HermesRuntime &runtime) {
   std::lock_guard<std::mutex> lock(mutex_);
 
   for (auto &it : conns_) {
     int pageId = it.first;
     auto &conn = it.second;
 
-    if (&(conn->getRuntimeAdapter()) == &adapter) {
+    if (&(conn->getRuntime()) == &runtime) {
       removePage(pageId);
 
       // must break here. removePage mutates conns_, so range-for iterator is

ReactCommon/hermes/inspector/chrome/ConnectionDemux.h

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

ReactCommon/hermes/inspector/chrome/Registration.cpp

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

ReactCommon/hermes/inspector/chrome/Registration.h

@@ -23,13 +23,15 @@ 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(RuntimeAdapter &adapter, const std::string &title);
+extern void enableDebugging(
+    std::unique_ptr<RuntimeAdapter> adapter,
+    const std::string &title);
 
 /*
  * disableDebugging removes this runtime from the list of debuggable JS targets
  * in this process.
  */
-extern void disableDebugging(RuntimeAdapter &adapter);
+extern void disableDebugging(HermesRuntime &runtime);
 
 } // namespace chrome
 } // namespace inspector

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

@@ -214,9 +214,10 @@ static void runDebuggerLoop(
 static void runScript(const std::string &scriptSource, const std::string &url) {
   std::shared_ptr<fbhermes::HermesRuntime> runtime(
       fbhermes::makeHermesRuntime());
-  fbhermes::inspector::SharedRuntimeAdapter adapter(runtime);
+  auto adapter =
+      std::make_unique<fbhermes::inspector::SharedRuntimeAdapter>(runtime);
   fbhermes::inspector::chrome::Connection conn(
-      adapter, "hermes-chrome-debug-server");
+      std::move(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};
 
-  SharedRuntimeAdapter adapter1(runtime1);
-  int id1 = demux.enableDebugging(adapter1, "page1");
-  SharedRuntimeAdapter adapter2(runtime2);
-  int id2 = demux.enableDebugging(adapter2, "page2");
+  int id1 = demux.enableDebugging(
+      std::make_unique<SharedRuntimeAdapter>(runtime1), "page1");
+  int id2 = demux.enableDebugging(
+      std::make_unique<SharedRuntimeAdapter>(runtime2), "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(adapter2);
+  demux.disableDebugging(*runtime2);
   expectPages(*inspector, {{id1, "page1"}});
   remoteData2->expectDisconnected();
 

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

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

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

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

ReactCommon/hermes/inspector/tests/InspectorTests.cpp

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