diff --git a/autotests/integration/CMakeLists.txt b/autotests/integration/CMakeLists.txt index b552630dd3..332dae62b7 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 e38232f75a..8739b66c4e 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 0000000000..83895fd33e --- /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 a9b11a6d2a..0b59606e39 100644 --- a/kwin.kcfg +++ b/kwin.kcfg @@ -302,4 +302,16 @@ 0 + + + + + + + XwaylandCrashPolicy::Restart + + + 3 + + diff --git a/options.cpp b/options.cpp index af19228b2d..4bce7ee393 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 9f30283443..6d72017d3e 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 ade3c68780..fc72766105 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 eaa9897dba..8ae2cc5c4a 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) };