[PATCH] compositor: unmap subsurface views before destroying the subsurfaces

[Pekka Paalanen]

On Fri, 13 Jun 2014 18:10:26 +0200
George Kiagiadakis <george.kiagiadakis at collabora.com> wrote:

> This is to avoid recursing into weston_compositor_build_view_list()
> and therefore fix crashing when destroying a stack of visible subsurfaces
> due to weston_compositor_build_view_list() being called recursively
> and corrupting the lists it works on.
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=79684
> ---
>  src/compositor.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/compositor.c b/src/compositor.c
> index 3c5c8e3..2fbfdbf 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -1662,8 +1662,10 @@ surface_free_unused_subsurface_views(struct weston_surface *surface)
>  		if (sub->surface == surface)
>  			continue;
>  
> -		wl_list_for_each_safe(view, nv, &sub->unused_views, surface_link)
> +		wl_list_for_each_safe(view, nv, &sub->unused_views, surface_link) {
> +			weston_view_unmap (view);
>  			weston_view_destroy(view);
> +		}
>  
>  		surface_free_unused_subsurface_views(sub->surface);
>  	}
> @@ -2764,8 +2766,10 @@ weston_subsurface_destroy(struct weston_subsurface *sub)
>  		assert(sub->parent_destroy_listener.notify ==
>  		       subsurface_handle_parent_destroy);
>  
> -		wl_list_for_each_safe(view, next, &sub->surface->views, surface_link)
> +		wl_list_for_each_safe(view, next, &sub->surface->views, surface_link) {
> +			weston_view_unmap(view);
>  			weston_view_destroy(view);
> +		}
>  
>  		if (sub->parent)
>  			weston_subsurface_unlink_parent(sub);

Hi,

this patch was already merged and it fixes a real issue, but I am not
completely sure the latter hunk is totally correct alone.

weston_subsurface_destroy() is called when a sub-surface gets destroyed
for any reason. The above hunk prevents the compositor->view_list from
being updated, and this could be a theoretical problem, if the
destruction is caused directly by a client destroying a wl_sub_surface
or wl_surface, and that sub-surface has sub-surfaces of its own. These
nested sub-surfaces would need to be removed from the view_list too, as
they should become unmapped. Weston_compositor_pick_view() could
erroneusly target this supposedly unmapped-but-not-actually
sub-sub-surface.

In practise, I think hitting this problem will be very hard. It is
probably quite rare to use nested sub-surfaces, and the very next
compositor repaint will fix up the view_list, so we won't even get a
visual glitch. The only thing that could happen AFAIU is that some
input events targets a wl_surface they shouldn't, and that wl_surface
will disappear very soon anyway. Also, since destroying a surface so
that it unmaps causes a repaint, so the window for the fault is really
theoretical at best.

So, all is probably just fine. *shrug*


Thanks,
pq