<div dir="ltr">Yeah, the logic is pretty sketchy now - "if it's a shell surface, do the error checking; otherwise, do nothing" - but I don't understand the code well enough to know if this is the expected behavior.<div> </div><div>Should the panel just be a shell surface? Then we could require that the popup's parent is a shell surface and simplify the error check.</div></div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Apr 6, 2015 at 12:33 PM, Derek Foreman <<a href="mailto:derekf@osg.samsung.com" target="_blank">derekf@osg.samsung.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This bug was introduced in commits da6ecd0cc52 and 24185e2561 I guess nobody right clicked on the panel for over a month. :) I've CC'd Jasper and Jonas in case they haven't noticed this yet... On 06/04/15 01:52 AM, Dima Ryazanov wrote: > It looks like the error-checking code assumes the popup's parent is > a shell surface - but that's not always the case. > > Signed-off-by: Dima Ryazanov <<a href="mailto:dima@gmail.com">dima@gmail.com</a>> > --- > desktop-shell/shell.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c > index f7c928e..0159547 100644 > --- a/desktop-shell/shell.c > +++ b/desktop-shell/shell.c > @@ -3392,7 +3392,7 @@ add_popup_grab(struct shell_surface *shsurf, > > parent = get_shell_surface(shsurf->parent); > top_surface = get_top_popup(shseat); > - if (shell_surface_is_xdg_popup(shsurf) && > + if (parent && shell_surface_is_xdg_popup(shsurf) && This part broke in da6ecd0cc52 Your patch definitely stops the crash for me, which is good. :) I think this part's ok, but I'm not 100% sure we're still going to send the new not-top-level-popup error when we're supposed to now... > ((top_surface == NULL && !shell_surface_is_xdg_surface(parent)) || > (top_surface != NULL && parent != top_surface))) { > wl_resource_post_error(shsurf->owner->resource, > @@ -4098,12 +4098,13 @@ create_xdg_popup(struct shell_client *owner, void *shell, > { > struct shell_surface *shsurf, *parent_shsurf; > > - /* Verify that we are creating the top most popup when mapping, > - * as its not until then we know whether it was mapped as most > + /* Verify that we are creating the topmost popup when mapping, > + * as it's not until then we know whether it was mapped as most > * top level or not. */ The grammar fix-ups look good to me. > parent_shsurf = get_shell_surface(parent); > - if (!shell_surface_is_xdg_popup(parent_shsurf) && > + if (parent_shsurf && This part broke in 24185e2561... I wonder if we should be doing a more comprehensive test here - it looks like some invalid parent resources could get past this test? <div class="HOEnZb"><div class="h5"> > + !shell_surface_is_xdg_popup(parent_shsurf) && > !shell_surface_is_xdg_surface(parent_shsurf)) { > wl_resource_post_error(owner->resource, > XDG_POPUP_ERROR_INVALID_PARENT, > </div></div></blockquote></div> </div>