Bug 1245260 - Ignore redundant calls to RestyleManager::IncrementAnimationGeneration; r=dbaron a=ritu
authorBrian Birtles <birtles@gmail.com>
Wed, 16 Mar 2016 15:05:10 +0800
changeset 325568 cb2cfeb012658983883b51021a54a57bd2c69c89
parent 325567 407be5a3b4f28626900f16c15a24a4420d1a56d0
child 325569 93256cc82d8940b8955b1d54ac4645b12810c3aa
push id1128
push userjlund@mozilla.com
push dateWed, 01 Jun 2016 01:31:59 +0000
treeherdermozilla-release@fe0d30de989d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron, ritu
Bug 1245260 - Ignore redundant calls to RestyleManager::IncrementAnimationGeneration; r=dbaron a=ritu While processing restyles and starting transitions, we may trigger a call to EffectCompositor::UpdateCascadeResults which may, in turn, call EffectCompositor::RequestRestyle with RestyleType::Layer, which ultimately results in a call to RestyleManager::IncrementAnimationGeneration(). Typically, nsTransitionManager::StyleContextChanged compares the animation generation on its collection with that of the restyle manager and uses this to ignore the restyle that it generates. However, given the sequence of events above, that check may no longer help since the restyle manager's animation generation will be out of step. As a result, nsTransitionManager::StyleContextChanged will fail to ignore a subsequent and redundant restyle. With certain combinations of content, this can mean that restyles are posted in such a manner than an infinite cycle of restyles ensues. This patch causes RestyleManager to ignore calls to IncrementAnimationGeneration when it is already processing restyles such that the animation generation is only ever updated once per restyle. This makes the check for a matching animation generation in nsTransitionManager::StyleContextChanged work as expected, preventing us from generating needless transitions which can produce this endless loop. MozReview-Commit-ID: 9HYDrknKPAI
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -93,19 +93,17 @@ RestyleManager::RestyleManager(nsPresCon
   , mRebuildAllExtraHint(nsChangeHint(0))
   , mRebuildAllRestyleHint(nsRestyleHint(0))
   , mAnimationGeneration(0)
   , mReframingStyleContexts(nullptr)
   , mAnimationsWithDestroyedFrame(nullptr)
-#ifdef DEBUG
   , mIsProcessingRestyles(false)
   , mLoggingDepth(0)
@@ -1722,28 +1720,26 @@ RestyleManager::ProcessPendingRestyles()
   // First do any queued-up frame creation.  (We should really
   // merge this into the rest of the process, though; see bug 827239.)
   // Process non-animation restyles...
              "Nesting calls to ProcessPendingRestyles?");
-#ifdef DEBUG
   mIsProcessingRestyles = true;
   // Before we process any restyles, we need to ensure that style
   // resulting from any animations is up-to-date, so that if any style
   // changes we cause trigger transitions, we have the correct old style
   // for starting the transition.
   bool haveNonAnimation =
     mHavePendingNonAnimationRestyles || mDoRebuildAllStyleData;
   if (haveNonAnimation) {
-    IncrementAnimationGeneration();
+    ++mAnimationGeneration;
   } else {
     // If we don't have non-animation style updates, then we have queued
     // up animation style updates from the refresh driver tick.  This
     // doesn't necessarily include *all* animation style updates, since
     // we might be suppressing main-thread updates for some animations,
     // so we don't want to call UpdateOnlyAnimationStyles, which updates
     // all animations.  In other words, the work that we're about to do
@@ -1766,19 +1762,17 @@ RestyleManager::ProcessPendingRestyles()
   if (!haveNonAnimation) {
-#ifdef DEBUG
   mIsProcessingRestyles = false;
   NS_ASSERTION(haveNonAnimation || !mHavePendingNonAnimationRestyles,
                "should not have added restyles");
   mHavePendingNonAnimationRestyles = false;
   if (mDoRebuildAllStyleData) {
     // We probably wasted a lot of work up above, but this seems safest
     // and it should be rarely used.
--- a/layout/base/RestyleManager.h
+++ b/layout/base/RestyleManager.h
@@ -103,17 +103,24 @@ public:
   static uint64_t GetAnimationGenerationForFrame(nsIFrame* aFrame);
   // Update the animation generation count to mark that animation state
   // has changed.
   // This is normally performed automatically by ProcessPendingRestyles
   // but it is also called when we have out-of-band changes to animations
   // such as changes made through the Web Animations API.
-  void IncrementAnimationGeneration() { ++mAnimationGeneration; }
+  void IncrementAnimationGeneration() {
+    // We update the animation generation at start of each call to
+    // ProcessPendingRestyles so we should ignore any subsequent (redundant)
+    // calls that occur while we are still processing restyles.
+    if (!mIsProcessingRestyles) {
+      ++mAnimationGeneration;
+    }
+  }
   // Whether rule matching should skip styles associated with animation
   bool SkipAnimationRules() const { return mSkipAnimationRules; }
   void SetSkipAnimationRules(bool aSkipAnimationRules) {
     mSkipAnimationRules = aSkipAnimationRules;
@@ -560,19 +567,21 @@ private:
   // Used to keep the layer and animation manager in sync.
   uint64_t mAnimationGeneration;
   ReframingStyleContexts* mReframingStyleContexts;
   AnimationsWithDestroyedFrame* mAnimationsWithDestroyedFrame;
   RestyleTracker mPendingRestyles;
-#ifdef DEBUG
+  // Are we currently in the middle of a call to ProcessRestyles?
+  // This flag is used both as a debugging aid to assert that we are not
+  // performing nested calls to ProcessPendingRestyles, as well as to ignore
+  // redundant calls to IncrementAnimationGeneration.
   bool mIsProcessingRestyles;
   int32_t mLoggingDepth;
  * An ElementRestyler is created for *each* element in a subtree that we