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
This commit is contained in:
Vlad Zahorodnii 2024-08-20 00:35:55 +03:00
parent d028e3b1e9
commit 9c611ddaea

View file

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