Fix race condition on frame callback

If the frame listener callback is moved to a different queue while the
frame event is being processed, the event will not get delivered. (It
will not even show up in the WAYLAND_DEBUG output.) This will cause
waitForFrameSync() to hang until it times out.

To avoid this, perform the move just after the callback has been
created.

This exposed an issue with single-threaded rendering, where a new
update would be started from inside the callback, resetting
mWaitingForFrameCallback before waitForFrameSync could react to it.
This caused all rendering to freeze. To avoid that problem, do not
deliver update requests directly from the frame callback.

With the callback always on a separate queue, we then have to make
sure that queue is also dispatched during the main event loop,
otherwise the events may not be processed. To do this, we need a
mutex lock. But it turns out that we no longer need a global mutex
lock as long as the frame events are being dispatched on their own
queues, but can manage with per-window locks instead.

A final thing needed is to make sure the frame callback does not
request additional updates while we are already waiting for the main
thread to process the last one. This is to avoid flooding the main
event loop with events, when the main thread is processing them at
a slower pace than the frame rate.

Fixes: QTBUG-83263
Pick-to: 5.15
Done-with: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@qt.io>
Change-Id: I0db9da64fc8ced147177391c2a7999c4cc7a0d58
Reviewed-by: Eskil Abrahamsen Blomfeldt <eskil.abrahamsen-blomfeldt@qt.io>
Reviewed-by: David Edmundson <davidedmundson@kde.org>
Reviewed-by: Paul Olav Tvete <paul.tvete@qt.io>

5 files changed, 73 insertions, 28 deletions

diff --git a/src/client/qwaylanddisplay.cpp b/src/client/qwaylanddisplay.cpp
index e39f91280..e759c5d64 100644
--- a/src/client/qwaylanddisplay.cpp
+++ b/src/client/qwaylanddisplay.cpp
@@ -225,6 +225,17 @@ void QWaylandDisplay::flushRequests()
     if (wl_display_dispatch_pending(mDisplay) < 0)
         checkError();
 
+    {
+        QReadLocker locker(&m_frameQueueLock);
+        for (const FrameQueue &q : mExternalQueues) {
+            QMutexLocker locker(q.mutex);
+            while (wl_display_prepare_read_queue(mDisplay, q.queue) != 0)
+                wl_display_dispatch_queue_pending(mDisplay, q.queue);
+            wl_display_read_events(mDisplay);
+            wl_display_dispatch_queue_pending(mDisplay, q.queue);
+        }
+    }
+
     wl_display_flush(mDisplay);
 }
 
@@ -234,6 +245,27 @@ void QWaylandDisplay::blockingReadEvents()
         checkError();
 }
 
+void QWaylandDisplay::destroyFrameQueue(const QWaylandDisplay::FrameQueue &q)
+{
+    QWriteLocker locker(&m_frameQueueLock);
+    auto it = std::find_if(mExternalQueues.begin(),
+                           mExternalQueues.end(),
+                           [&q] (const QWaylandDisplay::FrameQueue &other){ return other.queue == q.queue; });
+    Q_ASSERT(it != mExternalQueues.end());
+    mExternalQueues.erase(it);
+    if (q.queue != nullptr)
+        wl_event_queue_destroy(q.queue);
+    delete q.mutex;
+}
+
+QWaylandDisplay::FrameQueue QWaylandDisplay::createFrameQueue()
+{
+    QWriteLocker locker(&m_frameQueueLock);
+    FrameQueue q{createEventQueue()};
+    mExternalQueues.append(q);
+    return q;
+}
+
 wl_event_queue *QWaylandDisplay::createEventQueue()
 {
     return wl_display_create_queue(mDisplay);
diff --git a/src/client/qwaylanddisplay_p.h b/src/client/qwaylanddisplay_p.h
index 68d72812f..86e3b729f 100644
--- a/src/client/qwaylanddisplay_p.h
+++ b/src/client/qwaylanddisplay_p.h
@@ -55,6 +55,8 @@
 #include <QtCore/QObject>
 #include <QtCore/QPointer>
 #include <QtCore/QRect>
+#include <QtCore/QMutex>
+#include <QtCore/QReadWriteLock>
 
 #include <QtCore/QWaitCondition>
 #include <QtCore/QLoggingCategory>
@@ -119,6 +121,12 @@ class Q_WAYLAND_CLIENT_EXPORT QWaylandDisplay : public QObject, public QtWayland
     Q_OBJECT
 
 public:
+    struct FrameQueue {
+        FrameQueue(wl_event_queue *q = nullptr) : queue(q), mutex(new QMutex) {}
+        wl_event_queue *queue;
+        QMutex *mutex;
+    };
+
     QWaylandDisplay(QWaylandIntegration *waylandIntegration);
     ~QWaylandDisplay(void) override;
 
@@ -207,6 +215,8 @@ public:
     void handleWindowDestroyed(QWaylandWindow *window);
 
     wl_event_queue *createEventQueue();
+    FrameQueue createFrameQueue();
+    void destroyFrameQueue(const FrameQueue &q);
     void dispatchQueueWhile(wl_event_queue *queue, std::function<bool()> condition, int timeout = -1);
 
 public slots:
@@ -284,8 +294,10 @@ private:
     QPointer<QWaylandWindow> mLastInputWindow;
     QPointer<QWaylandWindow> mLastKeyboardFocus;
     QList<QWaylandWindow *> mActiveWindows;
+    QList<FrameQueue> mExternalQueues;
     struct wl_callback *mSyncCallback = nullptr;
     static const wl_callback_listener syncCallbackListener;
+    QReadWriteLock m_frameQueueLock;
 
     bool mClientSideInputContextRequested = !QPlatformInputContextFactory::requested().isNull();
     bool mUsingInputContextFromCompositor = false;
diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp
index 382c258f3..76f9e9f7a 100644
--- a/src/client/qwaylandwindow.cpp
+++ b/src/client/qwaylandwindow.cpp
@@ -76,7 +76,7 @@ QWaylandWindow *QWaylandWindow::mMouseGrab = nullptr;
 QWaylandWindow::QWaylandWindow(QWindow *window, QWaylandDisplay *display)
     : QPlatformWindow(window)
     , mDisplay(display)
-    , mFrameQueue(mDisplay->createEventQueue())
+    , mFrameQueue(mDisplay->createFrameQueue())
     , mResizeAfterSwap(qEnvironmentVariableIsSet("QT_WAYLAND_RESIZE_AFTER_SWAP"))
 {
     {
@@ -95,6 +95,7 @@ QWaylandWindow::QWaylandWindow(QWindow *window, QWaylandDisplay *display)
 
 QWaylandWindow::~QWaylandWindow()
 {
+    mDisplay->destroyFrameQueue(mFrameQueue);
     mDisplay->handleWindowDestroyed(this);
 
     delete mWindowDecoration;
@@ -624,33 +625,29 @@ void QWaylandWindow::handleFrameCallback()
     mFrameCallbackElapsedTimer.invalidate();
 
     // The rest can wait until we can run it on the correct thread
-    auto doHandleExpose = [this]() {
-        bool wasExposed = isExposed();
-        mFrameCallbackTimedOut = false;
-        if (!wasExposed && isExposed()) // Did setting mFrameCallbackTimedOut make the window exposed?
-            sendExposeEvent(QRect(QPoint(), geometry().size()));
-        if (wasExposed && hasPendingUpdateRequest())
-            deliverUpdateRequest();
-    };
-
-    if (thread() != QThread::currentThread()) {
-        QMetaObject::invokeMethod(this, doHandleExpose);
-    } else {
-        doHandleExpose();
+    if (!mWaitingForUpdateDelivery) {
+        auto doHandleExpose = [this]() {
+            bool wasExposed = isExposed();
+            mFrameCallbackTimedOut = false;
+            if (!wasExposed && isExposed()) // Did setting mFrameCallbackTimedOut make the window exposed?
+                sendExposeEvent(QRect(QPoint(), geometry().size()));
+            if (wasExposed && hasPendingUpdateRequest())
+                deliverUpdateRequest();
+
+            mWaitingForUpdateDelivery = false;
+        };
+
+        // Queued connection, to make sure we don't call handleUpdate() from inside waitForFrameSync()
+        // in the single-threaded case.
+        mWaitingForUpdateDelivery = true;
+        QMetaObject::invokeMethod(this, doHandleExpose, Qt::QueuedConnection);
     }
 }
 
-QMutex QWaylandWindow::mFrameSyncMutex;
-
 bool QWaylandWindow::waitForFrameSync(int timeout)
 {
-    if (!mWaitingForFrameCallback)
-        return true;
-
-    QMutexLocker locker(&mFrameSyncMutex);
-
-    wl_proxy_set_queue(reinterpret_cast<wl_proxy *>(mFrameCallback), mFrameQueue);
-    mDisplay->dispatchQueueWhile(mFrameQueue, [&]() { return mWaitingForFrameCallback; }, timeout);
+    QMutexLocker locker(mFrameQueue.mutex);
+    mDisplay->dispatchQueueWhile(mFrameQueue.queue, [&]() { return mWaitingForFrameCallback; }, timeout);
 
     if (mWaitingForFrameCallback) {
         qCDebug(lcWaylandBackingstore) << "Didn't receive frame callback in time, window should now be inexposed";
@@ -1124,6 +1121,7 @@ void QWaylandWindow::timerEvent(QTimerEvent *event)
     }
     if (mFrameCallbackElapsedTimer.isValid() && callbackTimerExpired) {
         mFrameCallbackElapsedTimer.invalidate();
+
         qCDebug(lcWaylandBackingstore) << "Didn't receive frame callback in time, window should now be inexposed";
         mFrameCallbackTimedOut = true;
         mWaitingForUpdate = false;
@@ -1172,7 +1170,11 @@ void QWaylandWindow::handleUpdate()
         mFrameCallback = nullptr;
     }
 
-    mFrameCallback = mSurface->frame();
+    QMutexLocker locker(mFrameQueue.mutex);
+    struct ::wl_surface *wrappedSurface = reinterpret_cast<struct ::wl_surface *>(wl_proxy_create_wrapper(mSurface->object()));
+    wl_proxy_set_queue(reinterpret_cast<wl_proxy *>(wrappedSurface), mFrameQueue.queue);
+    mFrameCallback = wl_surface_frame(wrappedSurface);
+    wl_proxy_wrapper_destroy(wrappedSurface);
     wl_callback_add_listener(mFrameCallback, &QWaylandWindow::callbackListener, this);
     mWaitingForFrameCallback = true;
     mWaitingForUpdate = false;
diff --git a/src/client/qwaylandwindow_p.h b/src/client/qwaylandwindow_p.h
index 9272d3680..8b9a11e7f 100644
--- a/src/client/qwaylandwindow_p.h
+++ b/src/client/qwaylandwindow_p.h
@@ -63,6 +63,7 @@
 #include <qpa/qplatformwindow.h>
 
 #include <QtWaylandClient/private/qwayland-wayland.h>
+#include <QtWaylandClient/private/qwaylanddisplay_p.h>
 #include <QtWaylandClient/qtwaylandclientglobal.h>
 
 struct wl_egl_window;
@@ -226,10 +227,11 @@ protected:
     WId mWindowId;
     bool mWaitingForFrameCallback = false;
     bool mFrameCallbackTimedOut = false; // Whether the frame callback has timed out
+    bool mWaitingForUpdateDelivery = false;
     int mFrameCallbackCheckIntervalTimerId = -1;
     QElapsedTimer mFrameCallbackElapsedTimer;
     struct ::wl_callback *mFrameCallback = nullptr;
-    struct ::wl_event_queue *mFrameQueue = nullptr;
+    QWaylandDisplay::FrameQueue mFrameQueue;
     QWaitCondition mFrameSyncWait;
 
     // True when we have called deliverRequestUpdate, but the client has not yet attached a new buffer
@@ -280,7 +282,6 @@ private:
     static const wl_callback_listener callbackListener;
     void handleFrameCallback();
 
-    static QMutex mFrameSyncMutex;
     static QWaylandWindow *mMouseGrab;
 
     QReadWriteLock mSurfaceLock;
diff --git a/tests/auto/client/surface/tst_surface.cpp b/tests/auto/client/surface/tst_surface.cpp
index 289fbd621..c7b02e2cc 100644
--- a/tests/auto/client/surface/tst_surface.cpp
+++ b/tests/auto/client/surface/tst_surface.cpp
@@ -64,7 +64,6 @@ void tst_surface::createDestroySurface()
 
 void tst_surface::waitForFrameCallbackRaster()
 {
-    QSKIP("TODO: This currently fails, needs a fix");
     class TestWindow : public QRasterWindow {
     public:
         explicit TestWindow() { resize(40, 40); }
@@ -100,7 +99,6 @@ void tst_surface::waitForFrameCallbackRaster()
 #if QT_CONFIG(opengl)
 void tst_surface::waitForFrameCallbackGl()
 {
-    QSKIP("TODO: This currently fails, needs a fix");
     class TestWindow : public QOpenGLWindow {
     public:
         explicit TestWindow()