[server] Use deleteLater when a ClientConnection gets destroyed

Summary:
In the situation that a wl_client gets destroyed while still
wl_resources are around it can happen that one of them calls into the
ClientConnection during the cleanup handling which gets triggered at the
same time. This can then trigger a crash.

This change uses deleteLater for the ClientConnection and sets the hold
wl_client pointer to null instead of deleting directly. So the
ClientConnection is still around while the Resources gets cleaned up.
This is similar to the cleanup of Resource where on unbind the
wl_resource pointer is set to null and the Resource gets delete later.

BUG: 370232
FIXED-IN: 5.28

Reviewers: #plasma, bshah

Subscribers: plasma-devel

Tags: #plasma_on_wayland

Differential Revision: https://phabricator.kde.org/D3004

src/wayland/autotests/client/test_wayland_shell.cpp

@@ -65,6 +65,7 @@ private Q_SLOTS:
     void testResize();
     void testDisconnect();
     void testWhileDestroying();
+    void testClientDisconnecting();
 
 private:
     KWayland::Server::Display *m_display;
@@ -795,5 +796,46 @@ void TestWaylandShell::testWhileDestroying()
     QVERIFY(clientErrorSpy.isEmpty());
 }
 
+void TestWaylandShell::testClientDisconnecting()
+{
+    // this test tries to request a new surface size while the client is actually already destroyed
+    // see BUG: 370232
+    using namespace KWayland::Client;
+    using namespace KWayland::Server;
+    // create ShellSurface
+    QScopedPointer<Surface> s(m_compositor->createSurface());
+    QSignalSpy shellSurfaceCreatedSpy(m_shellInterface, &ShellInterface::surfaceCreated);
+    QVERIFY(shellSurfaceCreatedSpy.isValid());
+    QScopedPointer<ShellSurface> ps(m_shell->createSurface(s.data()));
+    QVERIFY(shellSurfaceCreatedSpy.wait());
+
+    auto serverShellSurface = shellSurfaceCreatedSpy.first().first().value<ShellSurfaceInterface*>();
+    QVERIFY(serverShellSurface);
+
+    QSignalSpy shellSurfaceUnboundSpy(serverShellSurface, &Resource::unbound);
+    QVERIFY(shellSurfaceUnboundSpy.isValid());
+
+    QScopedPointer<Surface> s2(m_compositor->createSurface());
+    QScopedPointer<ShellSurface> ps2(m_shell->createSurface(s2.data()));
+    QVERIFY(shellSurfaceCreatedSpy.wait());
+    auto serverShellSurface2 = shellSurfaceCreatedSpy.last().first().value<ShellSurfaceInterface*>();
+    QVERIFY(serverShellSurface2);
+
+    connect(serverShellSurface, &Resource::unbound, this,
+        [serverShellSurface, serverShellSurface2] {
+            serverShellSurface2->requestSize(QSize(100, 200));
+        }
+    );
+
+    m_connection->deleteLater();
+    m_connection = nullptr;
+    m_thread->quit();
+    m_thread->wait();
+    delete m_thread;
+    m_thread = nullptr;
+
+    QVERIFY(shellSurfaceUnboundSpy.wait());
+}
+
 QTEST_GUILESS_MAIN(TestWaylandShell)
 #include "test_wayland_shell.moc"

src/wayland/autotests/server/test_display.cpp

@@ -170,7 +170,10 @@ void TestWaylandServerDisplay::testClientConnection()
     QVERIFY(disconnectedSpy.isEmpty());
     wl_client_destroy(client);
     QCOMPARE(disconnectedSpy.count(), 1);
+    QSignalSpy clientDestroyedSpy(client2, &QObject::destroyed);
+    QVERIFY(clientDestroyedSpy.isValid());
     client2->destroy();
+    QVERIFY(clientDestroyedSpy.wait());
     QCOMPARE(disconnectedSpy.count(), 2);
     close(sv[0]);
     close(sv[1]);

src/wayland/clientconnection.cpp

@@ -66,7 +66,9 @@ ClientConnection::Private::Private(wl_client *c, Display *display, ClientConnect
 
 ClientConnection::Private::~Private()
 {
-    wl_list_remove(&listener.link);
+    if (client) {
+        wl_list_remove(&listener.link);
+    }
     s_allClients.removeAt(s_allClients.indexOf(this));
 }
 
@@ -80,9 +82,12 @@ void ClientConnection::Private::destroyListenerCallback(wl_listener *listener, v
         }
     );
     Q_ASSERT(it != s_allClients.constEnd());
-    auto q = (*it)->q;
+    auto p = (*it);
+    auto q = p->q;
+    p->client = nullptr;
+    wl_list_remove(&p->listener.link);
     emit q->disconnected(q);
-    delete q;
+    q->deleteLater();
 }
 
 ClientConnection::ClientConnection(wl_client *c, Display *parent)
@@ -95,21 +100,33 @@ ClientConnection::~ClientConnection() = default;
 
 void ClientConnection::flush()
 {
+    if (!d->client) {
+        return;
+    }
     wl_client_flush(d->client);
 }
 
 void ClientConnection::destroy()
 {
+    if (!d->client) {
+        return;
+    }
     wl_client_destroy(d->client);
 }
 
 wl_resource *ClientConnection::createResource(const wl_interface *interface, quint32 version, quint32 id)
 {
+    if (!d->client) {
+        return nullptr;
+    }
     return wl_resource_create(d->client, interface, version, id);
 }
 
 wl_resource *ClientConnection::getResource(quint32 id)
 {
+    if (!d->client) {
+        return nullptr;
+    }
     return wl_client_get_object(d->client, id);
 }