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

[Carlos Garnacho]

Hi :),

On Wed, Sep 28, 2016 at 9:25 AM, Olivier Fourdan <ofourdan at redhat.com> wrote:
> 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

Uhm, ok... tried to git annotate this code (also its original
incarnation) and got pointed to the first xwayland commit, without
much explanation about this. I suspected it had something to do with
reentrancy, but then the default mi* call we're overriding doesn't
attempt to recurse anyway.

>
>> +    *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)

Sure, note that the code has an IsFloating() check already. Probably
doing "master = GetMaster(...);if (!master) return NULL" looks
clearer.

>
>> +}
>>
>> -    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.

I think it could be useful when we handle (z)wp_tablet, tablet foci
would have the same issue as they all would take turns to drive the
VCP too, and it's not implicitly grabbed de facto like touch, so
transitioning from X11 to wayland surfaces is also possible. In that
case we might also need testing a different window than
xwl_seat->focus_window.

I guess we could also move the whole logic to a
check_needs_rootwin_crossing() function? It's admittedly hard to split
this in coherent bits.

Cheers,
  Carlos