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

@@ -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)

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; }
 

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;
     }
 
+    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

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:
+    case XCB_DESTROY_NOTIFY:
-        workspace()->updateFocusMousePosition(Cursor::pos());
+        release(ReleaseReason::Destroyed);
-        release();
         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<xcb_configure_notify_event_t*>(e));
         break;

tests/unmapdestroytest.qml

@@ -0,0 +1,86 @@
+/*
+ * Copyright 2014  Martin Gräßlin <mgraesslin@kde.org>
+ *
+ * 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 <http://www.gnu.org/licenses/>.
+*/
+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;
+    }
+}

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);

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();
+    finishCompositing(releaseReason);
-    if (!QWidget::find(window())) { // don't affect our own windows
+    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();

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;

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();