From 78a8b6877c50556fdfe1cee465e0e5d04c2e8364 Mon Sep 17 00:00:00 2001
From: David Edmundson <kde@davidedmundson.co.uk>
Date: Fri, 24 Apr 2020 15:45:39 +0100
Subject: [PATCH] Add wrapper for wl_global_remove

Summary:
Removes the Global from the registry, but does not delete the underlying
wl_global

Removal of a global is racey in wayland.
A client could be trying to bind at that moment.

Typically globals are static for the lifespan of the compositor, however
there are exceptions

For those cases this call will can remove the global from the registry,
but still keep the wl_global instance alive
and handling bind requests.

The compositor can then remove the Global wrapper (this object) deleting
the wl_global after an arbitrary delay or
keep it around for re-use for the duration of the compositor.

Test Plan:
Unit test
Made blur global outlive BlurEffect - no longer disconnects plasma on config changes

Reviewers: #plasma, apol

Reviewed By: apol

Subscribers: kde-frameworks-devel

Tags: #frameworks

Differential Revision: https://phabricator.kde.org/D28883
---
 .../autotests/client/test_wayland_blur.cpp    | 37 +++++++++++++++++++
 src/wayland/server/global.cpp                 | 12 +++++-
 src/wayland/server/global.h                   | 18 +++++++++
 3 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/src/wayland/autotests/client/test_wayland_blur.cpp b/src/wayland/autotests/client/test_wayland_blur.cpp
index f6cd202ade..b2010a17fc 100644
--- a/src/wayland/autotests/client/test_wayland_blur.cpp
+++ b/src/wayland/autotests/client/test_wayland_blur.cpp
@@ -31,6 +31,7 @@ private Q_SLOTS:
 
     void testCreate();
     void testSurfaceDestroy();
+    void testGlobalDestroy();
 
 private:
     KWayland::Server::Display *m_display;
@@ -197,5 +198,41 @@ void TestBlur::testSurfaceDestroy()
     QVERIFY(blurDestroyedSpy.wait());
 }
 
+
+void TestBlur::testGlobalDestroy()
+{
+    Registry registry;
+    QSignalSpy blurAnnouncedSpy(&registry, &Registry::blurAnnounced);
+    QSignalSpy blurRemovedSpy(&registry, &Registry::blurRemoved);
+
+    registry.setEventQueue(m_queue);
+
+    registry.create(m_connection->display());
+    QVERIFY(registry.isValid());
+    registry.setup();
+
+    QVERIFY(blurAnnouncedSpy.wait());
+
+    QSignalSpy serverSurfaceCreated(m_compositorInterface, &KWayland::Server::CompositorInterface::surfaceCreated);
+    QScopedPointer<KWayland::Client::Surface> surface(m_compositor->createSurface());
+    QVERIFY(serverSurfaceCreated.wait());
+
+    auto serverSurface = serverSurfaceCreated.first().first().value<KWayland::Server::SurfaceInterface*>();
+    QSignalSpy blurChanged(serverSurface, &KWayland::Server::SurfaceInterface::blurChanged);
+
+    m_blurManagerInterface->remove();
+
+    // we've deleted our global, but the clients have some operations in flight as they haven't processed this yet
+    // when we process them, we should not throw an error and kill the client
+
+    QScopedPointer<KWayland::Client::Blur> blur(m_blurManager->createBlur(surface.data()));
+    blur->setRegion(m_compositor->createRegion(QRegion(0, 0, 10, 20), nullptr));
+    blur->commit();
+    surface->commit(KWayland::Client::Surface::CommitFlag::None);
+
+    // client finally sees the blur is gone
+    QVERIFY(blurRemovedSpy.wait());
+}
+
 QTEST_GUILESS_MAIN(TestBlur)
 #include "test_wayland_blur.moc"
diff --git a/src/wayland/server/global.cpp b/src/wayland/server/global.cpp
index 2bbf79abc3..38da712d82 100644
--- a/src/wayland/server/global.cpp
+++ b/src/wayland/server/global.cpp
@@ -26,12 +26,14 @@ Global::Private::~Private() = default;
 void Global::Private::bind(wl_client *client, void *data, uint32_t version, uint32_t id)
 {
     auto d = reinterpret_cast<Private*>(data);
+    if (!d) {
+        return;
+    }
     d->bind(client, version, id);
 }
 
 void Global::Private::create()
 {
-    Q_ASSERT(!global);
     global = wl_global_create(*display, m_interface, m_version, this, bind);
 }
 
@@ -51,6 +53,14 @@ void Global::create()
     d->create();
 }
 
+void Global::remove()
+{
+    if (!d->global) {
+        return;
+    }
+    wl_global_remove(d->global);
+}
+
 void Global::destroy()
 {
     if (!d->global) {
diff --git a/src/wayland/server/global.h b/src/wayland/server/global.h
index e156dedf53..4a7b039218 100644
--- a/src/wayland/server/global.h
+++ b/src/wayland/server/global.h
@@ -53,6 +53,24 @@ public:
      * to the clients.
      **/
     void create();
+
+    /**
+     * Removes the Global from the registry, but does not delete the underlying wl_global
+     *
+     * Removal of a global is racey. A client could be trying to bind at that moment.
+     * Typically globals are static for the lifespan of the compositor, however there are exceptions
+     *
+     * For those cases this call will remove the global from the registry, but keep the wl_global instance alive
+     * and handling bind requests.
+     *
+     * The compositor can then remove the Global wrapper (this object) deleting the wl_global after an arbitrary delay or
+     * keep it around for re-use for the duration of the compositor.
+     *
+     * See https://gitlab.freedesktop.org/wayland/wayland/issues/10
+     *
+     * @since 5.70
+     */
+    void remove();
     /**
      * Destroys the low level wl_global. Afterwards the Global is no longer shown to clients.
      **/