[PATCH weston v4] libweston-desktop: fix stale ping when a wl_shell_surface is destroyed

[Giulio Camuffo]

2016-12-08 16:46 GMT+01:00 Quentin Glidic <sardemff7+wayland at sardemff7.net>:
> On 08/12/2016 16:20, Giulio Camuffo wrote:
>>
>> When sending a ping event to a surface using the wl_shell interface,
>> if that surface is destroyed before we receive the pong we will never
>> receive it, even if the client is actually responsive, since the
>> interface does not exist anymore. So when the surface if destroyed
>> pretend it's a pong and reset the ping state.
>>
>> Signed-off-by: Giulio Camuffo <giuliocamuffo at gmail.com>
>
>
> I made a few renames to match the current code.
> I was going to push it, but I found a last issue, see below.
>
>
>> ---
>>
>> v3: store the ping serial in the surface instead of the client wrapper
>> v4: removed leftover change
>>
>>  libweston-desktop/wl-shell.c | 22 +++++++++++++++++++++-
>>  1 file changed, 21 insertions(+), 1 deletion(-)
>>
>> diff --git a/libweston-desktop/wl-shell.c b/libweston-desktop/wl-shell.c
>> index 399139c..f75b022 100644
>> --- a/libweston-desktop/wl-shell.c
>> +++ b/libweston-desktop/wl-shell.c
>> @@ -57,6 +57,7 @@ struct weston_desktop_wl_shell_surface {
>>         struct weston_desktop_seat *popup_seat;
>>         enum weston_desktop_wl_shell_surface_state state;
>>         struct wl_listener wl_surface_resource_destroy_listener;
>> +       uint32_t ping_serial;
>>  };
>>
>>  static void
>> @@ -112,6 +113,7 @@ weston_desktop_wl_shell_surface_ping(struct
>> weston_desktop_surface *dsurface,
>>  {
>>         struct weston_desktop_wl_shell_surface *surface = user_data;
>>
>> +       surface->ping_serial = serial;
>>         wl_shell_surface_send_ping(surface->resource, serial);
>>  }
>>
>> @@ -181,11 +183,27 @@ weston_desktop_wl_shell_change_state(struct
>> weston_desktop_wl_shell_surface *sur
>>  }
>>
>>  static void
>> +pong_client(struct weston_desktop_wl_shell_surface *surface)
>
>
> weston_desktop_wl_shell_surface_pong_client
>
>> +{
>> +       struct weston_desktop_client *client =
>> +               weston_desktop_surface_get_client(surface->surface);
>> +
>> +       weston_desktop_client_pong(client, surface->ping_serial);
>> +       surface->ping_serial = 0;
>> +}
>> +
>> +static void
>>  weston_desktop_wl_shell_surface_destroy(struct weston_desktop_surface
>> *dsurface,
>>                                         void *user_data)
>>  {
>>         struct weston_desktop_wl_shell_surface *surface = user_data;
>>
>> +       /* If the surface being destroyed was the one that was pinged
>> before
>> +        * we need to fake a pong here, because it cannot answer the ping
>> anymore,
>> +        * even if the client is responsive. */
>> +       if (surface->ping_serial != 0)
>> +               pong_client(surface);
>> +
>>
>> wl_list_remove(&surface->wl_surface_resource_destroy_listener.link);
>>
>>         weston_desktop_wl_shell_surface_maybe_ungrab(surface);
>> @@ -203,8 +221,10 @@ weston_desktop_wl_shell_surface_protocol_pong(struct
>> wl_client *wl_client,
>>                                               uint32_t serial)
>>  {
>>         struct weston_desktop_surface *surface =
>> wl_resource_get_user_data(resource);
>
>
> dsurface
>
>> +       struct weston_desktop_wl_shell_surface *wls =
>> +               weston_desktop_surface_get_implementation_data(surface);
>
>
> surface
>
>> -
>> weston_desktop_client_pong(weston_desktop_surface_get_client(surface),
>> serial);
>> +       pong_client(wls);
>
>
> We should check that surface->ping_serial == serial somehow.
> Maybe it is safe to ignore serial for now, as it fixes something, then add a
> check in a follow-up commit.

Well, but weston_desktop_client_pong() already checks that, so i don't
think we need to check it here too.

>
> Thoughts? Pekka (aka assertman)?
>
> Cheers,
>
>
>>  }
>>
>>  static void
>>
>
>
> --
>
> Quentin “Sardem FF7” Glidic