[PATCH weston] desktop-shell: remove all running animations for a surface before fade out

[Pekka Paalanen]

On Fri, 10 Apr 2015 12:54:46 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> The completion of the fade out animation destroys a surface which fires
> the surface destroy signal.  Animations listen for the surface destroy
> signal and destroy themselves as a response.
> 
> If there's an animation running on a surface when the fade out completes
> the old animation will be removed from the animation list during the list
> walk which can lead to a crash.
> 
> This can happen when a surface is mapped then quickly destroyed and the
> fade out completes before the zoom in.
> 
> By removing all animations before starting the fade out we avoid this.
> 
> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> ---
>  desktop-shell/shell.c |  1 +
>  src/animation.c       | 22 ++++++++++++++++++++++
>  src/compositor.h      |  3 +++
>  3 files changed, 26 insertions(+)
> 
> diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> index f7c928e..38a3953 100644
> --- a/desktop-shell/shell.c
> +++ b/desktop-shell/shell.c
> @@ -3592,6 +3592,7 @@ handle_resource_destroy(struct wl_listener *listener, void *data)
>  	pixman_region32_fini(&shsurf->surface->input);
>  	pixman_region32_init(&shsurf->surface->input);
>  	if (shsurf->shell->win_close_animation_type == ANIMATION_FADE) {
> +		weston_view_animation_cancel(shsurf->view);
>  		weston_fade_run(shsurf->view, 1.0, 0.0, 300.0,
>  				fade_out_done, shsurf);
>  	} else {
> diff --git a/src/animation.c b/src/animation.c
> index 0febe41..578f73e 100644
> --- a/src/animation.c
> +++ b/src/animation.c
> @@ -144,6 +144,28 @@ weston_view_animation_destroy(struct weston_view_animation *animation)
>  	free(animation);
>  }
>  
> +/** Cancel all running animations on a view
> + *
> + * \param view The view to cancel animations for.
> + *
> + * Walks the output animation list for the view's output and
> + * removes any animations for the view.
> + */
> +WL_EXPORT void
> +weston_view_animation_cancel(struct weston_view *view)
> +{
> +	struct weston_animation *animation, *next;
> +
> +	wl_list_for_each_safe(animation, next, &view->output->animation_list, link) {
> +		struct weston_view_animation *wanim =
> +			container_of(animation,
> +				     struct weston_view_animation, animation);
> +
> +		if (wanim->view == view)
> +			weston_view_animation_destroy(wanim);
> +	}
> +}

Hi,

I tested this. I could reproduce a crash by setting both "animation"
and "close-animation" to "fade". (Would have been nice to mention that
in the commit message.) This patch does avoid the crash.

However, I wonder how reliable it is to look only in the list of
view->output. Can't a view spawn on one output, be moved to another
output before the animation finishes, and the surface get destroyed?

It probably doesn't make sense to run multiple animations... well, not
mapping and closing animations at least; at the same time, so why not
store the animation pointer in shell_surface, and use that for clean-up?

Would avoid looking through a list, too.

But that won't then fix the real problem of removing entries from an
animation list while in the wl_list_for_each_safe loop of animations,
right?

Maybe the closing animation ending should just schedule an idle task to
destroy the surface?


Thanks,
pq