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

[Pekka Paalanen]

On Thu, 8 Dec 2016 16:58:36 +0100
Quentin Glidic <sardemff7+wayland at sardemff7.net> wrote:

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

No asserts here. You cannot use asserts for validating external data.

The serial in pong could be from any pending ping. Can there be only
one ping in flight at a time per object?

The spec does not say what happens if the serial is not the expected
one, and wl_shell_surface does not define any error codes, so I don't
think we should enforce correct serials here.


Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161214/e13c697e/attachment-0001.sig>