After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 770906 - Wayland: Unmapping a toplevel from a menu/popup can lead to a protocol error
Wayland: Unmapping a toplevel from a menu/popup can lead to a protocol error
Product: gtk+
Classification: Platform
Component: Backend: Wayland
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
Depends on:
Reported: 2016-09-05 15:52 UTC by Olivier Fourdan
Modified: 2016-09-12 08:06 UTC
See Also:
GNOME target: ---
GNOME version: ---

WAYLAND_DEBUG log (32.42 KB, text/plain)
2016-09-05 15:52 UTC, Olivier Fourdan
[PATCH] wayland: unmap popup along with its toplevel (1.90 KB, patch)
2016-09-06 07:45 UTC, Olivier Fourdan
none Details | Review
[PATCH] wayland: unmap popup along with its toplevel (2.34 KB, patch)
2016-09-12 07:29 UTC, Olivier Fourdan
committed Details | Review

Description Olivier Fourdan 2016-09-05 15:52:39 UTC
Created attachment 334828 [details]


devassistant-gui uses a popup window to select the type of project, and when selected unmaps the toplevel window to map a new one.

Problem is sometimes, this leads to a protocol error "destroyed popup not top most popup" (WAYLAND_DEBUG logs attached)

I don't think this is the same as bug 768111 as I don't see more than popup being created.

Looking at mutter code (and instrumenting the code, as there are two cases to generate that same error "destroyed popup not top most popup" in mutter) show that this coming from handle_popup_parent_destroyed() in mutter.

So I think what happens is the callback is hiding the toplevel before the popup is unmapped, which causes this.
Comment 1 Olivier Fourdan 2016-09-05 18:49:45 UTC
Note, I thought I had a patch, but it needs more work...
Comment 2 Olivier Fourdan 2016-09-06 07:45:12 UTC
Created attachment 334878 [details] [review]
[PATCH] wayland: unmap popup along with its toplevel

If an application umaps the toplevel from its popup callback, this can
lead to a protocol error.

Make sure we mark popup parent and use that to check if their parent is
the toplevel being unmapped in which case we shall unmap the popup first
to avoid the protocol error.
Comment 3 Olivier Fourdan 2016-09-09 13:33:57 UTC
Could someone please make a review of this patch?
Comment 4 Matthias Clasen 2016-09-09 20:59:47 UTC
Jonas, can you take a look ?
Comment 5 Jonas Ådahl 2016-09-12 04:18:08 UTC
Review of attachment 334878 [details] [review]:

::: gdk/wayland/gdkwindow-wayland.c
@@ +2350,3 @@
+          gdk_window_hide (popup);
+        }
+    }

It looks like this would only work if there is one popup and the parent is unmapped. If there are two popups chained, g_list_last(current_popups) will be the top most one. The parent of the top most one will be the first (not topmost) popup, thus we'd still hide the toplevel.

I think what you need to do here is loop through current_popups, and compare each popup_parent until it matches 'window' and hide that (which will do the same thing recursively until the top most is hidden).
Comment 6 Olivier Fourdan 2016-09-12 07:29:08 UTC
Created attachment 335336 [details] [review]
[PATCH] wayland: unmap popup along with its toplevel

Updated patch.

Note, I just found out, this confuses mutter/gnome-shell stacking, the "next" window mapped from the application is placed beneath the previously focused window, but that's probably something to follow up on the mutter side.
Comment 7 Olivier Fourdan 2016-09-12 07:29:36 UTC
(still better than killing the client with a protocol error...)
Comment 8 Jonas Ådahl 2016-09-12 07:42:07 UTC
Review of attachment 335336 [details] [review]:

Looks good; just only one nit below.

::: gdk/wayland/gdkwindow-wayland.c
@@ +2349,3 @@
+         }
+       current_popups = g_list_next (current_popups);
+    }

Just a nit: this is an iterator of a GList. Those usually look like
GList *l;

for (l = ... ; l; l = l->next)

so it'd be clearer we just used the same method here as elsewhere. Also no need to pass the 'current_popups' as a parameter really, as that function will never be called with anything else.
Comment 9 Olivier Fourdan 2016-09-12 08:06:16 UTC
Comment on attachment 335336 [details] [review]
[PATCH] wayland: unmap popup along with its toplevel

attachment 335336 [details] [review] pushed in git master with changes from comment 8 as commit eb17ee1 - wayland: unmap popup along with its toplevel