Compositor xdg-decoration: Fix crash when clients disconnect

If clients were disconnected without shutting down nicely, e.g. if Qt clients
were terminated with SIGINT, then QWaylandXdgToplevelDecorationV1 would be
deleted before its Wayland resource, causing a crash in the resource destroy
handler.

This switches to a raw pointer in the toplevel, which is now cleared when the
decoration is deleted. Deletion order is also ensured by conditionally destroying
the resource in the toplevel destructor.

Warnings are also printed when clients destroy objects in the wrong order
(protocol errors).

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

5 files changed, 23 insertions, 4 deletions

diff --git a/src/compositor/extensions/qwaylandxdgdecorationv1.cpp b/src/compositor/extensions/qwaylandxdgdecorationv1.cpp
index cea8cfe18..6ece73b8e 100644
--- a/src/compositor/extensions/qwaylandxdgdecorationv1.cpp
+++ b/src/compositor/extensions/qwaylandxdgdecorationv1.cpp
@@ -192,10 +192,15 @@ QWaylandXdgToplevelDecorationV1::QWaylandXdgToplevelDecorationV1(QWaylandXdgTopl
     Q_ASSERT(toplevel);
     auto *toplevelPrivate = QWaylandXdgToplevelPrivate::get(toplevel);
     Q_ASSERT(!toplevelPrivate->m_decoration);
-    toplevelPrivate->m_decoration.reset(this);
+    toplevelPrivate->m_decoration = this;
     sendConfigure(manager->preferredMode());
 }
 
+QWaylandXdgToplevelDecorationV1::~QWaylandXdgToplevelDecorationV1()
+{
+    QWaylandXdgToplevelPrivate::get(m_toplevel)->m_decoration = nullptr;
+}
+
 void QWaylandXdgToplevelDecorationV1::sendConfigure(QWaylandXdgToplevelDecorationV1::DecorationMode mode)
 {
     if (configuredMode() == mode)
@@ -220,8 +225,7 @@ void QWaylandXdgToplevelDecorationV1::sendConfigure(QWaylandXdgToplevelDecoratio
 void QWaylandXdgToplevelDecorationV1::zxdg_toplevel_decoration_v1_destroy_resource(Resource *resource)
 {
     Q_UNUSED(resource);
-    auto *toplevelPrivate = QWaylandXdgToplevelPrivate::get(m_toplevel);
-    toplevelPrivate->m_decoration.reset();
+    delete this;
 }
 
 void QWaylandXdgToplevelDecorationV1::zxdg_toplevel_decoration_v1_destroy(Resource *resource)
diff --git a/src/compositor/extensions/qwaylandxdgdecorationv1_p.h b/src/compositor/extensions/qwaylandxdgdecorationv1_p.h
index 4c6c46334..3d181a919 100644
--- a/src/compositor/extensions/qwaylandxdgdecorationv1_p.h
+++ b/src/compositor/extensions/qwaylandxdgdecorationv1_p.h
@@ -83,6 +83,7 @@ public:
     explicit QWaylandXdgToplevelDecorationV1(QWaylandXdgToplevel *toplevel,
                                              QWaylandXdgDecorationManagerV1 *manager,
                                              wl_client *client, int id);
+    ~QWaylandXdgToplevelDecorationV1() override;
 
     DecorationMode configuredMode() const { return m_configuredMode; }
     void sendConfigure(DecorationMode mode);
diff --git a/src/compositor/extensions/qwaylandxdgshell.cpp b/src/compositor/extensions/qwaylandxdgshell.cpp
index eaa443e87..ce6add3de 100644
--- a/src/compositor/extensions/qwaylandxdgshell.cpp
+++ b/src/compositor/extensions/qwaylandxdgshell.cpp
@@ -731,6 +731,16 @@ QWaylandXdgToplevel::QWaylandXdgToplevel(QWaylandXdgSurface *xdgSurface, QWaylan
     sendConfigure({0, 0}, states);
 }
 
+QWaylandXdgToplevel::~QWaylandXdgToplevel()
+{
+    Q_D(QWaylandXdgToplevel);
+    // Usually, the decoration is destroyed by the client (according to the protocol),
+    // but if the client misbehaves, or is shut down, we need to clean up here.
+    if (Q_UNLIKELY(d->m_decoration))
+        wl_resource_destroy(d->m_decoration->resource()->handle);
+    Q_ASSERT(!d->m_decoration);
+}
+
 /*!
  * \qmlproperty XdgToplevel QtWaylandCompositor::XdgToplevel::parentToplevel
  *
@@ -1331,6 +1341,9 @@ void QWaylandXdgToplevelPrivate::xdg_toplevel_destroy_resource(QtWaylandServer::
 
 void QWaylandXdgToplevelPrivate::xdg_toplevel_destroy(QtWaylandServer::xdg_toplevel::Resource *resource)
 {
+    if (Q_UNLIKELY(m_decoration))
+        qWarning() << "Client error: xdg_toplevel destroyed before its decoration object";
+
     wl_resource_destroy(resource->handle);
     //TODO: Should the xdg surface be desroyed as well? Or is it allowed to recreate a new toplevel for it?
 }
diff --git a/src/compositor/extensions/qwaylandxdgshell.h b/src/compositor/extensions/qwaylandxdgshell.h
index 2b6f63564..aec6193a7 100644
--- a/src/compositor/extensions/qwaylandxdgshell.h
+++ b/src/compositor/extensions/qwaylandxdgshell.h
@@ -170,6 +170,7 @@ public:
     Q_ENUM(DecorationMode)
 
     explicit QWaylandXdgToplevel(QWaylandXdgSurface *xdgSurface, QWaylandResource &resource);
+    ~QWaylandXdgToplevel() override;
 
     QWaylandXdgToplevel *parentToplevel() const;
 
diff --git a/src/compositor/extensions/qwaylandxdgshell_p.h b/src/compositor/extensions/qwaylandxdgshell_p.h
index e87bb17f0..5ef8bde3d 100644
--- a/src/compositor/extensions/qwaylandxdgshell_p.h
+++ b/src/compositor/extensions/qwaylandxdgshell_p.h
@@ -178,7 +178,7 @@ public:
     QString m_appId;
     QSize m_maxSize;
     QSize m_minSize = {0, 0};
-    QScopedPointer<QWaylandXdgToplevelDecorationV1> m_decoration;
+    QWaylandXdgToplevelDecorationV1 *m_decoration = nullptr;
 
     static QWaylandSurfaceRole s_role;
 };