Improve the deconstruction of PlasmaWindows

Summary:
The protocol is extended by a dedicated destructor request. When a
PlasmaWindow is umapped we no longer destroy the resource directly,
but only send the unmap. The client is then supposed to clean up
(which it already did in that case) and will invoke the destructor.

The PlasmaWindowInterface object will be automatically deleted after
the unmap once all resources bound for it are destroyed.

The tests are extended by two new test cases which triggered protocol
errors on the client side prior to this change.

Reviewers: #plasma

Subscribers: plasma-devel

Projects: #plasma

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

src/wayland/autotests/client/test_wayland_windowmanagement.cpp

@@ -44,6 +44,8 @@ private Q_SLOTS:
 
     void testWindowTitle();
     void testMinimizedGeometry();
+    void testUseAfterUnmap();
+    void testServerDelete();
 
     void cleanup();
 
@@ -180,14 +182,6 @@ void TestWindowManagement::testMinimizedGeometry()
 
 void TestWindowManagement::cleanup()
 {
-    delete m_windowManagementInterface;
-    m_windowManagementInterface = nullptr;
-
-    delete m_windowInterface;
-    m_windowInterface = nullptr;
-
-    delete m_surfaceInterface;
-    m_surfaceInterface = nullptr;
 
     if (m_surface) {
         delete m_surface;
@@ -220,9 +214,49 @@ void TestWindowManagement::cleanup()
     }
     m_connection = nullptr;
 
+    delete m_windowManagementInterface;
+    m_windowManagementInterface = nullptr;
+
+    delete m_windowInterface;
+    m_windowInterface = nullptr;
+
+    delete m_surfaceInterface;
+    m_surfaceInterface = nullptr;
+
     delete m_display;
     m_display = nullptr;
 }
 
+void TestWindowManagement::testUseAfterUnmap()
+{
+    // this test verifies that when the client uses a window after it's unmapped, things don't break
+    QSignalSpy unmappedSpy(m_window, &KWayland::Client::PlasmaWindow::unmapped);
+    QVERIFY(unmappedSpy.isValid());
+    QSignalSpy destroyedSpy(m_window, &QObject::destroyed);
+    QVERIFY(destroyedSpy.isValid());
+    m_windowInterface->unmap();
+    m_window->requestClose();
+    QVERIFY(unmappedSpy.wait());
+    QVERIFY(destroyedSpy.wait());
+    m_window = nullptr;
+    QSignalSpy serverDestroyedSpy(m_windowInterface, &QObject::destroyed);
+    QVERIFY(serverDestroyedSpy.isValid());
+    QVERIFY(serverDestroyedSpy.wait());
+    m_windowInterface = nullptr;
+}
+
+void TestWindowManagement::testServerDelete()
+{
+    QSignalSpy unmappedSpy(m_window, &KWayland::Client::PlasmaWindow::unmapped);
+    QVERIFY(unmappedSpy.isValid());
+    QSignalSpy destroyedSpy(m_window, &QObject::destroyed);
+    QVERIFY(destroyedSpy.isValid());
+    delete m_windowInterface;
+    m_windowInterface = nullptr;
+    QVERIFY(unmappedSpy.wait());
+    QVERIFY(destroyedSpy.wait());
+    m_window = nullptr;
+}
+
 QTEST_GUILESS_MAIN(TestWindowManagement)
 #include "test_wayland_windowmanagement.moc"

src/wayland/plasmawindowmanagement_interface.cpp

@@ -74,17 +74,15 @@ public:
     void unmap();
     void setState(org_kde_plasma_window_management_state flag, bool set);
 
-    struct WindowResource {
-        wl_resource *resource;
-        wl_listener *destroyListener;
-    };
-    QList<WindowResource> resources;
+    QVector<wl_resource*> resources;
     quint32 windowId = 0;
     QHash<SurfaceInterface*, QRect> minimizedGeometries;
+    PlasmaWindowManagementInterface *wm;
+
+    bool unmapped = false;
 
 private:
     static void unbind(wl_resource *resource);
-    static void destroyListenerCallback(wl_listener *listener, void *data);
     static void setStateCallback(wl_client *client, wl_resource *resource, uint32_t flags, uint32_t state);
     static void setVirtualDesktopCallback(wl_client *client, wl_resource *resource, uint32_t number);
     static void closeCallback(wl_client *client, wl_resource *resource);
@@ -92,12 +90,12 @@ private:
     static void requestResizeCallback(wl_client *client, wl_resource *resource);
     static void setMinimizedGeometryCallback(wl_client *client, wl_resource *resource, wl_resource *panel, uint32_t x, uint32_t y, uint32_t width, uint32_t height);
     static void unsetMinimizedGeometryCallback(wl_client *client, wl_resource *resource, wl_resource *panel);
+    static void destroyCallback(wl_client *client, wl_resource *resource);
     static Private *cast(wl_resource *resource) {
         return reinterpret_cast<Private*>(wl_resource_get_user_data(resource));
     }
 
     PlasmaWindowInterface *q;
-    PlasmaWindowManagementInterface *wm;
     QString m_title;
     QString m_appId;
     QString m_themedIconName;
@@ -107,7 +105,7 @@ private:
     static const struct org_kde_plasma_window_interface s_interface;
 };
 
-const quint32 PlasmaWindowManagementInterface::Private::s_version = 3;
+const quint32 PlasmaWindowManagementInterface::Private::s_version = 4;
 
 PlasmaWindowManagementInterface::Private::Private(PlasmaWindowManagementInterface *q, Display *d)
     : Global::Private(d, &org_kde_plasma_window_management_interface, s_version)
@@ -245,6 +243,17 @@ PlasmaWindowInterface *PlasmaWindowManagementInterface::createWindow(QObject *pa
     return window;
 }
 
+void PlasmaWindowManagementInterface::unmapWindow(PlasmaWindowInterface *window)
+{
+    if (!window) {
+        return;
+    }
+    Q_D();
+    d->windows.removeOne(window);
+    Q_ASSERT(!d->windows.contains(window));
+    window->d->unmap();
+}
+
 #ifndef DOXYGEN_SHOULD_SKIP_THIS
 const struct org_kde_plasma_window_interface PlasmaWindowInterface::Private::s_interface = {
     setStateCallback,
@@ -253,7 +262,8 @@ const struct org_kde_plasma_window_interface PlasmaWindowInterface::Private::s_i
     unsetMinimizedGeometryCallback,
     closeCallback,
     requestMoveCallback,
-    requestResizeCallback
+    requestResizeCallback,
+    destroyCallback
 };
 #endif
 
@@ -268,32 +278,32 @@ PlasmaWindowInterface::Private::~Private()
     // need to copy, as destroy goes through the destroy listener and modifies the list as we iterate
     const auto c = resources;
     for (const auto &r : c) {
-        org_kde_plasma_window_send_unmapped(r.resource);
-        wl_resource_destroy(r.resource);
+        auto client = wl_resource_get_client(r);
+        org_kde_plasma_window_send_unmapped(r);
+        wl_resource_destroy(r);
+        wl_client_flush(client);
+    }
+}
+
+void PlasmaWindowInterface::Private::destroyCallback(wl_client *, wl_resource *r)
+{
+    Private *p = cast(r);
+    p->resources.removeAll(r);
+    wl_resource_destroy(r);
+    if (p->unmapped && p->resources.isEmpty()) {
+        p->q->deleteLater();
     }
 }
 
 void PlasmaWindowInterface::Private::unbind(wl_resource *resource)
 {
     Private *p = reinterpret_cast<Private*>(wl_resource_get_user_data(resource));
-    auto it = p->resources.begin();
-    while (it != p->resources.end()) {
-        if ((*it).resource == resource) {
-            wl_list_remove(&(*it).destroyListener->link);
-            delete (*it).destroyListener;
-            it = p->resources.erase(it);
-        } else {
-            it++;
-        }
+    p->resources.removeAll(resource);
+    if (p->unmapped && p->resources.isEmpty()) {
+        p->q->deleteLater();
     }
 }
 
-void PlasmaWindowInterface::Private::destroyListenerCallback(wl_listener *listener, void *data)
-{
-    Q_UNUSED(listener);
-    Private::unbind(reinterpret_cast<wl_resource*>(data));
-}
-
 void PlasmaWindowInterface::Private::createResource(wl_resource *parent, uint32_t id)
 {
     ClientConnection *c = wm->display()->getConnection(wl_resource_get_client(parent));
@@ -301,15 +311,8 @@ void PlasmaWindowInterface::Private::createResource(wl_resource *parent, uint32_
     if (!resource) {
         return;
     }
-    WindowResource r;
-    r.resource = resource;
-    r.destroyListener = new wl_listener;
-    r.destroyListener->notify = destroyListenerCallback;
-    r.destroyListener->link.prev = nullptr;
-    r.destroyListener->link.next = nullptr;
     wl_resource_set_implementation(resource, &s_interface, this, unbind);
-    wl_resource_add_destroy_listener(resource, r.destroyListener);
-    resources << r;
+    resources << resource;
 
     org_kde_plasma_window_send_virtual_desktop_changed(resource, m_virtualDesktop);
     if (!m_appId.isEmpty()) {
@@ -331,7 +334,7 @@ void PlasmaWindowInterface::Private::setAppId(const QString &appId)
     m_appId = appId;
     const QByteArray utf8 = m_appId.toUtf8();
     for (auto it = resources.constBegin(); it != resources.constEnd(); ++it) {
-        org_kde_plasma_window_send_app_id_changed((*it).resource, utf8.constData());
+        org_kde_plasma_window_send_app_id_changed(*it, utf8.constData());
     }
 }
 
@@ -343,7 +346,7 @@ void PlasmaWindowInterface::Private::setThemedIconName(const QString &iconName)
     m_themedIconName = iconName;
     const QByteArray utf8 = m_themedIconName.toUtf8();
     for (auto it = resources.constBegin(); it != resources.constEnd(); ++it) {
-        org_kde_plasma_window_send_themed_icon_name_changed((*it).resource, utf8.constData());
+        org_kde_plasma_window_send_themed_icon_name_changed(*it, utf8.constData());
     }
 }
 
@@ -355,7 +358,7 @@ void PlasmaWindowInterface::Private::setTitle(const QString &title)
     m_title = title;
     const QByteArray utf8 = m_title.toUtf8();
     for (auto it = resources.constBegin(); it != resources.constEnd(); ++it) {
-        org_kde_plasma_window_send_title_changed((*it).resource, utf8.constData());
+        org_kde_plasma_window_send_title_changed(*it, utf8.constData());
     }
 }
 
@@ -366,15 +369,18 @@ void PlasmaWindowInterface::Private::setVirtualDesktop(quint32 desktop)
     }
     m_virtualDesktop = desktop;
     for (auto it = resources.constBegin(); it != resources.constEnd(); ++it) {
-        org_kde_plasma_window_send_virtual_desktop_changed((*it).resource, m_virtualDesktop);
+        org_kde_plasma_window_send_virtual_desktop_changed(*it, m_virtualDesktop);
     }
 }
 
 void PlasmaWindowInterface::Private::unmap()
 {
+    unmapped = true;
     for (auto it = resources.constBegin(); it != resources.constEnd(); ++it) {
-        org_kde_plasma_window_send_unmapped((*it).resource);
-        wl_resource_destroy((*it).resource);
+        org_kde_plasma_window_send_unmapped(*it);
+    }
+    if (resources.isEmpty()) {
+        q->deleteLater();
     }
 }
 
@@ -391,7 +397,7 @@ void PlasmaWindowInterface::Private::setState(org_kde_plasma_window_management_s
     }
     m_state = newState;
     for (auto it = resources.constBegin(); it != resources.constEnd(); ++it) {
-        org_kde_plasma_window_send_state_changed((*it).resource, m_state);
+        org_kde_plasma_window_send_state_changed(*it, m_state);
     }
 }
 
@@ -544,7 +550,7 @@ void PlasmaWindowInterface::setVirtualDesktop(quint32 desktop)
 
 void PlasmaWindowInterface::unmap()
 {
-    d->unmap();
+    d->wm->unmapWindow(this);
 }
 
 QHash<SurfaceInterface*, QRect>  PlasmaWindowInterface::minimizedGeometries() const

src/wayland/plasmawindowmanagement_interface.h

@@ -52,6 +52,23 @@ public:
     PlasmaWindowInterface *createWindow(QObject *parent);
     QList<PlasmaWindowInterface*> windows() const;
 
+    /**
+     * Unmaps the @p window previously created with @link{createWindow}.
+     * The window will be unmapped and removed from the list of @link{windows}.
+     *
+     * Unmapping a @p window indicates to the client that it should destroy the
+     * resource created for the window. Once all resources for the @p window are
+     * destroyed, the @p window will get deleted automatically. There is no need
+     * to manually delete the @p window. A manual delete will trigger the unmap
+     * and resource destroy at the same time and can result in protocol errors on
+     * client side if it still accesses the resource before receiving the unmap event.
+     *
+     * @see createWindow
+     * @see windows
+     * @since 5.23
+     **/
+    void unmapWindow(PlasmaWindowInterface *window);
+
 Q_SIGNALS:
     void requestChangeShowingDesktop(ShowingDesktopState requestedState);
 
@@ -106,6 +123,13 @@ public:
      */
     void setVirtualDesktopChangeable(bool set);
 
+    /**
+     * This method removes the Window and the Client is supposed to release the resource
+     * bound for this Window. Once all resources are released the Window gets deleted.
+     *
+     * Prefer using @link{PlasmaWindowManagementInterface::unmapWindow}.
+     * @see PlasmaWindowManagementInterface::unmapWindow
+     **/
     void unmap();
 
     /**