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
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>
(cherry picked from commit 98aea22d839000a377d0fc646133f76e08b44f6e)

4 files changed, 73 insertions, 26 deletions

diff --git a/src/client/qwaylanddisplay.cpp b/src/client/qwaylanddisplay.cpp
index 37cb0e9bc..fe094f6fe 100644
--- a/src/client/qwaylanddisplay.cpp
+++ b/src/client/qwaylanddisplay.cpp
@@ -221,6 +221,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);
 }
 
@@ -230,6 +241,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 a52c89fe9..188e91318 100644
--- a/src/client/qwaylanddisplay_p.h
+++ b/src/client/qwaylanddisplay_p.h
@@ -55,6 +55,8 @@
 #include <QtCore/QRect>
 #include <QtCore/QPointer>
 #include <QtCore/QVector>
+#include <QtCore/QMutex>
+#include <QtCore/QReadWriteLock>
 
 #include <QtCore/QWaitCondition>
 #include <QtCore/QLoggingCategory>
@@ -118,6 +120,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;
 
@@ -205,6 +213,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:
@@ -266,8 +276,10 @@ private:
     QPointer<QWaylandWindow> mLastInputWindow;
     QPointer<QWaylandWindow> mLastKeyboardFocus;
     QVector<QWaylandWindow *> mActiveWindows;
+    QVector<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 1127a07d4..1e26ee91e 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;
@@ -616,33 +617,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";
@@ -1116,6 +1113,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;
@@ -1164,7 +1162,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 948e0ea7a..35a2c0321 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;