canonical / wlcs

Wayland Conformance Test Suite
GNU General Public License v2.0
50 stars 14 forks source link

ConfigurationWindow constructor is racy #318

Closed bwidawsk closed 12 months ago

bwidawsk commented 12 months ago

ConfigurationWindow waits for configure to be sent by the display server. However, the check/wait is only initialized after the client roundtrip. Configure may get sent immediately after roundtip but defore the dispatch_until, and if it does the check will never pass.

Proposed fix:

diff --git a/tests/xdg_toplevel_stable.cpp b/tests/xdg_toplevel_stable.cpp
index 676fc6b482a7..6535cbc75ff9 100644
--- a/tests/xdg_toplevel_stable.cpp
+++ b/tests/xdg_toplevel_stable.cpp
@@ -49,6 +49,7 @@ public:
           xdg_shell_surface{client, surface},
           toplevel{xdg_shell_surface}
     {
+       int prev = surface_configure_count;
         ON_CALL(xdg_shell_surface, configure).WillByDefault([&](auto serial)
             {
                 xdg_surface_ack_configure(xdg_shell_surface, serial);
@@ -64,16 +65,21 @@ public:
         client.roundtrip();
         surface.attach_buffer(window_width, window_height);
         wl_surface_commit(surface);
-        dispatch_until_configure();
+        __dispatch_until_configure(prev);
+    }
+
+    void __dispatch_until_configure(int prev)
+    {
+        client.dispatch_until(
+            [prev_count = prev, &current_count = surface_configure_count]()
+            {
+                return current_count > prev_count;
+            });
     }

     void dispatch_until_configure()
     {
-        client.dispatch_until(
-            [prev_count = surface_configure_count, &current_count = surface_configure_count]()
-            {
-                return current_count > prev_count;
-            });
+       __dispatch_until_configure(surface_configure_count);
     }
RAOF commented 12 months ago

I've slightly reworked that fix for aesthetic preference; does the linked PR work for you?

bwidawsk commented 12 months ago

That fix works for me, though I haven't verified if it causes any other regressions

RAOF commented 12 months ago

Hm, actually, I think the problem you're hitting is that we expect a second configure after the buffer is committed that sets the activated state. I guess we assume that prior to being mapped the surface is not in the activated state.

RAOF commented 12 months ago

I've updated that PR again; Mir passes this, and it matches the behaviour I see under GNOME Shell, but it's maybe not exactly behaviour mandated by the protocol.

bwidawsk commented 12 months ago

The patch that ended up getting merged doesn't work for my compositor :(