From 52c9dbb487f4ac98017b83a1deee912546cbff5f Mon Sep 17 00:00:00 2001
From: Vlad Zahorodnii <vlad.zahorodnii@kde.org>
Date: Wed, 29 Sep 2021 11:28:47 +0300
Subject: [PATCH] wayland: Reset Toplevel::surfaceId after surface is created

Xwayland will re-create the wl_surface object if the X11 window is
unmapped and mapped. That, and the fact that the order in which the
WL_SURFACE_ID client message event is received and the wl_surface object
is created is undefined can cause the following bug:

* WL_SURFACE_ID is received
* the old wl_surface object is destroyed, m_surfaceId is reset to 0
* new wl_surface is created but because m_surfaceId is 0, it won't be
  associated with the x11 window

This change ensures that kwin will associate the wl_surface with x11
window by making it not reset cached surface id when the old wl_surface
is destroyed.

However, we cannot leave m_surfaceId as is because wayland aggressively
re-uses object ids so kwin can associate wrong surface with x11 window.

To prevent that, this change also makes Toplevel::setSurface() reset
cached surface id.

CCBUG: 442936
CCBUG: 426069
---
 src/events.cpp         |  5 ++---
 src/toplevel.cpp       |  3 +--
 src/toplevel.h         | 19 ++++---------------
 src/wayland_server.cpp |  4 ++--
 4 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/src/events.cpp b/src/events.cpp
index 5ab1887eee..bb88e55035 100644
--- a/src/events.cpp
+++ b/src/events.cpp
@@ -1285,13 +1285,12 @@ void Toplevel::propertyNotifyEvent(xcb_property_notify_event_t *e)
 void Toplevel::clientMessageEvent(xcb_client_message_event_t *e)
 {
     if (e->type == atoms->wl_surface_id) {
-        m_surfaceId = e->data.data32[0];
+        m_pendingSurfaceId = e->data.data32[0];
         if (auto w = waylandServer()) {
-            if (auto s = KWaylandServer::SurfaceInterface::get(m_surfaceId, w->xWaylandConnection())) {
+            if (auto s = KWaylandServer::SurfaceInterface::get(m_pendingSurfaceId, w->xWaylandConnection())) {
                 setSurface(s);
             }
         }
-        Q_EMIT surfaceIdChanged(m_surfaceId);
     }
 }
 
diff --git a/src/toplevel.cpp b/src/toplevel.cpp
index 562919928d..091f1322a9 100644
--- a/src/toplevel.cpp
+++ b/src/toplevel.cpp
@@ -615,11 +615,10 @@ void Toplevel::setSurface(KWaylandServer::SurfaceInterface *surface)
         return;
     }
     m_surface = surface;
+    m_pendingSurfaceId = 0;
     connect(m_surface, &KWaylandServer::SurfaceInterface::destroyed, this, [this]() {
         m_surface = nullptr;
-        m_surfaceId = 0;
     });
-    m_surfaceId = surface->id();
     Q_EMIT surfaceChanged();
 }
 
diff --git a/src/toplevel.h b/src/toplevel.h
index 467f9d61d9..122e92014b 100644
--- a/src/toplevel.h
+++ b/src/toplevel.h
@@ -255,12 +255,6 @@ class KWIN_EXPORT Toplevel : public QObject
      */
     Q_PROPERTY(bool skipsCloseAnimation READ skipsCloseAnimation WRITE setSkipCloseAnimation NOTIFY skipCloseAnimationChanged)
 
-    /**
-     * The Id of the Wayland Surface associated with this Toplevel.
-     * On X11 only setups the value is @c 0.
-     */
-    Q_PROPERTY(quint32 surfaceId READ surfaceId NOTIFY surfaceIdChanged)
-
     /**
      * Interface to the Wayland Surface.
      * Relevant only in Wayland, in X11 it will be nullptr
@@ -494,7 +488,7 @@ public:
     bool skipsCloseAnimation() const;
     void setSkipCloseAnimation(bool set);
 
-    quint32 surfaceId() const;
+    quint32 pendingSurfaceId() const;
     KWaylandServer::SurfaceInterface *surface() const;
     void setSurface(KWaylandServer::SurfaceInterface *surface);
 
@@ -615,11 +609,6 @@ Q_SIGNALS:
      * @since 5.0
      */
     void windowClassChanged();
-    /**
-     * Emitted when a Wayland Surface gets associated with this Toplevel.
-     * @since 5.3
-     */
-    void surfaceIdChanged(quint32);
     /**
      * @since 5.4
      */
@@ -728,7 +717,7 @@ private:
     mutable bool m_shapeRegionIsValid = false;
     AbstractOutput *m_output = nullptr;
     bool m_skipCloseAnimation;
-    quint32 m_surfaceId = 0;
+    quint32 m_pendingSurfaceId = 0;
     KWaylandServer::SurfaceInterface *m_surface = nullptr;
     // when adding new data members, check also copyToDeleted()
     qreal m_screenScale = 1.0;
@@ -969,9 +958,9 @@ inline const ClientMachine *Toplevel::clientMachine() const
     return m_clientMachine;
 }
 
-inline quint32 Toplevel::surfaceId() const
+inline quint32 Toplevel::pendingSurfaceId() const
 {
-    return m_surfaceId;
+    return m_pendingSurfaceId;
 }
 
 inline KWaylandServer::SurfaceInterface *Toplevel::surface() const
diff --git a/src/wayland_server.cpp b/src/wayland_server.cpp
index e6559ad0b2..65dc1f9541 100644
--- a/src/wayland_server.cpp
+++ b/src/wayland_server.cpp
@@ -403,7 +403,7 @@ bool WaylandServer::init(InitializationFlags flags)
             }
 
             X11Client *client = ws->findClient([surface](const X11Client *client) {
-                return client->surfaceId() == surface->id();
+                return client->pendingSurfaceId() == surface->id();
             });
             if (client) {
                 client->setSurface(surface);
@@ -411,7 +411,7 @@ bool WaylandServer::init(InitializationFlags flags)
             }
 
             Unmanaged *unmanaged = ws->findUnmanaged([surface](const Unmanaged *unmanaged) {
-                return unmanaged->surfaceId() == surface->id();
+                return unmanaged->pendingSurfaceId() == surface->id();
             });
             if (unmanaged) {
                 unmanaged->setSurface(surface);