From 9c611ddaea17ff13f328f509dad5f3a5f75e8fbf Mon Sep 17 00:00:00 2001
From: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
Date: Tue, 20 Aug 2024 00:35:55 +0300
Subject: [PATCH] Fix a crash in computeLayer()

On X11, the stack order of a window can be changed in multiple ways:

- Opposite
- TopIf
- BottomIf
- Above
- Below

You would pass either of those modes plus maybe the above window id. For
this crash, the relevant mode is Above.

Since the Workspace only has one restack function that places the given
window right under the reference window, the Above stack mode
implementation performs a quirk: it walks through the stack in order to
find a window right above the reference window and pass it to the
restack() function.

However, it could be that the window that wants to be restacked is already
above the reference window, so that same window would be passed as the
"under" window to the restack() function.

It's nothing but a miracle that we have not received major complaints
about this issue until now.

The restack() function doesn't like `window` and `under` to be the same
because of code like

    unconstrained_stacking_order.removeAll(window);
    unconstrained_stacking_order.insert(unconstrained_stacking_order.indexOf(under), window);

The removeAll() function would effectively remove both `window` and `under`
from the unconstrained stack, which breaks the next line because the
indexOf() would return -1, i.e.

    unconstrained_stacking_order.insert(-1, window);

This is the reason why the unconstrained_stacking_order contains strange
values such as `0xe`.

In order to fix the crash, this change adds some code to short-circuit
the restack() function if the passed in window and the reference window
are the same. It would be great to clean up X11Window::restackWindow()
and also add ergonomic restack functions in the Workspace, but this can
be done later. The major blocker is lack of proper test coverage of
X11 bits at the moment.

Last but not least, I would like to express my profound gratitude to
Peter Strick for filing the crash report AND providing a VM image that
helped massively with reproducing the crash and finally fixing it.

BUG: 491618
---
 src/layers.cpp | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/layers.cpp b/src/layers.cpp
index 92ec81f06c..51943a8db1 100644
--- a/src/layers.cpp
+++ b/src/layers.cpp
@@ -451,23 +451,25 @@ void Workspace::restack(Window *window, Window *under, bool force)
         qCWarning(KWIN_CORE) << "Workspace::restack: closed window" << window << "cannot be restacked";
         return;
     }
-    Q_ASSERT(unconstrained_stacking_order.contains(under));
     if (!force && !Window::belongToSameApplication(under, window)) {
         // put in the stacking order below _all_ windows belonging to the active application
         for (int i = 0; i < unconstrained_stacking_order.size(); ++i) {
             auto other = unconstrained_stacking_order.at(i);
             if (other->isClient() && other->layer() == window->layer() && Window::belongToSameApplication(under, other)) {
-                under = (window == other) ? nullptr : other;
+                under = other;
                 break;
             }
         }
     }
-    if (under) {
-        unconstrained_stacking_order.removeAll(window);
-        unconstrained_stacking_order.insert(unconstrained_stacking_order.indexOf(under), window);
+
+    Q_ASSERT(unconstrained_stacking_order.contains(under));
+    if (under == window) {
+        return;
     }
 
-    Q_ASSERT(unconstrained_stacking_order.contains(window));
+    unconstrained_stacking_order.removeAll(window);
+    unconstrained_stacking_order.insert(unconstrained_stacking_order.indexOf(under), window);
+
     m_focusChain->moveAfterWindow(window, under);
     updateStackingOrder();
 }