From 580a2ed1f41640e14adf57c5c7921cdadbdbe14d Mon Sep 17 00:00:00 2001
From: Marco Martin <notmart@gmail.com>
Date: Wed, 2 Apr 2025 08:28:05 +0000
Subject: [PATCH] multiscreen: fix an incorrect assert in screenInvariants

Since OutputOrderWatcher at the time of screen removing can temporarly contain a dead entry, we can't check on screenInvariants that the count is the same as the real screen count, but check instead that outputorderwatcher doesn't have missing entries instead

BUG:494616


(cherry picked from commit 285cfe150efd941eed62b06604db0709977540c9)

Co-authored-by: Marco Martin <notmart@gmail.com>
---
 shell/autotests/screenpooltest.cpp | 60 ++++++++++++++++++++++++++++++
 shell/screenpool.cpp               |  6 ++-
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/shell/autotests/screenpooltest.cpp b/shell/autotests/screenpooltest.cpp
index bbb984a8738..033844e6195 100644
--- a/shell/autotests/screenpooltest.cpp
+++ b/shell/autotests/screenpooltest.cpp
@@ -41,6 +41,7 @@ private Q_SLOTS:
     void testLastScreenRemoval();
     void testFakeToRealScreen();
     void testFakeOutputInitially();
+    void testReorderRemoveRace();
 
 private:
     ScreenPool *m_screenPool;
@@ -459,6 +460,65 @@ void ScreenPoolTest::testFakeOutputInitially()
     QCOMPARE(screenPool.idForScreen(newScreen), 0);
 }
 
+void ScreenPoolTest::testReorderRemoveRace()
+{
+    QSignalSpy addedSpy(qGuiApp, SIGNAL(screenAdded(QScreen *)));
+    QSignalSpy orderChangeSpy(m_screenPool, &ScreenPool::screenOrderChanged);
+    QSignalSpy firstScreenResizedSpy(qGuiApp->screens()[0], &QScreen::geometryChanged);
+
+    // Add a new output
+    exec([this] {
+        OutputData data;
+        data.mode.resolution = {1920, 1080};
+        data.position = {1920, 0};
+        data.physicalSize = data.mode.physicalSizeForDpi(96);
+        // NOTE: assumes that when a screen is added it will already have the final geometry
+        auto *out = add<Output>(data);
+        auto *xdgOut = xdgOutput(out);
+        xdgOut->m_name = QStringLiteral("WL-2");
+        outputOrder()->setList({u"WL-1"_s, u"WL-2"_s});
+    });
+
+    QVERIFY(orderChangeSpy.wait());
+
+    QCOMPARE(orderChangeSpy.size(), 1);
+    QCOMPARE(QGuiApplication::screens().size(), 2);
+    QCOMPARE(m_screenPool->screenOrder().size(), 2);
+    QCOMPARE(addedSpy.size(), 1);
+
+    QScreen *newScreen = addedSpy.takeFirst().at(0).value<QScreen *>();
+    QCOMPARE(newScreen->name(), QStringLiteral("WL-2"));
+    QCOMPARE(newScreen->geometry(), QRect(1920, 0, 1920, 1080));
+    // Check mapping
+    QCOMPARE(m_screenPool->idForScreen(newScreen), 1);
+    QCOMPARE(m_screenPool->screenForId(1)->name(), QStringLiteral("WL-2"));
+
+    exec([this] {
+        // BUG 494616:
+        // When there are those 3 things happening in quick order
+        // * Setting the order
+        // * resizing an output
+        // * removing another output
+        // we used to get an inconsistent state in OutputOrderWatcher
+        // where the removed output is *not* removed from outputOrder
+        outputOrder()->setList({u"WL-2"_s, u"WL-1"_s});
+        auto *out = output(0);
+        auto *xdgOut = xdgOutput(output(0));
+        xdgOut->sendLogicalSize(QSize(1024, 600));
+        remove(output(1));
+        out->m_data.physicalSize = QSize(1024, 600);
+        out->sendGeometry();
+        out->sendDone();
+    });
+
+    QVERIFY(orderChangeSpy.wait());
+    QTRY_COMPARE(firstScreenResizedSpy.size(), 1);
+    QCOMPARE(m_screenPool->screenOrder().size(), 1);
+    QCOMPARE(m_screenPool->screenOrder().first()->name(), QStringLiteral("WL-1"));
+    QCOMPARE(qApp->screens().size(), 1);
+    QCOMPARE(qApp->screens().first()->geometry(), QRect(0, 0, 1024, 600));
+}
+
 QCOMPOSITOR_TEST_MAIN(ScreenPoolTest)
 
 #include "screenpooltest.moc"
diff --git a/shell/screenpool.cpp b/shell/screenpool.cpp
index 8d9c92b0d80..3c33dab8e9d 100644
--- a/shell/screenpool.cpp
+++ b/shell/screenpool.cpp
@@ -282,6 +282,7 @@ void ScreenPool::handleScreenRemoved(QScreen *screen)
 void ScreenPool::handleOutputOrderChanged(const QStringList &newOrder)
 {
     qCDebug(SCREENPOOL) << "handleOutputOrderChanged" << newOrder;
+
     QHash<QString, QScreen *> connMap;
     for (auto s : qApp->screens()) {
         connMap[s->name()] = s;
@@ -369,7 +370,10 @@ void ScreenPool::screenInvariants()
     // QScreen bookeeping integrity
     auto allScreens = qGuiApp->screens();
     // Do we actually track every screen?
-    Q_ASSERT_X((m_availableScreens.count() + m_redundantScreens.count()) == m_outputOrderWatcher->outputOrder().count(),
+    // (m_availableScreens.count() + m_redundantScreens.count() must be less or equal
+    // to the number of screens tracked by OutputOrderWatcher, because it can contain
+    // for a little while a screen that has just been removed
+    Q_ASSERT_X((m_availableScreens.count() + m_redundantScreens.count()) <= m_outputOrderWatcher->outputOrder().count(),
                Q_FUNC_INFO,
                qUtf8Printable(debugMessage())); // https://crash-reports.kde.org/organizations/kde/issues/5249/
     Q_ASSERT_X(allScreens.count() == m_sizeSortedScreens.count(),
-- 
GitLab