From d948d247fe4371462f2fe6b96b25fd8026abafb5 Mon Sep 17 00:00:00 2001
From: Vlad Zagorodniy <vladzzag@gmail.com>
Date: Tue, 30 Apr 2019 11:46:53 +0300
Subject: [PATCH] Don't crash when highlighted tabbox client is closed

Summary:
The compositor tries to switch to the next tabbox client when currently
highlighted client is closed. Though there is a small issue with that.
Because the switch happens too late, a dangling pointer can be inserted
into the unconstrained stacking order, which can lead to a crash later on.

There are two cases:
- compositing is on;
- compositing is off.

Compositing is on: TabBox will try to un-elevate currently highlighted
client, though by that time the client no longer owns EffectWindow, so
this is basically a no-op (that's why we haven't experienced this bug
before).

Compositing is off: TabBox will try to restack currently hightlighted
client under the next tabbox client. Given that the restack method
doesn't do any sanity checks(see Client::manage why), a client that is
about to be destroyed will be re-inserted back into the unconstrained
stacking order.

This change ensures that the switch happens before currently highlighted
client is removed from the stacking order.

BUG: 406784

Test Plan:
- Turn off compositing;
- Follow steps to reproduce in the bug report (see comment 2).

Reviewers: #kwin, davidedmundson

Reviewed By: #kwin, davidedmundson

Subscribers: kwin

Tags: #kwin

Differential Revision: https://phabricator.kde.org/D20916
---
 client.cpp    | 15 +++++++++++++++
 workspace.cpp |  7 +------
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/client.cpp b/client.cpp
index 9017c3907b..b6be143b2a 100644
--- a/client.cpp
+++ b/client.cpp
@@ -32,6 +32,9 @@ along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #include "focuschain.h"
 #include "group.h"
 #include "shadow.h"
+#ifdef KWIN_BUILD_TABBOX
+#include "tabbox.h"
+#endif
 #include "workspace.h"
 #include "screenedge.h"
 #include "decorations/decorationbridge.h"
@@ -205,6 +208,12 @@ void Client::releaseWindow(bool on_shutdown)
 {
     assert(!deleting);
     deleting = true;
+#ifdef KWIN_BUILD_TABBOX
+    TabBox::TabBox *tabBox = TabBox::TabBox::self();
+    if (tabBox->isDisplayed() && tabBox->currentClient() == this) {
+        tabBox->nextPrev(true);
+    }
+#endif
     destroyWindowManagementInterface();
     Deleted* del = NULL;
     if (!on_shutdown) {
@@ -275,6 +284,12 @@ void Client::destroyClient()
 {
     assert(!deleting);
     deleting = true;
+#ifdef KWIN_BUILD_TABBOX
+    TabBox::TabBox *tabBox = TabBox::TabBox::self();
+    if (tabBox->isDisplayed() && tabBox->currentClient() == this) {
+        tabBox->nextPrev(true);
+    }
+#endif
     destroyWindowManagementInterface();
     Deleted* del = Deleted::create(this);
     if (isMoveResize())
diff --git a/workspace.cpp b/workspace.cpp
index a0e6d5dbcc..26c1b5715a 100644
--- a/workspace.cpp
+++ b/workspace.cpp
@@ -651,12 +651,6 @@ void Workspace::removeClient(Client* c)
         clientShortcutUpdated(c);   // Needed, since this is otherwise delayed by setShortcut() and wouldn't run
     }
 
-#ifdef KWIN_BUILD_TABBOX
-    TabBox::TabBox *tabBox = TabBox::TabBox::self();
-    if (tabBox->isDisplayed() && tabBox->currentClient() == c)
-        tabBox->nextPrev(true);
-#endif
-
     Q_ASSERT(clients.contains(c) || desktops.contains(c));
     // TODO: if marked client is removed, notify the marked list
     clients.removeAll(c);
@@ -680,6 +674,7 @@ void Workspace::removeClient(Client* c)
     updateStackingOrder(true);
 
 #ifdef KWIN_BUILD_TABBOX
+    TabBox::TabBox *tabBox = TabBox::TabBox::self();
     if (tabBox->isDisplayed())
         tabBox->reset(true);
 #endif