On Wed, Jan 31, 2018 at 03:21:07PM +0200, Pekka Paalanen wrote:
> On Fri, 26 Jan 2018 18:47:58 +0200
> Alexandros Frantzis <alexandros.frant...@collabora.com> wrote:
>
> > Ensure the server can safely handle client requests for wl_seat resource
> > that have become inert due to weston_seat object release and subsequent
> > destruction.
> >
> > The clean-up involves, among other things, unsetting the destroyed
> > weston_seat object from the user data of wl_seat resources, and handling
> > this NULL user data case where required.
> >
> > Signed-off-by: Alexandros Frantzis <alexandros.frant...@collabora.com>
> > ---
> > libweston/input.c | 45 +++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 35 insertions(+), 10 deletions(-)
> >
> > diff --git a/libweston/input.c b/libweston/input.c
> > index 48bcc55c..e4daa56b 100644
> > --- a/libweston/input.c
> > +++ b/libweston/input.c
> > @@ -2412,6 +2412,13 @@ seat_get_pointer(struct wl_client *client, struct
> > wl_resource *resource,
> > uint32_t id)
> > {
> > struct weston_seat *seat = wl_resource_get_user_data(resource);
> > + struct weston_pointer *pointer;
> > + struct wl_resource *cr;
> > + struct weston_pointer_client *pointer_client;
> > +
> > + if (!seat)
> > + return;
> > +
> > /* We use the pointer_state directly, which means we'll
> > * give a wl_pointer if the seat has ever had one - even though
> > * the spec explicitly states that this request only takes effect
> > @@ -2420,10 +2427,7 @@ seat_get_pointer(struct wl_client *client, struct
> > wl_resource *resource,
> > * This prevents a race between the compositor sending new
> > * capabilities and the client trying to use the old ones.
> > */
> > - struct weston_pointer *pointer = seat->pointer_state;
> > - struct wl_resource *cr;
> > - struct weston_pointer_client *pointer_client;
> > -
> > + pointer = seat->pointer_state;
> > if (!pointer)
> > return;
> >
> > @@ -2499,6 +2503,12 @@ seat_get_keyboard(struct wl_client *client, struct
> > wl_resource *resource,
> > uint32_t id)
> > {
> > struct weston_seat *seat = wl_resource_get_user_data(resource);
> > + struct weston_keyboard *keyboard;
> > + struct wl_resource *cr;
> > +
> > + if (!seat)
> > + return;
> > +
> > /* We use the keyboard_state directly, which means we'll
> > * give a wl_keyboard if the seat has ever had one - even though
> > * the spec explicitly states that this request only takes effect
> > @@ -2507,9 +2517,7 @@ seat_get_keyboard(struct wl_client *client, struct
> > wl_resource *resource,
> > * This prevents a race between the compositor sending new
> > * capabilities and the client trying to use the old ones.
> > */
> > - struct weston_keyboard *keyboard = seat->keyboard_state;
> > - struct wl_resource *cr;
> > -
> > + keyboard = seat->keyboard_state;
> > if (!keyboard)
> > return;
> >
> > @@ -2579,6 +2587,12 @@ seat_get_touch(struct wl_client *client, struct
> > wl_resource *resource,
> > uint32_t id)
> > {
> > struct weston_seat *seat = wl_resource_get_user_data(resource);
> > + struct weston_touch *touch;
> > + struct wl_resource *cr;
> > +
> > + if (!seat)
> > + return;
> > +
> > /* We use the touch_state directly, which means we'll
> > * give a wl_touch if the seat has ever had one - even though
> > * the spec explicitly states that this request only takes effect
> > @@ -2587,9 +2601,7 @@ seat_get_touch(struct wl_client *client, struct
> > wl_resource *resource,
> > * This prevents a race between the compositor sending new
> > * capabilities and the client trying to use the old ones.
> > */
> > - struct weston_touch *touch = seat->touch_state;
> > - struct wl_resource *cr;
> > -
> > + touch = seat->touch_state;
> > if (!touch)
> > return;
>
> Hi,
Hi Pekka,
thanks for the review.
> all the seat_get_*() changes have the same problem that they skip
> calling wl_resource_create() which will lead to protocol state
> mismatch. These functions need to create inert wl_resources, but
> thankfully the earlier patches already take care of handling them
> further.
I was misunderstanding how this worked. I will update these to properly
create (inert) resources in v2.
> > @@ -3087,6 +3099,19 @@ weston_seat_init(struct weston_seat *seat, struct
> > weston_compositor *ec,
> > WL_EXPORT void
> > weston_seat_release(struct weston_seat *seat)
> > {
> > + struct wl_resource *resource;
> > +
> > + wl_resource_for_each(resource, &seat->base_resource_list) {
> > + wl_resource_set_user_data(resource, NULL);
> > + }
>
> Other requests which take wl_seat as argument are:
> - wl_data_device_manager.get_data_device
> - wl_shell_surface.move
> - wl_shell_surface.resize
> - wl_shell_surface.set_popup
>
> But there are even more in wayland-protocols, you can find them with
> $ git grep 'interface="wl_seat"'
>
> These are unlikely to cope with an inert wl_seat.
>
> Patching weston_desktop_seat_from_seat() will probably take care of a
> lot.
Ack, I will take care of these in v2.
> > +
> > + wl_resource_for_each(resource, &seat->drag_resource_list) {
> > + wl_resource_set_user_data(resource, NULL);
> > + }
> > +
> > + wl_list_remove(&seat->base_resource_list);
> > + wl_list_remove(&seat->drag_resource_list);
> > +
> > wl_list_remove(&seat->link);
> >
> > if (seat->saved_kbd_focus)
>
> Setting NULL with the drag_resource_list needs more changes in
> data-device.c. Looks like the user data of wl_data_device resource is
> the weston_seat, so all uses of wl_data_device needs to be audited.
> That's all the functions in data_device_interface. Luckily none of the
> wl_data_device requests create new protocol objects, and wl_data_device
> is never used as a request argument.
Ack.
> Thanks,
> pq
Thanks,
Alexandros
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
