wayland: Terminate client connections before Workspace is destroyed

Summary:
When ShellClient tears down, it needs to access RuleBook in order to
discard temporary rules. The problem is that WaylandServer outlives
Workspace and therefore so does ShellClient.

We can't guard against the case when RuleBook::self() is nullptr as it
is vital to discard temporary rules.

This change adjusts termination sequence so all shell clients are
destroyed before Workspace(and thus RuleBook) is gone.

ASAN output:
    ==19922==ERROR: AddressSanitizer: heap-use-after-free on address 0x606000142060 at pc 0x7fbc0fb878bb bp 0x7ffd7d464520 sp 0x7ffd7d464518
    READ of size 8 at 0x606000142060 thread T0
        #0 0x7fbc0fb878ba in QList<KWin::Rules*>::detach() /usr/include/qt5/QtCore/qlist.h:172
        #1 0x7fbc0fb8538d in QList<KWin::Rules*>::begin() /usr/include/qt5/QtCore/qlist.h:324
        #2 0x7fbc0fb808b6 in KWin::RuleBook::discardUsed(KWin::AbstractClient*, bool) /home/jenkins/workspace/Plasma/kwin/kf5-qt5 SUSEQt5.12/rules.cpp:1144
        #3 0x7fbc0fe36e32 in KWin::ShellClient::destroyClient() /home/jenkins/workspace/Plasma/kwin/kf5-qt5 SUSEQt5.12/shell_client.cpp:435
        #4 0x7fbc0fe7a726 in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (KWin::ShellClient::*)()>::call(void (KWin::ShellClient::*)(), KWin::ShellClient*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:152
        #5 0x7fbc0fe784c3 in void QtPrivate::FunctionPointer<void (KWin::ShellClient::*)()>::call<QtPrivate::List<>, void>(void (KWin::ShellClient::*)(), KWin::ShellClient*, void**) /usr/include/qt5/QtCore/qobjectdefs_impl.h:185
        #6 0x7fbc0fe74de9 in QtPrivate::QSlotObject<void (KWin::ShellClient::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (/home/jenkins/install-prefix/lib64/libkwin.so.5+0x1677de9)
        #7 0x7fbc04f27357 in QMetaObject::activate(QObject*, int, int, void**) (/usr/lib64/libQt5Core.so.5+0x2b3357)
        #8 0x7fbc074e1970 in KWayland::Server::Resource::unbound() /home/jenkins/workspace/Administration/Dependency Build Plasma kf5-qt5 SUSEQt5.12/kwayland/build/src/server/KF5WaylandServer_autogen/EWIEGA46WW/moc_resource.cpp:142
        #9 0x7fbc0766b4b4 in KWayland::Server::Resource::Private::unbind(wl_resource*) /home/jenkins/workspace/Administration/Dependency Build Plasma kf5-qt5 SUSEQt5.12/kwayland/src/server/resource.cpp:68
        #10 0x7fbc00bdc2ae  (/usr/lib64/libwayland-server.so.0+0x92ae)
        #11 0x7fbc00bdc32f in wl_resource_destroy (/usr/lib64/libwayland-server.so.0+0x932f)
        #12 0x7fbc0766b53f in KWayland::Server::Resource::Private::resourceDestroyedCallback(wl_client*, wl_resource*) /home/jenkins/workspace/Administration/Dependency Build Plasma kf5-qt5 SUSEQt5.12/kwayland/src/server/resource.cpp:76
        #13 0x7fbbff481d8c  (/usr/lib64/libffi.so.7+0x6d8c)
        #14 0x7fbbff481179  (/usr/lib64/libffi.so.7+0x6179)
        #15 0x7fbc00bdfa5f  (/usr/lib64/libwayland-server.so.0+0xca5f)
        #16 0x7fbc00bdc6d1  (/usr/lib64/libwayland-server.so.0+0x96d1)
        #17 0x7fbc00bddc71 in wl_event_loop_dispatch (/usr/lib64/libwayland-server.so.0+0xac71)
        #18 0x7fbc07541e50 in KWayland::Server::Display::Private::dispatch() /home/jenkins/workspace/Administration/Dependency Build Plasma kf5-qt5 SUSEQt5.12/kwayland/src/server/display.cpp:148
        #19 0x7fbc075432de in KWayland::Server::Display::dispatchEvents(int) /home/jenkins/workspace/Administration/Dependency Build Plasma kf5-qt5 SUSEQt5.12/kwayland/src/server/display.cpp:220
        #20 0x7fbc0fe864ca in KWin::WaylandServer::dispatch() /home/jenkins/workspace/Plasma/kwin/kf5-qt5 SUSEQt5.12/wayland_server.cpp:616
        #21 0x451ce0 in KWin::WaylandTestApplication::~WaylandTestApplication() /home/jenkins/workspace/Plasma/kwin/kf5-qt5 SUSEQt5.12/autotests/integration/kwin_wayland_test.cpp:91
        #22 0x42faa1 in main /home/jenkins/workspace/Plasma/kwin/kf5-qt5 SUSEQt5.12/autotests/integration/globalshortcuts_test.cpp:381
        #23 0x7fbc04796bca in __libc_start_main (/lib64/libc.so.6+0x26bca)
        #24 0x413ea9 in _start (/home/jenkins/workspace/Plasma/kwin/kf5-qt5 SUSEQt5.12/build/bin/testGlobalShortcuts+0x413ea9)

Reviewers: #kwin, davidedmundson

Reviewed By: #kwin, davidedmundson

Subscribers: kwin

Tags: #kwin

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

autotests/integration/kwin_wayland_test.cpp

@@ -83,18 +83,23 @@ WaylandTestApplication::~WaylandTestApplication()
     if (effects) {
         static_cast<EffectsHandlerImpl*>(effects)->unloadAllEffects();
     }
-    if (m_xwayland) {
-        // needs to be done before workspace gets destroyed
-        m_xwayland->prepareDestroy();
-    }
-    destroyWorkspace();
+
+    // Kill Xwayland before terminating its connection.
+    delete m_xwayland;
+    m_xwayland = nullptr;
+
+    // Terminate all client connections before Workspace is destroyed.
+    // Shell clients need to access RuleBook whose lifetime is bound
+    // to the Workspace class.
+    waylandServer()->terminateClientConnections();
     waylandServer()->dispatch();
+
+    destroyWorkspace();
+
     if (QStyle *s = style()) {
         s->unpolish(this);
     }
-    // kill Xwayland before terminating its connection
-    delete m_xwayland;
-    waylandServer()->terminateClientConnections();
+
     destroyCompositor();
 }
 

main_wayland.cpp

@@ -128,20 +128,23 @@ ApplicationWayland::~ApplicationWayland()
     if (effects) {
         static_cast<EffectsHandlerImpl*>(effects)->unloadAllEffects();
     }
-    if (m_xwayland) {
-        // needs to be done before workspace gets destroyed
-        m_xwayland->prepareDestroy();
-    }
-    destroyWorkspace();
+
+    // Kill Xwayland before terminating its connection.
+    delete m_xwayland;
+    m_xwayland = nullptr;
+
+    // Terminate all client connections before Workspace is destroyed.
+    // Shell clients need to access RuleBook whose lifetime is bound
+    // to the Workspace class.
+    waylandServer()->terminateClientConnections();
     waylandServer()->dispatch();
 
+    destroyWorkspace();
+
     if (QStyle *s = style()) {
         s->unpolish(this);
     }
-    // kill Xwayland before terminating its connection
-    delete m_xwayland;
-    m_xwayland = nullptr;
-    waylandServer()->terminateClientConnections();
+
     destroyCompositor();
 }
 

shell_client.cpp

@@ -425,11 +425,9 @@ void ShellClient::destroyClient()
     if (isMoveResize()) {
         leaveMoveResize();
     }
-    Deleted *del = nullptr;
-    if (workspace()) {
-        del = Deleted::create(this);
-    }
-    emit windowClosed(this, del);
+
+    Deleted *deleted = Deleted::create(this);
+    emit windowClosed(this, deleted);
 
     // Remove Force Temporarily rules.
     RuleBook::self()->discardUsed(this, true);
@@ -437,7 +435,6 @@ void ShellClient::destroyClient()
     destroyWindowManagementInterface();
     destroyDecoration();
 
-    if (workspace()) {
     StackingUpdatesBlocker blocker(workspace());
     if (transientFor()) {
         transientFor()->removeTransient(this);
@@ -450,12 +447,10 @@ void ShellClient::destroyClient()
             ++it;
         }
     }
-    }
+
     waylandServer()->removeClient(this);
 
-    if (del) {
-        del->unrefWindow();
-    }
+    deleted->unrefWindow();
     m_shellSurface = nullptr;
     m_xdgShellSurface = nullptr;
     m_xdgShellPopup = nullptr;

xwl/xwayland.cpp

@@ -88,6 +88,9 @@ Xwayland::Xwayland(ApplicationWaylandAbstract *app, QObject *parent)
 
 Xwayland::~Xwayland()
 {
+    delete m_dataBridge;
+    m_dataBridge = nullptr;
+
     disconnect(m_xwaylandFailConnection);
     if (m_app->x11Connection()) {
         Xcb::setInputFocus(XCB_INPUT_FOCUS_POINTER_ROOT);
@@ -178,12 +181,6 @@ void Xwayland::init()
     close(pipeFds[1]);
 }
 
-void Xwayland::prepareDestroy()
-{
-    delete m_dataBridge;
-    m_dataBridge = nullptr;
-}
-
 void Xwayland::createX11Connection()
 {
     int screenNumber = 0;

xwl/xwayland.h

@@ -47,7 +47,6 @@ public:
     ~Xwayland() override;
 
     void init();
-    void prepareDestroy();
 
     xcb_screen_t *xcbScreen() const {
         return m_xcbScreen;