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

[Quentin Glidic]

On 08/12/2016 16:52, Giulio Camuffo wrote:
> 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.

It checks that the last ping matches the passed serial. This (passed) 
serial should be the one the *client* sent. Here, we just pass 
->ping_serial, which is guaranteed to be the one 
weston_desktop_client_pong() is waiting for. But nothing prevents the 
client to send wl_shell_surface.pong(1337), and we wouldn’t catch these.


>
>>
>> Thoughts? Pekka (aka assertman)?
>>
>> Cheers,
>>
>>
>>>  }
>>>
>>>  static void
>>>


-- 

Quentin “Sardem FF7” Glidic