[PATCH weston] desktop-shell: Fix a crash when right-clicking the panel

[Dima Ryazanov]

Sure, I'll remove it then. (I was going to remove it originally - but
figured, it was useful for testing since it exposed this bug.)

On Wed, Apr 8, 2015 at 7:00 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Mon, 6 Apr 2015 13:27:23 -0700
> Dima Ryazanov <dima at gmail.com> wrote:
>
> > 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.
> >
> > 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.
>
> No, that won't work. "panel" is a special surface role, it cannot also
> be a shell_surface.
>
> I suppose the answer to this whole problem is to remove the whole panel
> popup thing. It never had anything useful, right?
>
>
> Thanks,
> pq
>
> > On Mon, Apr 6, 2015 at 12:33 PM, Derek Foreman <derekf at osg.samsung.com>
> > wrote:
> >
> > > 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 <dima at gmail.com>
> > > > ---
> > > >  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?
> > >
> > > > +         !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,
> > > >
> > >
> > >
> > >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150408/fe796cb7/attachment-0001.html>