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

@@ -94,9 +94,13 @@ ErrorOr<void> 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<Connection>(), 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()));
         });
     }
 

Libraries/LibDevTools/Connection.h

@@ -11,12 +11,15 @@
 #include <AK/NonnullOwnPtr.h>
 #include <AK/NonnullRefPtr.h>
 #include <AK/RefCounted.h>
+#include <AK/Weakable.h>
 #include <LibCore/Socket.h>
 #include <LibDevTools/Forward.h>
 
 namespace DevTools {
 
-class DEVTOOLS_API Connection : public RefCounted<Connection> {
+class DEVTOOLS_API Connection
+    : public RefCounted<Connection>
+    , public Weakable<Connection> {
 public:
     static NonnullRefPtr<Connection> create(NonnullOwnPtr<Core::BufferedTCPSocket>);
     ~Connection();

Libraries/LibDevTools/DevToolsServer.cpp

@@ -37,13 +37,25 @@ DevToolsServer::DevToolsServer(DevToolsDelegate& delegate, NonnullRefPtr<Core::T
     , m_delegate(delegate)
     , m_server_id(s_server_count++)
 {
-    m_server->on_ready_to_accept = [this]() {
-        if (auto result = on_new_client(); result.is_error())
+    m_server->on_ready_to_accept = [weak_self = make_weak_ptr<DevToolsServer>()] {
+        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<u16> 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<DevToolsServer>(), name] {
+        if (weak_self)
+            weak_self->m_actor_registry.remove(name);
     });
 }
 
@@ -79,12 +95,14 @@ ErrorOr<void> 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<DevToolsServer>()] {
+        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<DevToolsServer>()](auto message) {
+        if (weak_self)
+            weak_self->on_message_received(move(message));
     };
 
     m_root_actor = register_actor<RootActor>();
@@ -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<DevToolsServer>()] {
+        if (!weak_self)
+            return;
+
+        weak_self->m_connection = nullptr;
+        weak_self->m_actor_registry.clear();
+        weak_self->m_root_actor = nullptr;
     });
 }
 

Libraries/LibDevTools/DevToolsServer.h

@@ -12,6 +12,7 @@
 #include <AK/NonnullRefPtr.h>
 #include <AK/Optional.h>
 #include <AK/String.h>
+#include <AK/Weakable.h>
 #include <LibCore/Socket.h>
 #include <LibDevTools/Actors/RootActor.h>
 #include <LibDevTools/Forward.h>
@@ -20,7 +21,7 @@ namespace DevTools {
 
 using ActorRegistry = HashMap<String, NonnullRefPtr<Actor>>;
 
-class DEVTOOLS_API DevToolsServer {
+class DEVTOOLS_API DevToolsServer : public Weakable<DevToolsServer> {
 public:
     static ErrorOr<NonnullOwnPtr<DevToolsServer>> 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 };
 };
 
 }

Tests/LibDevTools/TestDevToolsProtocol.cpp

@@ -14,6 +14,7 @@
 #include <LibCore/EventLoop.h>
 #include <LibCore/Socket.h>
 #include <LibCore/System.h>
+#include <LibDevTools/Connection.h>
 #include <LibDevTools/DevToolsDelegate.h>
 #include <LibDevTools/DevToolsServer.h>
 #include <LibHTTP/Header.h>
@@ -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();