GNOME Bugzilla – Bug 790358
A few xdg-shell popup fixes
Last modified: 2017-11-30 03:03:12 UTC
When debugging a GTK+ issue related to popups, I at one point ran into the following crash: orig=orig@entry=0x7ffd85150ad0, new=new@entry=0x7ffd85150af0) at core/constraints.c:299 frame_rect=...) at core/window.c:3944 at core/window.c:4100 data@entry=0xcbe60a0) at src/connection.c:935 I have not been able to reproduce it since. What seems to have happened is that when a popup was mapped, the parent had already been unmapped. Some investigation results: * There seems to be two popups and one toplevel, with the relationship: toplevel <- popup A <- popup B * Both popup A and B are "grabbing" popups, and they grab using the same serial (225313) * The seat pointer's grab serial is still 225313 * We're crashing when trying to map popup B, because popup A has already been dismissed. One suspicion of what might have happened is: 1) A client mapped popup A and immediately after popup B on top of popup A in response to a single event (reasoning: same serial) 2) The compositor processes the mapping of popup A -> mapped (window created) (passed serial check) 3) The compositor dismisses the popup A (without changing pointer focus) (window destroyed) 4) The compositor processes the mapping of popup B -> map allowed (passes serial check) 5) The compositor tries to calculate the position of popup B, but popup A is unmapped, causing NULL pointer dereference
Created attachment 363660 [details] [review] wayland/xdg-shell: Fix top-most check when grabbing Move the top-most-popup correctness check to the finish_popup_setup() function after checking the serial. If we pass the serial check, we should have reached a state that if there are any popups they should be the one from the same client. Also avoid failing a client that correctly set the top-most popup at map time, but where the at the time of processing the top most popup have already been dismissed by the compositor for some arbitrary reason.
Created attachment 363661 [details] [review] wayland/xdg-shell: Check popup parent type when assigning We only allow mapping popups on top of surfaces with a xdg_surface based role. Add a check and fail clients that doesn't follow this rule.
Created attachment 363662 [details] [review] wayland/xdg-shell: Dismiss a popup on map if parent already dismissed If a parent doesn't have a window, it means it could have been dismissed (for example due to a input serial race), but the more recent popup might win the input serial race and try to map anyway. This would result in a crash later on when trying to process the placement rule, as the parent already has no window.
Review of attachment 363660 [details] [review]: Nit: "but where the at the time of processing the top most popup have already been dismissed by the compositor" There's either a word missing after the first "the", or the phrase should be something like "but where at the time of processing ... has already been dismissed" ::: src/wayland/meta-wayland-xdg-shell.c @@ -463,3 @@ - if ((top_popup == NULL && - !META_IS_WAYLAND_XDG_SURFACE (parent_surface->role)) || - (top_popup != NULL && parent_surface != top_popup)) Just for clarification - the first condition that is removed here is replaced by the check in the next patch, right?
Review of attachment 363661 [details] [review]: Nit: "fail clients that *don't* follow this rule" (also I would pronounce xdg-surface as "eggs-dee-gee surface", in which case it's "an xdg-surface", but I'm not sure that pronunciation is correct)
Review of attachment 363662 [details] [review]: Makes sense to me
(In reply to Florian Müllner from comment #4) > Review of attachment 363660 [details] [review] [review]: > > Nit: > "but where the at the time of processing the top most popup > have already been dismissed by the compositor" > > There's either a word missing after the first "the", or the phrase should be > something like "but where at the time of processing ... has already been > dismissed" Yea, that sentence was broken, it should be something like the second option you wrote. > > ::: src/wayland/meta-wayland-xdg-shell.c > @@ -463,3 @@ > - if ((top_popup == NULL && > - !META_IS_WAYLAND_XDG_SURFACE (parent_surface->role)) || > - (top_popup != NULL && parent_surface != top_popup)) > > Just for clarification - the first condition that is removed here is > replaced by the check in the next patch, right? Not exactly. The check *should* have been _IS_WAYLAND_XDG_TOPLEVEL() as that's the only valid parent type if there is no top-popup. ... except that some other client could have mapped a popup and this client is just too late, which made the check wrong all together. (In reply to Florian Müllner from comment #5) > Review of attachment 363661 [details] [review] [review]: > > Nit: "fail clients that *don't* follow this rule" > > (also I would pronounce xdg-surface as "eggs-dee-gee surface", in which case > it's "an xdg-surface", but I'm not sure that pronunciation is correct) I pronounce it as "ecks-dee-gee surface", which also would be "an". I'll fix the message.
Attachment 363660 [details] pushed as 5d3b4f0 - wayland/xdg-shell: Fix top-most check when grabbing Attachment 363661 [details] pushed as c533a06 - wayland/xdg-shell: Check popup parent type when assigning Attachment 363662 [details] pushed as 4508978 - wayland/xdg-shell: Dismiss a popup on map if parent already dismissed