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