From 93e5ebac63486678caa1be86c96a157d049e5564 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Gr=C3=A4=C3=9Flin?= Date: Mon, 7 Apr 2014 16:23:17 +0200 Subject: [PATCH] Try to wait for DESTROY_NOTIFY before releasing an Unmanaged So far the Unmanaged got released after an XCB_UNMAP_NOTIFY. This event gets created after xcb_unmap_window or after xcb_destroy_window. In the latter case the window is already distroyed and any of KWin's cleanup calls will cause a BadWindow (or similar) error. The idea to circumvent these errors is to try to wait for the DESTROY_NOTIFY event. To do so the processing of the release is slightly delayed. If KWin gets the destroy notify before the delay times out the Unamanged gets released immediately but with a Destroy flag. For this a new enum ReleaseToplevel is introduced and Unmanage::release takes this as an argument instead of the bool which indicated OnShutdown. Also this enum is added to Toplevel::finishCompositing so that it can ignore the destroyed case and not generate an error. REVIEW: 117422 --- client.cpp | 2 +- client.h | 2 +- composite.cpp | 10 +++-- events.cpp | 26 ++++++++++-- tests/unmapdestroytest.qml | 86 ++++++++++++++++++++++++++++++++++++++ toplevel.h | 11 ++++- unmanaged.cpp | 10 ++--- unmanaged.h | 3 +- workspace.cpp | 2 +- 9 files changed, 135 insertions(+), 17 deletions(-) create mode 100644 tests/unmapdestroytest.qml diff --git a/client.cpp b/client.cpp index f117815223..dedffbe05a 100644 --- a/client.cpp +++ b/client.cpp @@ -426,7 +426,7 @@ void Client::destroyClient() if (moveResizeMode) emit clientFinishUserMovedResized(this); emit windowClosed(this, del); - finishCompositing(); + finishCompositing(ReleaseReason::Destroyed); RuleBook::self()->discardUsed(this, true); // Remove ForceTemporarily rules StackingUpdatesBlocker blocker(workspace()); if (moveResizeMode) diff --git a/client.h b/client.h index ab6fd83395..96dd948ed3 100644 --- a/client.h +++ b/client.h @@ -490,7 +490,7 @@ public: bool hiddenPreview() const; ///< Window is mapped in order to get a window pixmap virtual bool setupCompositing(); - virtual void finishCompositing(); + void finishCompositing(ReleaseReason releaseReason = ReleaseReason::Release) override; void setBlockingCompositing(bool block); inline bool isBlockingCompositing() { return blocks_compositing; } diff --git a/composite.cpp b/composite.cpp index 187d05f58f..d5a730a89d 100644 --- a/composite.cpp +++ b/composite.cpp @@ -897,7 +897,7 @@ bool Toplevel::setupCompositing() return true; } -void Toplevel::finishCompositing() +void Toplevel::finishCompositing(ReleaseReason releaseReason) { if (damage_handle == XCB_NONE) return; @@ -907,7 +907,9 @@ void Toplevel::finishCompositing() delete effect_window; } - xcb_damage_destroy(connection(), damage_handle); + if (releaseReason != ReleaseReason::Destroyed) { + xcb_damage_destroy(connection(), damage_handle); + } damage_handle = XCB_NONE; damage_region = QRegion(); @@ -1153,9 +1155,9 @@ bool Client::setupCompositing() return true; } -void Client::finishCompositing() +void Client::finishCompositing(ReleaseReason releaseReason) { - Toplevel::finishCompositing(); + Toplevel::finishCompositing(releaseReason); updateVisibility(); if (!deleting) { // only recreate the decoration if we are not shutting down completely diff --git a/events.cpp b/events.cpp index 4f13abc138..c40a87befb 100644 --- a/events.cpp +++ b/events.cpp @@ -1566,10 +1566,30 @@ bool Unmanaged::windowEvent(xcb_generic_event_t *e) } const uint8_t eventType = e->response_type & ~0x80; switch (eventType) { - case XCB_UNMAP_NOTIFY: - workspace()->updateFocusMousePosition(Cursor::pos()); - release(); + case XCB_DESTROY_NOTIFY: + release(ReleaseReason::Destroyed); break; + case XCB_UNMAP_NOTIFY:{ + workspace()->updateFocusMousePosition(Cursor::pos()); + + // unmap notify might have been emitted due to a destroy notify + // but unmap notify gets emitted before the destroy notify, nevertheless at this + // point the window is already destroyed. This means any XCB request with the window + // will cause an error. + // To not run into these errors we try to wait for the destroy notify. For this we + // generate a round trip to the X server and wait a very short time span before + // handling the release. + updateXTime(); + // using 1 msec to not just move it at the end of the event loop but add an very short + // timespan to cover cases like unmap() followed by destroy(). The only other way to + // ensure that the window is not destroyed when we do the release handling is to grab + // the XServer which we do not want to do for an Unmanaged. The timespan of 1 msec is + // short enough to not cause problems in the close window animations. + // It's of course still possible that we miss the destroy in which case non-fatal + // X errors are reported to the event loop and logged by Qt. + QTimer::singleShot(1, this, SLOT(release())); + break; + } case XCB_CONFIGURE_NOTIFY: configureNotifyEvent(reinterpret_cast(e)); break; diff --git a/tests/unmapdestroytest.qml b/tests/unmapdestroytest.qml new file mode 100644 index 0000000000..363f58cf89 --- /dev/null +++ b/tests/unmapdestroytest.qml @@ -0,0 +1,86 @@ +/* + * Copyright 2014 Martin Gräßlin + * + * 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 the Free Software Foundation; either version 2 of + * the License or (at your option) version 3 or any later version + * accepted by the membership of KDE e.V. (or its successor approved + * by the membership of KDE e.V.), which shall act as a proxy + * defined in Section 14 of version 3 of the license. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . +*/ +import QtQuick 2.1 +import QtQuick.Window 2.1 +import QtQuick.Layouts 1.1 +import QtQuick.Controls 1.1 + +/* + * Small test application to test the difference between unmap and destroy. + * The window has two buttons: one called "Unmap" which will hide() the QWindow, + * one called "Destroy" which will close() the QWindow. + * + * The test application can be run with qmlscene: + * + * @code + * qmlscene unmapdestroytest.qml + * @endcode + * + * In order to test different modes, the test application understands some arguments: + * --unmanaged Creates an override redirect window + * --frameless Creates a frameless window (comparable Plasma Dialog) + */ + +Window { + id: window + visible: false + x: 0 + y: 0 + width: layout.implicitWidth + height: layout.implicitHeight + color: "black" + + RowLayout { + id: layout + Button { + Timer { + id: timer + interval: 2000 + running: false + repeat: false + onTriggered: window.show() + } + text: "unmap" + onClicked: { + timer.start(); + window.hide(); + } + } + Button { + text: "destroy" + onClicked: window.close() + } + } + + Component.onCompleted: { + var flags = Qt.Window; + for (var i = 0; i < Qt.application.arguments.length; ++i) { + var argument = Qt.application.arguments[i]; + if (argument == "--unmanaged") { + flags = flags | Qt.BypassWindowManagerHint; + } + if (argument == "--frameless") { + flags = flags | Qt.FramelessWindowHint + } + } + window.flags = flags; + window.visible = true; + } +} diff --git a/toplevel.h b/toplevel.h index 8fe138975b..305f667be7 100644 --- a/toplevel.h +++ b/toplevel.h @@ -48,6 +48,15 @@ class ClientMachine; class EffectWindowImpl; class Shadow; +/** + * Enum to describe the reason why a Toplevel has to be released. + */ +enum class ReleaseReason { + Release, ///< Normal Release after e.g. an Unmap notify event (window still valid) + Destroyed, ///< Release after an Destroy notify event (window no longer valid) + KWinShutsDown ///< Release on KWin Shutdown (window still valid) +}; + class Toplevel : public QObject, public KDecorationDefines { @@ -245,7 +254,7 @@ public: int depth() const; bool hasAlpha() const; virtual bool setupCompositing(); - virtual void finishCompositing(); + virtual void finishCompositing(ReleaseReason releaseReason = ReleaseReason::Release); bool updateUnredirectedState(); bool unredirected() const; void suspendUnredirect(bool suspend); diff --git a/unmanaged.cpp b/unmanaged.cpp index db5ab7c5bf..5f32007a98 100644 --- a/unmanaged.cpp +++ b/unmanaged.cpp @@ -84,20 +84,20 @@ bool Unmanaged::track(Window w) return true; } -void Unmanaged::release(bool on_shutdown) +void Unmanaged::release(ReleaseReason releaseReason) { Deleted* del = NULL; - if (!on_shutdown) { + if (releaseReason != ReleaseReason::KWinShutsDown) { del = Deleted::create(this); } emit windowClosed(this, del); - finishCompositing(); - if (!QWidget::find(window())) { // don't affect our own windows + finishCompositing(releaseReason); + if (!QWidget::find(window()) && releaseReason != ReleaseReason::Destroyed) { // don't affect our own windows if (Xcb::Extensions::self()->isShapeAvailable()) xcb_shape_select_input(connection(), window(), false); Xcb::selectInput(window(), XCB_EVENT_MASK_NO_EVENT); } - if (!on_shutdown) { + if (releaseReason != ReleaseReason::KWinShutsDown) { workspace()->removeUnmanaged(this); addWorkspaceRepaint(del->visibleRect()); disownDataPassedToDeleted(); diff --git a/unmanaged.h b/unmanaged.h index 7dbecc8879..6e63e69a5d 100644 --- a/unmanaged.h +++ b/unmanaged.h @@ -35,7 +35,6 @@ class Unmanaged public: explicit Unmanaged(); bool windowEvent(xcb_generic_event_t *e); - void release(bool on_shutdown = false); bool track(Window w); static void deleteUnmanaged(Unmanaged* c); virtual int desktop() const; @@ -51,6 +50,8 @@ public: void sendPointerButtonEvent(uint32_t button, InputRedirection::PointerButtonState state) override; void sendPointerAxisEvent(InputRedirection::PointerAxis axis, qreal delta) override; void sendKeybordKeyEvent(uint32_t key, InputRedirection::KeyboardKeyState state) override; +public Q_SLOTS: + void release(ReleaseReason releaseReason = ReleaseReason::Release); protected: virtual void debug(QDebug& stream) const; virtual bool shouldUnredirect() const; diff --git a/workspace.cpp b/workspace.cpp index 8840591699..80f9634a2e 100644 --- a/workspace.cpp +++ b/workspace.cpp @@ -438,7 +438,7 @@ Workspace::~Workspace() desktops.removeAll(c); } for (UnmanagedList::iterator it = unmanaged.begin(), end = unmanaged.end(); it != end; ++it) - (*it)->release(true); + (*it)->release(ReleaseReason::KWinShutsDown); xcb_delete_property(connection(), rootWindow(), atoms->kwin_running); delete RuleBook::self();