From 432cfb44c007accec4ea8c3b0427e09c4962f5aa Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Thu, 27 Aug 2020 12:05:28 +0300 Subject: [PATCH] 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 | 3 +- ...test.cpp => xwaylandserver_crash_test.cpp} | 18 ++- .../xwaylandserver_restart_test.cpp | 115 ++++++++++++++++++ kwin.kcfg | 12 ++ options.cpp | 22 ++++ options.h | 30 +++++ xwl/xwayland.cpp | 54 +++++++- xwl/xwayland.h | 10 +- 8 files changed, 250 insertions(+), 14 deletions(-) rename autotests/integration/{xwaylandserver_test.cpp => xwaylandserver_crash_test.cpp} (88%) create mode 100644 autotests/integration/xwaylandserver_restart_test.cpp diff --git a/autotests/integration/CMakeLists.txt b/autotests/integration/CMakeLists.txt index b552630dd..332dae62b 100644 --- a/autotests/integration/CMakeLists.txt +++ b/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) diff --git a/autotests/integration/xwaylandserver_test.cpp b/autotests/integration/xwaylandserver_crash_test.cpp similarity index 88% rename from autotests/integration/xwaylandserver_test.cpp rename to autotests/integration/xwaylandserver_crash_test.cpp index e38232f75..8739b66c4 100644 --- a/autotests/integration/xwaylandserver_test.cpp +++ b/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(); qRegisterMetaType(); @@ -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) -#include "xwaylandserver_test.moc" +WAYLANDTEST_MAIN(KWin::XwaylandServerCrashTest) +#include "xwaylandserver_crash_test.moc" diff --git a/autotests/integration/xwaylandserver_restart_test.cpp b/autotests/integration/xwaylandserver_restart_test.cpp new file mode 100644 index 000000000..83895fd33 --- /dev/null +++ b/autotests/integration/xwaylandserver_restart_test.cpp @@ -0,0 +1,115 @@ +/* + SPDX-FileCopyrightText: 2020 Vlad Zahorodnii + + 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 + +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(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 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(); + 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" diff --git a/kwin.kcfg b/kwin.kcfg index a9b11a6d2..0b59606e3 100644 --- a/kwin.kcfg +++ b/kwin.kcfg @@ -302,4 +302,16 @@ 0 + + + + + + + XwaylandCrashPolicy::Restart + + + 3 + + diff --git a/options.cpp b/options.cpp index af19228b2..4bce7ee39 100644 --- a/options.cpp +++ b/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()); diff --git a/options.h b/options.h index 9f3028344..6d72017d3 100644 --- a/options.h +++ b/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; diff --git a/xwl/xwayland.cpp b/xwl/xwayland.cpp index ade3c6878..fc7276610 100644 --- a/xwl/xwayland.cpp +++ b/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 #include #include +#include #include // 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 - // 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(); + switch (exitStatus) { + case QProcess::NormalExit: + 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"; diff --git a/xwl/xwayland.h b/xwl/xwayland.h index eaa9897db..8ae2cc5c4 100644 --- a/xwl/xwayland.h +++ b/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) };