GNOME Bugzilla – Bug 695158
Animated property transitions cause the animated actor to be leaked
Last modified: 2013-03-05 01:34:59 UTC
I've been trying to debug a hairy object leak. I managed to figure out it's happening when I animate a property transition on the actor -- something was holding a ref to the actor. Turns out, it's the transition. The problem seems to be that the transition takes a ref on the actor for each property being animated, which is very sensible. It then will unref the actor in clutter_transition_stopped(). However, clutter_transition_stopped() is never getting called. You can reproduce this simply using examples/basic-actor.c with this quickie patch: diff --git i/examples/basic-actor.c w/examples/basic-actor.c index e74f33e..e5ce4ed 100644 --- i/examples/basic-actor.c +++ w/examples/basic-actor.c @@ -56,6 +56,8 @@ on_transition_stopped (ClutterActor *actor, clutter_actor_set_rotation_angle (actor, CLUTTER_Y_AXIS, 0.0f); clutter_actor_restore_easing_state (actor); + printf("Refcount: %d\n", G_OBJECT(actor)->ref_count); + /* disconnect so we don't get multiple notifications */ g_signal_handlers_disconnect_by_func (actor, on_transition_stopped, Then run and click the green square a few times. Each time through, the refcount on the actor is going up and up and up. The clutter_transition_stopped() was added in de4d70, but I think it's not hooked up right to get called when the timeline reaches its end and has no loop, and therefore it's never going to deref the actor. This means that any actor that ever animates a property transition is going to get leaked...
Created attachment 238052 [details] [review] actor: Release a reference on the implicit Transitions This should remove the additional reference. Could you please test it?
Review of attachment 238052 [details] [review]: no, I'm obviously on crack: the Transition is unreferenced correctly - and it should release the reference on the Animatable instance as well when it's disposed.
Nope, that breaks things badly. The extra ref is on the actor from the transition, not on the transition from the actor. With this patch, basic-actor.c acts very cranky with a lot of critical errors because the timeline that's now ticking doesn't exist since it's been fully unreffed, and all kinds of callbacks blow up. I think the problem is that while the timeline is emitting the stopped signal at clutter-timeline.c:1057, the transition's clutter_transition_stopped() is never getting called.
Created attachment 238059 [details] [review] actor: Release a reference on the implicit Transitions Working version; this seems to fix the leak of the transition, which cause the additional reference on the actor.
Attachment 238059 [details] pushed as f3659d3 - actor: Release a reference on the implicit Transitions
That now does seem to release the ref on the actor. But shouldn't clutter_transition_stopped() be getting called when the transition stops?
In other words, I think the effect of the patch is correct, but it should call clutter_transition_stopped there instead of just dereffing the actor. ie clutter_transition_stopped does this: clutter_transition_detach (CLUTTER_TRANSITION (timeline), priv->animatable); g_clear_object (&priv->animatable); g_object_unref (timeline); The same stuff ends up happening in the transition's dispose, so I guess it's not a huge deal, but I'd say you should either take out the dead code in clutter_transition_stopped, or else call it so it's not dead :)
(In reply to comment #7) thanks for testing! > In other words, I think the effect of the patch is correct, but it should call > clutter_transition_stopped clutter_transition_stopped() is not a public function: it's called once the ::stopped signal is emitted. > there instead of just dereffing the actor. ie > clutter_transition_stopped does this: > > clutter_transition_detach (CLUTTER_TRANSITION (timeline), > priv->animatable); > g_clear_object (&priv->animatable); > g_object_unref (timeline); > > The same stuff ends up happening in the transition's dispose, so I guess it's > not a huge deal, but I'd say you should either take out the dead code in > clutter_transition_stopped, or else call it so it's not dead :) the ::stopped signal can be emitted outside of the dispose sequence, and the dispose sequence can happen without the timeline being stopped, so we need the code in two places. we could add a simple wrapper function, but the code is pretty much manageable, and I don't expect the Transition class to be more complicated than what it is for the foreseeable future.
Yes, I know it's not a public function. But it is *NOT* called when ::stopped is emitted. I've added some debug printing in there, and it's never called. At least in my code... Could you check if yours is ever getting called?
ah, yes: a copy-and-paste error prevented the class signal handler for the ClutterTimeline::stopped signal to be called — we lose type safety in the signal registration, sadly, and cannot catch these case automatically. :-/ the original on_transition_stopped() and clutter_transition_stopped() functions were written for the ::completed signal, so the surrounding code was correct; when we transitioned to the ::stopped signal, I just assumed that the signal was implemented correctly. I've fixed the signal definition, and reverted the patch - and it's been applied in master, clutter-1.14, and clutter-1.12. thanks for the investigative work!
No problem. Thanks for the quick fix! I'm actually a little amazed nobody seems to have notice this yet. It's a major source of leaks for any app that's using property transition animations...