Close inspector connections immediately when a page is removed (#42308)

Summary:
Pull Request resolved: https://github.com/facebook/react-native/pull/42308

Changelog: [Internal]

Guarantees cleanup of `ILocalConnection` when the associated page is unregistered from `IInspector`.

NOTE: This only applies to the C++ version of `InspectorPackagerConnection`. The legacy pure-Java and pure-ObjC implementations are of this class are unchanged.

In the upcoming modern CDP backend architecture, this will help guarantee the validity of Target references (specifically PageTarget) held by Agents (specifically PageAgent), without introducing unnecessary shared ownership and dynamism.

Reviewed By: hoxyq

Differential Revision: D52786331

fbshipit-source-id: 162425d6435246a95ac9c076bc5c59a34f331f16

packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp

@@ -7,6 +7,8 @@
 
 #include "InspectorInterfaces.h"
 
+#include <cassert>
+#include <list>
 #include <mutex>
 #include <tuple>
 #include <unordered_map>
@@ -20,6 +22,7 @@ IDestructible::~IDestructible() {}
 ILocalConnection::~ILocalConnection() {}
 IRemoteConnection::~IRemoteConnection() {}
 IInspector::~IInspector() {}
+IPageStatusListener::~IPageStatusListener() {}
 
 namespace {
 
@@ -36,6 +39,9 @@ class InspectorImpl : public IInspector {
       int pageId,
       std::unique_ptr<IRemoteConnection> remote) override;
 
+  void registerPageStatusListener(
+      std::weak_ptr<IPageStatusListener> listener) override;
+
  private:
   class Page {
    public:
@@ -57,6 +63,7 @@ class InspectorImpl : public IInspector {
   mutable std::mutex mutex_;
   int nextPageId_{1};
   std::unordered_map<int, Page> pages_;
+  std::list<std::weak_ptr<IPageStatusListener>> listeners_;
 };
 
 InspectorImpl::Page::Page(
@@ -85,6 +92,7 @@ int InspectorImpl::addPage(
   std::scoped_lock lock(mutex_);
 
   int pageId = nextPageId_++;
+  assert(pages_.count(pageId) == 0 && "Unexpected duplicate page ID");
   pages_.emplace(pageId, Page{pageId, title, vm, std::move(connectFunc)});
 
   return pageId;
@@ -93,7 +101,13 @@ int InspectorImpl::addPage(
 void InspectorImpl::removePage(int pageId) {
   std::scoped_lock lock(mutex_);
 
-  pages_.erase(pageId);
+  if (pages_.erase(pageId) != 0) {
+    for (auto listenerWeak : listeners_) {
+      if (auto listener = listenerWeak.lock()) {
+        listener->onPageRemoved(pageId);
+      }
+    }
+  }
 }
 
 std::vector<InspectorPageDescription> InspectorImpl::getPages() const {
@@ -124,6 +138,19 @@ std::unique_ptr<ILocalConnection> InspectorImpl::connect(
   return connectFunc ? connectFunc(std::move(remote)) : nullptr;
 }
 
+void InspectorImpl::registerPageStatusListener(
+    std::weak_ptr<IPageStatusListener> listener) {
+  std::scoped_lock lock(mutex_);
+  // Remove expired listeners
+  for (auto it = listeners_.begin(); it != listeners_.end();) {
+    if (it->expired()) {
+      it = listeners_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+  listeners_.push_back(listener);
+}
 } // namespace
 
 IInspector& getInspectorInstance() {

packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h

@@ -53,9 +53,21 @@ class JSINSPECTOR_EXPORT ILocalConnection : public IDestructible {
  public:
   virtual ~ILocalConnection() = 0;
   virtual void sendMessage(std::string message) = 0;
+
+  /**
+   * Called by the inspector singleton to notify that the connection has been
+   * closed, either by the remote party or because the local page/VM is no
+   * longer registered with the inspector.
+   */
   virtual void disconnect() = 0;
 };
 
+class JSINSPECTOR_EXPORT IPageStatusListener : public IDestructible {
+ public:
+  virtual ~IPageStatusListener() = 0;
+  virtual void onPageRemoved(int pageId) = 0;
+};
+
 /// IInspector tracks debuggable JavaScript targets (pages).
 class JSINSPECTOR_EXPORT IInspector : public IDestructible {
  public:
@@ -82,6 +94,13 @@ class JSINSPECTOR_EXPORT IInspector : public IDestructible {
   virtual std::unique_ptr<ILocalConnection> connect(
       int pageId,
       std::unique_ptr<IRemoteConnection> remote) = 0;
+
+  /**
+   * registerPageStatusListener registers a listener that will receive events
+   * when pages are removed.
+   */
+  virtual void registerPageStatusListener(
+      std::weak_ptr<IPageStatusListener> listener) = 0;
 };
 
 /// getInspectorInstance retrieves the singleton inspector that tracks all

packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp

@@ -33,8 +33,10 @@ InspectorPackagerConnection::Impl::create(
     std::string app,
     std::unique_ptr<InspectorPackagerConnectionDelegate> delegate) {
   // No make_shared because the constructor is private
-  return std::shared_ptr<InspectorPackagerConnection::Impl>(
+  std::shared_ptr<InspectorPackagerConnection::Impl> impl(
       new InspectorPackagerConnection::Impl(url, app, std::move(delegate)));
+  getInspectorInstance().registerPageStatusListener(impl);
+  return impl;
 }
 
 InspectorPackagerConnection::Impl::Impl(
@@ -195,6 +197,13 @@ void InspectorPackagerConnection::Impl::didClose() {
   }
 }
 
+void InspectorPackagerConnection::Impl::onPageRemoved(int pageId) {
+  auto connection = removeConnectionForPage(std::to_string(pageId));
+  if (connection) {
+    connection->disconnect();
+  }
+}
+
 bool InspectorPackagerConnection::Impl::isConnected() const {
   return webSocket_ != nullptr;
 }

packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnectionImpl.h

@@ -20,6 +20,7 @@ namespace facebook::react::jsinspector_modern {
  */
 class InspectorPackagerConnection::Impl
     : public IWebSocketDelegate,
+      public IPageStatusListener,
       // Used to generate `weak_ptr`s we can pass around.
       public std::enable_shared_from_this<InspectorPackagerConnection::Impl> {
  public:
@@ -72,6 +73,9 @@ class InspectorPackagerConnection::Impl
   virtual void didReceiveMessage(std::string_view message) override;
   virtual void didClose() override;
 
+  // IPageStatusListener methods
+  virtual void onPageRemoved(int pageId) override;
+
   std::string url_;
   std::string app_;
   std::unique_ptr<InspectorPackagerConnectionDelegate> delegate_;

packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp

@@ -43,15 +43,33 @@ class InspectorPackagerConnectionTest : public testing::Test {
                        std::weak_ptr<IWebSocketDelegate>>());
   }
 
-  ~InspectorPackagerConnectionTest() override {
-    // Clean up all pages currently registered with the inspector.
+  void TearDown() override {
+    // Forcibly clean up all pages currently registered with the inspector in
+    // order to isolate state between tests. NOTE: Using TearDown instead of a
+    // destructor so that we can use FAIL() etc.
     std::vector<int> pagesToRemove;
-    for (auto& page : getInspectorInstance().getPages()) {
-      pagesToRemove.push_back(page.id);
+    auto pages = getInspectorInstance().getPages();
+    int liveConnectionCount = 0;
+    for (size_t i = 0; i != localConnections_.objectsVended(); ++i) {
+      if (localConnections_[i]) {
+        liveConnectionCount++;
+        // localConnections_[i] is a strict mock and will complain when we
+        // removePage if the call is unexpected.
+        EXPECT_CALL(*localConnections_[i], disconnect());
+      }
     }
-    for (auto id : pagesToRemove) {
-      getInspectorInstance().removePage(id);
+    for (auto& page : pages) {
+      getInspectorInstance().removePage(page.id);
     }
+    if (!pages.empty() && liveConnectionCount) {
+      if (!::testing::Test::HasFailure()) {
+        FAIL()
+            << "Test case ended with " << liveConnectionCount
+            << " open connection(s) and " << pages.size()
+            << " registered page(s). You must manually call removePage for each page.";
+      }
+    }
+    ::testing::Test::TearDown();
   }
 
   MockInspectorPackagerConnectionDelegate* packagerConnectionDelegate() {
@@ -260,6 +278,9 @@ TEST_F(InspectorPackagerConnectionTest, TestSendReceiveEvents) {
                 "id": 1234,
                 "params": ["arg1", "arg2"]
               })")));
+
+  EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
+  getInspectorInstance().removePage(pageId);
 }
 
 TEST_F(InspectorPackagerConnectionTest, TestSendReceiveEventsToMultiplePages) {
@@ -287,11 +308,11 @@ TEST_F(InspectorPackagerConnectionTest, TestSendReceiveEventsToMultiplePages) {
     // Connect to the i-th page.
     webSockets_[0]->getDelegate().didReceiveMessage(sformat(
         R"({{
-          "event": "connect",
-          "payload": {{
-            "pageId": {0}
-          }}
-        }})",
+        "event": "connect",
+        "payload": {{
+          "pageId": {0}
+        }}
+      }})",
         toJson(std::to_string(pageIds[i]))));
     ASSERT_TRUE(localConnections_[i]);
   }
@@ -329,15 +350,20 @@ TEST_F(InspectorPackagerConnectionTest, TestSendReceiveEventsToMultiplePages) {
         .RetiresOnSaturation();
     webSockets_[0]->getDelegate().didReceiveMessage(sformat(
         R"({{
-            "event": "wrappedEvent",
-            "payload": {{
-              "pageId": {0},
-              "wrappedEvent": {1}
-            }}
-          }})",
+          "event": "wrappedEvent",
+          "payload": {{
+            "pageId": {0},
+            "wrappedEvent": {1}
+          }}
+        }})",
         toJson(std::to_string(pageIds[i])),
         toJson(toJson(dynamic::object("method", method)))));
   }
+
+  for (int i = 0; i < kNumPages; ++i) {
+    EXPECT_CALL(*localConnections_[i], disconnect()).RetiresOnSaturation();
+    getInspectorInstance().removePage(pageIds[i]);
+  }
 }
 
 TEST_F(InspectorPackagerConnectionTest, TestSendEventToAllConnections) {
@@ -376,6 +402,9 @@ TEST_F(InspectorPackagerConnectionTest, TestSendEventToAllConnections) {
     "id": 1234,
     "params": ["arg1", "arg2"]
   })");
+
+  EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
+  getInspectorInstance().removePage(pageId);
 }
 
 TEST_F(InspectorPackagerConnectionTest, TestConnectThenDisconnect) {
@@ -887,4 +916,33 @@ TEST_F(
   EXPECT_FALSE(webSockets_[0]);
 }
 
+TEST_F(InspectorPackagerConnectionTest, TestDestroyConnectionOnPageRemoved) {
+  // Configure gmock to expect calls in a specific order.
+  InSequence mockCallsMustBeInSequence;
+
+  packagerConnection_->connect();
+  ASSERT_TRUE(webSockets_[0]);
+
+  auto pageId = getInspectorInstance().addPage(
+      "mock-title",
+      "mock-vm",
+      localConnections_
+          .lazily_make_unique<std::unique_ptr<IRemoteConnection>>());
+
+  // Connect to the page.
+  webSockets_[0]->getDelegate().didReceiveMessage(sformat(
+      R"({{
+          "event": "connect",
+          "payload": {{
+            "pageId": {0}
+          }}
+        }})",
+      toJson(std::to_string(pageId))));
+  EXPECT_TRUE(localConnections_[0]);
+
+  // Remove the page.
+  EXPECT_CALL(*localConnections_[0], disconnect()).RetiresOnSaturation();
+  getInspectorInstance().removePage(pageId);
+  EXPECT_FALSE(localConnections_[0]);
+}
 } // namespace facebook::react::jsinspector_modern