Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ConfigurationWindow constructor is racy #318

Closed
bwidawsk opened this issue Nov 29, 2023 · 5 comments · Fixed by #319 or #321
Closed

ConfigurationWindow constructor is racy #318

bwidawsk opened this issue Nov 29, 2023 · 5 comments · Fixed by #319 or #321
Assignees

Comments

@bwidawsk
Copy link

bwidawsk commented Nov 29, 2023

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
Copy link
Contributor

RAOF commented Nov 30, 2023

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

@bwidawsk
Copy link
Author

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

@RAOF
Copy link
Contributor

RAOF commented Dec 1, 2023

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 added a commit that referenced this issue Dec 1, 2023
@RAOF
Copy link
Contributor

RAOF commented Dec 1, 2023

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
Copy link
Author

bwidawsk commented Dec 3, 2023

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
2 participants