From a43c5242522f19bca67a6762916f236004d569df Mon Sep 17 00:00:00 2001 From: Andrew Tropin Date: Wed, 18 Jan 2023 14:16:31 +0400 Subject: [PATCH] gnu: qtwayland: Fix crashes from excessive number of frame callbacks. QWaylandWindow::handleUpdate could create thousands of pending frame callbacks, causing compositor to terminate client connection. https://bugreports.qt.io/browse/QTBUG-81504 * gnu/packages/patches/qtwayland-dont-recreate-callbacks.patch: New file. * gnu/packages/patches/qtwayland-cleanup-callbacks.patch: New file. * gnu/local.mk (dist_patch_DATA): Adjust accordingly. * gnu/packages/qt.scm (qtwayland)[source](patches): Add patches. --- gnu/local.mk | 2 + .../patches/qtwayland-cleanup-callbacks.patch | 52 +++++++++++++ .../qtwayland-dont-recreate-callbacks.patch | 76 +++++++++++++++++++ gnu/packages/qt.scm | 4 +- 4 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 gnu/packages/patches/qtwayland-cleanup-callbacks.patch create mode 100644 gnu/packages/patches/qtwayland-dont-recreate-callbacks.patch diff --git a/gnu/local.mk b/gnu/local.mk index dfe0d92a2f..408f7c376b 100644 --- a/gnu/local.mk +++ b/gnu/local.mk @@ -1775,6 +1775,8 @@ dist_patch_DATA = \ %D%/packages/patches/quagga-reproducible-build.patch \ %D%/packages/patches/quickswitch-fix-dmenu-check.patch \ %D%/packages/patches/qtwayland-gcc-11.patch \ + %D%/packages/patches/qtwayland-dont-recreate-callbacks.patch \ + %D%/packages/patches/qtwayland-cleanup-callbacks.patch \ %D%/packages/patches/qtwebkit-pbutils-include.patch \ %D%/packages/patches/qtwebkit-fix-building-with-bison-3.7.patch \ %D%/packages/patches/qtwebkit-fix-building-with-python-3.9.patch \ diff --git a/gnu/packages/patches/qtwayland-cleanup-callbacks.patch b/gnu/packages/patches/qtwayland-cleanup-callbacks.patch new file mode 100644 index 0000000000..b7618432cb --- /dev/null +++ b/gnu/packages/patches/qtwayland-cleanup-callbacks.patch @@ -0,0 +1,52 @@ +From 42cdc61a93cf2acb09936aebb5e431fdbc0a26c6 Mon Sep 17 00:00:00 2001 +From: Georges Basile Stavracas Neto +Date: Thu, 27 May 2021 20:02:53 -0300 +Subject: [PATCH] Client: Always destroy frame callback in the actual callback + +It's good hygiene to destroy all frame callbacks. Destroy the +frame callback and cleanup the mFrameCallback class member in +the callback itself. The callback destruction happens before +calling handleFrameCallback() to avoid the theoretical case +where another frame callback is queued by handleFrameCallback(), +and then immediately destroyed in the callback handler. + +Change-Id: Ide6dc95e3402932c58bfc088a9d471fda821e9a1 +Reviewed-by: Eskil Abrahamsen Blomfeldt +--- + src/client/qwaylandwindow.cpp | 14 +++++--------- + 1 file changed, 5 insertions(+), 9 deletions(-) + +diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp +index d83d51695..5561f58f7 100644 +--- a/src/client/qwaylandwindow.cpp ++++ b/src/client/qwaylandwindow.cpp +@@ -659,9 +659,13 @@ void QWaylandWindow::commit() + + const wl_callback_listener QWaylandWindow::callbackListener = { + [](void *data, wl_callback *callback, uint32_t time) { +- Q_UNUSED(callback); + Q_UNUSED(time); + auto *window = static_cast(data); ++ ++ Q_ASSERT(callback == window->mFrameCallback); ++ wl_callback_destroy(callback); ++ window->mFrameCallback = nullptr; ++ + window->handleFrameCallback(); + } + }; +@@ -1366,11 +1370,6 @@ void QWaylandWindow::handleUpdate() + if (!mSurface) + return; + +- if (mFrameCallback) { +- wl_callback_destroy(mFrameCallback); +- mFrameCallback = nullptr; +- } +- + QMutexLocker locker(mFrameQueue.mutex); + struct ::wl_surface *wrappedSurface = reinterpret_cast(wl_proxy_create_wrapper(mSurface->object())); + wl_proxy_set_queue(reinterpret_cast(wrappedSurface), mFrameQueue.queue); +-- +2.38.1 + diff --git a/gnu/packages/patches/qtwayland-dont-recreate-callbacks.patch b/gnu/packages/patches/qtwayland-dont-recreate-callbacks.patch new file mode 100644 index 0000000000..dda2b99844 --- /dev/null +++ b/gnu/packages/patches/qtwayland-dont-recreate-callbacks.patch @@ -0,0 +1,76 @@ +From cbc74ba6d7186457d8d07183272e952dee5f34f9 Mon Sep 17 00:00:00 2001 +From: Georges Basile Stavracas Neto +Date: Thu, 27 May 2021 19:55:04 -0300 +Subject: [PATCH] Client: Don't always recreate frame callbacks + +The main QWaylandWindow method that is executed when handling updates is +QWaylandWindow::handleUpdate(). This method always, unconditionally queues +a frame callback, regardless of whether any other one is already queued. + +On some circumstances, e.g. when a window is hidden or completely obscured +by other windows, it stops receiving frame callbacks from the compositor. +However, QWaylandWindow would continue to request for them, which eventually +fills up the Wayland socket, and causes the application to crash. + +This can be avoided by checking if the platform window is already waiting +for a frame callback, before queueing another one. + +In QWaylandWindow::handleUpdate(), check if mWaitingForFrameCallback is true +before queueing frame callbacks, and early return if that's the case. + +The XDG-shell test needed to be updated for this: The mock compositor is +not responding to any frame callbacks, so the window will be unexposed, +no longer get paint events and therefore not trigger any commit. This +worked by accident before because we were issuing updates quickly enough +to reset the timer before it had a chance to unexpose the window. The +easiest fix is just to disable the dependency on frame callbacks in +this test, since that is clearly not what it's testing. + +Task-number: QTBUG-81504 +Change-Id: Ieacb05c7d5a5fcf662243d9177ebcc308cb9ca84 +Reviewed-by: Qt CI Bot +Reviewed-by: Georges Basile Stavracas Neto +Reviewed-by: Eskil Abrahamsen Blomfeldt +--- + src/client/qwaylandwindow.cpp | 4 ++++ + tests/auto/client/xdgshell/tst_xdgshell.cpp | 2 ++ + 2 files changed, 6 insertions(+) + +diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp +index a708afce..d83d5169 100644 +--- a/src/client/qwaylandwindow.cpp ++++ b/src/client/qwaylandwindow.cpp +@@ -1357,6 +1357,10 @@ void QWaylandWindow::requestUpdate() + void QWaylandWindow::handleUpdate() + { + qCDebug(lcWaylandBackingstore) << "handleUpdate" << QThread::currentThread(); ++ ++ if (mWaitingForFrameCallback) ++ return; ++ + // TODO: Should sync subsurfaces avoid requesting frame callbacks? + QReadLocker lock(&mSurfaceLock); + if (!mSurface) +diff --git a/tests/auto/client/xdgshell/tst_xdgshell.cpp b/tests/auto/client/xdgshell/tst_xdgshell.cpp +index 1d2a2014..962093c7 100644 +--- a/tests/auto/client/xdgshell/tst_xdgshell.cpp ++++ b/tests/auto/client/xdgshell/tst_xdgshell.cpp +@@ -138,6 +138,7 @@ void tst_xdgshell::configureSize() + + void tst_xdgshell::configureStates() + { ++ QVERIFY(qputenv("QT_WAYLAND_FRAME_CALLBACK_TIMEOUT", "0")); + QRasterWindow window; + window.resize(64, 48); + window.show(); +@@ -186,6 +187,7 @@ void tst_xdgshell::configureStates() + QCOMPARE(window.windowStates(), Qt::WindowNoState); + QCOMPARE(window.frameGeometry().size(), windowedSize); + // QCOMPARE(window.frameGeometry().topLeft(), QPoint()); // TODO: this doesn't currently work when window decorations are enabled ++ QVERIFY(qunsetenv("QT_WAYLAND_FRAME_CALLBACK_TIMEOUT")); + } + + void tst_xdgshell::popup() +-- +2.38.1 + diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm index 6406fd5c49..14fc73ef28 100644 --- a/gnu/packages/qt.scm +++ b/gnu/packages/qt.scm @@ -1522,7 +1522,9 @@ (define-public qtwayland-5 (source (origin (method url-fetch) (uri (qt-urls name version)) - (patches (search-patches "qtwayland-gcc-11.patch")) + (patches (search-patches "qtwayland-gcc-11.patch" + "qtwayland-dont-recreate-callbacks.patch" + "qtwayland-cleanup-callbacks.patch")) (sha256 (base32 "0yy8qf9kn15iqsxi2r7jbcsc0vsdyfz7bbxmfn4i9qmz1yvg0jgr"))))