xwayland: Restart the Xwayland server after it has crashed

If the Xwayland process has crashed due to some bug, the user should
still be able to start applications in Xwayland mode. There is no reason
to restart the whole session just to be able to launch some application
that doesn't have native support for Wayland.

autotests/integration/CMakeLists.txt

@@ -78,7 +78,8 @@ if (XCB_ICCCM_FOUND)
     integrationTest(NAME testSceneQPainterShadow SRCS scene_qpainter_shadow_test.cpp LIBS XCB::ICCCM)
     integrationTest(NAME testStackingOrder SRCS stacking_order_test.cpp LIBS XCB::ICCCM)
     integrationTest(NAME testDbusInterface SRCS dbus_interface_test.cpp LIBS XCB::ICCCM)
-    integrationTest(NAME testXwaylandServer SRCS xwaylandserver_test.cpp LIBS XCB::ICCCM)
+    integrationTest(NAME testXwaylandServerCrash SRCS xwaylandserver_crash_test.cpp LIBS XCB::ICCCM)
+    integrationTest(NAME testXwaylandServerRestart SRCS xwaylandserver_restart_test.cpp LIBS XCB::ICCCM)
 
     if (KWIN_BUILD_ACTIVITIES)
         integrationTest(NAME testActivities SRCS activities_test.cpp LIBS XCB::ICCCM)

autotests/integration/xwaylandserver_crash_test.cpp

@@ -27,9 +27,9 @@ struct XcbConnectionDeleter
     }
 };
 
-static const QString s_socketName = QStringLiteral("wayland_test_kwin_xwayland_server-0");
+static const QString s_socketName = QStringLiteral("wayland_test_kwin_xwayland_server_crash-0");
 
-class XwaylandServerTest : public QObject
+class XwaylandServerCrashTest : public QObject
 {
     Q_OBJECT
 
@@ -38,7 +38,7 @@ private Q_SLOTS:
     void testCrash();
 };
 
-void XwaylandServerTest::initTestCase()
+void XwaylandServerCrashTest::initTestCase()
 {
     qRegisterMetaType<Unmanaged *>();
     qRegisterMetaType<X11Client *>();
@@ -48,6 +48,12 @@ void XwaylandServerTest::initTestCase()
     QVERIFY(waylandServer()->init(s_socketName.toLocal8Bit()));
     QMetaObject::invokeMethod(kwinApp()->platform(), "setVirtualOutputs", Qt::DirectConnection, Q_ARG(int, 2));
 
+    KSharedConfig::Ptr config = KSharedConfig::openConfig(QString(), KConfig::SimpleConfig);
+    KConfigGroup xwaylandGroup = config->group("Xwayland");
+    xwaylandGroup.writeEntry(QStringLiteral("XwaylandCrashPolicy"), QStringLiteral("Stop"));
+    xwaylandGroup.sync();
+    kwinApp()->setConfig(config);
+
     kwinApp()->start();
     QVERIFY(applicationStartedSpy.wait());
     QCOMPARE(screens()->count(), 2);
@@ -56,7 +62,7 @@ void XwaylandServerTest::initTestCase()
     waylandServer()->initWorkspace();
 }
 
-void XwaylandServerTest::testCrash()
+void XwaylandServerCrashTest::testCrash()
 {
     // This test verifies that all connected X11 clients get destroyed when Xwayland crashes.
 
@@ -122,5 +128,5 @@ void XwaylandServerTest::testCrash()
 
 } // namespace KWin
 
-WAYLANDTEST_MAIN(KWin::XwaylandServerTest)
+WAYLANDTEST_MAIN(KWin::XwaylandServerCrashTest)
-#include "xwaylandserver_test.moc"
+#include "xwaylandserver_crash_test.moc"

autotests/integration/xwaylandserver_restart_test.cpp

@@ -0,0 +1,115 @@
+/*
+    SPDX-FileCopyrightText: 2020 Vlad Zahorodnii <vlad.zahorodnii@kde.org>
+
+    SPDX-License-Identifier: GPL-2.0-or-later
+*/
+
+#include "kwin_wayland_test.h"
+#include "main.h"
+#include "platform.h"
+#include "screens.h"
+#include "wayland_server.h"
+#include "workspace.h"
+#include "x11client.h"
+#include "xwl/xwayland.h"
+
+#include <xcb/xcb_icccm.h>
+
+namespace KWin
+{
+
+struct XcbConnectionDeleter
+{
+    static inline void cleanup(xcb_connection_t *pointer)
+    {
+        xcb_disconnect(pointer);
+    }
+};
+
+static const QString s_socketName = QStringLiteral("wayland_test_kwin_xwayland_server_restart-0");
+
+class XwaylandServerRestartTest : public QObject
+{
+    Q_OBJECT
+
+private Q_SLOTS:
+    void initTestCase();
+    void testRestart();
+};
+
+void XwaylandServerRestartTest::initTestCase()
+{
+    QSignalSpy applicationStartedSpy(kwinApp(), &Application::started);
+    QVERIFY(applicationStartedSpy.isValid());
+    kwinApp()->platform()->setInitialWindowSize(QSize(1280, 1024));
+    QVERIFY(waylandServer()->init(s_socketName.toLocal8Bit()));
+    QMetaObject::invokeMethod(kwinApp()->platform(), "setVirtualOutputs", Qt::DirectConnection, Q_ARG(int, 2));
+
+    KSharedConfig::Ptr config = KSharedConfig::openConfig(QString(), KConfig::SimpleConfig);
+    KConfigGroup xwaylandGroup = config->group("Xwayland");
+    xwaylandGroup.writeEntry(QStringLiteral("XwaylandCrashPolicy"), QStringLiteral("Restart"));
+    xwaylandGroup.sync();
+    kwinApp()->setConfig(config);
+
+    kwinApp()->start();
+    QVERIFY(applicationStartedSpy.wait());
+    QCOMPARE(screens()->count(), 2);
+    QCOMPARE(screens()->geometry(0), QRect(0, 0, 1280, 1024));
+    QCOMPARE(screens()->geometry(1), QRect(1280, 0, 1280, 1024));
+    waylandServer()->initWorkspace();
+}
+
+static void kwin_safe_kill(QProcess *process)
+{
+    // The SIGKILL signal must be sent when the event loop is spinning.
+    QTimer::singleShot(1, process, &QProcess::kill);
+}
+
+void XwaylandServerRestartTest::testRestart()
+{
+    // This test verifies that the Xwayland server will be restarted after a crash.
+
+    Xwl::Xwayland *xwayland = static_cast<Xwl::Xwayland *>(XwaylandInterface::self());
+
+    // Pretend that the Xwayland process has crashed by sending a SIGKILL to it.
+    QSignalSpy startedSpy(xwayland, &Xwl::Xwayland::started);
+    QVERIFY(startedSpy.isValid());
+    kwin_safe_kill(xwayland->process());
+    QVERIFY(startedSpy.wait());
+    QCOMPARE(startedSpy.count(), 1);
+
+    // Check that the compositor still accepts new X11 clients.
+    QScopedPointer<xcb_connection_t, XcbConnectionDeleter> c(xcb_connect(nullptr, nullptr));
+    QVERIFY(!xcb_connection_has_error(c.data()));
+    const QRect rect(0, 0, 100, 200);
+    xcb_window_t window = xcb_generate_id(c.data());
+    xcb_create_window(c.data(), XCB_COPY_FROM_PARENT, window, rootWindow(),
+                      rect.x(), rect.y(), rect.width(), rect.height(), 0,
+                      XCB_WINDOW_CLASS_INPUT_OUTPUT, XCB_COPY_FROM_PARENT, 0, nullptr);
+    xcb_size_hints_t hints;
+    memset(&hints, 0, sizeof(hints));
+    xcb_icccm_size_hints_set_position(&hints, 1, rect.x(), rect.y());
+    xcb_icccm_size_hints_set_size(&hints, 1, rect.width(), rect.height());
+    xcb_icccm_size_hints_set_min_size(&hints, rect.width(), rect.height());
+    xcb_icccm_set_wm_normal_hints(c.data(), window, &hints);
+    xcb_map_window(c.data(), window);
+    xcb_flush(c.data());
+
+    QSignalSpy windowCreatedSpy(workspace(), &Workspace::clientAdded);
+    QVERIFY(windowCreatedSpy.isValid());
+    QVERIFY(windowCreatedSpy.wait());
+    X11Client *client = windowCreatedSpy.last().first().value<X11Client *>();
+    QVERIFY(client);
+    QCOMPARE(client->windowId(), window);
+    QVERIFY(client->isDecorated());
+
+    // Destroy the test window.
+    xcb_destroy_window(c.data(), window);
+    xcb_flush(c.data());
+    QVERIFY(Test::waitForWindowDestroyed(client));
+}
+
+} // namespace KWin
+
+WAYLANDTEST_MAIN(KWin::XwaylandServerRestartTest)
+#include "xwaylandserver_restart_test.moc"

kwin.kcfg

@@ -302,4 +302,16 @@
             <min>0</min>
         </entry>
     </group>
+    <group name="Xwayland">
+        <entry name="XwaylandCrashPolicy" type="Enum">
+            <choices name="KWin::XwaylandCrashPolicy">
+                <choice name="Stop"/>
+                <choice name="Restart"/>
+            </choices>
+            <default>XwaylandCrashPolicy::Restart</default>
+        </entry>
+        <entry name="XwaylandMaxCrashCount" type="UInt">
+            <default>3</default>
+        </entry>
+    </group>
 </kcfg>

options.cpp

@@ -95,6 +95,8 @@ Options::Options(QObject *parent)
     , m_focusStealingPreventionLevel(0)
     , m_killPingTimeout(0)
     , m_hideUtilityWindowsForInactive(false)
+    , m_xwaylandCrashPolicy(Options::defaultXwaylandCrashPolicy())
+    , m_xwaylandMaxCrashCount(Options::defaultXwaylandMaxCrashCount())
     , m_compositingMode(Options::defaultCompositingMode())
     , m_useCompositing(Options::defaultUseCompositing())
     , m_hiddenPreviews(Options::defaultHiddenPreviews())
@@ -171,6 +173,24 @@ void Options::setNextFocusPrefersMouse(bool nextFocusPrefersMouse)
     emit nextFocusPrefersMouseChanged();
 }
 
+void Options::setXwaylandCrashPolicy(XwaylandCrashPolicy crashPolicy)
+{
+    if (m_xwaylandCrashPolicy == crashPolicy) {
+        return;
+    }
+    m_xwaylandCrashPolicy = crashPolicy;
+    emit xwaylandCrashPolicyChanged();
+}
+
+void Options::setXwaylandMaxCrashCount(int maxCrashCount)
+{
+    if (m_xwaylandMaxCrashCount == maxCrashCount) {
+        return;
+    }
+    m_xwaylandMaxCrashCount = maxCrashCount;
+    emit xwaylandMaxCrashCountChanged();
+}
+
 void Options::setClickRaise(bool clickRaise)
 {
     if (m_autoRaise) {
@@ -803,6 +823,8 @@ void Options::syncFromKcfgc()
     setSeparateScreenFocus(m_settings->separateScreenFocus());
     setRollOverDesktops(m_settings->rollOverDesktops());
     setFocusStealingPreventionLevel(m_settings->focusStealingPreventionLevel());
+    setXwaylandCrashPolicy(m_settings->xwaylandCrashPolicy());
+    setXwaylandMaxCrashCount(m_settings->xwaylandMaxCrashCount());
 
 #ifdef KWIN_BUILD_DECORATIONS
     setPlacement(m_settings->placement());

options.h

@@ -34,18 +34,29 @@ enum HiddenPreviews {
     HiddenPreviewsAlways
 };
 
+/**
+ * This enum type specifies whether the Xwayland server must be restarted after a crash.
+ */
+enum XwaylandCrashPolicy {
+    Stop,
+    Restart,
+};
+
 class Settings;
 
 class KWIN_EXPORT Options : public QObject
 {
     Q_OBJECT
     Q_ENUMS(FocusPolicy)
+    Q_ENUMS(XwaylandCrashPolicy)
     Q_ENUMS(GlSwapStrategy)
     Q_ENUMS(MouseCommand)
     Q_ENUMS(MouseWheelCommand)
     Q_ENUMS(WindowOperation)
 
     Q_PROPERTY(FocusPolicy focusPolicy READ focusPolicy WRITE setFocusPolicy NOTIFY focusPolicyChanged)
+    Q_PROPERTY(XwaylandCrashPolicy xwaylandCrashPolicy READ xwaylandCrashPolicy WRITE setXwaylandCrashPolicy NOTIFY xwaylandCrashPolicyChanged)
+    Q_PROPERTY(int xwaylandMaxCrashCount READ xwaylandMaxCrashCount WRITE setXwaylandMaxCrashCount NOTIFY xwaylandMaxCrashCountChanged)
     Q_PROPERTY(bool nextFocusPrefersMouse READ isNextFocusPrefersMouse WRITE setNextFocusPrefersMouse NOTIFY nextFocusPrefersMouseChanged)
     /**
      * Whether clicking on a window raises it in FocusFollowsMouse
@@ -223,6 +234,13 @@ public:
         return m_nextFocusPrefersMouse;
     }
 
+    XwaylandCrashPolicy xwaylandCrashPolicy() const {
+        return m_xwaylandCrashPolicy;
+    }
+    int xwaylandMaxCrashCount() const {
+        return m_xwaylandMaxCrashCount;
+    }
+
     /**
      * Whether clicking on a window raises it in FocusFollowsMouse
      * mode or not.
@@ -576,6 +594,8 @@ public:
 
     // setters
     void setFocusPolicy(FocusPolicy focusPolicy);
+    void setXwaylandCrashPolicy(XwaylandCrashPolicy crashPolicy);
+    void setXwaylandMaxCrashCount(int maxCrashCount);
     void setNextFocusPrefersMouse(bool nextFocusPrefersMouse);
     void setClickRaise(bool clickRaise);
     void setAutoRaise(bool autoRaise);
@@ -735,6 +755,12 @@ public:
     static OpenGLPlatformInterface defaultGlPlatformInterface() {
         return kwinApp()->shouldUseWaylandForCompositing() ? EglPlatformInterface : GlxPlatformInterface;
     }
+    static XwaylandCrashPolicy defaultXwaylandCrashPolicy() {
+        return XwaylandCrashPolicy::Restart;
+    }
+    static int defaultXwaylandMaxCrashCount() {
+        return 3;
+    }
     /**
      * Performs loading all settings except compositing related.
      */
@@ -752,6 +778,8 @@ Q_SIGNALS:
     // for properties
     void focusPolicyChanged();
     void focusPolicyIsResonableChanged();
+    void xwaylandCrashPolicyChanged();
+    void xwaylandMaxCrashCountChanged();
     void nextFocusPrefersMouseChanged();
     void clickRaiseChanged();
     void autoRaiseChanged();
@@ -835,6 +863,8 @@ private:
     int m_focusStealingPreventionLevel;
     int m_killPingTimeout;
     bool m_hideUtilityWindowsForInactive;
+    XwaylandCrashPolicy m_xwaylandCrashPolicy;
+    int m_xwaylandMaxCrashCount;
 
     CompositingType m_compositingMode;
     bool m_useCompositing;

xwl/xwayland.cpp

@@ -12,6 +12,7 @@
 #include "databridge.h"
 
 #include "main_wayland.h"
+#include "options.h"
 #include "utils.h"
 #include "wayland_server.h"
 #include "xcbutils.h"
@@ -23,6 +24,7 @@
 #include <QAbstractEventDispatcher>
 #include <QFile>
 #include <QFutureWatcher>
+#include <QTimer>
 #include <QtConcurrentRun>
 
 // system
@@ -64,6 +66,9 @@ Xwayland::Xwayland(ApplicationWaylandAbstract *app, QObject *parent)
     : XwaylandInterface(parent)
     , m_app(app)
 {
+    m_resetCrashCountTimer = new QTimer(this);
+    m_resetCrashCountTimer->setSingleShot(true);
+    connect(m_resetCrashCountTimer, &QTimer::timeout, this, &Xwayland::resetCrashCount);
 }
 
 Xwayland::~Xwayland()
@@ -161,6 +166,12 @@ void Xwayland::stop()
     waylandServer()->destroyXWaylandConnection(); // This one must be destroyed last!
 }
 
+void Xwayland::restart()
+{
+    stop();
+    start();
+}
+
 void Xwayland::dispatchEvents()
 {
     xcb_connection_t *connection = kwinApp()->x11Connection();
@@ -216,14 +227,45 @@ void Xwayland::handleXwaylandStarted()
     watcher->setFuture(QtConcurrent::run(readDisplay, m_displayFileDescriptor));
 }
 
-void Xwayland::handleXwaylandFinished(int exitCode)
+void Xwayland::handleXwaylandFinished(int exitCode, QProcess::ExitStatus exitStatus)
 {
     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
+    switch (exitStatus) {
-    // Xwayland or shut down all X11 related components. For now, we do the latter, we simply
+    case QProcess::NormalExit:
-    // tear down everything that has any connection to X11.
+        stop();
-    stop();
+        break;
+    case QProcess::CrashExit:
+        handleXwaylandCrashed();
+        break;
+    }
+}
+
+void Xwayland::handleXwaylandCrashed()
+{
+    m_resetCrashCountTimer->stop();
+
+    switch (options->xwaylandCrashPolicy()) {
+    case XwaylandCrashPolicy::Restart:
+        if (++m_crashCount <= options->xwaylandMaxCrashCount()) {
+            restart();
+            m_resetCrashCountTimer->start(std::chrono::minutes(10));
+        } else {
+            qCWarning(KWIN_XWL, "Stopping Xwayland server because it has crashed %d times "
+                      "over the past 10 minutes", m_crashCount);
+            stop();
+        }
+        break;
+    case XwaylandCrashPolicy::Stop:
+        stop();
+        break;
+    }
+}
+
+void Xwayland::resetCrashCount()
+{
+    qCDebug(KWIN_XWL) << "Resetting the crash counter, its current value is" << m_crashCount;
+    m_crashCount = 0;
 }
 
 void Xwayland::handleXwaylandError(QProcess::ProcessError error)
@@ -234,7 +276,7 @@ void Xwayland::handleXwaylandError(QProcess::ProcessError error)
         emit criticalError(1);
         return;
     case QProcess::Crashed:
-        qCWarning(KWIN_XWL) << "Xwayland process crashed. Shutting down X11 components";
+        qCWarning(KWIN_XWL) << "Xwayland process crashed";
         break;
     case QProcess::Timedout:
         qCWarning(KWIN_XWL) << "Xwayland operation timed out";

xwl/xwayland.h

@@ -61,6 +61,10 @@ public Q_SLOTS:
      * @see start()
      */
     void stop();
+    /**
+     * Restarts the Xwayland server. This method is equivalent to calling stop() and start().
+     */
+    void restart();
 
 Q_SIGNALS:
     /**
@@ -72,9 +76,11 @@ Q_SIGNALS:
 
 private Q_SLOTS:
     void dispatchEvents();
+    void resetCrashCount();
 
     void handleXwaylandStarted();
-    void handleXwaylandFinished(int exitCode);
+    void handleXwaylandFinished(int exitCode, QProcess::ExitStatus exitStatus);
+    void handleXwaylandCrashed();
     void handleXwaylandError(QProcess::ProcessError error);
 
 private:
@@ -91,7 +97,9 @@ private:
     int m_xcbConnectionFd = -1;
     QProcess *m_xwaylandProcess = nullptr;
     QSocketNotifier *m_socketNotifier = nullptr;
+    QTimer *m_resetCrashCountTimer = nullptr;
     ApplicationWaylandAbstract *m_app;
+    int m_crashCount = 0;
 
     Q_DISABLE_COPY(Xwayland)
 };