Closed Bug 1523229 Opened 9 months ago Closed 9 months ago

Don't hang onto animations from AnimationTimeline just because they're relevant

Categories

(Core :: DOM: Animation, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: birtles, Assigned: birtles)

References

Details

Attachments

(1 file)

In working on bug 1253476 I came across a situation where Animations weren't being garbage collected when I thought they could. I dug into it and discovered we're keeping a reference to Animations from their AnimationTimeline so long as they're relevant. I couldn't find any good reason why and everything seems to work without it so I'm going to try dropping it in this bug.

I want to land this separately in case it produces any regressions and because if it doesn't, we should land this regardless of what approach we end up taking in bug 1253476.

I don't know why this check was ever added. A comment here suggests we expected
irrelevant animations to be removed from their timeline:

https://hg.mozilla.org/mozilla-central/rev/8406c5300ab7051ae6fe9bf41a1d30261cf70a4a#l2.16

Furthermore, a comment in the changeset description for that same changeset
suggests that to be the case:

"For example, if a CSS animation is finished (IsRelevant() == false so that
animation will have been removed from the timeline)"

Another comment removed in that patch has:

"Note that we only store relevant animations on the timeline since they are
the only ones that need ticks and are the only ones returned from
AnimationTimeline::GetAnimations"

which suggests it was added a point when we had a GetAnimations() method on
AnimationTimeline and hence it was needed for that.

The other possibility is that we were preempting a point when timelines would
switch between being active and inactive:

"FIXME: Once we expect animations to go back and forth betweeen being inactive
and active, we will need to store more than just relevant animations on the
timeline. This is because an animation might be deemed irrelevant because its
timeline is inactive. If it is removed from the timeline at that point the
timeline will have no way of getting the animation to add itself again once it
becomes active."

Indeed, we might need this for ScrollTimelines. For now, however, it seems
unnecessary (a try run with simply this check removed passes all test).

(Furthermore, in bug 1253476 or one of its dependencies, this check will prevent
us from combining filling animations since the original (filling) animations
will be kept alive by the timeline. Should this indeed prove necessary for bug
1253476, that bug will add an automated test that will fail if we re-introduce
this condition.)

Pushed by bbirtles@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b17af654b9f1
Don't reference animations from AnimationTimeline just because they're relevant; r=hiro
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.