[PATCH RFC xserver] wayland: clear resource for pixmap on unrealize

[Rui Tiago Cação Matos]

On Fri, Jun 3, 2016 at 5:10 PM, Olivier Fourdan <ofourdan at redhat.com> wrote:
> On cursor unrealize, the associated pixmap is destroyed, make sure we
> clear the pointer from the private resource and check for the value
> being non-null when setting or destroying the cursor.
>
> Signed-off-by: Olivier Fourdan <ofourdan at redhat.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96246
> ---
>  Hi,
>
>  Sending this patch as an RFC, I am not sure if it cures just the effects
>  of a more complex issue, or if it's the right fix for the bug mentioned
>  above.
>
>  For Fedora we have quite of few similar reports that appeared after we
>  switched to 1.18.3, and the most likely related change is commit 1815540
>
>      xwayland: Clear pending cursor frame callbacks on pointer enter

Oh, I see how this may happen. Basically, the dix unrealizes a cursor
while a frame callback is pending without setting a new cursor. When
we get either the frame callback or a wayland pointer enter event we
call xwl_seat_set_cursor() with the old and unrealized ->x_cursor and
crash since the pixmap has been destroyed.

>  But I am not entirely sure how such a change can cause this issue.
>
>  Anyway, if you have a better understanding of the problem, free free to
>  chime in! :)
>
>  Cheers,
>  Olivier
>
>  hw/xwayland/xwayland-cursor.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/hw/xwayland/xwayland-cursor.c b/hw/xwayland/xwayland-cursor.c
> index 76729db..44fadf4 100644
> --- a/hw/xwayland/xwayland-cursor.c
> +++ b/hw/xwayland/xwayland-cursor.c
> @@ -78,6 +78,10 @@ xwl_unrealize_cursor(DeviceIntPtr device, ScreenPtr screen, CursorPtr cursor)
>      PixmapPtr pixmap;
>
>      pixmap = dixGetPrivate(&cursor->devPrivates, &xwl_cursor_private_key);
> +    if (!pixmap)
> +        return TRUE;
> +
> +    dixSetPrivate(&cursor->devPrivates, &xwl_cursor_private_key, NULL);

This is a good thing to do regardless.

But, I think we should also add

if (cursor == xwl_seat->x_cursor)
  xwl_seat->x_cursor = NULL;

here, so that xwl_seat_set_cursor() unsets the pointer surface on the
wayland compositor.

Rui

>
>      return xwl_shm_destroy_pixmap(pixmap);
>  }
> @@ -122,6 +126,9 @@ xwl_seat_set_cursor(struct xwl_seat *xwl_seat)
>
>      cursor = xwl_seat->x_cursor;
>      pixmap = dixGetPrivate(&cursor->devPrivates, &xwl_cursor_private_key);
> +    if (!pixmap)
> +        return;
> +
>      stride = cursor->bits->width * 4;
>      if (cursor->bits->argb)
>          memcpy(pixmap->devPrivate.ptr,
> --
> 2.7.4
>