Bug 1541767 - Don't post animation restyles when unbinding an element; r=hiro
authorBrian Birtles <birtles@gmail.com>
Thu, 18 Apr 2019 06:49:25 +0000
changeset 470033 0d047d6f122b7fec64ce39da784f10e181ce8d66
parent 470032 a751128e51f71ddd7df3eaa1ba8c5c34b6876e31
child 470034 928f042ca74e2b75be1eace7fcc138ba571fd3a0
push id112839
push userapavel@mozilla.com
push dateThu, 18 Apr 2019 21:50:57 +0000
treeherdermozilla-inbound@e0a826fcd85b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1541767, 1223445, 1396041
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1541767 - Don't post animation restyles when unbinding an element; r=hiro Currently we avoid posting animation restyles when unbinding an element by removing the element from the document before deleting its animation collections. As a result, when canceled animations go to post a restyle, they can't find a pres context and give up posting a restyle. However, this is problematic for two reasons: * It means we can't remove such canceled animations from the PendingAnimationTracker if they are present there (i.e. it regresses the fix from bug 1223445). * It means we can't post cancel events for such animations/transitions since we can't lookup the appropriate AnimationEventDispatcher. In the next patch in this series we will change that order to fix the above problems but before we do that, we need to introduce another mechanism to make sure that we don't post restyles when unbinding an element or else we will regress bug 1396041. This patch does that by introducing a flag which causes us to not post restyles when we are doing DOM surgery. For all other cases we actually _do_ need to post restyles in order to update the style correctly. Without this patch, layout/style/crashtests/1396041.html would fail after applying the next patch in this series. Differential Revision: https://phabricator.services.mozilla.com/D28021
dom/animation/Animation.cpp
dom/animation/Animation.h
dom/animation/KeyframeEffect.cpp
dom/animation/KeyframeEffect.h
dom/animation/PostRestyleMode.h
dom/animation/moz.build
layout/style/AnimationCollection.cpp
layout/style/AnimationCollection.h
layout/style/nsAnimationManager.cpp
layout/style/nsAnimationManager.h
layout/style/nsTransitionManager.cpp
layout/style/nsTransitionManager.h
--- a/dom/animation/Animation.cpp
+++ b/dom/animation/Animation.cpp
@@ -463,18 +463,22 @@ Promise* Animation::GetFinished(ErrorRes
   }
   if (mFinishedIsResolved) {
     MaybeResolveFinishedPromise();
   }
   return mFinished;
 }
 
 // https://drafts.csswg.org/web-animations/#cancel-an-animation
-void Animation::Cancel() {
+void Animation::Cancel(PostRestyleMode aPostRestyle) {
+  bool newlyIdle = false;
+
   if (PlayState() != AnimationPlayState::Idle) {
+    newlyIdle = true;
+
     ResetPendingTasks();
 
     if (mFinished) {
       mFinished->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
     }
     ResetFinishedPromise();
 
     QueuePlaybackEvent(NS_LITERAL_STRING("cancel"),
@@ -483,24 +487,26 @@ void Animation::Cancel() {
 
   StickyTimeDuration activeTime =
       mEffect ? mEffect->GetComputedTiming().mActiveTime : StickyTimeDuration();
 
   mHoldTime.SetNull();
   mStartTime.SetNull();
 
   // Allow our effect to remove itself from the its target element's EffectSet.
-  UpdateEffect();
+  UpdateEffect(aPostRestyle);
 
   if (mTimeline) {
     mTimeline->RemoveAnimation(this);
   }
   MaybeQueueCancelEvent(activeTime);
 
-  PostUpdate();
+  if (newlyIdle && aPostRestyle == PostRestyleMode::IfNeeded) {
+    PostUpdate();
+  }
 }
 
 // https://drafts.csswg.org/web-animations/#finish-an-animation
 void Animation::Finish(ErrorResult& aRv) {
   double effectivePlaybackRate = CurrentOrPendingPlaybackRate();
 
   if (effectivePlaybackRate == 0 ||
       (effectivePlaybackRate > 0 && EffectEnd() == TimeDuration::Forever())) {
@@ -1205,17 +1211,17 @@ void Animation::PauseAt(const TimeDurati
   }
 }
 
 void Animation::UpdateTiming(SeekFlag aSeekFlag,
                              SyncNotifyFlag aSyncNotifyFlag) {
   // We call UpdateFinishedState before UpdateEffect because the former
   // can change the current time, which is used by the latter.
   UpdateFinishedState(aSeekFlag, aSyncNotifyFlag);
-  UpdateEffect();
+  UpdateEffect(PostRestyleMode::IfNeeded);
 
   if (mTimeline) {
     mTimeline->NotifyAnimationUpdated(*this);
   }
 }
 
 // https://drafts.csswg.org/web-animations/#update-an-animations-finished-state
 void Animation::UpdateFinishedState(SeekFlag aSeekFlag,
@@ -1260,23 +1266,23 @@ void Animation::UpdateFinishedState(Seek
   } else if (!currentFinishedState && mFinishedIsResolved) {
     ResetFinishedPromise();
   }
   // We must recalculate the current time to take account of any mHoldTime
   // changes the code above made.
   mPreviousCurrentTime = GetCurrentTimeAsDuration();
 }
 
-void Animation::UpdateEffect() {
+void Animation::UpdateEffect(PostRestyleMode aPostRestyle) {
   if (mEffect) {
     UpdateRelevance();
 
     KeyframeEffect* keyframeEffect = mEffect->AsKeyframeEffect();
     if (keyframeEffect) {
-      keyframeEffect->NotifyAnimationTimingUpdated();
+      keyframeEffect->NotifyAnimationTimingUpdated(aPostRestyle);
     }
   }
 }
 
 void Animation::FlushUnanimatedStyle() const {
   if (Document* doc = GetRenderedDocument()) {
     doc->FlushPendingNotifications(
         ChangesToFlush(FlushType::Style, false /* flush animations */));
--- a/dom/animation/Animation.h
+++ b/dom/animation/Animation.h
@@ -10,16 +10,17 @@
 #include "nsWrapperCache.h"
 #include "nsCycleCollectionParticipant.h"
 #include "mozilla/AnimationPerformanceWarning.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/CycleCollectedJSContext.h"
 #include "mozilla/DOMEventTargetHelper.h"
 #include "mozilla/EffectCompositor.h"  // For EffectCompositor::CascadeLevel
 #include "mozilla/LinkedList.h"
+#include "mozilla/PostRestyleMode.h"
 #include "mozilla/TimeStamp.h"             // for TimeStamp, TimeDuration
 #include "mozilla/dom/AnimationBinding.h"  // for AnimationPlayState
 #include "mozilla/dom/AnimationEffect.h"
 #include "mozilla/dom/AnimationTimeline.h"
 #include "mozilla/dom/Promise.h"
 #include "nsCSSPropertyID.h"
 #include "nsIGlobalObject.h"
 
@@ -121,17 +122,17 @@ class Animation : public DOMEventTargetH
   virtual bool PendingFromJS() const { return Pending(); }
 
   virtual Promise* GetReady(ErrorResult& aRv);
   Promise* GetFinished(ErrorResult& aRv);
 
   IMPL_EVENT_HANDLER(finish);
   IMPL_EVENT_HANDLER(cancel);
 
-  void Cancel();
+  void Cancel(PostRestyleMode aPostRestyle = PostRestyleMode::IfNeeded);
 
   void Finish(ErrorResult& aRv);
 
   virtual void Play(ErrorResult& aRv, LimitBehavior aLimitBehavior);
   virtual void PlayFromJS(ErrorResult& aRv) {
     Play(aRv, LimitBehavior::AutoRewind);
   }
 
@@ -422,17 +423,17 @@ class Animation : public DOMEventTargetH
    * to a seek or regular playback.
    */
   enum class SeekFlag { NoSeek, DidSeek };
 
   enum class SyncNotifyFlag { Sync, Async };
 
   virtual void UpdateTiming(SeekFlag aSeekFlag, SyncNotifyFlag aSyncNotifyFlag);
   void UpdateFinishedState(SeekFlag aSeekFlag, SyncNotifyFlag aSyncNotifyFlag);
-  void UpdateEffect();
+  void UpdateEffect(PostRestyleMode aPostRestyle);
   /**
    * Flush all pending styles other than throttled animation styles (e.g.
    * animations running on the compositor).
    */
   void FlushUnanimatedStyle() const;
   void PostUpdate();
   void ResetFinishedPromise();
   void MaybeResolveFinishedPromise();
--- a/dom/animation/KeyframeEffect.cpp
+++ b/dom/animation/KeyframeEffect.cpp
@@ -149,32 +149,34 @@ void KeyframeEffect::NotifySpecifiedTimi
     if (mAnimation->IsRelevant()) {
       nsNodeUtils::AnimationChanged(mAnimation);
     }
 
     RequestRestyle(EffectCompositor::RestyleType::Layer);
   }
 }
 
-void KeyframeEffect::NotifyAnimationTimingUpdated() {
+void KeyframeEffect::NotifyAnimationTimingUpdated(
+    PostRestyleMode aPostRestyle) {
   UpdateTargetRegistration();
 
   // If the effect is not relevant it will be removed from the target
   // element's effect set. However, effects not in the effect set
   // will not be included in the set of candidate effects for running on
   // the compositor and hence they won't have their compositor status
   // updated. As a result, we need to make sure we clear their compositor
   // status here.
   bool isRelevant = mAnimation && mAnimation->IsRelevant();
   if (!isRelevant) {
     ResetIsRunningOnCompositor();
   }
 
   // Request restyle if necessary.
-  if (mAnimation && !mProperties.IsEmpty() && HasComputedTimingChanged()) {
+  if (aPostRestyle == PostRestyleMode::IfNeeded && mAnimation &&
+      !mProperties.IsEmpty() && HasComputedTimingChanged()) {
     EffectCompositor::RestyleType restyleType =
         CanThrottle() ? EffectCompositor::RestyleType::Throttled
                       : EffectCompositor::RestyleType::Standard;
     RequestRestyle(restyleType);
   }
 
   // Detect changes to "in effect" status since we need to recalculate the
   // animation cascade for this element whenever that changes.
@@ -1657,17 +1659,17 @@ void KeyframeEffect::SetAnimation(Animat
 
   // The order of these function calls is important:
   // NotifyAnimationTimingUpdated() need the updated mIsRelevant flag to check
   // if it should create the effectSet or not, and MarkCascadeNeedsUpdate()
   // needs a valid effectSet, so we should call them in this order.
   if (mAnimation) {
     mAnimation->UpdateRelevance();
   }
-  NotifyAnimationTimingUpdated();
+  NotifyAnimationTimingUpdated(PostRestyleMode::IfNeeded);
   if (mAnimation) {
     MarkCascadeNeedsUpdate();
   }
 }
 
 bool KeyframeEffect::CanIgnoreIfNotVisible() const {
   if (!AnimationUtils::IsOffscreenThrottlingEnabled()) {
     return false;
--- a/dom/animation/KeyframeEffect.h
+++ b/dom/animation/KeyframeEffect.h
@@ -18,16 +18,17 @@
 #include "mozilla/AnimationPerformanceWarning.h"
 #include "mozilla/AnimationPropertySegment.h"
 #include "mozilla/AnimationTarget.h"
 #include "mozilla/Attributes.h"
 #include "mozilla/ComputedTimingFunction.h"
 #include "mozilla/EffectCompositor.h"
 #include "mozilla/Keyframe.h"
 #include "mozilla/KeyframeEffectParams.h"
+#include "mozilla/PostRestyleMode.h"
 // RawServoDeclarationBlock and associated RefPtrTraits
 #include "mozilla/ServoBindingTypes.h"
 #include "mozilla/StyleAnimationValue.h"
 #include "mozilla/dom/AnimationEffect.h"
 #include "mozilla/dom/BindingDeclarations.h"
 #include "mozilla/dom/Element.h"
 
 struct JSContext;
@@ -169,17 +170,17 @@ class KeyframeEffect : public AnimationE
   IterationCompositeOperation IterationComposite() const;
   void SetIterationComposite(
       const IterationCompositeOperation& aIterationComposite);
 
   CompositeOperation Composite() const;
   void SetComposite(const CompositeOperation& aComposite);
 
   void NotifySpecifiedTimingUpdated();
-  void NotifyAnimationTimingUpdated();
+  void NotifyAnimationTimingUpdated(PostRestyleMode aPostRestyle);
   void RequestRestyle(EffectCompositor::RestyleType aRestyleType);
   void SetAnimation(Animation* aAnimation) override;
   void SetKeyframes(JSContext* aContext, JS::Handle<JSObject*> aKeyframes,
                     ErrorResult& aRv);
   void SetKeyframes(nsTArray<Keyframe>&& aKeyframes,
                     const ComputedStyle* aStyle);
 
   // Returns true if the effect includes a property in |aPropertySet| regardless
new file mode 100644
--- /dev/null
+++ b/dom/animation/PostRestyleMode.h
@@ -0,0 +1,16 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef mozilla_PostRestyleMode_h
+#define mozilla_PostRestyleMode_h
+
+namespace mozilla {
+
+enum class PostRestyleMode { IfNeeded, Never };
+
+}  // namespace mozilla
+
+#endif  // mozilla_PostRestyleMode_h
--- a/dom/animation/moz.build
+++ b/dom/animation/moz.build
@@ -29,16 +29,17 @@ EXPORTS.mozilla += [
     'ComputedTiming.h',
     'ComputedTimingFunction.h',
     'EffectCompositor.h',
     'EffectSet.h',
     'Keyframe.h',
     'KeyframeEffectParams.h',
     'KeyframeUtils.h',
     'PendingAnimationTracker.h',
+    'PostRestyleMode.h',
     'PseudoElementHashEntry.h',
     'TimingParams.h',
 ]
 
 UNIFIED_SOURCES += [
     'Animation.cpp',
     'AnimationEffect.cpp',
     'AnimationEventDispatcher.cpp',
--- a/layout/style/AnimationCollection.cpp
+++ b/layout/style/AnimationCollection.cpp
@@ -17,21 +17,25 @@ template <class AnimationType>
 /* static */ void AnimationCollection<AnimationType>::PropertyDtor(
     void* aObject, nsAtom* aPropertyName, void* aPropertyValue, void* aData) {
   AnimationCollection* collection =
       static_cast<AnimationCollection*>(aPropertyValue);
 #ifdef DEBUG
   MOZ_ASSERT(!collection->mCalledPropertyDtor, "can't call dtor twice");
   collection->mCalledPropertyDtor = true;
 #endif
+
+  PostRestyleMode postRestyle = collection->mCalledDestroy
+                                    ? PostRestyleMode::IfNeeded
+                                    : PostRestyleMode::Never;
   {
     nsAutoAnimationMutationBatch mb(collection->mElement->OwnerDoc());
 
     for (size_t animIdx = collection->mAnimations.Length(); animIdx-- != 0;) {
-      collection->mAnimations[animIdx]->CancelFromStyle();
+      collection->mAnimations[animIdx]->CancelFromStyle(postRestyle);
     }
   }
   delete collection;
 }
 
 template <class AnimationType>
 /* static */ AnimationCollection<AnimationType>*
 AnimationCollection<AnimationType>::GetAnimationCollection(
--- a/layout/style/AnimationCollection.h
+++ b/layout/style/AnimationCollection.h
@@ -31,35 +31,31 @@ struct AnimationTypeTraits {};
 
 template <class AnimationType>
 class AnimationCollection
     : public LinkedListElement<AnimationCollection<AnimationType>> {
   typedef AnimationCollection<AnimationType> SelfType;
   typedef AnimationTypeTraits<AnimationType> TraitsType;
 
   AnimationCollection(dom::Element* aElement, nsAtom* aElementProperty)
-      : mElement(aElement),
-        mElementProperty(aElementProperty)
-#ifdef DEBUG
-        ,
-        mCalledPropertyDtor(false)
-#endif
-  {
+      : mElement(aElement), mElementProperty(aElementProperty) {
     MOZ_COUNT_CTOR(AnimationCollection);
   }
 
  public:
   ~AnimationCollection() {
     MOZ_ASSERT(mCalledPropertyDtor,
                "must call destructor through element property dtor");
     MOZ_COUNT_DTOR(AnimationCollection);
     LinkedListElement<SelfType>::remove();
   }
 
   void Destroy() {
+    mCalledDestroy = true;
+
     // This will call our destructor.
     mElement->DeleteProperty(mElementProperty);
   }
 
   static void PropertyDtor(void* aObject, nsAtom* aPropertyName,
                            void* aPropertyValue, void* aData);
 
   // Get the collection of animations for the given |aElement| and
@@ -90,16 +86,26 @@ class AnimationCollection
   // i.e., in an atom list)
   nsAtom* mElementProperty;
 
   InfallibleTArray<RefPtr<AnimationType>> mAnimations;
 
  private:
   static nsAtom* GetPropertyAtomForPseudoType(PseudoStyleType aPseudoType);
 
+  // We distinguish between destroying this by calling Destroy() vs directly
+  // calling DeleteProperty on an element.
+  //
+  // The former case represents regular updating due to style changes and should
+  // trigger subsequent restyles.
+  //
+  // The latter case represents document tear-down or other DOM surgery in
+  // which case we should not trigger restyles.
+  bool mCalledDestroy = false;
+
 #ifdef DEBUG
-  bool mCalledPropertyDtor;
+  bool mCalledPropertyDtor = false;
 #endif
 };
 
 }  // namespace mozilla
 
 #endif  // mozilla_AnimationCollection_h
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -601,11 +601,11 @@ void nsAnimationManager::DoUpdateAnimati
       AddElementCollection(collection);
     }
   }
   collection->mAnimations.SwapElements(newAnimations);
 
   // Cancel removed animations
   for (size_t newAnimIdx = newAnimations.Length(); newAnimIdx-- != 0;) {
     aBuilder.NotifyNewOrRemovedAnimation(*newAnimations[newAnimIdx]);
-    newAnimations[newAnimIdx]->CancelFromStyle();
+    newAnimations[newAnimIdx]->CancelFromStyle(PostRestyleMode::IfNeeded);
   }
 }
--- a/layout/style/nsAnimationManager.h
+++ b/layout/style/nsAnimationManager.h
@@ -79,31 +79,31 @@ class CSSAnimation final : public Animat
   // following override), then we should update tabbrowser.xml to check
   // the playState instead.
   AnimationPlayState PlayStateFromJS() const override;
   bool PendingFromJS() const override;
   void PlayFromJS(ErrorResult& aRv) override;
 
   void PlayFromStyle();
   void PauseFromStyle();
-  void CancelFromStyle() {
+  void CancelFromStyle(PostRestyleMode aPostRestyle) {
     // When an animation is disassociated with style it enters an odd state
     // where its composite order is undefined until it first transitions
     // out of the idle state.
     //
     // Even if the composite order isn't defined we don't want it to be random
     // in case we need to determine the order to dispatch events associated
     // with an animation in this state. To solve this we treat the animation as
     // if it had been added to the end of the global animation list so that
     // its sort order is defined. We'll update this index again once the
     // animation leaves the idle state.
     mAnimationIndex = sNextAnimationIndex++;
     mNeedsNewAnimationIndexWhenRun = true;
 
-    Animation::Cancel();
+    Animation::Cancel(aPostRestyle);
 
     // We need to do this *after* calling Cancel() since
     // Cancel() might synchronously trigger a cancel event for which
     // we need an owning element to target the event at.
     mOwningElement = OwningElementRef();
   }
 
   void Tick() override;
--- a/layout/style/nsTransitionManager.cpp
+++ b/layout/style/nsTransitionManager.cpp
@@ -516,17 +516,17 @@ bool nsTransitionManager::DoUpdateTransi
           currentValue != anim->ToValue()) {
         // stop the transition
         if (anim->HasCurrentEffect()) {
           EffectSet* effectSet = EffectSet::GetEffectSet(aElement, aPseudoType);
           if (effectSet) {
             effectSet->UpdateAnimationGeneration(mPresContext);
           }
         }
-        anim->CancelFromStyle();
+        anim->CancelFromStyle(PostRestyleMode::IfNeeded);
         animations.RemoveElementAt(i);
       }
     } while (i != 0);
 
     if (animations.IsEmpty()) {
       aElementTransitions->Destroy();
       aElementTransitions = nullptr;
     }
@@ -663,17 +663,17 @@ bool nsTransitionManager::ConsiderInitia
       // We're in the middle of a transition, and just got a non-transition
       // style change to something that we can't animate.  This might happen
       // because we got a non-transition style change changing to the current
       // in-progress value (which is particularly easy to cause when we're
       // currently in the 'transition-delay').  It also might happen because we
       // just got a style change to a value that can't be interpolated.
       OwningCSSTransitionPtrArray& animations =
           aElementTransitions->mAnimations;
-      animations[currentIndex]->CancelFromStyle();
+      animations[currentIndex]->CancelFromStyle(PostRestyleMode::IfNeeded);
       oldPT = nullptr;  // Clear pointer so it doesn't dangle
       animations.RemoveElementAt(currentIndex);
       EffectSet* effectSet = EffectSet::GetEffectSet(aElement, aPseudoType);
       if (effectSet) {
         effectSet->UpdateAnimationGeneration(mPresContext);
       }
 
       if (animations.IsEmpty()) {
@@ -796,17 +796,17 @@ bool nsTransitionManager::ConsiderInitia
       const AnimationPropertySegment& segment =
           oldPT->Properties()[0].mSegments[0];
       pt->mReplacedTransition.emplace(
           ElementPropertyTransition::ReplacedTransitionProperties(
               {oldPT->GetAnimation()->GetStartTime().Value(),
                oldPT->GetAnimation()->PlaybackRate(), oldPT->SpecifiedTiming(),
                segment.mTimingFunction, segment.mFromValue, segment.mToValue}));
     }
-    animations[currentIndex]->CancelFromStyle();
+    animations[currentIndex]->CancelFromStyle(PostRestyleMode::IfNeeded);
     oldPT = nullptr;  // Clear pointer so it doesn't dangle
     animations[currentIndex] = animation;
   } else {
     if (!animations.AppendElement(animation)) {
       NS_WARNING("out of memory");
       return false;
     }
   }
--- a/layout/style/nsTransitionManager.h
+++ b/layout/style/nsTransitionManager.h
@@ -137,27 +137,27 @@ class CSSTransition final : public Anima
   // is expected to be called whilst already updating style.
   void PlayFromStyle() {
     ErrorResult rv;
     PlayNoUpdate(rv, Animation::LimitBehavior::Continue);
     // play() should not throw when LimitBehavior is Continue
     MOZ_ASSERT(!rv.Failed(), "Unexpected exception playing transition");
   }
 
-  void CancelFromStyle() {
+  void CancelFromStyle(PostRestyleMode aPostRestyle) {
     // The animation index to use for compositing will be established when
     // this transition next transitions out of the idle state but we still
     // update it now so that the sort order of this transition remains
     // defined until that moment.
     //
     // See longer explanation in CSSAnimation::CancelFromStyle.
     mAnimationIndex = sNextAnimationIndex++;
     mNeedsNewAnimationIndexWhenRun = true;
 
-    Animation::Cancel();
+    Animation::Cancel(aPostRestyle);
 
     // It is important we do this *after* calling Cancel().
     // This is because Cancel() will end up posting a restyle and
     // that restyle should target the *transitions* level of the cascade.
     // However, once we clear the owning element, CascadeLevel() will begin
     // returning CascadeLevel::Animations.
     mOwningElement = OwningElementRef();
   }