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

[Quentin Glidic]

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.


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