From 027a7131543b0194a7d9e98a6b53928e30bc080f Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Wed, 30 Aug 2017 14:00:51 +0200 Subject: [PATCH 1/4] 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 --- .../platforms/wayland/qwaylandwindow.cpp | 13 +++++---- .../platforms/wayland/qwaylandwindow_p.h | 3 +-- tests/auto/wayland/client/tst_client.cpp | 27 +++++++++++++++++++ 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/plugins/platforms/wayland/qwaylandwindow.cpp b/src/plugins/platforms/wayland/qwaylandwindow.cpp index 4a60a661684..eeba6c85fb2 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow.cpp +++ b/src/plugins/platforms/wayland/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(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/plugins/platforms/wayland/qwaylandwindow_p.h b/src/plugins/platforms/wayland/qwaylandwindow_p.h index 29eb6c59687..fa213d07ae8 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow_p.h +++ b/src/plugins/platforms/wayland/qwaylandwindow_p.h @@ -53,7 +53,6 @@ #include #include -#include #include #include @@ -222,7 +221,7 @@ protected: WId mWindowId; bool mWaitingForFrameSync; - QAtomicPointer mFrameCallback; + struct ::wl_callback *mFrameCallback = nullptr; QWaitCondition mFrameSyncWait; QMutex mResizeLock; diff --git a/tests/auto/wayland/client/tst_client.cpp b/tests/auto/wayland/client/tst_client.cpp index 0c9c007a298..8acddfbe740 100644 --- a/tests/auto/wayland/client/tst_client.cpp +++ b/tests/auto/wayland/client/tst_client.cpp @@ -142,6 +142,7 @@ private slots: void backingStore(); void touchDrag(); void mouseDrag(); + void dontCrashOnMultipleCommits(); private: MockCompositor *compositor; @@ -333,6 +334,32 @@ void tst_WaylandClient::mouseDrag() QTRY_VERIFY(window.dragStarted); } +void tst_WaylandClient::dontCrashOnMultipleCommits() +{ + auto window = new TestWindow(); + window->show(); + + QRect rect(QPoint(), window->size()); + + QBackingStore backingStore(window); + backingStore.resize(rect.size()); + backingStore.beginPaint(rect); + QPainter p(backingStore.paintDevice()); + p.fillRect(rect, Qt::magenta); + p.end(); + backingStore.endPaint(); + + backingStore.flush(rect); + backingStore.flush(rect); + backingStore.flush(rect); + + compositor->processWaylandEvents(); + + delete window; + + QTRY_VERIFY(!compositor->surface()); +} + int main(int argc, char **argv) { setenv("XDG_RUNTIME_DIR", ".", 1); From 844002cb81960c2408b2dee44c56f1f7561bdcbb Mon Sep 17 00:00:00 2001 From: Paul Olav Tvete Date: Fri, 21 Jul 2017 10:21:22 +0200 Subject: [PATCH 2/4] Close popups in the correct order According to the protocol, child popups have to be closed before parents, but QMenuBar/QMenu will sometimes close the parent first. Change-Id: Id027ac483b727a19388df619fe1503d794e12c12 Task-number: QTBUG-62048 Reviewed-by: Johan Helsing --- .../platforms/wayland/qwaylandwindow.cpp | 19 +++++++++++++++++++ .../platforms/wayland/qwaylandwindow_p.h | 1 + 2 files changed, 20 insertions(+) diff --git a/src/plugins/platforms/wayland/qwaylandwindow.cpp b/src/plugins/platforms/wayland/qwaylandwindow.cpp index eeba6c85fb2..6649493b6c6 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow.cpp +++ b/src/plugins/platforms/wayland/qwaylandwindow.cpp @@ -341,9 +341,26 @@ void QWaylandWindow::sendExposeEvent(const QRect &rect) QWindowSystemInterface::handleExposeEvent(window(), rect); } + +static QVector> activePopups; + +void QWaylandWindow::closePopups(QWaylandWindow *parent) +{ + while (!activePopups.isEmpty()) { + auto popup = activePopups.takeLast(); + if (popup.isNull()) + continue; + if (popup.data() == parent) + return; + popup->reset(); + } +} + void QWaylandWindow::setVisible(bool visible) { if (visible) { + if (window()->type() == Qt::Popup) + activePopups << this; initWindow(); mDisplay->flushRequests(); @@ -357,6 +374,8 @@ void QWaylandWindow::setVisible(bool visible) // case 'this' will be deleted. When that happens, we must abort right away. QPointer deleteGuard(this); QWindowSystemInterface::flushWindowSystemEvents(); + if (!deleteGuard.isNull() && window()->type() == Qt::Popup) + closePopups(this); if (!deleteGuard.isNull()) reset(); } diff --git a/src/plugins/platforms/wayland/qwaylandwindow_p.h b/src/plugins/platforms/wayland/qwaylandwindow_p.h index fa213d07ae8..e7b9e3d3852 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow_p.h +++ b/src/plugins/platforms/wayland/qwaylandwindow_p.h @@ -253,6 +253,7 @@ private: bool shouldCreateSubSurface() const; void reset(); void sendExposeEvent(const QRect &rect); + static void closePopups(QWaylandWindow *parent); void handleMouseEventWithDecoration(QWaylandInputDevice *inputDevice, const QWaylandPointerEvent &e); From f35dc39ae9029f27ea1eed949139e4827995bc5a Mon Sep 17 00:00:00 2001 From: Johan Klokkhammer Helsing Date: Thu, 17 Aug 2017 13:20:04 +0200 Subject: [PATCH 3/4] Set window screen from wl_surface.enter and leave events Removes the pointer mScreen, which would previously cause a crash if the screen was removed. Ensures that QWindow::screen() is correct, except in the cases where: - The compositor has not yet sent enter and leave events (in which case the primary output is returned). - The compositor is not sending enter and leave events (although this is mandatory, some compositors don't do this). - The application developer has tried to move the window with QWindow::setScreen(QScreen *). Since there is no way for a client to ask to be moved to a specific monitor in windowed mode, we return the requested screen until a new enter or leave event is received. This will also be useful when implementing/fixing features where the current screen matters. Examples are QT_AUTO_SCREEN_SCALE_FACTOR and the optional output parameter to wl_shell_surface.set_fullscreen. Task-number: QTBUG-62044 Change-Id: Iafde2e278fbc8876e8dafe5b2a4d2482fdc7961a Reviewed-by: Paul Olav Tvete --- .../platforms/wayland/qwaylandscreen.cpp | 6 ++ .../platforms/wayland/qwaylandscreen_p.h | 1 + .../platforms/wayland/qwaylandwindow.cpp | 65 ++++++++++++++++++- .../platforms/wayland/qwaylandwindow_p.h | 11 +++- 4 files changed, 78 insertions(+), 5 deletions(-) diff --git a/src/plugins/platforms/wayland/qwaylandscreen.cpp b/src/plugins/platforms/wayland/qwaylandscreen.cpp index ac595457793..334e0ec4602 100644 --- a/src/plugins/platforms/wayland/qwaylandscreen.cpp +++ b/src/plugins/platforms/wayland/qwaylandscreen.cpp @@ -175,6 +175,12 @@ QWaylandScreen * QWaylandScreen::waylandScreenFromWindow(QWindow *window) return static_cast(platformScreen); } +QWaylandScreen *QWaylandScreen::fromWlOutput(::wl_output *output) +{ + auto wlOutput = static_cast(wl_output_get_user_data(output)); + return static_cast(wlOutput); +} + void QWaylandScreen::output_mode(uint32_t flags, int width, int height, int refresh) { if (!(flags & WL_OUTPUT_MODE_CURRENT)) diff --git a/src/plugins/platforms/wayland/qwaylandscreen_p.h b/src/plugins/platforms/wayland/qwaylandscreen_p.h index b2900a964f1..3d4df6e6619 100644 --- a/src/plugins/platforms/wayland/qwaylandscreen_p.h +++ b/src/plugins/platforms/wayland/qwaylandscreen_p.h @@ -99,6 +99,7 @@ public: ::wl_output *output() { return object(); } static QWaylandScreen *waylandScreenFromWindow(QWindow *window); + static QWaylandScreen *fromWlOutput(::wl_output *output); private: void output_mode(uint32_t flags, int width, int height, int refresh) override; diff --git a/src/plugins/platforms/wayland/qwaylandwindow.cpp b/src/plugins/platforms/wayland/qwaylandwindow.cpp index 6649493b6c6..e5edd0e5e5c 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow.cpp +++ b/src/plugins/platforms/wayland/qwaylandwindow.cpp @@ -69,6 +69,8 @@ #include +#include + QT_BEGIN_NAMESPACE namespace QtWaylandClient { @@ -78,8 +80,7 @@ QWaylandWindow *QWaylandWindow::mMouseGrab = 0; QWaylandWindow::QWaylandWindow(QWindow *window) : QObject() , QPlatformWindow(window) - , mScreen(QWaylandScreen::waylandScreenFromWindow(window)) - , mDisplay(mScreen->display()) + , mDisplay(screen()->display()) , mShellSurface(0) , mSubSurfaceWindow(0) , mWindowDecoration(0) @@ -99,6 +100,7 @@ QWaylandWindow::QWaylandWindow(QWindow *window) { static WId id = 1; mWindowId = id++; + connect(qApp, &QGuiApplication::screenRemoved, this, &QWaylandWindow::handleScreenRemoved); initializeWlSurface(); } @@ -356,6 +358,11 @@ void QWaylandWindow::closePopups(QWaylandWindow *parent) } } +QWaylandScreen *QWaylandWindow::calculateScreenFromSurfaceEvents() const +{ + return mScreens.isEmpty() ? screen() : mScreens.first(); +} + void QWaylandWindow::setVisible(bool visible) { if (visible) { @@ -491,6 +498,53 @@ void QWaylandWindow::requestResize() QWindowSystemInterface::flushWindowSystemEvents(); } +void QWaylandWindow::surface_enter(wl_output *output) +{ + QWaylandScreen *oldScreen = calculateScreenFromSurfaceEvents(); + auto addedScreen = QWaylandScreen::fromWlOutput(output); + + if (mScreens.contains(addedScreen)) { + qWarning() << "Unexpected wl_surface.enter received for output with id:" + << wl_proxy_get_id(reinterpret_cast(output)) + << "screen name:" << addedScreen->name() << "screen model:" << addedScreen->model(); + return; + } + + mScreens.append(addedScreen); + + QWaylandScreen *newScreen = calculateScreenFromSurfaceEvents(); + if (oldScreen != newScreen) //currently this will only happen if the first wl_surface.enter is for a non-primary screen + QWindowSystemInterface::handleWindowScreenChanged(window(), newScreen->QPlatformScreen::screen()); +} + +void QWaylandWindow::surface_leave(wl_output *output) +{ + QWaylandScreen *oldScreen = calculateScreenFromSurfaceEvents(); + auto *removedScreen = QWaylandScreen::fromWlOutput(output); + bool wasRemoved = mScreens.removeOne(removedScreen); + if (!wasRemoved) { + qWarning() << "Unexpected wl_surface.leave received for output with id:" + << wl_proxy_get_id(reinterpret_cast(output)) + << "screen name:" << removedScreen->name() << "screen model:" << removedScreen->model(); + return; + } + + QWaylandScreen *newScreen = calculateScreenFromSurfaceEvents(); + if (oldScreen != newScreen) + QWindowSystemInterface::handleWindowScreenChanged(window(), newScreen->QPlatformScreen::screen()); +} + +void QWaylandWindow::handleScreenRemoved(QScreen *qScreen) +{ + QWaylandScreen *oldScreen = calculateScreenFromSurfaceEvents(); + bool wasRemoved = mScreens.removeOne(static_cast(qScreen->handle())); + if (wasRemoved) { + QWaylandScreen *newScreen = calculateScreenFromSurfaceEvents(); + if (oldScreen != newScreen) + QWindowSystemInterface::handleWindowScreenChanged(window(), newScreen->QPlatformScreen::screen()); + } +} + void QWaylandWindow::attach(QWaylandBuffer *buffer, int x, int y) { if (mFrameCallback) { @@ -579,6 +633,11 @@ QWaylandSubSurface *QWaylandWindow::subSurfaceWindow() const return mSubSurfaceWindow; } +QWaylandScreen *QWaylandWindow::screen() const +{ + return static_cast(QPlatformWindow::screen()); +} + void QWaylandWindow::handleContentOrientationChange(Qt::ScreenOrientation orientation) { if (mDisplay->compositorVersion() < 2) @@ -843,7 +902,7 @@ void QWaylandWindow::handleMouseEventWithDecoration(QWaylandInputDevice *inputDe #if QT_CONFIG(cursor) void QWaylandWindow::setMouseCursor(QWaylandInputDevice *device, const QCursor &cursor) { - device->setCursor(cursor, mScreen); + device->setCursor(cursor, screen()); } void QWaylandWindow::restoreMouseCursor(QWaylandInputDevice *device) diff --git a/src/plugins/platforms/wayland/qwaylandwindow_p.h b/src/plugins/platforms/wayland/qwaylandwindow_p.h index e7b9e3d3852..bd4a359090f 100644 --- a/src/plugins/platforms/wayland/qwaylandwindow_p.h +++ b/src/plugins/platforms/wayland/qwaylandwindow_p.h @@ -142,7 +142,7 @@ public: QWaylandDisplay *display() const { return mDisplay; } QWaylandShellSurface *shellSurface() const; QWaylandSubSurface *subSurfaceWindow() const; - QWaylandScreen *screen() const { return mScreen; } + QWaylandScreen *screen() const; void handleContentOrientationChange(Qt::ScreenOrientation orientation) override; void setOrientationMask(Qt::ScreenOrientations mask); @@ -209,7 +209,10 @@ public slots: void requestResize(); protected: - QWaylandScreen *mScreen; + void surface_enter(struct ::wl_output *output) override; + void surface_leave(struct ::wl_output *output) override; + + QVector mScreens; //As seen by wl_surface.enter/leave events. Chronological order. QWaylandDisplay *mDisplay; QWaylandShellSurface *mShellSurface; QWaylandSubSurface *mSubSurfaceWindow; @@ -244,6 +247,9 @@ protected: QWaylandShmBackingStore *mBackingStore; +private slots: + void handleScreenRemoved(QScreen *qScreen); + private: bool setWindowStateInternal(Qt::WindowState flags); void setGeometry_helper(const QRect &rect); @@ -254,6 +260,7 @@ private: void reset(); void sendExposeEvent(const QRect &rect); static void closePopups(QWaylandWindow *parent); + QWaylandScreen *calculateScreenFromSurfaceEvents() const; void handleMouseEventWithDecoration(QWaylandInputDevice *inputDevice, const QWaylandPointerEvent &e); From 1a0939a8de214108783aaa646ebf63e3bc2641f9 Mon Sep 17 00:00:00 2001 From: Paul Olav Tvete Date: Fri, 8 Sep 2017 11:33:20 +0200 Subject: [PATCH 4/4] Fix server buffer integration override logic Assign to the actual variable instead of a new one with the same name. Change-Id: I9e4c3525891ff53f0194198b9c11f223df4c14a9 Reviewed-by: David Edmundson Reviewed-by: Eirik Aavitsland --- src/plugins/platforms/wayland/qwaylandintegration.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/platforms/wayland/qwaylandintegration.cpp b/src/plugins/platforms/wayland/qwaylandintegration.cpp index 400f3517c81..bffadf29a49 100644 --- a/src/plugins/platforms/wayland/qwaylandintegration.cpp +++ b/src/plugins/platforms/wayland/qwaylandintegration.cpp @@ -359,7 +359,7 @@ void QWaylandIntegration::initializeServerBufferIntegration() disableHardwareIntegration = disableHardwareIntegration || !mDisplay->hardwareIntegration(); if (disableHardwareIntegration) { QByteArray serverBufferIntegrationName = qgetenv("QT_WAYLAND_SERVER_BUFFER_INTEGRATION"); - QString targetKey = QString::fromLocal8Bit(serverBufferIntegrationName); + targetKey = QString::fromLocal8Bit(serverBufferIntegrationName); } else { targetKey = mDisplay->hardwareIntegration()->serverBufferIntegration(); }