GNOME Bugzilla – Bug 642925
workspaceThumbnail: Remove tweens when the actor is destroyed
Last modified: 2011-02-22 17:38:33 UTC
Fix a situation where we end up with this exception in a loop (see patch for details): JS ERROR: !!! Exception in callback for signal: prepare-frame JS ERROR: !!! message = 'this.actor is null' JS ERROR: !!! lineNumber = '226' JS ERROR: !!! fileName = '/home/linux/gnome-shell/source/gnome-shell/js/ui/workspaceThumbnail.js' JS ERROR: !!! stack = '(0)@/home/linux/gnome-shell/source/gnome-shell/js/ui/workspaceThumbnail.js:226 _updateTweenByIndex(0)@/home/linux/gnome-shell/install/share/gjs-1.0/tweener/tweener.js:316 _updateTweens()@/home/linux/gnome-shell/install/share/gjs-1.0/tweener/tweener.js:344 _onEnterFrame([object Object])@/home/linux/gnome-shell/install/share/gjs-1.0/tweener/tweener.js:359 _emit("prepare-frame")@/home/linux/gnome-shell/install/share/gjs-1.0/signals.js:124 (13214)@/home/linux/gnome-shell/source/gnome-shell/js/ui/tweener.js:237 ([object _private_Clutter_Timeline],13214)@/home/linux/gnome-shell/source/gnome-shell/js/ui/tweener.js:213
Created attachment 181534 [details] [review] workspaceThumbnail: Remove tweens when the actor is destroyed ThumbnailsBox._updateStates tweens the slidePosition property on WorkspaceThumbnails which has a setter which calls queue_relayout() on the thumbnails's actor. When the actor is destroyed this call will throw an expection in an infinite loop. Fix this by removing the tweens when destroying the actor, at this point (actor destroyed) there is nothing to animate anyway.
This looks right to me, in that it corresponds to what we do in js/ui/tweener.js for actors - we automatically remove tweens from an actor when that actor is destroyed. Questions in my mind: 1) Should we maker tweener.js do this automatically if it's a JS object with an actor property that is a Clutter.Actor? Too magic? 2) Are there other cases where this problem occurs. Grepping for properties, reveals the following properties used for tweening (possibly may have missed a few): dash.js: get childScale() { dash.js: get childOpacity() { this.actor exists, this.actor can be destroyed; no obvious code path in which it would be destroyed while being tweened. messageTray.js: get _expandedSummaryItemTitleWidth() { this.actor exists, this.actor never destroyed windowManager.js: get dimFraction() { this.actor exists, this.actor can be destroyed, if mutter is working properly this.actor shouldn't be destroyed until after the tween finishes. workspacesView.js: get zoomFraction() { this.actor exists, this.actor never destroyed workspaceThumbnail.js: get slidePosition() { workspaceThumbnail.js: get collapseFraction() { this bug workspaceThumbnail.js: get scale() { workspaceThumbnail.js: get indicatorY() { this.actor exists, this.actor never destroyed So, there are some places where this bug might occur if things get changed around, but no other places where it obviously occurs. Dan: any opinion on whether magic handling of tweenedObject.actor makes sense?
we already do special-casing when tweening a ClutterActor directly, and make use of its ._delegate property in that case, so doing the reverse makes sense as well. If you have a JS object with an "actor" property which is a Clutter.Actor but which does not correspond to the JS object in the "normal" way, then you deserve to lose anyway, because it will just confuse people looking at the code.
Created attachment 181588 [details] [review] tweener: remove tweens when target.actor is destroyed We already remove tweens automatically when a Clutter actor is destroyed; do the same when the target is a JS delegate with an actor property.
I was able to reproduce the original problem on the first try without the patch I just attached, and did not succeed in reproducing without, so I think it does fix things properly.
Review of attachment 181588 [details] [review]: I cannot reproduce it with this patch applied (and mine reverted) either, I can reproduce it almost reliably with both reverted. ::: js/ui/tweener.js @@ +67,3 @@ + state.destroyedId = target.connect('destroy', _actorDestroyed); + else if (target.actor && target.actor instanceof Clutter.Actor) + target.actor.connect('destroy', function() { _actorDestroyed(target); }); Shouldn't you assign this to state.destroyedId too?
Created attachment 181589 [details] [review] tweener: remove tweens when target.actor is destroyed assign state.destroyedId (thanks, Adel!)
Comment on attachment 181589 [details] [review] tweener: remove tweens when target.actor is destroyed You'd need to update the "target.disconnect(state.destroyedId);" in _resetTweenState() too. It might be easier to have the state object have an "actor" property that is set to either target, target.actor, or null as appropriate, and then do the connect/disconnect on state.actor.
Created attachment 181601 [details] [review] tweener: remove tweens when target.actor is destroyed Try 3
Comment on attachment 181601 [details] [review] tweener: remove tweens when target.actor is destroyed Hm... I was thinking you'd only need a single "state.actor.connect('destroy')", but that would actually end up being more complicated to get it right. Looks good.
Thought of doing the single connect() but that meant allocating an extra closure object in the common case, which felt dirty, so I micro-optimized and wrote it this way. Attachment 181601 [details] pushed as 192d3a9 - tweener: remove tweens when target.actor is destroyed