From 80a31ab4b79280317ead1bd19b73a377e7903733 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Mon, 20 Jul 2020 22:33:19 +0300 Subject: [PATCH] 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 | 11 ++-- autotests/integration/internal_window.cpp | 29 +++++++++ autotests/integration/x11_client_test.cpp | 50 +++++++++++++++ autotests/integration/xdgshellclient_test.cpp | 29 +++++++++ internal_client.cpp | 22 ++++--- x11client.cpp | 61 +++++++++++-------- 6 files changed, 163 insertions(+), 39 deletions(-) diff --git a/abstract_client.cpp b/abstract_client.cpp index 3a18b467d8..a4eff0a9d9 100644 --- a/abstract_client.cpp +++ b/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 clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); + emit bufferGeometryChanged(this, oldBufferGeometry); + emit clientGeometryChanged(this, oldClientGeometry); + emit frameGeometryChanged(this, oldFrameGeometry); addRepaintDuringGeometryUpdates(); - updateGeometryBeforeUpdateBlocking(); } bool AbstractClient::startMoveResize() diff --git a/autotests/integration/internal_window.cpp b/autotests/integration/internal_window.cpp index fb48db92b2..ba2780d6d0 100644 --- a/autotests/integration/internal_window.cpp +++ b/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(); + 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) diff --git a/autotests/integration/x11_client_test.cpp b/autotests/integration/x11_client_test.cpp index 9a966d5e41..31e7dd9f1a 100644 --- a/autotests/integration/x11_client_test.cpp +++ b/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 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(); + 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" diff --git a/autotests/integration/xdgshellclient_test.cpp b/autotests/integration/xdgshellclient_test.cpp index e3ca678fc4..ed74211572 100644 --- a/autotests/integration/xdgshellclient_test.cpp +++ b/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(Test::createSurface()); + QScopedPointer 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" diff --git a/internal_client.cpp b/internal_client.cpp index 91c0e0adcc..958ce3b47e 100644 --- a/internal_client.cpp +++ b/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()) { - emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking()); + if (oldClientGeometry != m_clientGeometry) { + emit bufferGeometryChanged(this, oldClientGeometry); + emit clientGeometryChanged(this, oldClientGeometry); } - if (clientGeometryBeforeUpdateBlocking() != clientGeometry()) { - emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); + if (oldFrameGeometry != m_frameGeometry) { + emit frameGeometryChanged(this, oldFrameGeometry); } - if (frameGeometryBeforeUpdateBlocking() != frameGeometry()) { - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); - } - emit geometryShapeChanged(this, frameGeometryBeforeUpdateBlocking()); - updateGeometryBeforeUpdateBlocking(); + emit geometryShapeChanged(this, oldFrameGeometry); if (isResize()) { performMoveResize(); diff --git a/x11client.cpp b/x11client.cpp index bbc76f1398..55ec57d3ed 100644 --- a/x11client.cpp +++ b/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()) { - emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking()); + if (oldBufferGeometry != bufferGeometry()) { + emit bufferGeometryChanged(this, oldBufferGeometry); } - if (clientGeometryBeforeUpdateBlocking() != clientGeometry()) { - emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); + if (oldClientGeometry != clientGeometry()) { + emit clientGeometryChanged(this, oldClientGeometry); } - if (frameGeometryBeforeUpdateBlocking() != frameGeometry()) { - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); + if (oldFrameGeometry != frameGeometry()) { + 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) { - emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking()); + if (oldBufferGeometry != m_bufferGeometry) { + emit bufferGeometryChanged(this, oldBufferGeometry); } - if (clientGeometryBeforeUpdateBlocking() != m_clientGeometry) { - emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); + if (oldClientGeometry != m_clientGeometry) { + emit clientGeometryChanged(this, oldClientGeometry); } - if (frameGeometryBeforeUpdateBlocking() != m_frameGeometry) { - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); + if (oldFrameGeometry != m_frameGeometry) { + 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()) { - emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking()); + if (oldBufferGeometry != m_bufferGeometry) { + emit bufferGeometryChanged(this, oldBufferGeometry); } - if (clientGeometryBeforeUpdateBlocking() != clientGeometry()) { - emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); + if (oldClientGeometry != m_clientGeometry) { + emit clientGeometryChanged(this, oldClientGeometry); } - if (frameGeometryBeforeUpdateBlocking() != frameGeometry()) { - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); + if (oldFrameGeometry != m_frameGeometry) { + emit frameGeometryChanged(this, oldFrameGeometry); } - emit geometryShapeChanged(this, frameGeometryBeforeUpdateBlocking()); + emit geometryShapeChanged(this, oldFrameGeometry); addRepaintDuringGeometryUpdates(); - updateGeometryBeforeUpdateBlocking(); } void X11Client::updateServerGeometry()