GNOME Bugzilla – Bug 770906
Wayland: Unmapping a toplevel from a menu/popup can lead to a protocol error
Last modified: 2016-09-12 08:06:49 UTC
Created attachment 334828 [details] WAYLAND_DEBUG log Description: 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.
Note, I thought I had a patch, but it needs more work...
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.
Could someone please make a review of this patch?
Jonas, can you take a look ?
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).
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.
(still better than killing the client with a protocol error...)
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 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