Survive Xwayland crashes

If the Xwayland process crashes, it will bring down the entire session
together with itself. Obviously, we don't want that. At least, Wayland
clients should survive the crash.

This change refactors relevant X11 parts to handle Xwayland crashes in a
less fatal way.

In order to handle Xwayland crashes better, a pair of start() and stop()
methods had been introduced in the Xwayland class to allow starting and
stopping the Xwayland process at any moment.

If we detect that the Xwayland process has crashed, we will immediately
stop the Xwayland server, which in its turn will deactivate the socket
notifier and destroy all connected X11 clients. Unfortunately, a couple
of subtle changes in X11Client::releaseWindow() and Unmanaged::release()
had to be made to ensure that we are left with a valid state after the
Xwayland server has been stopped.

autotests/integration/kwin_wayland_test.cpp

@@ -83,17 +83,13 @@ WaylandTestApplication::~WaylandTestApplication()
     if (effects) {
         static_cast<EffectsHandlerImpl*>(effects)->unloadAllEffects();
     }
-    if (m_xwayland) {
+    delete m_xwayland;
-        // needs to be done before workspace gets destroyed
+    m_xwayland = nullptr;
-        m_xwayland->prepareDestroy();
-    }
     destroyWorkspace();
     waylandServer()->dispatch();
     if (QStyle *s = style()) {
         s->unpolish(this);
     }
-    // kill Xwayland before terminating its connection
-    delete m_xwayland;
     waylandServer()->terminateClientConnections();
     destroyCompositor();
 }
@@ -133,7 +129,7 @@ void WaylandTestApplication::continueStartupWithScreens()
 void WaylandTestApplication::finalizeStartup()
 {
     if (m_xwayland) {
-        disconnect(m_xwayland, &Xwl::Xwayland::initialized, this, &WaylandTestApplication::finalizeStartup);
+        disconnect(m_xwayland, &Xwl::Xwayland::started, this, &WaylandTestApplication::finalizeStartup);
     }
     notifyStarted();
 }
@@ -160,8 +156,8 @@ void WaylandTestApplication::continueStartupWithScene()
         std::cerr << "Xwayland had a critical error. Going to exit now." << std::endl;
         exit(code);
     });
-    connect(m_xwayland, &Xwl::Xwayland::initialized, this, &WaylandTestApplication::finalizeStartup);
+    connect(m_xwayland, &Xwl::Xwayland::started, this, &WaylandTestApplication::finalizeStartup);
-    m_xwayland->init();
+    m_xwayland->start();
 }
 
 }

composite.cpp

@@ -194,7 +194,7 @@ bool Compositor::setupStart()
 
     options->reloadCompositingSettings(true);
 
-    setupX11Support();
+    initializeX11();
 
     // There might still be a deleted around, needs to be cleared before
     // creating the scene (BUG 333275).
@@ -306,8 +306,13 @@ bool Compositor::setupStart()
     return true;
 }
 
-void Compositor::claimCompositorSelection()
+void Compositor::initializeX11()
 {
+    xcb_connection_t *connection = kwinApp()->x11Connection();
+    if (!connection) {
+        return;
+    }
+
     if (!m_selectionOwner) {
         char selection_name[ 100 ];
         sprintf(selection_name, "_NET_WM_CM_S%d", Application::x11ScreenNumber());
@@ -315,40 +320,34 @@ void Compositor::claimCompositorSelection()
         connect(m_selectionOwner, &CompositorSelectionOwner::lostOwnership,
                 this, &Compositor::stop);
     }
-
-    if (!m_selectionOwner) {
-        // No X11 yet.
-        return;
-    }
     if (!m_selectionOwner->owning()) {
         // Force claim ownership.
         m_selectionOwner->claim(true);
         m_selectionOwner->setOwning(true);
     }
+
+    xcb_composite_redirect_subwindows(connection, kwinApp()->x11RootWindow(),
+                                      XCB_COMPOSITE_REDIRECT_MANUAL);
 }
 
-void Compositor::setupX11Support()
+void Compositor::cleanupX11()
 {
-    auto *con = kwinApp()->x11Connection();
+    delete m_selectionOwner;
-    if (!con) {
+    m_selectionOwner = nullptr;
-        delete m_selectionOwner;
-        m_selectionOwner = nullptr;
-        return;
-    }
-    claimCompositorSelection();
-    xcb_composite_redirect_subwindows(con, kwinApp()->x11RootWindow(),
-                                      XCB_COMPOSITE_REDIRECT_MANUAL);
 }
 
 void Compositor::startupWithWorkspace()
 {
     connect(kwinApp(), &Application::x11ConnectionChanged,
-            this, &Compositor::setupX11Support, Qt::UniqueConnection);
+            this, &Compositor::initializeX11, Qt::UniqueConnection);
+    connect(kwinApp(), &Application::x11ConnectionAboutToBeDestroyed,
+            this, &Compositor::cleanupX11, Qt::UniqueConnection);
+    initializeX11();
+
     Workspace::self()->markXStackingOrderAsDirty();
     Q_ASSERT(m_scene);
 
     connect(workspace(), &Workspace::destroyed, this, [this] { compositeTimer.stop(); });
-    setupX11Support();
     fpsInterval = options->maxFpsInterval();
 
     if (m_scene->syncsToVBlank()) {

composite.h

@@ -141,9 +141,8 @@ protected:
     static Compositor *s_compositor;
 
 private:
-    void claimCompositorSelection();
+    void initializeX11();
-
+    void cleanupX11();
-    void setupX11Support();
 
     void setCompositeTimer();
     bool windowRepaintsPending() const;

libkwineffects/kwinglobals.h

@@ -152,22 +152,13 @@ Q_ENUM_NS(SessionState)
 inline
 KWIN_EXPORT xcb_connection_t *connection()
 {
-    static xcb_connection_t *s_con = nullptr;
+    return reinterpret_cast<xcb_connection_t*>(qApp->property("x11Connection").value<void*>());
-    if (!s_con) {
-        s_con = reinterpret_cast<xcb_connection_t*>(qApp->property("x11Connection").value<void*>());
-    }
-    Q_ASSERT(qApp);
-    return s_con;
 }
 
 inline
 KWIN_EXPORT xcb_window_t rootWindow()
 {
-    static xcb_window_t s_rootWindow = XCB_WINDOW_NONE;
+    return qApp->property("x11RootWindow").value<quint32>();
-    if (s_rootWindow == XCB_WINDOW_NONE) {
-        s_rootWindow = qApp->property("x11RootWindow").value<quint32>();
-    }
-    return s_rootWindow;
 }
 
 inline
@@ -179,19 +170,19 @@ KWIN_EXPORT xcb_timestamp_t xTime()
 inline
 KWIN_EXPORT xcb_screen_t *defaultScreen()
 {
-    static xcb_screen_t *s_screen = nullptr;
+    xcb_connection_t *c = connection();
-    if (s_screen) {
+    if (!c) {
-        return s_screen;
+        return nullptr;
     }
     int screen = qApp->property("x11ScreenNumber").toInt();
-    for (xcb_screen_iterator_t it = xcb_setup_roots_iterator(xcb_get_setup(connection()));
+    for (xcb_screen_iterator_t it = xcb_setup_roots_iterator(xcb_get_setup(c));
             it.rem;
             --screen, xcb_screen_next(&it)) {
         if (screen == 0) {
-            s_screen = it.data;
+            return it.data;
         }
     }
-    return s_screen;
+    return nullptr;
 }
 
 inline

main.cpp

@@ -309,11 +309,16 @@ void Application::createOptions()
     options = new Options;
 }
 
-void Application::setupEventFilters()
+void Application::installNativeX11EventFilter()
 {
     installNativeEventFilter(m_eventFilter.data());
 }
 
+void Application::removeNativeX11EventFilter()
+{
+    removeNativeEventFilter(m_eventFilter.data());
+}
+
 void Application::destroyWorkspace()
 {
     delete Workspace::self();

main.h

@@ -206,7 +206,8 @@ protected:
     void createWorkspace();
     void createAtoms();
     void createOptions();
-    void setupEventFilters();
+    void installNativeX11EventFilter();
+    void removeNativeX11EventFilter();
     void destroyWorkspace();
     void destroyCompositor();
     /**

main_wayland.cpp

@@ -134,19 +134,14 @@ ApplicationWayland::~ApplicationWayland()
     if (effects) {
         static_cast<EffectsHandlerImpl*>(effects)->unloadAllEffects();
     }
-    if (m_xwayland) {
+    delete m_xwayland;
-        // needs to be done before workspace gets destroyed
+    m_xwayland = nullptr;
-        m_xwayland->prepareDestroy();
-    }
     destroyWorkspace();
     waylandServer()->dispatch();
 
     if (QStyle *s = style()) {
         s->unpolish(this);
     }
-    // kill Xwayland before terminating its connection
-    delete m_xwayland;
-    m_xwayland = nullptr;
     waylandServer()->terminateClientConnections();
     destroyCompositor();
 }
@@ -196,7 +191,7 @@ void ApplicationWayland::continueStartupWithScreens()
 void ApplicationWayland::finalizeStartup()
 {
     if (m_xwayland) {
-        disconnect(m_xwayland, &Xwl::Xwayland::initialized, this, &ApplicationWayland::finalizeStartup);
+        disconnect(m_xwayland, &Xwl::Xwayland::started, this, &ApplicationWayland::finalizeStartup);
     }
     startSession();
     notifyStarted();
@@ -225,8 +220,8 @@ void ApplicationWayland::continueStartupWithScene()
         std::cerr << "Xwayland had a critical error. Going to exit now." << std::endl;
         exit(code);
     });
-    connect(m_xwayland, &Xwl::Xwayland::initialized, this, &ApplicationWayland::finalizeStartup);
+    connect(m_xwayland, &Xwl::Xwayland::started, this, &ApplicationWayland::finalizeStartup);
-    m_xwayland->init();
+    m_xwayland->start();
 }
 
 void ApplicationWayland::startSession()

main_x11.cpp

@@ -223,7 +223,7 @@ void ApplicationX11::performStartup()
     });
     connect(owner.data(), SIGNAL(lostOwnership()), SLOT(lostSelection()));
     connect(owner.data(), &KSelectionOwner::claimedOwnership, [this]{
-        setupEventFilters();
+        installNativeX11EventFilter();
         // first load options - done internally by a different thread
         createOptions();
 

rules.cpp

@@ -936,8 +936,9 @@ RuleBook::RuleBook(QObject *parent)
     , m_updatesDisabled(false)
     , m_temporaryRulesMessages()
 {
-    initWithX11();
+    initializeX11();
-    connect(kwinApp(), &Application::x11ConnectionChanged, this, &RuleBook::initWithX11);
+    connect(kwinApp(), &Application::x11ConnectionChanged, this, &RuleBook::initializeX11);
+    connect(kwinApp(), &Application::x11ConnectionAboutToBeDestroyed, this, &RuleBook::cleanupX11);
     connect(m_updateTimer, SIGNAL(timeout()), SLOT(save()));
     m_updateTimer->setInterval(1000);
     m_updateTimer->setSingleShot(true);
@@ -949,17 +950,21 @@ RuleBook::~RuleBook()
     deleteAll();
 }
 
-void RuleBook::initWithX11()
+void RuleBook::initializeX11()
 {
     auto c = kwinApp()->x11Connection();
     if (!c) {
-        m_temporaryRulesMessages.reset();
         return;
     }
     m_temporaryRulesMessages.reset(new KXMessages(c, kwinApp()->x11RootWindow(), "_KDE_NET_WM_TEMPORARY_RULES", nullptr));
     connect(m_temporaryRulesMessages.data(), SIGNAL(gotMessage(QString)), SLOT(temporaryRulesMessage(QString)));
 }
 
+void RuleBook::cleanupX11()
+{
+    m_temporaryRulesMessages.reset();
+}
+
 void RuleBook::deleteAll()
 {
     qDeleteAll(m_rules);

rules.h

@@ -312,7 +312,8 @@ private Q_SLOTS:
 
 private:
     void deleteAll();
-    void initWithX11();
+    void initializeX11();
+    void cleanupX11();
     QTimer *m_updateTimer;
     bool m_updatesDisabled;
     QList<Rules*> m_rules;

unmanaged.cpp

@@ -109,9 +109,9 @@ void Unmanaged::release(ReleaseReason releaseReason)
             xcb_shape_select_input(connection(), window(), false);
         Xcb::selectInput(window(), XCB_EVENT_MASK_NO_EVENT);
     }
+    workspace()->removeUnmanaged(this);
+    addWorkspaceRepaint(visibleRect());
     if (releaseReason != ReleaseReason::KWinShutsDown) {
-        workspace()->removeUnmanaged(this);
-        addWorkspaceRepaint(del->visibleRect());
         disownDataPassedToDeleted();
         del->unrefWindow();
     }

wayland_server.cpp

@@ -101,6 +101,11 @@ WaylandServer::~WaylandServer()
     destroyInputMethodConnection();
 }
 
+KWaylandServer::ClientConnection *WaylandServer::xWaylandConnection() const
+{
+    return m_xwaylandConnection;
+}
+
 void WaylandServer::destroyInternalConnection()
 {
     emit terminatingInternalClientConnection();
@@ -603,23 +608,17 @@ int WaylandServer::createXWaylandConnection()
     if (!socket.connection) {
         return -1;
     }
-    m_xwayland.client = socket.connection;
+    m_xwaylandConnection = socket.connection;
-    m_xwayland.destroyConnection = connect(m_xwayland.client, &KWaylandServer::ClientConnection::disconnected, this,
-        [] {
-            qFatal("Xwayland Connection died");
-        }
-    );
     return socket.fd;
 }
 
 void WaylandServer::destroyXWaylandConnection()
 {
-    if (!m_xwayland.client) {
+    if (!m_xwaylandConnection) {
         return;
     }
-    disconnect(m_xwayland.destroyConnection);
+    m_xwaylandConnection->destroy();
-    m_xwayland.client->destroy();
+    m_xwaylandConnection = nullptr;
-    m_xwayland.client = nullptr;
 }
 
 int WaylandServer::createInputMethodConnection()

wayland_server.h

@@ -184,9 +184,7 @@ public:
     void createInternalConnection();
     void initWorkspace();
 
-    KWaylandServer::ClientConnection *xWaylandConnection() const {
+    KWaylandServer::ClientConnection *xWaylandConnection() const;
-        return m_xwayland.client;
-    }
     KWaylandServer::ClientConnection *inputMethodConnection() const {
         return m_inputMethodServerConnection;
     }
@@ -281,10 +279,7 @@ private:
     KWaylandServer::LinuxDmabufUnstableV1Interface *m_linuxDmabuf = nullptr;
     KWaylandServer::KeyboardShortcutsInhibitManagerV1Interface *m_keyboardShortcutsInhibitManager = nullptr;
     QSet<KWaylandServer::LinuxDmabufUnstableV1Buffer*> m_linuxDmabufBuffers;
-    struct {
+    QPointer<KWaylandServer::ClientConnection> m_xwaylandConnection;
-        KWaylandServer::ClientConnection *client = nullptr;
-        QMetaObject::Connection destroyConnection;
-    } m_xwayland;
     KWaylandServer::ClientConnection *m_inputMethodServerConnection = nullptr;
     KWaylandServer::ClientConnection *m_screenLockerClientConnection = nullptr;
     struct {

workspace.cpp

@@ -125,7 +125,6 @@ Workspace::Workspace()
     , client_keys_client(nullptr)
     , global_shortcuts_disabled_for_client(false)
     , workspaceInit(true)
-    , startup(nullptr)
     , set_active_client_recursion(0)
     , block_stacking_updates(0)
     , m_sessionManager(new SessionManager(this))
@@ -295,7 +294,11 @@ void Workspace::init()
 
     active_client = nullptr;
 
-    initWithX11();
+    // We want to have some xcb connection while tearing down X11 components. We don't really
+    // care if the xcb connection is broken or has an error.
+    connect(kwinApp(), &Application::x11ConnectionChanged, this, &Workspace::initializeX11);
+    connect(kwinApp(), &Application::x11ConnectionAboutToBeDestroyed, this, &Workspace::cleanupX11);
+    initializeX11();
 
     Scripting::create(this);
 
@@ -315,24 +318,22 @@ void Workspace::init()
     // TODO: ungrabXServer()
 }
 
-void Workspace::initWithX11()
+void Workspace::initializeX11()
 {
     if (!kwinApp()->x11Connection()) {
-        connect(kwinApp(), &Application::x11ConnectionChanged, this, &Workspace::initWithX11, Qt::UniqueConnection);
         return;
     }
-    disconnect(kwinApp(), &Application::x11ConnectionChanged, this, &Workspace::initWithX11);
 
     atoms->retrieveHelpers();
 
     // first initialize the extensions
     Xcb::Extensions::self();
-    ColorMapper *colormaps = new ColorMapper(this);
+    m_colorMapper.reset(new ColorMapper(this));
-    connect(this, &Workspace::clientActivated, colormaps, &ColorMapper::update);
+    connect(this, &Workspace::clientActivated, m_colorMapper.data(), &ColorMapper::update);
 
     // Call this before XSelectInput() on the root window
-    startup = new KStartupInfo(
+    m_startup.reset(new KStartupInfo(
-        KStartupInfo::DisableKWinModule | KStartupInfo::AnnounceSilenceChanges, this);
+        KStartupInfo::DisableKWinModule | KStartupInfo::AnnounceSilenceChanges, this));
 
     // Select windowmanager privileges
     selectWmInputEventMask();
@@ -455,32 +456,54 @@ void Workspace::initWithX11()
         activateClient(new_active_client);
 }
 
+void Workspace::cleanupX11()
+{
+    // We expect that other components will unregister their X11 event filters after the
+    // connection to the X server has been lost.
+
+    StackingUpdatesBlocker blocker(this);
+
+    // Use stacking_order, so that kwin --replace keeps stacking order.
+    const QList<X11Client *> orderedClients = ensureStackingOrder(clients);
+    for (X11Client *client : orderedClients) {
+        client->releaseWindow(true);
+        unconstrained_stacking_order.removeOne(client);
+        stacking_order.removeOne(client);
+    }
+
+    for (Unmanaged *overrideRedirect : unmanaged) {
+        overrideRedirect->release(ReleaseReason::KWinShutsDown);
+        unconstrained_stacking_order.removeOne(overrideRedirect);
+        stacking_order.removeOne(overrideRedirect);
+    }
+
+    manual_overlays.clear();
+
+    VirtualDesktopManager *desktopManager = VirtualDesktopManager::self();
+    desktopManager->setRootInfo(nullptr);
+
+    X11Client::cleanupX11();
+    RootInfo::destroy();
+    Xcb::Extensions::destroy();
+
+    if (xcb_connection_t *connection = kwinApp()->x11Connection()) {
+        xcb_delete_property(connection, kwinApp()->x11RootWindow(), atoms->kwin_running);
+    }
+
+    m_colorMapper.reset();
+    m_movingClientFilter.reset();
+    m_startup.reset();
+    m_nullFocus.reset();
+    m_syncAlarmFilter.reset();
+    m_wasUserInteractionFilter.reset();
+    m_xStackingQueryTree.reset();
+}
+
 Workspace::~Workspace()
 {
     blockStackingUpdates(true);
 
-    // TODO: grabXServer();
+    cleanupX11();
-
-    // Use stacking_order, so that kwin --replace keeps stacking order
-    const QList<Toplevel *> stack = stacking_order;
-    // "mutex" the stackingorder, since anything trying to access it from now on will find
-    // many dangeling pointers and crash
-    stacking_order.clear();
-
-    for (auto it = stack.constBegin(), end = stack.constEnd(); it != end; ++it) {
-        X11Client *c = qobject_cast<X11Client *>(const_cast<Toplevel*>(*it));
-        if (!c) {
-            continue;
-        }
-        // Only release the window
-        c->releaseWindow(true);
-        // No removeClient() is called, it does more than just removing.
-        // However, remove from some lists to e.g. prevent performTransiencyCheck()
-        // from crashing.
-        clients.removeAll(c);
-        m_allClients.removeAll(c);
-    }
-    X11Client::cleanupX11();
 
     if (waylandServer()) {
         const QList<AbstractClient *> shellClients = waylandServer()->clients();
@@ -489,17 +512,10 @@ Workspace::~Workspace()
         }
     }
 
-    for (auto it = unmanaged.begin(), end = unmanaged.end(); it != end; ++it)
-        (*it)->release(ReleaseReason::KWinShutsDown);
-
     for (InternalClient *client : m_internalClients) {
         client->destroyClient();
     }
 
-    if (auto c = kwinApp()->x11Connection()) {
-        xcb_delete_property(c, kwinApp()->x11RootWindow(), atoms->kwin_running);
-    }
-
     for (auto it = deleted.begin(); it != deleted.end();) {
         emit deletedRemoved(*it);
         it = deleted.erase(it);
@@ -508,15 +524,10 @@ Workspace::~Workspace()
     delete RuleBook::self();
     kwinApp()->config()->sync();
 
-    RootInfo::destroy();
-    delete startup;
     delete Placement::self();
     delete client_keys_dialog;
     qDeleteAll(session);
 
-    // TODO: ungrabXServer();
-
-    Xcb::Extensions::destroy();
     _self = nullptr;
 }
 
@@ -642,7 +653,8 @@ void Workspace::removeClient(X11Client *c)
     if (c == most_recently_raised)
         most_recently_raised = nullptr;
     should_get_focus.removeAll(c);
-    Q_ASSERT(c != active_client);
+    if (c == active_client)
+        active_client = nullptr;
     if (c == last_active_client)
         last_active_client = nullptr;
     if (c == delayfocus_client)
@@ -757,6 +769,7 @@ void Workspace::addShellClient(AbstractClient *client)
 
 void Workspace::removeShellClient(AbstractClient *client)
 {
+    clientHidden(client);
     m_allClients.removeAll(client);
     if (client == most_recently_raised) {
         most_recently_raised = nullptr;
@@ -764,6 +777,9 @@ void Workspace::removeShellClient(AbstractClient *client)
     if (client == delayfocus_client) {
         cancelDelayFocus();
     }
+    if (client == active_client) {
+        active_client = nullptr;
+    }
     if (client == last_active_client) {
         last_active_client = nullptr;
     }
@@ -773,7 +789,6 @@ void Workspace::removeShellClient(AbstractClient *client)
     if (!client->shortcut().isEmpty()) {
         client->setShortcut(QString());   // Remove from client_keys
     }
-    clientHidden(client);
     emit clientRemoved(client);
     markXStackingOrderAsDirty();
     updateStackingOrder(true);
@@ -1258,7 +1273,7 @@ void Workspace::cancelDelayFocus()
 
 bool Workspace::checkStartupNotification(xcb_window_t w, KStartupInfoId &id, KStartupInfoData &data)
 {
-    return startup->checkStartup(w, id, data) == KStartupInfo::Match;
+    return m_startup->checkStartup(w, id, data) == KStartupInfo::Match;
 }
 
 /**

workspace.h

@@ -52,6 +52,7 @@ class Window;
 }
 
 class AbstractClient;
+class ColorMapper;
 class Compositor;
 class Deleted;
 class Group;
@@ -530,7 +531,8 @@ Q_SIGNALS:
 
 private:
     void init();
-    void initWithX11();
+    void initializeX11();
+    void cleanupX11();
     void initShortcuts();
     template <typename Slot>
     void initShortcut(const QString &actionName, const QString &description, const QKeySequence &shortcut,
@@ -642,7 +644,8 @@ private:
 
     bool workspaceInit;
 
-    KStartupInfo* startup;
+    QScopedPointer<KStartupInfo> m_startup;
+    QScopedPointer<ColorMapper> m_colorMapper;
 
     QVector<QRect> workarea; // Array of workareas for virtual desktops
     // Array of restricted areas that window cannot be moved into

x11client.cpp

@@ -248,8 +248,8 @@ void X11Client::releaseWindow(bool on_shutdown)
     m_frame.unmap();  // Destroying decoration would cause ugly visual effect
     destroyDecoration();
     cleanGrouping();
+    workspace()->removeClient(this);
     if (!on_shutdown) {
-        workspace()->removeClient(this);
         // Only when the window is being unmapped, not when closing down KWin (NETWM sections 5.5,5.7)
         info->setDesktop(0);
         info->setState(NET::States(), info->state());  // Reset all state flags

xwl/xwayland.cpp

@@ -4,6 +4,7 @@
 
 Copyright 2014 Martin Gräßlin <mgraesslin@kde.org>
 Copyright 2019 Roman Gilg <subdiff@gmail.com>
+Copyright (C) 2020 Vlad Zahorodnii <vlad.zahorodnii@kde.org>
 
 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License as published by
@@ -25,6 +26,7 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "utils.h"
 #include "wayland_server.h"
 #include "xcbutils.h"
+#include "xwayland_logging.h"
 
 #include <KLocalizedString>
 #include <KSelectionOwner>
@@ -32,9 +34,6 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include <QAbstractEventDispatcher>
 #include <QFile>
 #include <QFutureWatcher>
-#include <QProcess>
-#include <QSocketNotifier>
-#include <QThread>
 #include <QtConcurrentRun>
 
 // system
@@ -88,16 +87,7 @@ Xwayland::Xwayland(ApplicationWaylandAbstract *app, QObject *parent)
 
 Xwayland::~Xwayland()
 {
-    disconnect(m_xwaylandFailConnection);
+    stop();
-    destroyX11Connection();
-
-    if (m_xwaylandProcess) {
-        m_xwaylandProcess->terminate();
-        while (m_xwaylandProcess->state() != QProcess::NotRunning) {
-            m_app->processEvents(QEventLoop::WaitForMoreEvents);
-        }
-        waylandServer()->destroyXWaylandConnection();
-    }
     s_self = nullptr;
 }
 
@@ -106,7 +96,7 @@ QProcess *Xwayland::process() const
     return m_xwaylandProcess;
 }
 
-void Xwayland::init()
+void Xwayland::start()
 {
     int pipeFds[2];
     if (pipe(pipeFds) != 0) {
@@ -141,6 +131,7 @@ void Xwayland::init()
     }
 
     m_xcbConnectionFd = sx[0];
+    m_displayFileDescriptor = pipeFds[0];
 
     m_xwaylandProcess = new Process(this);
     m_xwaylandProcess->setProcessChannelMode(QProcess::ForwardedErrorChannel);
@@ -154,33 +145,126 @@ void Xwayland::init()
                            QStringLiteral("-rootless"),
                            QStringLiteral("-wm"),
                            QString::number(fd)});
-    m_xwaylandFailConnection = connect(m_xwaylandProcess, &QProcess::errorOccurred, this,
+    connect(m_xwaylandProcess, &QProcess::errorOccurred, this, &Xwayland::handleXwaylandError);
-        [this] (QProcess::ProcessError error) {
+    connect(m_xwaylandProcess, &QProcess::started, this, &Xwayland::handleXwaylandStarted);
-            if (error == QProcess::FailedToStart) {
+    connect(m_xwaylandProcess, QOverload<int, QProcess::ExitStatus>::of(&QProcess::finished),
-                std::cerr << "FATAL ERROR: failed to start Xwayland" << std::endl;
+            this, &Xwayland::handleXwaylandFinished);
-            } else {
-                std::cerr << "FATAL ERROR: Xwayland failed, going to exit now" << std::endl;
-            }
-            Q_EMIT criticalError(1);
-        }
-    );
-    const int xDisplayPipe = pipeFds[0];
-    connect(m_xwaylandProcess, &QProcess::started, this,
-        [this, xDisplayPipe] {
-            QFutureWatcher<void> *watcher = new QFutureWatcher<void>(this);
-            QObject::connect(watcher, &QFutureWatcher<void>::finished, this, &Xwayland::continueStartupWithX, Qt::QueuedConnection);
-            QObject::connect(watcher, &QFutureWatcher<void>::finished, watcher, &QFutureWatcher<void>::deleteLater, Qt::QueuedConnection);
-            watcher->setFuture(QtConcurrent::run(readDisplay, xDisplayPipe));
-        }
-    );
     m_xwaylandProcess->start();
     close(pipeFds[1]);
 }
 
-void Xwayland::prepareDestroy()
+void Xwayland::stop()
 {
+    if (!m_xwaylandProcess) {
+        return;
+    }
+
+    // If Xwayland has crashed, we must deactivate the socket notifier and ensure that no X11
+    // events will be dispatched before blocking; otherwise we will simply hang...
+    uninstallSocketNotifier();
+
     delete m_dataBridge;
     m_dataBridge = nullptr;
+
+    destroyX11Connection();
+
+    // When the Xwayland process is finally terminated, the finished() signal will be emitted,
+    // however we don't actually want to process it anymore. Furthermore, we also don't really
+    // want to handle any errors that may occur during the teardown.
+    if (m_xwaylandProcess->state() != QProcess::NotRunning) {
+        disconnect(m_xwaylandProcess, nullptr, this, nullptr);
+        m_xwaylandProcess->terminate();
+        m_xwaylandProcess->waitForFinished(5000);
+    }
+    delete m_xwaylandProcess;
+    m_xwaylandProcess = nullptr;
+
+    waylandServer()->destroyXWaylandConnection(); // This one must be destroyed last!
+}
+
+void Xwayland::dispatchEvents()
+{
+    xcb_connection_t *connection = kwinApp()->x11Connection();
+    if (!connection) {
+        qCWarning(KWIN_XWL, "Attempting to dispatch X11 events with no connection");
+        return;
+    }
+
+    while (xcb_generic_event_t *event = xcb_poll_for_event(connection)) {
+        if (m_dataBridge->filterEvent(event)) {
+            free(event);
+            continue;
+        }
+        long result = 0;
+        QAbstractEventDispatcher *dispatcher = QCoreApplication::eventDispatcher();
+        dispatcher->filterNativeEvent(QByteArrayLiteral("xcb_generic_event_t"), event, &result);
+        free(event);
+    }
+
+    xcb_flush(connection);
+}
+
+void Xwayland::installSocketNotifier()
+{
+    const int fileDescriptor = xcb_get_file_descriptor(kwinApp()->x11Connection());
+
+    m_socketNotifier = new QSocketNotifier(fileDescriptor, QSocketNotifier::Read, this);
+    connect(m_socketNotifier, &QSocketNotifier::activated, this, &Xwayland::dispatchEvents);
+
+    QAbstractEventDispatcher *dispatcher = QCoreApplication::eventDispatcher();
+    connect(dispatcher, &QAbstractEventDispatcher::aboutToBlock, this, &Xwayland::dispatchEvents);
+    connect(dispatcher, &QAbstractEventDispatcher::awake, this, &Xwayland::dispatchEvents);
+}
+
+void Xwayland::uninstallSocketNotifier()
+{
+    QAbstractEventDispatcher *dispatcher = QCoreApplication::eventDispatcher();
+    disconnect(dispatcher, &QAbstractEventDispatcher::aboutToBlock, this, &Xwayland::dispatchEvents);
+    disconnect(dispatcher, &QAbstractEventDispatcher::awake, this, &Xwayland::dispatchEvents);
+
+    delete m_socketNotifier;
+    m_socketNotifier = nullptr;
+}
+
+void Xwayland::handleXwaylandStarted()
+{
+    QFutureWatcher<void> *watcher = new QFutureWatcher<void>(this);
+    connect(watcher, &QFutureWatcher<void>::finished, this, &Xwayland::continueStartupWithX);
+    connect(watcher, &QFutureWatcher<void>::finished, watcher, &QFutureWatcher<void>::deleteLater);
+    watcher->setFuture(QtConcurrent::run(readDisplay, m_displayFileDescriptor));
+}
+
+void Xwayland::handleXwaylandFinished(int exitCode)
+{
+    qCDebug(KWIN_XWL) << "Xwayland process has quit with exit code" << exitCode;
+
+    // The Xwayland server has crashed... At this moment we have two choices either restart
+    // Xwayland or shut down all X11 related components. For now, we do the latter, we simply
+    // tear down everything that has any connection to X11.
+    stop();
+}
+
+void Xwayland::handleXwaylandError(QProcess::ProcessError error)
+{
+    switch (error) {
+    case QProcess::FailedToStart:
+        qCWarning(KWIN_XWL) << "Xwayland process failed to start";
+        emit criticalError(1);
+        return;
+    case QProcess::Crashed:
+        qCWarning(KWIN_XWL) << "Xwayland process crashed. Shutting down X11 components";
+        break;
+    case QProcess::Timedout:
+        qCWarning(KWIN_XWL) << "Xwayland operation timed out";
+        break;
+    case QProcess::WriteError:
+    case QProcess::ReadError:
+        qCWarning(KWIN_XWL) << "An error occurred while communicating with Xwayland";
+        break;
+    case QProcess::UnknownError:
+        qCWarning(KWIN_XWL) << "An unknown error has occurred in Xwayland";
+        break;
+    }
 }
 
 void Xwayland::createX11Connection()
@@ -199,7 +283,9 @@ void Xwayland::createX11Connection()
     m_app->setX11RootWindow(defaultScreen()->root);
 
     m_app->createAtoms();
-    m_app->setupEventFilters();
+    m_app->installNativeX11EventFilter();
+
+    installSocketNotifier();
 
     // Note that it's very important to have valid x11RootWindow(), x11ScreenNumber(), and
     // atoms when the rest of kwin is notified about the new X11 connection.
@@ -212,12 +298,18 @@ void Xwayland::destroyX11Connection()
         return;
     }
 
+    emit m_app->x11ConnectionAboutToBeDestroyed();
+
     Xcb::setInputFocus(XCB_INPUT_FOCUS_POINTER_ROOT);
     m_app->destroyAtoms();
+    m_app->removeNativeX11EventFilter();
 
-    emit m_app->x11ConnectionAboutToBeDestroyed();
     xcb_disconnect(m_app->x11Connection());
 
+    m_xcbScreen = nullptr;
+    m_xcbConnectionFd = -1;
+    m_xfixes = nullptr;
+
     m_app->setX11Connection(nullptr);
     m_app->setX11ScreenNumber(-1);
     m_app->setX11RootWindow(XCB_WINDOW_NONE);
@@ -234,22 +326,6 @@ void Xwayland::continueStartupWithX()
         Q_EMIT criticalError(1);
         return;
     }
-    QSocketNotifier *notifier = new QSocketNotifier(xcb_get_file_descriptor(xcbConn), QSocketNotifier::Read, this);
-    auto processXcbEvents = [this, xcbConn] {
-        while (auto event = xcb_poll_for_event(xcbConn)) {
-            if (m_dataBridge->filterEvent(event)) {
-                free(event);
-                continue;
-            }
-            long result = 0;
-            QThread::currentThread()->eventDispatcher()->filterNativeEvent(QByteArrayLiteral("xcb_generic_event_t"), event, &result);
-            free(event);
-        }
-        xcb_flush(xcbConn);
-    };
-    connect(notifier, &QSocketNotifier::activated, this, processXcbEvents);
-    connect(QThread::currentThread()->eventDispatcher(), &QAbstractEventDispatcher::aboutToBlock, this, processXcbEvents);
-    connect(QThread::currentThread()->eventDispatcher(), &QAbstractEventDispatcher::awake, this, processXcbEvents);
 
     xcb_prefetch_extension_data(xcbConn, &xcb_xfixes_id);
     m_xfixes = xcb_get_extension_data(xcbConn, &xcb_xfixes_id);
@@ -264,7 +340,7 @@ void Xwayland::continueStartupWithX()
     env.insert(QStringLiteral("DISPLAY"), QString::fromUtf8(qgetenv("DISPLAY")));
     m_app->setProcessStartupEnvironment(env);
 
-    emit initialized();
+    emit started();
 
     Xcb::sync(); // Trigger possible errors, there's still a chance to abort
 }

xwl/xwayland.h

@@ -3,6 +3,7 @@
  This file is part of the KDE project.
 
 Copyright 2019 Roman Gilg <subdiff@gmail.com>
+Copyright (C) 2020 Vlad Zahorodnii <vlad.zahorodnii@kde.org>
 
 This program is free software; you can redistribute it and/or modify
 it under the terms of the GNU General Public License as published by
@@ -22,6 +23,9 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 #include "xwayland_interface.h"
 
+#include <QProcess>
+#include <QSocketNotifier>
+
 #include <xcb/xproto.h>
 
 class xcb_screen_t;
@@ -44,9 +48,6 @@ public:
     Xwayland(ApplicationWaylandAbstract *app, QObject *parent = nullptr);
     ~Xwayland() override;
 
-    void init();
-    void prepareDestroy();
-
     xcb_screen_t *xcbScreen() const {
         return m_xcbScreen;
     }
@@ -54,27 +55,70 @@ public:
         return m_xfixes;
     }
 
+    /**
+     * Returns the associated Xwayland process or @c null if the Xwayland server is inactive.
+     */
     QProcess *process() const override;
 
+public Q_SLOTS:
+    /**
+     * Starts the Xwayland server.
+     *
+     * This method will spawn an Xwayland process and will establish a new XCB connection to it.
+     * If a fatal error has occurred during the startup, the criticalError() signal is going to
+     * be emitted. If the Xwayland server has started successfully, the started() signal will be
+     * emitted.
+     *
+     * @see started(), stop()
+     */
+    void start();
+    /**
+     * Stops the Xwayland server.
+     *
+     * This method will destroy the existing XCB connection as well all connected X11 clients.
+     *
+     * A SIGTERM signal will be sent to the Xwayland process. If Xwayland doesn't shut down
+     * within a reasonable amount of time (5 seconds), a SIGKILL signal will be sent and thus
+     * the process will be killed for good.
+     *
+     * If the Xwayland process crashes, the server will be stopped automatically.
+     *
+     * @see start()
+     */
+    void stop();
+
 Q_SIGNALS:
-    void initialized();
+    /**
+     * This signal is emitted when the Xwayland server has been started successfully and it is
+     * ready to accept and manage X11 clients.
+     */
+    void started();
     void criticalError(int code);
 
+private Q_SLOTS:
+    void dispatchEvents();
+
+    void handleXwaylandStarted();
+    void handleXwaylandFinished(int exitCode);
+    void handleXwaylandError(QProcess::ProcessError error);
+
 private:
+    void installSocketNotifier();
+    void uninstallSocketNotifier();
+
     void createX11Connection();
     void destroyX11Connection();
     void continueStartupWithX();
 
     DragEventReply dragMoveFilter(Toplevel *target, const QPoint &pos) override;
 
+    int m_displayFileDescriptor = -1;
     int m_xcbConnectionFd = -1;
     QProcess *m_xwaylandProcess = nullptr;
-    QMetaObject::Connection m_xwaylandFailConnection;
-
     xcb_screen_t *m_xcbScreen = nullptr;
     const xcb_query_extension_reply_t *m_xfixes = nullptr;
     DataBridge *m_dataBridge = nullptr;
-
+    QSocketNotifier *m_socketNotifier = nullptr;
     ApplicationWaylandAbstract *m_app;
 
     Q_DISABLE_COPY(Xwayland)