Fix crash when window attach is called without waiting for frame callbacks

If QWaylandWindow::attach was called before getting the frame callback, it
would overwrite mFrameCallback. Hence, all but the last frame callback would
still be alive after the QWaylandWindow destructor.

When the dangling callbacks got invoked the data pointer was statically casted
to the deleted QWaylandWindow, resulting in undefined behavior.

In this change we only delete frame callbacks from the render thread, avoiding
a race condition we fixed earlier. And we always destroy the frame callback
when adding a new one, ensuring that the destructor will clean up the only
remaining callback.

There's a test confirming that the crash has been fixed.

This fixes the flakiness of many of the qtbase auto tests.

Change-Id: Iecb08ab48216eac61b1ebc5c0e0664d4aac900c0
Reviewed-by: Paul Olav Tvete <paul.tvete@qt.io>

2 files changed, 7 insertions, 9 deletions

diff --git a/src/client/qwaylandwindow.cpp b/src/client/qwaylandwindow.cpp
index 4a60a6616..eeba6c85f 100644
--- a/src/client/qwaylandwindow.cpp
+++ b/src/client/qwaylandwindow.cpp
@@ -86,7 +86,6 @@ QWaylandWindow::QWaylandWindow(QWindow *window)
     , mMouseEventsInContentArea(false)
     , mMousePressedInContentArea(Qt::NoButton)
     , mWaitingForFrameSync(false)
-    , mFrameCallback(nullptr)
     , mRequestResizeSent(false)
     , mCanResize(true)
     , mResizeDirty(false)
@@ -475,12 +474,14 @@ void QWaylandWindow::requestResize()
 
 void QWaylandWindow::attach(QWaylandBuffer *buffer, int x, int y)
 {
-    mFrameCallback = nullptr;
+    if (mFrameCallback) {
+        wl_callback_destroy(mFrameCallback);
+        mFrameCallback = nullptr;
+    }
 
     if (buffer) {
-        auto callback = frame();
-        wl_callback_add_listener(callback, &QWaylandWindow::callbackListener, this);
-        mFrameCallback = callback;
+        mFrameCallback = frame();
+        wl_callback_add_listener(mFrameCallback, &QWaylandWindow::callbackListener, this);
         mWaitingForFrameSync = true;
         buffer->setBusy();
 
@@ -523,8 +524,6 @@ void QWaylandWindow::frameCallback(void *data, struct wl_callback *callback, uin
     QWaylandWindow *self = static_cast<QWaylandWindow*>(data);
 
     self->mWaitingForFrameSync = false;
-    wl_callback_destroy(callback);
-    self->mFrameCallback.testAndSetRelaxed(callback, nullptr);
     if (self->mUpdateRequested) {
         QWindowPrivate *w = QWindowPrivate::get(self->window());
         self->mUpdateRequested = false;
diff --git a/src/client/qwaylandwindow_p.h b/src/client/qwaylandwindow_p.h
index 29eb6c596..fa213d07a 100644
--- a/src/client/qwaylandwindow_p.h
+++ b/src/client/qwaylandwindow_p.h
@@ -53,7 +53,6 @@
 
 #include <QtCore/QWaitCondition>
 #include <QtCore/QMutex>
-#include <QtCore/QAtomicPointer>
 #include <QtGui/QIcon>
 #include <QtCore/QVariant>
 
@@ -222,7 +221,7 @@ protected:
 
     WId mWindowId;
     bool mWaitingForFrameSync;
-    QAtomicPointer<struct wl_callback> mFrameCallback;
+    struct ::wl_callback *mFrameCallback = nullptr;
     QWaitCondition mFrameSyncWait;
 
     QMutex mResizeLock;