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

[Quentin Glidic]

On 7/12/17 12:07 PM, Jonas Ådahl wrote:
> 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.

"next" is only set on ack_configure, so the only way for it to be 
outdated is (-> is client to server):

<- configure(1, fullscreen)
-> ack_configure(1)
-> commit(1)
<- configure(2, not fullscreen)
<- configure(3, fullscreen)
-> ack_configure(2) # ignored because not last one
-> commit(2)

At that point, the compositor expects a specific size (1) but client 
commits a free size (2).

This will be fixed with a configure list based on serials.


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


-- 

Quentin “Sardem FF7” Glidic