[PATCH xserver 1/2] xwayland: Apply "last pointer window" check only to the pointer device

[Olivier Fourdan]

Hey Carlos,

----- Original Message -----
> The checks in xwayland's XYToWindow handler pretty much assumes that the
> sprite is managed by the wl_pointer, which is not entirely right, given
> 1) The Virtual Core Pointer may be controlled from other interfaces, and
> 2) there may be other SpriteRecs than the VCP's.
> 
> This makes XYToWindow calls return a sprite trace with just the root
> window if any of those two assumptions are broken, eg. on touch events.
> 
> So turn the check upside down, first assume that the default XYToWindow
> proc behavior is right, and later cut down the spriteTrace if the
> current device happens to be the pointer. We work our way to the
> device's lastSlave here so as not to break assumption #1 above.
> 
> Also, simplify the actual nested call to XYToWindow, the method pointer
> in ScreenPtr was reinstaurated before calling, and moved back to
> xwl_screen domain afterwards. This seems kind of pointless, as we have
> the function pointer anyway.
> 
> Signed-off-by: Carlos Garnacho <carlosg at gnome.org>
> ---
>  hw/xwayland/xwayland-input.c | 64
>  ++++++++++++++++++++++++++------------------
>  1 file changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 32cfb35..9b385dc 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -945,43 +945,55 @@ DDXRingBell(int volume, int pitch, int duration)
>  {
>  }
>  
> -static WindowPtr
> -xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int x, int y)
> +static DeviceIntPtr
> +sprite_get_last_active_device(SpritePtr sprite, struct xwl_seat **xwl_seat)
>  {
> -    struct xwl_seat *xwl_seat = NULL;
>      DeviceIntPtr device;
> -    WindowPtr ret;
>  
>      for (device = inputInfo.devices; device; device = device->next) {
> -        if (device->deviceProc == xwl_pointer_proc &&
> -            device->spriteInfo->sprite == sprite) {
> -            xwl_seat = device->public.devicePrivate;
> +        /* Ignore non-wayland devices */
> +        if (device->deviceProc != xwl_pointer_proc &&
> +            device->deviceProc != xwl_touch_proc &&
> +            device->deviceProc != xwl_keyboard_proc)
> +            continue;
> +
> +        if (device->spriteInfo->sprite == sprite)
>              break;
> -        }
>      }
>  
> -    if (xwl_seat == NULL) {
> -        sprite->spriteTraceGood = 1;
> -        return sprite->spriteTrace[0];
> -    }
> +    if (!device || IsFloating(device))
> +        return NULL;
>  
> -    screen->XYToWindow = xwl_seat->xwl_screen->XYToWindow;
> -    ret = screen->XYToWindow(screen, sprite, x, y);
> -    xwl_seat->xwl_screen->XYToWindow = screen->XYToWindow;
> -    screen->XYToWindow = xwl_xy_to_window;

That wrapping is on purpose, see Daniel's review on my initial patch here:

https://lists.x.org/archives/xorg-devel/2016-June/050238.html

> +    *xwl_seat = device->public.devicePrivate;
>  
> -    /* If the pointer has left the Wayland surface but the DIX still
> -     * finds the pointer within the previous X11 window, it means that
> -     * the pointer has crossed to another native Wayland window, in this
> -     * case, pretend we entered the root window so that a LeaveNotify
> -     * event is emitted.
> +    /* We do want the last active slave, we only check on slave xwayland
> devices
> +     * so we can find out the xwl_seat, but those don't actually own their
> +     * sprite, so the match doesn't mean a lot.
>       */
> -    if (xwl_seat->focus_window == NULL && xwl_seat->last_xwindow == ret) {
> -        sprite->spriteTraceGood = 1;
> -        return sprite->spriteTrace[0];
> -    }
> +    return device->master->lastSlave;

I wonder, would it make sense to use the GetMaster() API here or else check if master is actually non-null?

(note, I tried to detach the xwayland-pointer (with xinput float) from its master, it doesn't crash because we actually get no more events from Xwayland for this device after that)

> +}
>  
> -    xwl_seat->last_xwindow = ret;
> +static WindowPtr
> +xwl_xy_to_window(ScreenPtr screen, SpritePtr sprite, int x, int y)
> +{
> +    struct xwl_seat *xwl_seat = NULL;
> +    struct xwl_screen *xwl_screen;
> +    DeviceIntPtr device;
> +    WindowPtr ret;
> +
> +    xwl_screen = xwl_screen_get(screen);
> +    ret = xwl_screen->XYToWindow(screen, sprite, x, y);
> +
> +    device = sprite_get_last_active_device(sprite, &xwl_seat);

Do you plan to reuse that sprite_get_last_active_device() elsewhere?

If not, the whole logic below (device && xwl_seat && xwl_seat->pointer == device) could be moved to the function that would return either the right xwl_seat or NULL, as we don't actually use the value of device here, apart from this test.

> +    if (device && xwl_seat && xwl_seat->pointer == device) {
> +        if (xwl_seat->focus_window == NULL && xwl_seat->last_xwindow == ret)
> {
> +            sprite->spriteTraceGood = 1;
> +            return sprite->spriteTrace[0];
> +        }
> +
> +        xwl_seat->last_xwindow = ret;
> +    }
>  
>      return ret;
>  }
> --
> 2.10.0

Cheers,
Olivier