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

[Pekka Paalanen]

On Mon, 13 Apr 2015 22:47:00 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

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

I didn't mean strictly only one animation per view... not sure if other
plugins would be adding animations regardless. It was just about making
the opening and closing animations mutually exclusive, but now that I
think of it, it's probably a far too limited measure for just the
immediate problem. Maybe not a good idea after all.

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

Yeah. I was actually confused by the function naming and docs, because
I assumed the intent was to remove all animations, not only the
animations that would conflict with the closing animation. That is, I
didn't realize it was enough to remove animations from only one
output's list.

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

That's a good question, I don't know.

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

Right, but I'm starting to think it is a too specific fix for a generic
class of bugs.

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

I think that would be the best, yes. We could make it a rule of thumb:
never destroy weston_views or weston_surfaces from animation hooks.
With that simple rule in place, all similar future bugs would be
avoided. It would be a generic solution to a generic class of bugs.

That rule has a lot of potential for further generalizations, because
the essence of the problem is manipulating a list while traversing the
list. "Don't destroy weston_views from weston_view signals" etc.


Thanks,
pq