Bug 1923344 - r=smaug
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 08 Oct 2024 16:25:12 +0000 (9 months ago)
changeset 757087 0ee07613d0506da465539cfaff1826cdc8bf0384
parent 757086 3c4e941c5578d1874bae9f523a86b2f9a0364891
child 757165 fb6534af58dde1e08a7ab8314994d2b8ce3b5ab0
push id42216
push userarchaeopteryx@coole-files.de
push dateTue, 08 Oct 2024 16:57:30 +0000 (9 months ago)
treeherdermozilla-central@0ee07613d050 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1923344
milestone133.0a1
first release with
nightly linux32
0ee07613d050 / 133.0a1 / 20241008165730 / files
nightly linux64
0ee07613d050 / 133.0a1 / 20241008165730 / files
nightly mac
0ee07613d050 / 133.0a1 / 20241008165730 / files
nightly win32
0ee07613d050 / 133.0a1 / 20241008165730 / files
nightly win64
0ee07613d050 / 133.0a1 / 20241008165730 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1923344 - r=smaug Differential Revision: https://phabricator.services.mozilla.com/D224958
dom/animation/AnimationTimeline.cpp
dom/animation/DocumentTimeline.cpp
dom/animation/ScrollTimelineAnimationTracker.cpp
layout/base/nsRefreshDriver.cpp
--- a/dom/animation/AnimationTimeline.cpp
+++ b/dom/animation/AnimationTimeline.cpp
@@ -35,71 +35,64 @@ AnimationTimeline::AnimationTimeline(nsI
   MOZ_ASSERT(mWindow);
 }
 
 AnimationTimeline::~AnimationTimeline() { mAnimationOrder.clear(); }
 
 bool AnimationTimeline::Tick(TickState& aState) {
   bool needsTicks = false;
 
-  nsTArray<Animation*> animationsToRemove;
-
-  for (Animation* animation = mAnimationOrder.getFirst(); animation;
-       animation =
-           static_cast<LinkedListElement<Animation>*>(animation)->getNext()) {
+  AutoTArray<RefPtr<Animation>, 32> animationsToTick;
+  for (Animation* animation : mAnimationOrder) {
     MOZ_ASSERT(mAnimations.Contains(animation),
                "The sampling order list should be a subset of the hashset");
     MOZ_ASSERT(!animation->IsHiddenByContentVisibility(),
                "The sampling order list should not contain any animations "
                "that are hidden by content-visibility");
+    animationsToTick.AppendElement(animation);
+  }
 
+  for (Animation* animation : animationsToTick) {
     // Skip any animations that are longer need associated with this timeline.
     if (animation->GetTimeline() != this) {
-      // If animation has some other timeline, it better not be also in the
-      // animation list of this timeline object!
-      MOZ_ASSERT(!animation->GetTimeline());
-      animationsToRemove.AppendElement(animation);
+      RemoveAnimation(animation);
       continue;
     }
 
     needsTicks |= animation->NeedsTicks();
-    // Even if |animation| doesn't need future ticks, we should still
-    // Tick it this time around since it might just need a one-off tick in
-    // order to dispatch events.
+    // Even if |animation| doesn't need future ticks, we should still Tick it
+    // this time around since it might just need a one-off tick in order to
+    // queue events.
     animation->Tick(aState);
-
     if (!animation->NeedsTicks()) {
-      animationsToRemove.AppendElement(animation);
+      RemoveAnimation(animation);
     }
   }
 
-  for (Animation* animation : animationsToRemove) {
-    RemoveAnimation(animation);
-  }
-
   return needsTicks;
 }
 
 void AnimationTimeline::NotifyAnimationUpdated(Animation& aAnimation) {
   if (mAnimations.EnsureInserted(&aAnimation)) {
     if (aAnimation.GetTimeline() && aAnimation.GetTimeline() != this) {
       aAnimation.GetTimeline()->RemoveAnimation(&aAnimation);
     }
     if (!aAnimation.IsHiddenByContentVisibility()) {
       mAnimationOrder.insertBack(&aAnimation);
     }
   }
 }
 
 void AnimationTimeline::RemoveAnimation(Animation* aAnimation) {
-  MOZ_ASSERT(!aAnimation->GetTimeline() || aAnimation->GetTimeline() == this);
-  if (static_cast<LinkedListElement<Animation>*>(aAnimation)->isInList()) {
+  if (static_cast<LinkedListElement<Animation>*>(aAnimation)->isInList() &&
+      MOZ_LIKELY(!aAnimation->GetTimeline() ||
+                 aAnimation->GetTimeline() == this)) {
+    static_cast<LinkedListElement<Animation>*>(aAnimation)->remove();
     MOZ_ASSERT(mAnimations.Contains(aAnimation),
                "The sampling order list should be a subset of the hashset");
-    static_cast<LinkedListElement<Animation>*>(aAnimation)->remove();
   }
   mAnimations.Remove(aAnimation);
 }
 
 void AnimationTimeline::NotifyAnimationContentVisibilityChanged(
     Animation* aAnimation, bool aIsVisible) {
   bool inList =
       static_cast<LinkedListElement<Animation>*>(aAnimation)->isInList();
--- a/dom/animation/DocumentTimeline.cpp
+++ b/dom/animation/DocumentTimeline.cpp
@@ -155,17 +155,22 @@ void DocumentTimeline::NotifyAnimationUp
                  "We should not register with the refresh driver if we are not"
                  " in the document's list of timelines");
       refreshDriver->EnsureAnimationUpdate();
     }
   }
 }
 
 void DocumentTimeline::TriggerAllPendingAnimationsNow() {
+  AutoTArray<RefPtr<Animation>, 32> animationsToTrigger;
   for (Animation* animation : mAnimationOrder) {
+    animationsToTrigger.AppendElement(animation);
+  }
+
+  for (Animation* animation : animationsToTrigger) {
     animation->TryTriggerNow();
   }
 }
 
 void DocumentTimeline::WillRefresh() {
   if (!mDocument->GetPresShell()) {
     // If we're not displayed, don't tick animations.
     return;
@@ -183,19 +188,16 @@ void DocumentTimeline::WillRefresh() {
   }
   // We already assert that GetRefreshDriver() is non-null at the beginning
   // of this function but we check it again here to be sure that ticking
   // animations does not have any side effects that cause us to lose the
   // connection with the refresh driver, such as triggering the destruction
   // of mDocument's PresShell.
   if (nsRefreshDriver* refreshDriver = GetRefreshDriver()) {
     refreshDriver->EnsureAnimationUpdate();
-  } else {
-    MOZ_ASSERT_UNREACHABLE(
-        "Refresh driver should still be valid at end of WillRefresh");
   }
 }
 
 void DocumentTimeline::RemoveAnimation(Animation* aAnimation) {
   AnimationTimeline::RemoveAnimation(aAnimation);
 }
 
 void DocumentTimeline::NotifyAnimationContentVisibilityChanged(
--- a/dom/animation/ScrollTimelineAnimationTracker.cpp
+++ b/dom/animation/ScrollTimelineAnimationTracker.cpp
@@ -8,23 +8,20 @@
 
 #include "mozilla/dom/Document.h"
 
 namespace mozilla {
 
 NS_IMPL_CYCLE_COLLECTION(ScrollTimelineAnimationTracker, mPendingSet, mDocument)
 
 void ScrollTimelineAnimationTracker::TriggerPendingAnimations() {
-  for (auto iter = mPendingSet.begin(), end = mPendingSet.end(); iter != end;
-       ++iter) {
-    dom::Animation* animation = *iter;
-
+  for (RefPtr<dom::Animation>& animation :
+       ToTArray<AutoTArray<RefPtr<dom::Animation>, 32>>(mPendingSet)) {
     MOZ_ASSERT(animation->GetTimeline() &&
                !animation->GetTimeline()->IsMonotonicallyIncreasing());
-
     // FIXME: Trigger now may not be correct because the spec says:
     // If a user agent determines that animation is immediately ready, it may
     // schedule the task (i.e. ResumeAt()) as a microtask such that it runs at
     // the next microtask checkpoint, but it must not perform the task
     // synchronously.
     // Note: So, for now, we put the animation into the tracker, and trigger
     // them immediately until the frames are ready. Using TriggerOnNextTick()
     // for scroll-driven animations may have issues because we don't tick if
@@ -34,15 +31,13 @@ void ScrollTimelineAnimationTracker::Tri
       // inactive. It's pretty hard to tell its future status, for example, it's
       // possible that the scroll container is in display:none subtree but the
       // animating element isn't the subtree, then we need to keep tracking the
       // situation until the scroll container gets framed. so in general we make
       // this animation be pending (i.e. not ready) if its scroll-timeline is
       // inactive, and this also matches the current spec definition.
       continue;
     }
-
-    // Note: Remove() is legitimately called once per entry during the loop.
-    mPendingSet.Remove(iter);
+    mPendingSet.Remove(animation);
   }
 }
 
 }  // namespace mozilla
--- a/layout/base/nsRefreshDriver.cpp
+++ b/layout/base/nsRefreshDriver.cpp
@@ -2310,18 +2310,25 @@ void nsRefreshDriver::DetermineProximity
                                                        ShouldCollect);
 
   for (const RefPtr<Document>& doc : documents) {
     MOZ_KnownLive(doc)->DetermineProximityToViewportAndNotifyResizeObservers();
   }
 }
 
 static CallState UpdateAndReduceAnimations(Document& aDocument) {
-  for (DocumentTimeline* timeline : aDocument.Timelines()) {
-    timeline->WillRefresh();
+  {
+    AutoTArray<RefPtr<DocumentTimeline>, 32> timelinesToTick;
+    for (DocumentTimeline* timeline : aDocument.Timelines()) {
+      timelinesToTick.AppendElement(timeline);
+    }
+
+    for (DocumentTimeline* tl : timelinesToTick) {
+      tl->WillRefresh();
+    }
   }
 
   if (nsPresContext* pc = aDocument.GetPresContext()) {
     if (pc->EffectCompositor()->NeedsReducing()) {
       pc->EffectCompositor()->ReduceAnimations();
     }
   }
   aDocument.EnumerateSubDocuments(UpdateAndReduceAnimations);
@@ -2341,17 +2348,18 @@ void nsRefreshDriver::UpdateAnimationsAn
     // run these, however, until we have fully updated the animation state. As
     // per the "update animations and send events" procedure[1], we should
     // remove replaced animations and then run these microtasks before
     // dispatching the corresponding animation events.
     //
     // [1]:
     // https://drafts.csswg.org/web-animations-1/#update-animations-and-send-events
     nsAutoMicroTask mt;
-    UpdateAndReduceAnimations(*mPresContext->Document());
+    RefPtr doc = mPresContext->Document();
+    UpdateAndReduceAnimations(*doc);
   }
 
   // Hold all AnimationEventDispatcher in mAnimationEventFlushObservers as
   // a RefPtr<> array since each AnimationEventDispatcher might be destroyed
   // during processing the previous dispatcher.
   AutoTArray<RefPtr<AnimationEventDispatcher>, 16> dispatchers;
   dispatchers.AppendElements(mAnimationEventFlushObservers);
   mAnimationEventFlushObservers.Clear();