[PATCH weston 3/3] libweston-desktop/xdg-shell: Check surface size against acknowledged size

[Jonas Ådahl]

On Wed, Jul 12, 2017 at 09:53:04AM +0200, Quentin Glidic wrote:
> From: Quentin Glidic <sardemff7+git at sardemff7.net>
> 
> We were checking against the pending size, which lead some clients
> (simple-egl) to crash because they sent a buffer before acknowledging
> the latest configure event.
> 
> Signed-off-by: Quentin Glidic <sardemff7+git at sardemff7.net>
> ---
>  libweston-desktop/xdg-shell-v5.c | 6 ++++--
>  libweston-desktop/xdg-shell-v6.c | 6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/libweston-desktop/xdg-shell-v5.c b/libweston-desktop/xdg-shell-v5.c
> index 0fb067ab..b32b7812 100644
> --- a/libweston-desktop/xdg-shell-v5.c
> +++ b/libweston-desktop/xdg-shell-v5.c
> @@ -60,6 +60,7 @@ struct weston_desktop_xdg_surface {
>  	} pending;
>  	struct {
>  		struct weston_desktop_xdg_surface_state state;
> +		struct weston_size size;
>  	} next;
>  	struct {
>  		struct weston_desktop_xdg_surface_state state;
> @@ -244,8 +245,8 @@ weston_desktop_xdg_surface_committed(struct weston_desktop_surface *dsurface,
>  	bool reconfigure = false;
>  
>  	if (surface->next.state.maximized || surface->next.state.fullscreen)
> -		reconfigure = surface->pending.size.width != wsurface->width ||
> -			      surface->pending.size.height != wsurface->height;
> +		reconfigure = surface->next.size.width != wsurface->width ||
> +			      surface->next.size.height != wsurface->height;
>  
>  	if (reconfigure) {
>  		weston_desktop_xdg_surface_schedule_configure(surface, true);
> @@ -447,6 +448,7 @@ weston_desktop_xdg_surface_protocol_ack_configure(struct wl_client *wl_client,
>  		return;
>  
>  	surface->next.state = surface->pending.state;
> +	surface->next.size = surface->pending.size;
>  }
>  
>  static void
> diff --git a/libweston-desktop/xdg-shell-v6.c b/libweston-desktop/xdg-shell-v6.c
> index db894d4a..f5e46daa 100644
> --- a/libweston-desktop/xdg-shell-v6.c
> +++ b/libweston-desktop/xdg-shell-v6.c
> @@ -95,6 +95,7 @@ struct weston_desktop_xdg_toplevel {
>  	} pending;
>  	struct {
>  		struct weston_desktop_xdg_toplevel_state state;
> +		struct weston_size size;
>  		struct weston_size min_size, max_size;
>  	} next;
>  	struct {
> @@ -424,6 +425,7 @@ static void
>  weston_desktop_xdg_toplevel_ack_configure(struct weston_desktop_xdg_toplevel *toplevel)
>  {
>  	toplevel->next.state = toplevel->pending.state;
> +	toplevel->next.size = toplevel->pending.size;
>  }
>  
>  static void
> @@ -626,8 +628,8 @@ weston_desktop_xdg_toplevel_committed(struct weston_desktop_xdg_toplevel *toplev
>  		return;
>  
>  	if ((toplevel->next.state.maximized || toplevel->next.state.fullscreen) &&
> -	    (toplevel->pending.size.width != wsurface->width ||
> -	     toplevel->pending.size.height != wsurface->height)) {
> +	    (toplevel->next.size.width != wsurface->width ||
> +	     toplevel->next.size.height != wsurface->height)) {

Unrelated to this patch, but wsurface->width/height should rather be the
geometry, not the size, because IIRC the surface size comes from the
buffer or wp_viewporter, while the next.size.width/height is window
geometry size. We only enforce the window geometry.

Shouldn't we also compare the serial here? If I understand things
correctly, "next" is the state+size the last time a client
ack_configure:ed a serial without any more pending one on the way.

So if that next state is fullscreen+WxH from when the client acked that
at some point in time, then we quickly went toplevel->fullscreen but
fullscreen on another output. In the intermediate state, the client acks
the old configure, thus we won't update next, end next will still be
fullscreen+WxH, while the surface size will be wxh (w != W, h != H).

Maybe could be fixed by adding the serial to the next and pending
structsand only enforce if the next (that is being committed) state is
the pending one.


Jonas

>  		struct weston_desktop_client *client =
>  			weston_desktop_surface_get_client(toplevel->base.desktop_surface);
>  		struct wl_resource *client_resource =
> -- 
> 2.13.2
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel