Make setFrameGeometry() re-entrant for X and internal clients

If AbstractClient::setFrameGeometry() is called from a slot connected
directly to the frameGeometryChanged() signal, then is there a good
chance that kwin will fall into an infinite recursion. However, that's
the case with only X11 and internal clients.

The root cause of the infinite recursion is that both X11Client and
InternalClient compare the new geometry against the geometry before
update blocking. In order to fix the bug, we simply need to ensure that
updateGeometryBeforeUpdateBlocking() has been called before we start
emitting the frameGeometryChanged() signal.

Furthermore, a couple of tests were added to ensure that we won't hit
this subtle bug again.

abstract_client.cpp

@@ -903,16 +903,19 @@ void AbstractClient::move(int x, int y, ForceGeometry_t force)
             setPendingGeometryUpdate(PendingGeometryNormal);
         return;
     }
+    const QRect oldBufferGeometry = bufferGeometryBeforeUpdateBlocking();
+    const QRect oldClientGeometry = clientGeometryBeforeUpdateBlocking();
+    const QRect oldFrameGeometry = frameGeometryBeforeUpdateBlocking();
     doMove(x, y);
+    updateGeometryBeforeUpdateBlocking();
     updateWindowRules(Rules::Position);
     screens()->setCurrent(this);
     workspace()->updateStackingOrder();
     // client itself is not damaged
-    emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking());
+    emit bufferGeometryChanged(this, oldBufferGeometry);
-    emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking());
+    emit clientGeometryChanged(this, oldClientGeometry);
-    emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking());
+    emit frameGeometryChanged(this, oldFrameGeometry);
     addRepaintDuringGeometryUpdates();
-    updateGeometryBeforeUpdateBlocking();
 }
 
 bool AbstractClient::startMoveResize()

autotests/integration/internal_window.cpp

@@ -75,6 +75,7 @@ private Q_SLOTS:
     void testChangeWindowType_data();
     void testChangeWindowType();
     void testEffectWindow();
+    void testReentrantSetFrameGeometry();
 };
 
 class HelperWindow : public QRasterWindow
@@ -791,6 +792,34 @@ void InternalWindowTest::testEffectWindow()
     QCOMPARE(effects->findWindow(&win)->internalWindow(), &win);
 }
 
+void InternalWindowTest::testReentrantSetFrameGeometry()
+{
+    // This test verifies that calling setFrameGeometry() from a slot connected directly
+    // to the frameGeometryChanged() signal won't cause an infinite recursion.
+
+    // Create an internal window.
+    QSignalSpy clientAddedSpy(workspace(), &Workspace::internalClientAdded);
+    QVERIFY(clientAddedSpy.isValid());
+    HelperWindow win;
+    win.setGeometry(0, 0, 100, 100);
+    win.show();
+    QTRY_COMPARE(clientAddedSpy.count(), 1);
+    auto client = clientAddedSpy.first().first().value<InternalClient *>();
+    QVERIFY(client);
+    QCOMPARE(client->pos(), QPoint(0, 0));
+
+    // Let's pretend that there is a script that really wants the client to be at (100, 100).
+    connect(client, &AbstractClient::frameGeometryChanged, this, [client]() {
+        client->setFrameGeometry(QRect(QPoint(100, 100), client->size()));
+    });
+
+    // Trigger the lambda above.
+    client->move(QPoint(40, 50));
+
+    // Eventually, the client will end up at (100, 100).
+    QCOMPARE(client->pos(), QPoint(100, 100));
+}
+
 }
 
 WAYLANDTEST_MAIN(KWin::InternalWindowTest)

autotests/integration/x11_client_test.cpp

@@ -61,6 +61,7 @@ private Q_SLOTS:
     void testCaptionMultipleWindows();
     void testFullscreenWindowGroups();
     void testActivateFocusedWindow();
+    void testReentrantSetFrameGeometry();
 };
 
 void X11ClientTest::initTestCase()
@@ -1069,5 +1070,54 @@ void X11ClientTest::testActivateFocusedWindow()
     QVERIFY(Test::waitForWindowDestroyed(client1));
 }
 
+void X11ClientTest::testReentrantSetFrameGeometry()
+{
+    // This test verifies that calling setFrameGeometry() from a slot connected directly
+    // to the frameGeometryChanged() signal won't cause an infinite recursion.
+
+    // Create a test window.
+    QScopedPointer<xcb_connection_t, XcbConnectionDeleter> c(xcb_connect(nullptr, nullptr));
+    QVERIFY(!xcb_connection_has_error(c.data()));
+    const QRect windowGeometry(0, 0, 100, 200);
+    xcb_window_t w = xcb_generate_id(c.data());
+    xcb_create_window(c.data(), XCB_COPY_FROM_PARENT, w, rootWindow(),
+                      windowGeometry.x(),
+                      windowGeometry.y(),
+                      windowGeometry.width(),
+                      windowGeometry.height(),
+                      0, XCB_WINDOW_CLASS_INPUT_OUTPUT, XCB_COPY_FROM_PARENT, 0, nullptr);
+    xcb_size_hints_t hints;
+    memset(&hints, 0, sizeof(hints));
+    xcb_icccm_size_hints_set_position(&hints, 1, windowGeometry.x(), windowGeometry.y());
+    xcb_icccm_size_hints_set_size(&hints, 1, windowGeometry.width(), windowGeometry.height());
+    xcb_icccm_set_wm_normal_hints(c.data(), w, &hints);
+    xcb_change_property(c.data(), XCB_PROP_MODE_REPLACE, w, atoms->wm_client_leader, XCB_ATOM_WINDOW, 32, 1, &w);
+    xcb_map_window(c.data(), w);
+    xcb_flush(c.data());
+
+    QSignalSpy windowCreatedSpy(workspace(), &Workspace::clientAdded);
+    QVERIFY(windowCreatedSpy.isValid());
+    QVERIFY(windowCreatedSpy.wait());
+    X11Client *client = windowCreatedSpy.first().first().value<X11Client *>();
+    QVERIFY(client);
+    QCOMPARE(client->pos(), QPoint(0, 0));
+
+    // Let's pretend that there is a script that really wants the client to be at (100, 100).
+    connect(client, &AbstractClient::frameGeometryChanged, this, [client]() {
+        client->setFrameGeometry(QRect(QPoint(100, 100), client->size()));
+    });
+
+    // Trigger the lambda above.
+    client->move(QPoint(40, 50));
+
+    // Eventually, the client will end up at (100, 100).
+    QCOMPARE(client->pos(), QPoint(100, 100));
+
+    // Destroy the test window.
+    xcb_destroy_window(c.data(), w);
+    xcb_flush(c.data());
+    QVERIFY(Test::waitForWindowDestroyed(client));
+}
+
 WAYLANDTEST_MAIN(X11ClientTest)
 #include "x11_client_test.moc"

autotests/integration/xdgshellclient_test.cpp

@@ -121,6 +121,7 @@ private Q_SLOTS:
     void testXdgWindowGeometryFullScreen();
     void testXdgWindowGeometryMaximize();
     void testPointerInputTransform();
+    void testReentrantSetFrameGeometry();
 };
 
 void TestXdgShellClient::initTestCase()
@@ -1645,5 +1646,33 @@ void TestXdgShellClient::testPointerInputTransform()
     QVERIFY(Test::waitForWindowDestroyed(client));
 }
 
+void TestXdgShellClient::testReentrantSetFrameGeometry()
+{
+    // This test verifies that calling setFrameGeometry() from a slot connected directly
+    // to the frameGeometryChanged() signal won't cause an infinite recursion.
+
+    // Create an xdg-toplevel surface and wait for the compositor to catch up.
+    QScopedPointer<Surface> surface(Test::createSurface());
+    QScopedPointer<XdgShellSurface> shellSurface(Test::createXdgShellStableSurface(surface.data()));
+    AbstractClient *client = Test::renderAndWaitForShown(surface.data(), QSize(200, 100), Qt::red);
+    QVERIFY(client);
+    QCOMPARE(client->pos(), QPoint(0, 0));
+
+    // Let's pretend that there is a script that really wants the client to be at (100, 100).
+    connect(client, &AbstractClient::frameGeometryChanged, this, [client]() {
+        client->setFrameGeometry(QRect(QPoint(100, 100), client->size()));
+    });
+
+    // Trigger the lambda above.
+    client->move(QPoint(40, 50));
+
+    // Eventually, the client will end up at (100, 100).
+    QCOMPARE(client->pos(), QPoint(100, 100));
+
+    // Destroy the xdg-toplevel surface.
+    shellSurface.reset();
+    QVERIFY(Test::waitForWindowDestroyed(client));
+}
+
 WAYLANDTEST_MAIN(TestXdgShellClient)
 #include "xdgshellclient_test.moc"

internal_client.cpp

@@ -100,7 +100,7 @@ bool InternalClient::eventFilter(QObject *watched, QEvent *event)
 
 QRect InternalClient::bufferGeometry() const
 {
-    return frameGeometry() - frameMargins();
+    return m_clientGeometry;
 }
 
 QStringList InternalClient::activities() const
@@ -525,23 +525,25 @@ void InternalClient::commitGeometry(const QRect &rect)
         return;
     }
 
+    // The client geometry and the buffer geometry are the same.
+    const QRect oldClientGeometry = m_clientGeometry;
+    const QRect oldFrameGeometry = m_frameGeometry;
+
     m_clientGeometry = frameRectToClientRect(rect);
     m_frameGeometry = rect;
 
     addWorkspaceRepaint(visibleRect());
+    updateGeometryBeforeUpdateBlocking();
     syncGeometryToInternalWindow();
 
-    if (bufferGeometryBeforeUpdateBlocking() != bufferGeometry()) {
+    if (oldClientGeometry != m_clientGeometry) {
-        emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking());
+        emit bufferGeometryChanged(this, oldClientGeometry);
+        emit clientGeometryChanged(this, oldClientGeometry);
     }
-    if (clientGeometryBeforeUpdateBlocking() != clientGeometry()) {
+    if (oldFrameGeometry != m_frameGeometry) {
-        emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking());
+        emit frameGeometryChanged(this, oldFrameGeometry);
     }
-    if (frameGeometryBeforeUpdateBlocking() != frameGeometry()) {
+    emit geometryShapeChanged(this, oldFrameGeometry);
-        emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking());
-    }
-    emit geometryShapeChanged(this, frameGeometryBeforeUpdateBlocking());
-    updateGeometryBeforeUpdateBlocking();
 
     if (isResize()) {
         performMoveResize();

x11client.cpp

@@ -2892,22 +2892,25 @@ void X11Client::move(int x, int y, ForceGeometry_t force)
         }
         return;
     }
+    const QRect oldBufferGeometry = bufferGeometryBeforeUpdateBlocking();
+    const QRect oldFrameGeometry = frameGeometryBeforeUpdateBlocking();
+    const QRect oldClientGeometry = clientGeometryBeforeUpdateBlocking();
     updateServerGeometry();
     updateWindowRules(Rules::Position);
+    updateGeometryBeforeUpdateBlocking();
     screens()->setCurrent(this);
     workspace()->updateStackingOrder();
     // client itself is not damaged
-    if (bufferGeometryBeforeUpdateBlocking() != bufferGeometry()) {
+    if (oldBufferGeometry != bufferGeometry()) {
-        emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking());
+        emit bufferGeometryChanged(this, oldBufferGeometry);
     }
-    if (clientGeometryBeforeUpdateBlocking() != clientGeometry()) {
+    if (oldClientGeometry != clientGeometry()) {
-        emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking());
+        emit clientGeometryChanged(this, oldClientGeometry);
     }
-    if (frameGeometryBeforeUpdateBlocking() != frameGeometry()) {
+    if (oldFrameGeometry != frameGeometry()) {
-        emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking());
+        emit frameGeometryChanged(this, oldFrameGeometry);
     }
     addRepaintDuringGeometryUpdates();
-    updateGeometryBeforeUpdateBlocking();
 }
 
 bool X11Client::belongToSameApplication(const X11Client *c1, const X11Client *c2, SameApplicationChecks checks)
@@ -4183,8 +4186,14 @@ void X11Client::setFrameGeometry(const QRect &rect, ForceGeometry_t force)
             setPendingGeometryUpdate(PendingGeometryNormal);
         return;
     }
+
+    const QRect oldBufferGeometry = bufferGeometryBeforeUpdateBlocking();
+    const QRect oldFrameGeometry = frameGeometryBeforeUpdateBlocking();
+    const QRect oldClientGeometry = clientGeometryBeforeUpdateBlocking();
+
     updateServerGeometry();
     updateWindowRules(Rules::Position|Rules::Size);
+    updateGeometryBeforeUpdateBlocking();
 
     // keep track of old maximize mode
     // to detect changes
@@ -4192,21 +4201,20 @@ void X11Client::setFrameGeometry(const QRect &rect, ForceGeometry_t force)
     workspace()->updateStackingOrder();
 
     // Need to regenerate decoration pixmaps when the buffer size is changed.
-    if (bufferGeometryBeforeUpdateBlocking().size() != m_bufferGeometry.size()) {
+    if (oldBufferGeometry.size() != m_bufferGeometry.size()) {
         discardWindowPixmap();
     }
-    if (bufferGeometryBeforeUpdateBlocking() != m_bufferGeometry) {
+    if (oldBufferGeometry != m_bufferGeometry) {
-        emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking());
+        emit bufferGeometryChanged(this, oldBufferGeometry);
     }
-    if (clientGeometryBeforeUpdateBlocking() != m_clientGeometry) {
+    if (oldClientGeometry != m_clientGeometry) {
-        emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking());
+        emit clientGeometryChanged(this, oldClientGeometry);
     }
-    if (frameGeometryBeforeUpdateBlocking() != m_frameGeometry) {
+    if (oldFrameGeometry != m_frameGeometry) {
-        emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking());
+        emit frameGeometryChanged(this, oldFrameGeometry);
     }
-    emit geometryShapeChanged(this, frameGeometryBeforeUpdateBlocking());
+    emit geometryShapeChanged(this, oldFrameGeometry);
     addRepaintDuringGeometryUpdates();
-    updateGeometryBeforeUpdateBlocking();
 }
 
 void X11Client::plainResize(int w, int h, ForceGeometry_t force)
@@ -4251,25 +4259,28 @@ void X11Client::plainResize(int w, int h, ForceGeometry_t force)
             setPendingGeometryUpdate(PendingGeometryNormal);
         return;
     }
+    const QRect oldBufferGeometry = bufferGeometryBeforeUpdateBlocking();
+    const QRect oldFrameGeometry = frameGeometryBeforeUpdateBlocking();
+    const QRect oldClientGeometry = clientGeometryBeforeUpdateBlocking();
     updateServerGeometry();
     updateWindowRules(Rules::Position|Rules::Size);
+    updateGeometryBeforeUpdateBlocking();
     screens()->setCurrent(this);
     workspace()->updateStackingOrder();
-    if (bufferGeometryBeforeUpdateBlocking().size() != m_bufferGeometry.size()) {
+    if (oldBufferGeometry.size() != m_bufferGeometry.size()) {
         discardWindowPixmap();
     }
-    if (bufferGeometryBeforeUpdateBlocking() != bufferGeometry()) {
+    if (oldBufferGeometry != m_bufferGeometry) {
-        emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking());
+        emit bufferGeometryChanged(this, oldBufferGeometry);
     }
-    if (clientGeometryBeforeUpdateBlocking() != clientGeometry()) {
+    if (oldClientGeometry != m_clientGeometry) {
-        emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking());
+        emit clientGeometryChanged(this, oldClientGeometry);
     }
-    if (frameGeometryBeforeUpdateBlocking() != frameGeometry()) {
+    if (oldFrameGeometry != m_frameGeometry) {
-        emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking());
+        emit frameGeometryChanged(this, oldFrameGeometry);
     }
-    emit geometryShapeChanged(this, frameGeometryBeforeUpdateBlocking());
+    emit geometryShapeChanged(this, oldFrameGeometry);
     addRepaintDuringGeometryUpdates();
-    updateGeometryBeforeUpdateBlocking();
 }
 
 void X11Client::updateServerGeometry()