From 372955bf05a00930f96b8362b9af7e365406f41b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Thu, 12 May 2016 07:40:27 +0200 Subject: [PATCH] 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 --- .../client/test_wayland_windowmanagement.cpp | 50 +++++++++-- .../plasmawindowmanagement_interface.cpp | 90 ++++++++++--------- .../plasmawindowmanagement_interface.h | 24 +++++ 3 files changed, 114 insertions(+), 50 deletions(-) diff --git a/src/wayland/autotests/client/test_wayland_windowmanagement.cpp b/src/wayland/autotests/client/test_wayland_windowmanagement.cpp index a9d8f43450..e82f5dc77d 100644 --- a/src/wayland/autotests/client/test_wayland_windowmanagement.cpp +++ b/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" diff --git a/src/wayland/plasmawindowmanagement_interface.cpp b/src/wayland/plasmawindowmanagement_interface.cpp index 942ddca34d..494944e845 100644 --- a/src/wayland/plasmawindowmanagement_interface.cpp +++ b/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 resources; + QVector resources; quint32 windowId = 0; QHash 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(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(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(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 PlasmaWindowInterface::minimizedGeometries() const diff --git a/src/wayland/plasmawindowmanagement_interface.h b/src/wayland/plasmawindowmanagement_interface.h index e97c0606b4..d004519d68 100644 --- a/src/wayland/plasmawindowmanagement_interface.h +++ b/src/wayland/plasmawindowmanagement_interface.h @@ -52,6 +52,23 @@ public: PlasmaWindowInterface *createWindow(QObject *parent); QList 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(); /**