From 262b49d7131267b499251ef942b6973484d77448 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Fri, 17 Jun 2016 15:32:31 +0200 Subject: [PATCH] [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 --- .../autotests/client/test_plasmashell.cpp | 33 +++++++++++++++++ .../autotests/client/test_wayland_shell.cpp | 36 +++++++++++++++++++ src/wayland/plasmashell_interface.cpp | 6 ++++ .../server/qtsurfaceextension_interface.cpp | 6 ++++ src/wayland/server/shell_interface.cpp | 6 ++++ 5 files changed, 87 insertions(+) diff --git a/src/wayland/autotests/client/test_plasmashell.cpp b/src/wayland/autotests/client/test_plasmashell.cpp index 288cdb7cb8..415b8eec57 100644 --- a/src/wayland/autotests/client/test_plasmashell.cpp +++ b/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 s(m_compositor->createSurface()); + QVERIFY(surfaceCreatedSpy.wait()); + auto serverSurface = surfaceCreatedSpy.first().first().value(); + QVERIFY(serverSurface); + + // create ShellSurface + QSignalSpy shellSurfaceCreatedSpy(m_plasmaShellInterface, &PlasmaShellInterface::surfaceCreated); + QVERIFY(shellSurfaceCreatedSpy.isValid()); + QScopedPointer 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" diff --git a/src/wayland/autotests/client/test_wayland_shell.cpp b/src/wayland/autotests/client/test_wayland_shell.cpp index 26f4db562c..1a1070818d 100644 --- a/src/wayland/autotests/client/test_wayland_shell.cpp +++ b/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 s(m_compositor->createSurface()); + QVERIFY(surfaceCreatedSpy.wait()); + auto serverSurface = surfaceCreatedSpy.first().first().value(); + QVERIFY(serverSurface); + + // create ShellSurface + QSignalSpy shellSurfaceCreatedSpy(m_shellInterface, &ShellInterface::surfaceCreated); + QVERIFY(shellSurfaceCreatedSpy.isValid()); + QScopedPointer 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" diff --git a/src/wayland/plasmashell_interface.cpp b/src/wayland/plasmashell_interface.cpp index 8aba3b8cec..4a70bac1c7 100644 --- a/src/wayland/plasmashell_interface.cpp +++ b/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; diff --git a/src/wayland/server/qtsurfaceextension_interface.cpp b/src/wayland/server/qtsurfaceextension_interface.cpp index 0d6ea8c71a..7b388afa8d 100644 --- a/src/wayland/server/qtsurfaceextension_interface.cpp +++ b/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; diff --git a/src/wayland/server/shell_interface.cpp b/src/wayland/server/shell_interface.cpp index 37f88f3488..9a7226cca4 100644 --- a/src/wayland/server/shell_interface.cpp +++ b/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;