From 1b3296b574e50ac19e9870d68e027e3be89b89ab Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Thu, 4 Jun 2026 15:16:10 +0100 Subject: [PATCH] LibDevTools: Avoid stale callbacks during server teardown DevTools server callbacks can outlive the server when deferred work is queued during connection shutdown. Capture weak pointers for those tasks and clear socket callbacks while the server is being destroyed so later actor cleanup cannot touch freed state. Add protocol coverage that destroys a server with deferred actor cleanup pending, before adding the style-rule actor path that depends on this. --- Libraries/LibDevTools/Connection.cpp | 10 +++-- Libraries/LibDevTools/Connection.h | 5 ++- Libraries/LibDevTools/DevToolsServer.cpp | 50 ++++++++++++++++------ Libraries/LibDevTools/DevToolsServer.h | 4 +- Tests/LibDevTools/TestDevToolsProtocol.cpp | 14 ++++++ 5 files changed, 65 insertions(+), 18 deletions(-) diff --git a/Libraries/LibDevTools/Connection.cpp b/Libraries/LibDevTools/Connection.cpp index 95bbc9b913..3892f86b6c 100644 --- a/Libraries/LibDevTools/Connection.cpp +++ b/Libraries/LibDevTools/Connection.cpp @@ -94,9 +94,13 @@ ErrorOr Connection::on_ready_to_read() if (!message.is_object()) continue; - Core::deferred_invoke([this, message = move(message)]() mutable { - if (on_message_received) - on_message_received(move(message.as_object())); + Core::deferred_invoke([weak_self = make_weak_ptr(), message = move(message)]() mutable { + auto self = weak_self.strong_ref(); + if (!self) + return; + + if (self->on_message_received) + self->on_message_received(move(message.as_object())); }); } diff --git a/Libraries/LibDevTools/Connection.h b/Libraries/LibDevTools/Connection.h index 2f05666674..a0e5d48618 100644 --- a/Libraries/LibDevTools/Connection.h +++ b/Libraries/LibDevTools/Connection.h @@ -11,12 +11,15 @@ #include #include #include +#include #include #include namespace DevTools { -class DEVTOOLS_API Connection : public RefCounted { +class DEVTOOLS_API Connection + : public RefCounted + , public Weakable { public: static NonnullRefPtr create(NonnullOwnPtr); ~Connection(); diff --git a/Libraries/LibDevTools/DevToolsServer.cpp b/Libraries/LibDevTools/DevToolsServer.cpp index 39fdfff8e4..82b1671a29 100644 --- a/Libraries/LibDevTools/DevToolsServer.cpp +++ b/Libraries/LibDevTools/DevToolsServer.cpp @@ -37,13 +37,25 @@ DevToolsServer::DevToolsServer(DevToolsDelegate& delegate, NonnullRefPtron_ready_to_accept = [this]() { - if (auto result = on_new_client(); result.is_error()) + m_server->on_ready_to_accept = [weak_self = make_weak_ptr()] { + if (!weak_self) + return; + + if (auto result = weak_self->on_new_client(); result.is_error()) warnln("Failed to accept DevTools client: {}", result.error()); }; } -DevToolsServer::~DevToolsServer() = default; +DevToolsServer::~DevToolsServer() +{ + m_is_shutting_down = true; + m_server->on_ready_to_accept = {}; + + if (m_connection) { + m_connection->on_connection_closed = {}; + m_connection->on_message_received = {}; + } +} Optional DevToolsServer::local_port() const { @@ -64,8 +76,12 @@ void DevToolsServer::refresh_tab_list() void DevToolsServer::unregister_actor(String const& name) { - Core::deferred_invoke([this, name] { - m_actor_registry.remove(name); + if (m_is_shutting_down) + return; + + Core::deferred_invoke([weak_self = make_weak_ptr(), name] { + if (weak_self) + weak_self->m_actor_registry.remove(name); }); } @@ -79,12 +95,14 @@ ErrorOr DevToolsServer::on_new_client() m_connection = Connection::create(move(buffered_socket)); - m_connection->on_connection_closed = [this]() { - close_connection(); + m_connection->on_connection_closed = [weak_self = make_weak_ptr()] { + if (weak_self) + weak_self->close_connection(); }; - m_connection->on_message_received = [this](auto message) { - on_message_received(move(message)); + m_connection->on_message_received = [weak_self = make_weak_ptr()](auto message) { + if (weak_self) + weak_self->on_message_received(move(message)); }; m_root_actor = register_actor(); @@ -124,10 +142,16 @@ void DevToolsServer::close_connection() { dbgln_if(DEVTOOLS_DEBUG, "Lost connection to the DevTools client"); - Core::deferred_invoke([this]() { - m_connection = nullptr; - m_actor_registry.clear(); - m_root_actor = nullptr; + if (m_is_shutting_down) + return; + + Core::deferred_invoke([weak_self = make_weak_ptr()] { + if (!weak_self) + return; + + weak_self->m_connection = nullptr; + weak_self->m_actor_registry.clear(); + weak_self->m_root_actor = nullptr; }); } diff --git a/Libraries/LibDevTools/DevToolsServer.h b/Libraries/LibDevTools/DevToolsServer.h index 475835cfc5..2bc83a770a 100644 --- a/Libraries/LibDevTools/DevToolsServer.h +++ b/Libraries/LibDevTools/DevToolsServer.h @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -20,7 +21,7 @@ namespace DevTools { using ActorRegistry = HashMap>; -class DEVTOOLS_API DevToolsServer { +class DEVTOOLS_API DevToolsServer : public Weakable { public: static ErrorOr> create(DevToolsDelegate&, u16 port); ~DevToolsServer(); @@ -69,6 +70,7 @@ private: u64 m_server_id { 0 }; u64 m_actor_count { 0 }; + bool m_is_shutting_down { false }; }; } diff --git a/Tests/LibDevTools/TestDevToolsProtocol.cpp b/Tests/LibDevTools/TestDevToolsProtocol.cpp index d2a25f1249..55ccd9fe67 100644 --- a/Tests/LibDevTools/TestDevToolsProtocol.cpp +++ b/Tests/LibDevTools/TestDevToolsProtocol.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -1903,6 +1904,19 @@ TEST_CASE(styles_and_stylesheets) EXPECT_EQ(client.request(move(bad_get_text)).get_string("error"sv).value(), "unknownActor"sv); } +TEST_CASE(devtools_server_teardown_with_pending_actor_cleanup) +{ + auto session = create_session(); + auto& client = *session->client; + (void)client.read_message(); + + auto tab_actor = actor_from(get_tab(client), "actor"sv); + session->server->unregister_actor(tab_actor); + session->server->connection()->on_connection_closed(); + session->server.clear(); + pump(session->loop); +} + TEST_CASE(console_network_navigation_and_accessibility) { auto session = create_session();