From 19ad172584c1071c063a647dac457d206d2429ac Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Mon, 20 Jul 2020 11:07:08 +0300 Subject: [PATCH] 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 | 14 +- composite.cpp | 37 ++-- composite.h | 5 +- libkwineffects/kwinglobals.h | 25 +-- main.cpp | 7 +- main.h | 3 +- main_wayland.cpp | 15 +- main_x11.cpp | 2 +- rules.cpp | 13 +- rules.h | 3 +- unmanaged.cpp | 4 +- wayland_server.cpp | 19 +- wayland_server.h | 9 +- workspace.cpp | 107 +++++++----- workspace.h | 7 +- x11client.cpp | 2 +- xwl/xwayland.cpp | 182 ++++++++++++++------ xwl/xwayland.h | 58 ++++++- 18 files changed, 318 insertions(+), 194 deletions(-) diff --git a/autotests/integration/kwin_wayland_test.cpp b/autotests/integration/kwin_wayland_test.cpp index dae6bf9f25..51e146d871 100644 --- a/autotests/integration/kwin_wayland_test.cpp +++ b/autotests/integration/kwin_wayland_test.cpp @@ -83,17 +83,13 @@ WaylandTestApplication::~WaylandTestApplication() if (effects) { static_cast(effects)->unloadAllEffects(); } - if (m_xwayland) { - // needs to be done before workspace gets destroyed - m_xwayland->prepareDestroy(); - } + delete m_xwayland; + m_xwayland = nullptr; 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); - m_xwayland->init(); + connect(m_xwayland, &Xwl::Xwayland::started, this, &WaylandTestApplication::finalizeStartup); + m_xwayland->start(); } } diff --git a/composite.cpp b/composite.cpp index acd26e760a..cf744a3cb6 100644 --- a/composite.cpp +++ b/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(); - if (!con) { - delete m_selectionOwner; - m_selectionOwner = nullptr; - return; - } - claimCompositorSelection(); - xcb_composite_redirect_subwindows(con, kwinApp()->x11RootWindow(), - XCB_COMPOSITE_REDIRECT_MANUAL); + delete m_selectionOwner; + m_selectionOwner = nullptr; } 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()) { diff --git a/composite.h b/composite.h index 84783c8316..a0b210f530 100644 --- a/composite.h +++ b/composite.h @@ -141,9 +141,8 @@ protected: static Compositor *s_compositor; private: - void claimCompositorSelection(); - - void setupX11Support(); + void initializeX11(); + void cleanupX11(); void setCompositeTimer(); bool windowRepaintsPending() const; diff --git a/libkwineffects/kwinglobals.h b/libkwineffects/kwinglobals.h index 8bfcfe8ce8..04e92a1b08 100644 --- a/libkwineffects/kwinglobals.h +++ b/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; - if (!s_con) { - s_con = reinterpret_cast(qApp->property("x11Connection").value()); - } - Q_ASSERT(qApp); - return s_con; + return reinterpret_cast(qApp->property("x11Connection").value()); } inline KWIN_EXPORT xcb_window_t rootWindow() { - static xcb_window_t s_rootWindow = XCB_WINDOW_NONE; - if (s_rootWindow == XCB_WINDOW_NONE) { - s_rootWindow = qApp->property("x11RootWindow").value(); - } - return s_rootWindow; + return qApp->property("x11RootWindow").value(); } 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; - if (s_screen) { - return s_screen; + xcb_connection_t *c = connection(); + if (!c) { + 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 diff --git a/main.cpp b/main.cpp index 41c0418f80..fdb465c42c 100644 --- a/main.cpp +++ b/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(); diff --git a/main.h b/main.h index cbae0fbf89..1290b3671d 100644 --- a/main.h +++ b/main.h @@ -206,7 +206,8 @@ protected: void createWorkspace(); void createAtoms(); void createOptions(); - void setupEventFilters(); + void installNativeX11EventFilter(); + void removeNativeX11EventFilter(); void destroyWorkspace(); void destroyCompositor(); /** diff --git a/main_wayland.cpp b/main_wayland.cpp index b3dff99798..73c4f9e3d1 100644 --- a/main_wayland.cpp +++ b/main_wayland.cpp @@ -134,19 +134,14 @@ ApplicationWayland::~ApplicationWayland() if (effects) { static_cast(effects)->unloadAllEffects(); } - if (m_xwayland) { - // needs to be done before workspace gets destroyed - m_xwayland->prepareDestroy(); - } + delete m_xwayland; + m_xwayland = nullptr; 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); - m_xwayland->init(); + connect(m_xwayland, &Xwl::Xwayland::started, this, &ApplicationWayland::finalizeStartup); + m_xwayland->start(); } void ApplicationWayland::startSession() diff --git a/main_x11.cpp b/main_x11.cpp index 5897404563..051058ca0a 100644 --- a/main_x11.cpp +++ b/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(); diff --git a/rules.cpp b/rules.cpp index ed74841147..af8418006c 100644 --- a/rules.cpp +++ b/rules.cpp @@ -936,8 +936,9 @@ RuleBook::RuleBook(QObject *parent) , m_updatesDisabled(false) , m_temporaryRulesMessages() { - initWithX11(); - connect(kwinApp(), &Application::x11ConnectionChanged, this, &RuleBook::initWithX11); + initializeX11(); + 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); diff --git a/rules.h b/rules.h index c6f6061dbc..c5f8181807 100644 --- a/rules.h +++ b/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 m_rules; diff --git a/unmanaged.cpp b/unmanaged.cpp index 3c3f9e2f8f..8e3695362c 100644 --- a/unmanaged.cpp +++ b/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(); } diff --git a/wayland_server.cpp b/wayland_server.cpp index a1fbc197c3..80204e982e 100644 --- a/wayland_server.cpp +++ b/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_xwayland.destroyConnection = connect(m_xwayland.client, &KWaylandServer::ClientConnection::disconnected, this, - [] { - qFatal("Xwayland Connection died"); - } - ); + m_xwaylandConnection = socket.connection; return socket.fd; } void WaylandServer::destroyXWaylandConnection() { - if (!m_xwayland.client) { + if (!m_xwaylandConnection) { return; } - disconnect(m_xwayland.destroyConnection); - m_xwayland.client->destroy(); - m_xwayland.client = nullptr; + m_xwaylandConnection->destroy(); + m_xwaylandConnection = nullptr; } int WaylandServer::createInputMethodConnection() diff --git a/wayland_server.h b/wayland_server.h index 55c3798f02..97f71a4183 100644 --- a/wayland_server.h +++ b/wayland_server.h @@ -184,9 +184,7 @@ public: void createInternalConnection(); void initWorkspace(); - KWaylandServer::ClientConnection *xWaylandConnection() const { - return m_xwayland.client; - } + KWaylandServer::ClientConnection *xWaylandConnection() const; KWaylandServer::ClientConnection *inputMethodConnection() const { return m_inputMethodServerConnection; } @@ -281,10 +279,7 @@ private: KWaylandServer::LinuxDmabufUnstableV1Interface *m_linuxDmabuf = nullptr; KWaylandServer::KeyboardShortcutsInhibitManagerV1Interface *m_keyboardShortcutsInhibitManager = nullptr; QSet m_linuxDmabufBuffers; - struct { - KWaylandServer::ClientConnection *client = nullptr; - QMetaObject::Connection destroyConnection; - } m_xwayland; + QPointer m_xwaylandConnection; KWaylandServer::ClientConnection *m_inputMethodServerConnection = nullptr; KWaylandServer::ClientConnection *m_screenLockerClientConnection = nullptr; struct { diff --git a/workspace.cpp b/workspace.cpp index ab6583ae41..b1592a3db1 100644 --- a/workspace.cpp +++ b/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); - connect(this, &Workspace::clientActivated, colormaps, &ColorMapper::update); + m_colorMapper.reset(new ColorMapper(this)); + connect(this, &Workspace::clientActivated, m_colorMapper.data(), &ColorMapper::update); // Call this before XSelectInput() on the root window - startup = new KStartupInfo( - KStartupInfo::DisableKWinModule | KStartupInfo::AnnounceSilenceChanges, this); + m_startup.reset(new KStartupInfo( + 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 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(); - - // Use stacking_order, so that kwin --replace keeps stacking order - const QList 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(const_cast(*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(); + cleanupX11(); if (waylandServer()) { const QList 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; } /** diff --git a/workspace.h b/workspace.h index 073ef3759b..18206d5cf6 100644 --- a/workspace.h +++ b/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 void initShortcut(const QString &actionName, const QString &description, const QKeySequence &shortcut, @@ -642,7 +644,8 @@ private: bool workspaceInit; - KStartupInfo* startup; + QScopedPointer m_startup; + QScopedPointer m_colorMapper; QVector workarea; // Array of workareas for virtual desktops // Array of restricted areas that window cannot be moved into diff --git a/x11client.cpp b/x11client.cpp index 8f48a26202..6f8eab45ab 100644 --- a/x11client.cpp +++ b/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 diff --git a/xwl/xwayland.cpp b/xwl/xwayland.cpp index 7b23bf821c..0f4d63d904 100644 --- a/xwl/xwayland.cpp +++ b/xwl/xwayland.cpp @@ -4,6 +4,7 @@ Copyright 2014 Martin Gräßlin Copyright 2019 Roman Gilg +Copyright (C) 2020 Vlad Zahorodnii 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 . #include "utils.h" #include "wayland_server.h" #include "xcbutils.h" +#include "xwayland_logging.h" #include #include @@ -32,9 +34,6 @@ along with this program. If not, see . #include #include #include -#include -#include -#include #include // system @@ -88,16 +87,7 @@ Xwayland::Xwayland(ApplicationWaylandAbstract *app, QObject *parent) Xwayland::~Xwayland() { - disconnect(m_xwaylandFailConnection); - destroyX11Connection(); - - if (m_xwaylandProcess) { - m_xwaylandProcess->terminate(); - while (m_xwaylandProcess->state() != QProcess::NotRunning) { - m_app->processEvents(QEventLoop::WaitForMoreEvents); - } - waylandServer()->destroyXWaylandConnection(); - } + stop(); 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, - [this] (QProcess::ProcessError error) { - if (error == QProcess::FailedToStart) { - std::cerr << "FATAL ERROR: failed to start Xwayland" << std::endl; - } 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 *watcher = new QFutureWatcher(this); - QObject::connect(watcher, &QFutureWatcher::finished, this, &Xwayland::continueStartupWithX, Qt::QueuedConnection); - QObject::connect(watcher, &QFutureWatcher::finished, watcher, &QFutureWatcher::deleteLater, Qt::QueuedConnection); - watcher->setFuture(QtConcurrent::run(readDisplay, xDisplayPipe)); - } - ); + connect(m_xwaylandProcess, &QProcess::errorOccurred, this, &Xwayland::handleXwaylandError); + connect(m_xwaylandProcess, &QProcess::started, this, &Xwayland::handleXwaylandStarted); + connect(m_xwaylandProcess, QOverload::of(&QProcess::finished), + this, &Xwayland::handleXwaylandFinished); 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 *watcher = new QFutureWatcher(this); + connect(watcher, &QFutureWatcher::finished, this, &Xwayland::continueStartupWithX); + connect(watcher, &QFutureWatcher::finished, watcher, &QFutureWatcher::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 } diff --git a/xwl/xwayland.h b/xwl/xwayland.h index df359d6c55..5d2f06ebeb 100644 --- a/xwl/xwayland.h +++ b/xwl/xwayland.h @@ -3,6 +3,7 @@ This file is part of the KDE project. Copyright 2019 Roman Gilg +Copyright (C) 2020 Vlad Zahorodnii 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 . #include "xwayland_interface.h" +#include +#include + #include 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)