Coordinate inspector and transport shutdown

Summary:
Retain the inspector until serialized target and socket cleanup has completed, reject late work during shutdown, and join the dispatcher from its external owner. Keep transport close requests callable from IO callbacks without self-joining or holding the socket lock across a join. Serialize writes and pings on the IO executor, discard expired implementation references, and make external-owner destruction finish cleanup when a closed callback throws.

Add focused inspector lifetime and real loopback transport regressions, including active-IO destruction, nested close, late connection completions, and invalid descriptors. Destruction of a socket wrapper on its own IO thread remains unsupported.

Changelog:
[General][Fixed] - Fix native inspector ownership and transport shutdown races

Differential Revision: D124675718

Author: Abbondanzo

packages/react-native/ReactCxxPlatform/react/devsupport/inspector/Inspector.cpp

@@ -135,18 +135,36 @@ Inspector::Inspector(
       httpClientFactory_(std::move(httpClientFactory)) {}
 
 Inspector::~Inspector() noexcept {
-  if (pageId_) {
-    jsinspector_modern::getInspectorInstance().removePage(*pageId_);
-    taskDispatchThread_.runSync([weakThis = weak_from_this()]() {
-      if (auto strongThis = weakThis.lock()) {
-        strongThis->pageId_.reset();
-        strongThis->target_.reset();
-      }
-    });
+  shutdown();
+}
+
+void Inspector::shutdown() noexcept {
+  CHECK(!taskDispatchThread_.isOnThread());
+  if (shuttingDown_.exchange(true)) {
+    return;
   }
+  taskDispatchThread_.runSync([this]() {
+    taskDispatchThread_.cancelPendingTasks();
+    if (pageId_) {
+      jsinspector_modern::getInspectorInstance().removePage(*pageId_);
+      pageId_.reset();
+    }
+    if (packagerConnection_) {
+      packagerConnection_->closeQuietly();
+      packagerConnection_.reset();
+    }
+    target_.reset();
+    hostDelegate_.reset();
+  });
   taskDispatchThread_.quit();
 }
 
+void Inspector::runOnInspectorThreadSync(
+    TaskDispatchThread::TaskFn&& callback) {
+  CHECK(!shuttingDown_);
+  taskDispatchThread_.runSync(std::move(callback));
+}
+
 void Inspector::connectDebugger(const std::string& inspectorUrl) noexcept {
   invokeElsePost([weakThis = weak_from_this(), inspectorUrl]() {
     auto self = weakThis.lock();
@@ -205,11 +223,20 @@ void Inspector::ensureHostTarget(
 void Inspector::invokeElsePost(
     TaskDispatchThread::TaskFn&& callback,
     std::chrono::milliseconds delayMs) {
+  if (shuttingDown_) {
+    return;
+  }
   if (taskDispatchThread_.isOnThread() &&
       delayMs == std::chrono::milliseconds::zero()) {
     callback();
   } else {
-    taskDispatchThread_.runAsync(std::move(callback), delayMs);
+    taskDispatchThread_.runAsync(
+        [weakThis = weak_from_this(), callback = std::move(callback)]() {
+          if (auto self = weakThis.lock(); self && !self->shuttingDown_) {
+            callback();
+          }
+        },
+        delayMs);
   }
 }
 

packages/react-native/ReactCxxPlatform/react/devsupport/inspector/Inspector.h

@@ -14,6 +14,7 @@
 #include <react/http/IHttpClient.h>
 #include <react/http/IWebSocketClient.h>
 #include <react/threading/TaskDispatchThread.h>
+#include <atomic>
 #include <functional>
 #include <memory>
 #include <optional>
@@ -37,6 +38,11 @@ class Inspector : public InspectorThread, public std::enable_shared_from_this<In
   Inspector(Inspector &&other) = delete;
   Inspector &operator=(Inspector &&other) = delete;
 
+  // Call from the owner thread before releasing the last owning reference.
+  void shutdown() noexcept;
+
+  void runOnInspectorThreadSync(TaskDispatchThread::TaskFn &&callback);
+
   void connectDebugger(const std::string &inspectorUrl) noexcept;
 
   void ensureHostTarget(
@@ -50,6 +56,7 @@ class Inspector : public InspectorThread, public std::enable_shared_from_this<In
       TaskDispatchThread::TaskFn &&callback,
       std::chrono::milliseconds delayMs = std::chrono::milliseconds::zero()) override;
 
+  std::atomic<bool> shuttingDown_{false};
   TaskDispatchThread taskDispatchThread_{"InspectorThread"};
   std::string appName_;
   std::string deviceName_;

packages/react-native/ReactCxxPlatform/react/devsupport/inspector/tests/InspectorShutdownTests.cpp

@@ -0,0 +1,150 @@
+/*
+ * 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.
+ */
+
+#include <gtest/gtest.h>
+#include <jsinspector-modern/InspectorInterfaces.h>
+#include <react/devsupport/inspector/Inspector.h>
+#include <chrono>
+#include <thread>
+
+namespace facebook::react {
+
+class InspectorShutdownTest : public ::testing::Test {
+ protected:
+  struct SocketState {
+    IWebSocketClient::OnConnectCallback onConnect;
+    IWebSocketClient::OnClosedCallback onClosed;
+    IWebSocketClient::OnMessageCallback onMessage;
+    std::weak_ptr<Inspector> inspector;
+    std::thread::id closeThread;
+    bool ownedDuringClose{false};
+    int closes{0};
+  };
+
+  class Socket final : public IWebSocketClient {
+   public:
+    explicit Socket(std::shared_ptr<SocketState> state)
+        : state_(std::move(state)) {}
+
+    void setOnClosedCallback(OnClosedCallback&& callback) noexcept override {
+      state_->onClosed = std::move(callback);
+    }
+
+    void setOnMessageCallback(OnMessageCallback&& callback) noexcept override {
+      state_->onMessage = std::move(callback);
+    }
+
+    void connect(const std::string& /*url*/, OnConnectCallback&& callback)
+        override {
+      state_->onConnect = std::move(callback);
+    }
+
+    void close(const std::string& reason) override {
+      ++state_->closes;
+      state_->closeThread = std::this_thread::get_id();
+      state_->ownedDuringClose = !state_->inspector.expired();
+      if (state_->onClosed) {
+        state_->onClosed(reason);
+      }
+    }
+
+    void send(const std::string& /*message*/) override {}
+    void ping() override {}
+
+   private:
+    std::shared_ptr<SocketState> state_;
+  };
+
+  void SetUp() override {
+    socket = std::make_shared<SocketState>();
+    inspector = std::make_shared<Inspector>(
+        "shutdown-test",
+        "test-device",
+        [state = socket]() { return std::make_unique<Socket>(state); },
+        []() { return std::unique_ptr<IHttpClient>{}; });
+    socket->inspector = inspector;
+  }
+
+  void TearDown() override {
+    if (inspector) {
+      inspector->shutdown();
+    }
+  }
+
+  std::shared_ptr<SocketState> socket;
+  std::shared_ptr<Inspector> inspector;
+};
+
+TEST_F(InspectorShutdownTest, ShutdownDisposesPageAndSocketWhileOwned) {
+  auto pageCount = jsinspector_modern::getInspectorInstance().getPages().size();
+  inspector->ensureHostTarget({}, {});
+  EXPECT_NE(inspector->inspectorTarget(), nullptr);
+  inspector->connectDebugger("http://localhost:8081/inspector/device");
+  std::thread::id inspectorThread;
+  inspector->runOnInspectorThreadSync(
+      [&]() { inspectorThread = std::this_thread::get_id(); });
+  ASSERT_TRUE(socket->onConnect);
+  EXPECT_EQ(
+      jsinspector_modern::getInspectorInstance().getPages().size(),
+      pageCount + 1);
+
+  inspector->shutdown();
+
+  EXPECT_EQ(inspector->inspectorTarget(), nullptr);
+  EXPECT_EQ(
+      jsinspector_modern::getInspectorInstance().getPages().size(), pageCount);
+  EXPECT_EQ(socket->closes, 1);
+  EXPECT_EQ(socket->closeThread, inspectorThread);
+  EXPECT_TRUE(socket->ownedDuringClose);
+  EXPECT_EQ(inspector.use_count(), 1);
+  inspector->shutdown();
+  EXPECT_EQ(socket->closes, 1);
+  inspector.reset();
+  EXPECT_TRUE(socket->inspector.expired());
+}
+
+TEST_F(
+    InspectorShutdownTest,
+    ShutdownDropsDelayedOwnersAndRejectsLateCallbacks) {
+  InspectorThread& dispatch = *inspector;
+  bool ran = false;
+  dispatch.invokeElsePost(
+      [owner = inspector, &ran]() { ran = true; }, std::chrono::hours(1));
+
+  inspector->shutdown();
+
+  EXPECT_FALSE(ran);
+  EXPECT_EQ(inspector.use_count(), 1);
+  dispatch.invokeElsePost([&]() { ran = true; });
+  dispatch.invokeElsePost([&]() { ran = true; }, std::chrono::hours(1));
+  inspector->connectDebugger("http://localhost:8081/inspector/device");
+  EXPECT_FALSE(ran);
+  EXPECT_FALSE(socket->onConnect);
+  inspector.reset();
+  EXPECT_TRUE(socket->inspector.expired());
+}
+
+TEST_F(
+    InspectorShutdownTest,
+    LateTransportFailureCannotReconnectAfterShutdown) {
+  inspector->connectDebugger("http://localhost:8081/inspector/device");
+  inspector->runOnInspectorThreadSync([]() {});
+  ASSERT_TRUE(socket->onConnect);
+  auto onConnect = socket->onConnect;
+  auto onMessage = socket->onMessage;
+
+  inspector->shutdown();
+  onConnect(false, "late failure");
+  onMessage(R"({"event":"getPages"})");
+
+  EXPECT_EQ(socket->closes, 1);
+  EXPECT_EQ(inspector.use_count(), 1);
+  inspector.reset();
+  EXPECT_TRUE(socket->inspector.expired());
+}
+
+} // namespace facebook::react