From a064a2a05e95bcaaf57d7be18dfb079de624e171 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Thu, 18 Jan 2024 15:26:13 -0800 Subject: [PATCH] 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 --- .../InspectorInterfaces.cpp | 29 +++++- .../jsinspector-modern/InspectorInterfaces.h | 19 ++++ .../InspectorPackagerConnection.cpp | 11 ++- .../InspectorPackagerConnectionImpl.h | 4 + .../tests/InspectorPackagerConnectionTest.cpp | 92 +++++++++++++++---- 5 files changed, 136 insertions(+), 19 deletions(-) diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp index 6b692be11da..38d015e8357 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp @@ -7,6 +7,8 @@ #include "InspectorInterfaces.h" +#include +#include #include #include #include @@ -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 remote) override; + void registerPageStatusListener( + std::weak_ptr listener) override; + private: class Page { public: @@ -57,6 +63,7 @@ class InspectorImpl : public IInspector { mutable std::mutex mutex_; int nextPageId_{1}; std::unordered_map pages_; + std::list> 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 InspectorImpl::getPages() const { @@ -124,6 +138,19 @@ std::unique_ptr InspectorImpl::connect( return connectFunc ? connectFunc(std::move(remote)) : nullptr; } +void InspectorImpl::registerPageStatusListener( + std::weak_ptr 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() { diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h b/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h index 0a27198d899..ff25981fd91 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h +++ b/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 connect( int pageId, std::unique_ptr remote) = 0; + + /** + * registerPageStatusListener registers a listener that will receive events + * when pages are removed. + */ + virtual void registerPageStatusListener( + std::weak_ptr listener) = 0; }; /// getInspectorInstance retrieves the singleton inspector that tracks all diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp b/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp index 829c633f09f..c080022741a 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnection.cpp @@ -33,8 +33,10 @@ InspectorPackagerConnection::Impl::create( std::string app, std::unique_ptr delegate) { // No make_shared because the constructor is private - return std::shared_ptr( + std::shared_ptr 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; } diff --git a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnectionImpl.h b/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnectionImpl.h index 704e3f19961..aa60cfba1fe 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/InspectorPackagerConnectionImpl.h +++ b/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 { 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 delegate_; diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp index 5ed3edab6ad..2c19d6efb41 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp @@ -43,15 +43,33 @@ class InspectorPackagerConnectionTest : public testing::Test { std::weak_ptr>()); } - ~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 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>()); + + // 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