client: Change default lifespan of wl_surface to match the window

This brings it more in-line with other platforms where the underlying
backing store has the lifespan of the window, implicitly fixing a
teardown issue with Qt Vulkan.

With this commit the wl_surface lifetime matches the QPlatformWindow, on
show and hide the shell surface is still recreated, with a null buffer
attached inbetween to get a blank slate as per the specification.

There is still some paths where a wl_surface is reset during the
platform window lifetime if the user ever converted a subsurface to a
toplevel or vice-versa, also for handling compositing restarts.

Task-number: QTBUG-125592
Change-Id: I449ec170a085c7c0fca504687556d702daddf79a
Reviewed-by: David Edmundson <davidedmundson@kde.org>

src/plugins/platforms/wayland/qwaylandwindow.cpp

@@ -73,9 +73,7 @@ QWaylandWindow::QWaylandWindow(QWindow *window, QWaylandDisplay *display)
 QWaylandWindow::~QWaylandWindow()
 {
     mWindowDecoration.reset();
-
-    if (mSurface)
-        reset();
+    reset();
 
     const QWindow *parent = window();
     const auto tlw = QGuiApplication::topLevelWindows();
@@ -99,20 +97,19 @@ void QWaylandWindow::ensureSize()
 
 void QWaylandWindow::initWindow()
 {
+    /**
+     * Cleanup window state just before showing.
+     * This is necessary because a render could still have been running and commit
+     * after the window was last hidden and the last null was attached
+     *
+     * When we port to synchronous delivery it should be possible to drop this
+     */
+    mSurface->attach(nullptr, 0, 0);
+    mSurface->commit();
+
     if (window()->type() == Qt::Desktop)
         return;
 
-    if (!mSurface) {
-        initializeWlSurface();
-    }
-
-    if (mDisplay->fractionalScaleManager() && qApp->highDpiScaleFactorRoundingPolicy() == Qt::HighDpiScaleFactorRoundingPolicy::PassThrough) {
-        mFractionalScale.reset(new QWaylandFractionalScale(mDisplay->fractionalScaleManager()->get_fractional_scale(mSurface->object())));
-
-        connect(mFractionalScale.data(), &QWaylandFractionalScale::preferredScaleChanged,
-                this, &QWaylandWindow::updateScale);
-    }
-
     if (shouldCreateSubSurface()) {
         Q_ASSERT(!mSubSurfaceWindow);
 
@@ -189,12 +186,6 @@ void QWaylandWindow::initWindow()
         }
     }
 
-    // The fractional scale manager check is needed to work around Gnome < 36 where viewports don't work
-    // Right now viewports are only necessary when a fractional scale manager is used
-    if (display()->viewporter() && display()->fractionalScaleManager()) {
-        mViewport.reset(new QWaylandViewport(display()->createViewport(this)));
-    }
-
     // Enable high-dpi rendering. Scale() returns the screen scale factor and will
     // typically be integer 1 (normal-dpi) or 2 (high-dpi). Call set_buffer_scale()
     // to inform the compositor that high-resolution buffers will be provided.
@@ -236,6 +227,18 @@ void QWaylandWindow::initializeWlSurface()
         mSurface->m_window = this;
     }
     emit wlSurfaceCreated();
+
+    if (mDisplay->fractionalScaleManager() && qApp->highDpiScaleFactorRoundingPolicy() == Qt::HighDpiScaleFactorRoundingPolicy::PassThrough) {
+        mFractionalScale.reset(new QWaylandFractionalScale(mDisplay->fractionalScaleManager()->get_fractional_scale(mSurface->object())));
+
+        connect(mFractionalScale.data(), &QWaylandFractionalScale::preferredScaleChanged,
+                this, &QWaylandWindow::updateScale);
+    }
+    // The fractional scale manager check is needed to work around Gnome < 36 where viewports don't work
+    // Right now viewports are only necessary when a fractional scale manager is used
+    if (display()->viewporter() && display()->fractionalScaleManager()) {
+        mViewport.reset(new QWaylandViewport(display()->createViewport(this)));
+    }
 }
 
 void QWaylandWindow::setShellIntegration(QWaylandShellIntegration *shellIntegration)
@@ -282,23 +285,12 @@ void QWaylandWindow::endFrame()
 
 void QWaylandWindow::reset()
 {
-    closeChildPopups();
-
-    if (mTopPopup == this)
-        mTopPopup = mTransientParent && (mTransientParent->window()->type() == Qt::Popup) ? mTransientParent : nullptr;
+    resetSurfaceRole();
 
     if (mSurface) {
         {
             QWriteLocker lock(&mSurfaceLock);
             invalidateSurface();
-            if (mTransientParent)
-                mTransientParent->removeChildPopup(this);
-            delete mShellSurface;
-            mShellSurface = nullptr;
-            emit surfaceRoleDestroyed();
-            delete mSubSurfaceWindow;
-            mSubSurfaceWindow = nullptr;
-            mTransientParent = nullptr;
             mSurface.reset();
             mViewport.reset();
             mFractionalScale.reset();
@@ -306,27 +298,8 @@ void QWaylandWindow::reset()
         emit wlSurfaceDestroyed();
     }
 
-    {
-        QMutexLocker lock(&mFrameSyncMutex);
-        if (mFrameCallback) {
-            wl_callback_destroy(mFrameCallback);
-            mFrameCallback = nullptr;
-        }
 
-        mFrameCallbackElapsedTimer.invalidate();
-        mWaitingForFrameCallback = false;
-    }
-    if (mFrameCallbackCheckIntervalTimerId != -1) {
-        killTimer(mFrameCallbackCheckIntervalTimerId);
-        mFrameCallbackCheckIntervalTimerId = -1;
-    }
-
-    mFrameCallbackTimedOut = false;
-    mWaitingToApplyConfigure = false;
-    mCanResize = true;
-    mResizeDirty = false;
     mScale = std::nullopt;
-    mExposed = false;
     mOpaqueArea = QRegion();
     mMask = QRegion();
 
@@ -336,6 +309,39 @@ void QWaylandWindow::reset()
     mDisplay->handleWindowDestroyed(this);
 }
 
+void QWaylandWindow::resetSurfaceRole()
+{
+     // Old Reset
+    closeChildPopups();
+
+    if (mTopPopup == this)
+        mTopPopup = mTransientParent && (mTransientParent->window()->type() == Qt::Popup) ? mTransientParent : nullptr;
+    if (mTransientParent)
+        mTransientParent->removeChildPopup(this);
+    mTransientParent = nullptr;
+    delete std::exchange(mShellSurface, nullptr);
+    delete std::exchange(mSubSurfaceWindow, nullptr);
+    emit surfaceRoleDestroyed();
+    {
+        QMutexLocker lock(&mFrameSyncMutex);
+        if (mFrameCallback) {
+            wl_callback_destroy(mFrameCallback);
+            mFrameCallback = nullptr;
+        }
+        mFrameCallbackElapsedTimer.invalidate();
+        mWaitingForFrameCallback = false;
+    }
+    if (mFrameCallbackCheckIntervalTimerId != -1) {
+        killTimer(mFrameCallbackCheckIntervalTimerId);
+        mFrameCallbackCheckIntervalTimerId = -1;
+    }
+    mFrameCallbackTimedOut = false;
+    mWaitingToApplyConfigure = false;
+    mCanResize = true;
+    mResizeDirty = false;
+    mExposed = false;
+}
+
 QWaylandWindow *QWaylandWindow::fromWlSurface(::wl_surface *surface)
 {
     if (auto *s = QWaylandSurface::fromWlSurface(surface))
@@ -584,7 +590,9 @@ void QWaylandWindow::setVisible(bool visible)
         // make sure isExposed is false during the next event dispatch
         mExposed = false;
         sendExposeEvent(QRect());
-        reset();
+        resetSurfaceRole();
+        mSurface->attach(nullptr, 0, 0);
+        mSurface->commit();
     }
 }
 
@@ -1826,12 +1834,13 @@ void QWaylandWindow::removeChildPopup(QWaylandWindow *child)
 void QWaylandWindow::closeChildPopups() {
     while (!mChildPopups.isEmpty()) {
         auto popup = mChildPopups.takeLast();
-        popup->reset();
+        popup->resetSurfaceRole();
     }
 }
 
 void QWaylandWindow::reinit()
 {
+    initializeWlSurface();
     if (window()->isVisible()) {
         initWindow();
         if (hasPendingUpdateRequest())

src/plugins/platforms/wayland/qwaylandwindow_p.h

@@ -354,6 +354,7 @@ private:
     void initializeWlSurface();
     bool shouldCreateShellSurface() const;
     bool shouldCreateSubSurface() const;
+    void resetSurfaceRole();
     QPlatformScreen *calculateScreenFromSurfaceEvents() const;
     void setOpaqueArea(const QRegion &opaqueArea);
     bool isOpaque() const;

tests/auto/wayland/shared/coreprotocol.cpp

@@ -50,6 +50,11 @@ void Surface::map()
     m_mapped = true;
 }
 
+void Surface::unmap()
+{
+    m_mapped = false;
+}
+
 void Surface::surface_destroy_resource(Resource *resource)
 {
     Q_UNUSED(resource);
@@ -73,8 +78,10 @@ void Surface::surface_attach(Resource *resource, wl_resource *buffer, int32_t x,
     Q_UNUSED(resource);
     if (m_wlshell) {
         m_buffer = buffer;
-        if (!buffer)
+        if (!buffer) {
             m_image = QImage();
+            unmap();
+        }
     } else {
         QPoint offset(x, y);
         if (resource->version() < 5)
@@ -155,8 +162,6 @@ void Surface::surface_offset(Resource *resource, int32_t x, int32_t y)
 bool WlCompositor::isClean() {
     for (auto *surface : std::as_const(m_surfaces)) {
         if (!CursorRole::fromSurface(surface)) {
-            if (m_compositor->m_type != CoreCompositor::CompositorType::Legacy)
-                return false;
             if (surface->isMapped())
                 return false;
         }
@@ -639,8 +644,10 @@ WlShellSurface::WlShellSurface(WlShell *wlShell, wl_client *client, int id, Surf
 
 WlShellSurface::~WlShellSurface()
 {
-    if (m_surface)
+    if (m_surface) {
+        m_surface->unmap();
         m_surface->m_wlShellSurface = nullptr;
+    }
 }
 
 void WlShellSurface::sendConfigure(uint32_t edges, int32_t width, int32_t height)

tests/auto/wayland/shared/coreprotocol.h

@@ -84,6 +84,7 @@ public:
     void send_leave(::wl_resource *output) = delete;
 
     void map();
+    void unmap();
     bool isMapped() const { return m_mapped; }
     WlShellSurface *wlShellSurface() const { return m_wlShellSurface; }
 

tests/auto/wayland/shared/mockcompositor.cpp

@@ -40,7 +40,7 @@ DefaultCompositor::DefaultCompositor(CompositorType t, int socketFd)
 
         QObject::connect(get<WlCompositor>(), &WlCompositor::surfaceCreated, [this] (Surface *surface){
             QObject::connect(surface, &Surface::bufferCommitted, [this, surface] {
-                if (m_config.autoRelease) {
+                if (m_config.autoRelease && surface->m_committed.buffer) {
                     // Pretend we made a copy of the buffer and just release it immediately
                     surface->m_committed.buffer->send_release();
                 }

tests/auto/wayland/shared/xdgshell.cpp

@@ -59,7 +59,10 @@ XdgSurface::XdgSurface(XdgWmBase *xdgWmBase, Surface *surface, wl_client *client
     QVERIFY(!surface->m_pending.buffer);
     QVERIFY(!surface->m_committed.buffer);
     connect(this, &XdgSurface::toplevelCreated, xdgWmBase, &XdgWmBase::toplevelCreated, Qt::DirectConnection);
-    connect(surface, &Surface::attach, this, &XdgSurface::verifyConfigured);
+    connect(surface, &Surface::attach, this, [this]  (void *buffer) {
+        if (buffer)
+            verifyConfigured();
+    });
     connect(surface, &Surface::commit, this, [this] {
         m_committed = m_pending;
 
@@ -68,6 +71,13 @@ XdgSurface::XdgSurface(XdgWmBase *xdgWmBase, Surface *surface, wl_client *client
             emit configureCommitted(m_committedConfigureSerial);
         }
     });
+    connect(surface, &Surface::bufferCommitted, this, [this] {
+        if (m_surface->m_committed.buffer && (m_toplevel || m_popup)) {
+            m_surface->map();
+        } else {
+            m_surface->unmap();
+        }
+    });
 }
 
 void XdgSurface::sendConfigure(uint serial)