[server] Unset SurfaceInterface pointer in referenced objects when being destroyed

Summary:
This is a gotcha moment:
1. Create Surface with id 1
2. destroy that Surface
3. Create another Surface

Now if in step 3 the id is by pure chance getting reused and also 1, the
wl_resource pointer of the SurfaceInterface of step 1 and step 3 are
the same. This is rather unexpected and causes problems.

When creating a ShellSurface in step 1 and step 3 it would fail. KWayland
finds a ShellSurface which was already created for the Surface. The same
can happen with QtSurfaceExtensionInterface and PlasmaShellInterface which
also go into error state.

On client side this would trigger a protocol error and terminate the
application. An easy way to reproduce is opening the file open dialog
from within Kate multiple times.

This change addresses this problem by setting the surface pointer in
those classes to null when the parent Surface gets destroyed. Thus
creating a new ShellSurface won't find the old referenced Surface any
more.

For ShellSurface and PlasmaShellSurface a test case is added which hit
the condition without this change. For QtSurfaceExtension we don't have
the client side, so we cannot really simulate the condition.

Reviewers: #plasma_on_wayland

Subscribers: plasma-devel

Tags: #plasma_on_wayland

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

src/wayland/autotests/client/test_plasmashell.cpp

@@ -50,6 +50,7 @@ private Q_SLOTS:
     void testPanelBehavior_data();
     void testPanelBehavior();
     void testDisconnect();
+    void testWhileDestroying();
 
 private:
     Display *m_display = nullptr;
@@ -376,5 +377,37 @@ void TestPlasmaShell::testDisconnect()
     m_queue->destroy();
 }
 
+void TestPlasmaShell::testWhileDestroying()
+{
+    // this test tries to hit a condition that a Surface gets created with an ID which was already
+    // used for a previous Surface. For each Surface we try to create a PlasmaShellSurface.
+    // Even if there was a Surface in the past with the same ID, it should create the PlasmaShellSurface
+    QSignalSpy surfaceCreatedSpy(m_compositorInterface, &CompositorInterface::surfaceCreated);
+    QVERIFY(surfaceCreatedSpy.isValid());
+    QScopedPointer<Surface> s(m_compositor->createSurface());
+    QVERIFY(surfaceCreatedSpy.wait());
+    auto serverSurface = surfaceCreatedSpy.first().first().value<SurfaceInterface*>();
+    QVERIFY(serverSurface);
+
+    // create ShellSurface
+    QSignalSpy shellSurfaceCreatedSpy(m_plasmaShellInterface, &PlasmaShellInterface::surfaceCreated);
+    QVERIFY(shellSurfaceCreatedSpy.isValid());
+    QScopedPointer<PlasmaShellSurface> ps(m_plasmaShell->createSurface(s.data()));
+    QVERIFY(shellSurfaceCreatedSpy.wait());
+
+    // now try to create more surfaces
+    QSignalSpy clientErrorSpy(m_connection, &ConnectionThread::errorOccurred);
+    QVERIFY(clientErrorSpy.isValid());
+    for (int i = 0; i < 100; i++) {
+        s.reset();
+        s.reset(m_compositor->createSurface());
+        m_plasmaShell->createSurface(s.data(), this);
+        QVERIFY(surfaceCreatedSpy.wait());
+    }
+    QVERIFY(clientErrorSpy.isEmpty());
+    QVERIFY(!clientErrorSpy.wait(100));
+    QVERIFY(clientErrorSpy.isEmpty());
+}
+
 QTEST_GUILESS_MAIN(TestPlasmaShell)
 #include "test_plasmashell.moc"

src/wayland/autotests/client/test_wayland_shell.cpp

@@ -64,6 +64,7 @@ private Q_SLOTS:
     void testResize_data();
     void testResize();
     void testDisconnect();
+    void testWhileDestroying();
 
 private:
     KWayland::Server::Display *m_display;
@@ -759,5 +760,40 @@ void TestWaylandShell::testDisconnect()
     m_queue->destroy();
 }
 
+void TestWaylandShell::testWhileDestroying()
+{
+    // this test tries to hit a condition that a Surface gets created with an ID which was already
+    // used for a previous Surface. For each Surface we try to create a ShellSurface.
+    // Even if there was a Surface in the past with the same ID, it should create the ShellSurface
+    using namespace KWayland::Client;
+    using namespace KWayland::Server;
+    QSignalSpy surfaceCreatedSpy(m_compositorInterface, &CompositorInterface::surfaceCreated);
+    QVERIFY(surfaceCreatedSpy.isValid());
+    QScopedPointer<Surface> s(m_compositor->createSurface());
+    QVERIFY(surfaceCreatedSpy.wait());
+    auto serverSurface = surfaceCreatedSpy.first().first().value<SurfaceInterface*>();
+    QVERIFY(serverSurface);
+
+    // create ShellSurface
+    QSignalSpy shellSurfaceCreatedSpy(m_shellInterface, &ShellInterface::surfaceCreated);
+    QVERIFY(shellSurfaceCreatedSpy.isValid());
+    QScopedPointer<ShellSurface> ps(m_shell->createSurface(s.data()));
+    QVERIFY(shellSurfaceCreatedSpy.wait());
+
+    // now try to create more surfaces
+    QSignalSpy clientErrorSpy(m_connection, &ConnectionThread::errorOccurred);
+    QVERIFY(clientErrorSpy.isValid());
+    for (int i = 0; i < 100; i++) {
+        s.reset();
+        ps.reset();
+        s.reset(m_compositor->createSurface());
+        ps.reset(m_shell->createSurface(s.data()));
+        QVERIFY(surfaceCreatedSpy.wait());
+    }
+    QVERIFY(clientErrorSpy.isEmpty());
+    QVERIFY(!clientErrorSpy.wait(100));
+    QVERIFY(clientErrorSpy.isEmpty());
+}
+
 QTEST_GUILESS_MAIN(TestWaylandShell)
 #include "test_wayland_shell.moc"

src/wayland/plasmashell_interface.cpp

@@ -165,6 +165,12 @@ const struct org_kde_plasma_surface_interface PlasmaShellSurfaceInterface::Priva
 PlasmaShellSurfaceInterface::PlasmaShellSurfaceInterface(PlasmaShellInterface *shell, SurfaceInterface *parent, wl_resource *parentResource)
     : Resource(new Private(this, shell, parent, parentResource))
 {
+    auto unsetSurface = [this] {
+        Q_D();
+        d->surface = nullptr;
+    };
+    connect(parent, &Resource::unbound, this, unsetSurface);
+    connect(parent, &QObject::destroyed, this, unsetSurface);
 }
 
 PlasmaShellSurfaceInterface::~PlasmaShellSurfaceInterface() = default;

src/wayland/server/qtsurfaceextension_interface.cpp

@@ -189,6 +189,12 @@ void QtExtendedSurfaceInterface::Private::updateGenericPropertyCallback(wl_clien
 QtExtendedSurfaceInterface::QtExtendedSurfaceInterface(QtSurfaceExtensionInterface *shell, SurfaceInterface *parent, wl_resource *parentResource)
     : Resource(new Private(this, shell, parent, parentResource))
 {
+    auto unsetSurface = [this] {
+        Q_D();
+        d->surface = nullptr;
+    };
+    connect(parent, &Resource::unbound, this, unsetSurface);
+    connect(parent, &QObject::destroyed, this, unsetSurface);
 }
 
 QtExtendedSurfaceInterface::~QtExtendedSurfaceInterface() = default;

src/wayland/server/shell_interface.cpp

@@ -193,6 +193,12 @@ ShellSurfaceInterface::ShellSurfaceInterface(ShellInterface *shell, SurfaceInter
 {
     Q_D();
     connect(d->pingTimer.data(), &QTimer::timeout, this, &ShellSurfaceInterface::pingTimeout);
+    auto unsetSurface = [this] {
+        Q_D();
+        d->surface = nullptr;
+    };
+    connect(parent, &Resource::unbound, this, unsetSurface);
+    connect(parent, &QObject::destroyed, this, unsetSurface);
 }
 
 ShellSurfaceInterface::~ShellSurfaceInterface() = default;