diff options
author | Weng Xuetian <wengxt@gmail.com> | 2022-11-27 12:44:40 -0800 |
---|---|---|
committer | Weng Xuetian <wengxt@gmail.com> | 2022-12-03 20:33:33 -0800 |
commit | eaa7c2632b124cea5eec0844a38a3e6519c8151c (patch) | |
tree | f96415edd85dc6ea95edce0765e74c0fdbb6826a | |
parent | 471b2123400ef6936b5173553205549c7dd1a249 (diff) | |
download | qtwayland-eaa7c2632b124cea5eec0844a38a3e6519c8151c.tar.gz |
Fix frame sync related to unprotected multithread access
There is a few crashes happens in real life that frame callback is
double-free'd and hit an assertion in wayland-client. e.g.
https://bugs.kde.org/show_bug.cgi?id=450003
This is due to the WaylandEventThread and calls to QWaylandWindow::reset
may free and unset the mFrameCallback at the same time. mFrameSyncMutex
should be used to protect such access.
Pick-to: 6.4
Change-Id: Ie01d08d07a2f10f70606ed1935caac09cb4f0382
Reviewed-by: David Edmundson <davidedmundson@kde.org>
-rw-r--r-- | src/client/qwaylandwindow.cpp | 64 | ||||
-rw-r--r-- | src/client/qwaylandwindow_p.h | 11 |
2 files changed, 43 insertions, 32 deletions
diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp index dcd80f37..f4d49c84 100644 --- a/src/client/qwaylandwindow.cpp +++ b/src/client/qwaylandwindow.cpp @@ -228,13 +228,16 @@ void QWaylandWindow::reset() mSurface.reset(); } - if (mFrameCallback) { - wl_callback_destroy(mFrameCallback); - mFrameCallback = nullptr; - } + { + QMutexLocker lock(&mFrameSyncMutex); + if (mFrameCallback) { + wl_callback_destroy(mFrameCallback); + mFrameCallback = nullptr; + } - mFrameCallbackElapsedTimer.invalidate(); - mWaitingForFrameCallback = false; + mFrameCallbackElapsedTimer.invalidate(); + mWaitingForFrameCallback = false; + } mFrameCallbackTimedOut = false; mWaitingToApplyConfigure = false; @@ -672,18 +675,21 @@ const wl_callback_listener QWaylandWindow::callbackListener = { [](void *data, wl_callback *callback, uint32_t time) { Q_UNUSED(time); auto *window = static_cast<QWaylandWindow*>(data); - - Q_ASSERT(callback == window->mFrameCallback); - wl_callback_destroy(callback); - window->mFrameCallback = nullptr; - - window->handleFrameCallback(); + window->handleFrameCallback(callback); } }; -void QWaylandWindow::handleFrameCallback() +void QWaylandWindow::handleFrameCallback(wl_callback* callback) { QMutexLocker locker(&mFrameSyncMutex); + if (!mFrameCallback) { + // This means the callback is already unset by QWaylandWindow::reset. + // The wl_callback object will be destroyed there too. + return; + } + Q_ASSERT(callback == mFrameCallback); + wl_callback_destroy(callback); + mFrameCallback = nullptr; mWaitingForFrameCallback = false; mFrameCallbackElapsedTimer.invalidate(); @@ -1382,19 +1388,24 @@ void QWaylandWindow::timerEvent(QTimerEvent *event) if (event->timerId() != mFrameCallbackCheckIntervalTimerId) return; - bool callbackTimerExpired = mFrameCallbackElapsedTimer.hasExpired(mFrameCallbackTimeout); - if (!mFrameCallbackElapsedTimer.isValid() || callbackTimerExpired ) { - killTimer(mFrameCallbackCheckIntervalTimerId); - mFrameCallbackCheckIntervalTimerId = -1; - } - if (mFrameCallbackElapsedTimer.isValid() && callbackTimerExpired) { - mFrameCallbackElapsedTimer.invalidate(); + { + QMutexLocker lock(&mFrameSyncMutex); - qCDebug(lcWaylandBackingstore) << "Didn't receive frame callback in time, window should now be inexposed"; - mFrameCallbackTimedOut = true; - mWaitingForUpdate = false; - sendExposeEvent(QRect()); + bool callbackTimerExpired = mFrameCallbackElapsedTimer.hasExpired(mFrameCallbackTimeout); + if (!mFrameCallbackElapsedTimer.isValid() || callbackTimerExpired ) { + killTimer(mFrameCallbackCheckIntervalTimerId); + mFrameCallbackCheckIntervalTimerId = -1; + } + if (!mFrameCallbackElapsedTimer.isValid() || !callbackTimerExpired) { + return; + } + mFrameCallbackElapsedTimer.invalidate(); } + + qCDebug(lcWaylandBackingstore) << "Didn't receive frame callback in time, window should now be inexposed"; + mFrameCallbackTimedOut = true; + mWaitingForUpdate = false; + sendExposeEvent(QRect()); } void QWaylandWindow::requestUpdate() @@ -1437,15 +1448,14 @@ void QWaylandWindow::handleUpdate() { qCDebug(lcWaylandBackingstore) << "handleUpdate" << QThread::currentThread(); - if (mWaitingForFrameCallback) - return; - // TODO: Should sync subsurfaces avoid requesting frame callbacks? QReadLocker lock(&mSurfaceLock); if (!mSurface) return; QMutexLocker locker(&mFrameSyncMutex); + if (mWaitingForFrameCallback) + return; struct ::wl_surface *wrappedSurface = reinterpret_cast<struct ::wl_surface *>(wl_proxy_create_wrapper(mSurface->object())); wl_proxy_set_queue(reinterpret_cast<wl_proxy *>(wrappedSurface), mDisplay->frameEventQueue()); diff --git a/src/client/qwaylandwindow_p.h b/src/client/qwaylandwindow_p.h index 2e68cc2e..0f8c5515 100644 --- a/src/client/qwaylandwindow_p.h +++ b/src/client/qwaylandwindow_p.h @@ -261,12 +261,13 @@ protected: #endif WId mWindowId; - bool mWaitingForFrameCallback = false; bool mFrameCallbackTimedOut = false; // Whether the frame callback has timed out - QAtomicInt mWaitingForUpdateDelivery = false; int mFrameCallbackCheckIntervalTimerId = -1; - QElapsedTimer mFrameCallbackElapsedTimer; - struct ::wl_callback *mFrameCallback = nullptr; + QAtomicInt mWaitingForUpdateDelivery = false; + + bool mWaitingForFrameCallback = false; // Protected by mFrameSyncMutex + QElapsedTimer mFrameCallbackElapsedTimer; // Protected by mFrameSyncMutex + struct ::wl_callback *mFrameCallback = nullptr; // Protected by mFrameSyncMutex QMutex mFrameSyncMutex; QWaitCondition mFrameSyncWait; @@ -323,7 +324,7 @@ private: QRect mLastExposeGeometry; static const wl_callback_listener callbackListener; - void handleFrameCallback(); + void handleFrameCallback(struct ::wl_callback* callback); static QWaylandWindow *mMouseGrab; |