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

[Derek Foreman]

On 13/04/15 07:15 AM, Pekka Paalanen wrote:
> 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.

I must admit that sounds like a much nicer way to trigger the crash than
what I've been doing.  :)

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

The crash is triggered by manipulating the list while it's being walked.
 As long as the output you just inserted the animation on doesn't have
any other animations for the view we should be ok.

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

I wasn't willing to make the heavy handed "only one animation per view!"
decree myself.  If you'd like it written that way, I'm quite happy to do it.

My patch only removes animations if a close animation is being run.  Any
other multiple animation scenario is left alone - whether that makes
sense or not.  :)

I think we need to be able to have multiple views of the same
shell_surface on different outputs and animate them independently though?

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

Hmm.  I think if we used that stored animation pointer to remove the old
animation before starting a new one we'd not crash.

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

I don't think I can think of a valid reason to disagree - would you like
me to fix it that way?