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

[Jonas Ådahl]

On Wed, Jul 12, 2017 at 12:02:29PM +0200, Quentin Glidic wrote:
> On 7/12/17 11:23 AM, Jonas Ådahl wrote:
> > 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.
> 
> True, just need to make sure we update the geometry before calling the
> sub-type function.
> 
> 
> > 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.
> 
> I see two separate issues here:
> 
> 1. We only consider the last configure.
> I plan to fix that by keeping a list of configure states, dropping older
> states until the acked one. That would fix a number of racy scenarii, not
> just this one.
> 
> 2. The shell/compositor cannot know which (fullscreen) output is being
> associated with a commit.
> This may be tricky to get right, because there may be more than one. The
> client can only ask for one output in set_fullscreen, but the compositor is
> free to ignore that and have 2 views fullscreened on differently-sized
> outputs.
> I would wait until we have a real use case triggering that issue. Most
> clients should answer in a timely fashion to avoid that.
> 
> This patch just fixes what size is expected on commit, so that it matches
> the ack_configure.

It's indeed more correct, but don't we still need to compare the serial
of pending and next here to actually check the ack_configure size?

No need to check the output or whatever, just that we don't enforce an
outdated size.


Jonas

> 
> 
> > >   		struct weston_desktop_client *client =
> > >   			weston_desktop_surface_get_client(toplevel->base.desktop_surface);
> > >   		struct wl_resource *client_resource =
> > > -- 
> > > 2.13.2
> 
> 
> 
> -- 
> 
> Quentin “Sardem FF7” Glidic