Bug 1350115 - Squelch post-traversal generated by additional animation traversals when we're styling a fresh subtree. r=heycam,r=birtles
authorBobby Holley <bobbyholley@gmail.com>
Thu, 23 Mar 2017 15:21:18 -0700
changeset 349495 e3afa3f3c4b2e62325f644ce5a8b92fa4de2b52b
parent 349494 9fc87b160c2b7a1751568e2882a022c3811a8488
child 349496 3d08ee7b2eb82b9f5ed4a838cb9cba1b04aef606
push id31552
push userkwierso@gmail.com
push dateSat, 25 Mar 2017 00:03:33 +0000
treeherdermozilla-central@f9acfdca68a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam, birtles
bugs1350115
milestone55.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 1350115 - Squelch post-traversal generated by additional animation traversals when we're styling a fresh subtree. r=heycam,r=birtles This patch exists to avoid a crash in layout/style/test/test_animations.html. We end up generating some ::before content, which causes us to style the new subtree at [1]. In StyleNewSubtree, we fail the !postTraversalRequired assertion because PrepareAndTraverseSubtree decided to traverse the tree twice (once to style it, and again to restyle it for animations), and return that a post-traversal is needed. The reason this issue happens with my NAC patches and not without is that we were previously filtering out generated ::before content from the servo traversal, so the servo traversal wouldn't have reached it and (presumably) the animation restyle wouldn't have happened and we wouldn't have returned true for needing a post-traversal. [1] http://searchfox.org/mozilla-central/rev/c48398abd9f0f074c69f2223260939e30e8f99a8/layout/base/nsCSSFrameConstructor.cpp#1918 MozReview-Commit-ID: 8tgzLjV8B3A
dom/base/Element.h
layout/base/ServoRestyleManager.cpp
layout/base/ServoRestyleManager.h
layout/style/ServoStyleSet.cpp
--- a/dom/base/Element.h
+++ b/dom/base/Element.h
@@ -448,17 +448,17 @@ public:
   }
 
   inline void NoteDirtyDescendantsForServo();
 
 #ifdef DEBUG
   inline bool DirtyDescendantsBitIsPropagatedForServo();
 #endif
 
-  bool HasServoData() {
+  bool HasServoData() const {
 #ifdef MOZ_STYLO
     return !!mServoData.Get();
 #else
     MOZ_CRASH("Accessing servo node data in non-stylo build");
 #endif
   }
 
   void ClearServoData();
--- a/layout/base/ServoRestyleManager.cpp
+++ b/layout/base/ServoRestyleManager.cpp
@@ -106,20 +106,18 @@ ServoRestyleManager::ClearServoDataFromS
       ClearServoDataFromSubtree(n->AsElement());
     }
   }
 
   aElement->UnsetHasDirtyDescendantsForServo();
 }
 
 
-// Clears HasDirtyDescendants and RestyleData from all elements in the
-// subtree rooted at aElement.
-static void
-ClearRestyleStateFromSubtree(Element* aElement)
+/* static */ void
+ServoRestyleManager::ClearRestyleStateFromSubtree(Element* aElement)
 {
   if (aElement->HasDirtyDescendantsForServo()) {
     StyleChildrenIterator it(aElement);
     for (nsIContent* n = it.GetNextChild(); n; n = it.GetNextChild()) {
       if (n->IsElement()) {
         ClearRestyleStateFromSubtree(n->AsElement());
       }
     }
--- a/layout/base/ServoRestyleManager.h
+++ b/layout/base/ServoRestyleManager.h
@@ -87,16 +87,22 @@ public:
 
   /**
    * Clears the ServoElementData and HasDirtyDescendants from all elements
    * in the subtree rooted at aElement.
    */
   static void ClearServoDataFromSubtree(Element* aElement);
 
   /**
+   * Clears HasDirtyDescendants and RestyleData from all elements in the
+   * subtree rooted at aElement.
+   */
+  static void ClearRestyleStateFromSubtree(Element* aElement);
+
+  /**
    * Posts restyle hints for animations.
    * This is only called for the second traversal for CSS animations during
    * updating CSS animations in a SequentialTask.
    * This function does neither register a refresh observer nor flag that a
    * style flush is needed since this function is supposed to be called during
    * restyling process and this restyle event will be processed in the second
    * traversal of the same restyling process.
    */
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -215,24 +215,42 @@ ServoStyleSet::PrepareAndTraverseSubtree
 {
   // Get the Document's root element to ensure that the cache is valid before
   // calling into the (potentially-parallel) Servo traversal, where a cache hit
   // is necessary to avoid a data race when updating the cache.
   mozilla::Unused << aRoot->OwnerDoc()->GetRootElement();
 
   MOZ_ASSERT(!sInServoTraversal);
   sInServoTraversal = true;
+
+  bool isInitial = !aRoot->HasServoData();
   bool postTraversalRequired =
     Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior);
+  MOZ_ASSERT_IF(isInitial, !postTraversalRequired);
 
   // If there are still animation restyles needed, trigger a second traversal to
   // update CSS animations' styles.
-  if (mPresContext->EffectCompositor()->PreTraverse() &&
-      Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior)) {
-    postTraversalRequired = true;
+  if (mPresContext->EffectCompositor()->PreTraverse()) {
+    if (Servo_TraverseSubtree(aRoot, mRawSet.get(), aRootBehavior)) {
+      if (isInitial) {
+        // We're doing initial styling, and the additional animation
+        // traversal changed the styles that were set by the first traversal.
+        // This would normally require a post-traversal to update the style
+        // contexts, and the DOM now has dirty descendant bits and RestyleData
+        // in expectation of that post-traversal. But since this is actually
+        // the initial styling, there are no style contexts to update and no
+        // frames to apply the change hints to, so we don't need to do that
+        // post-traversal. Instead, just drop this state and tell the caller
+        // that no post-traversal is required.
+        MOZ_ASSERT(!postTraversalRequired);
+        ServoRestyleManager::ClearRestyleStateFromSubtree(const_cast<Element*>(aRoot));
+      } else {
+        postTraversalRequired = true;
+      }
+    }
   }
 
   sInServoTraversal = false;
   return postTraversalRequired;
 }
 
 already_AddRefed<nsStyleContext>
 ServoStyleSet::ResolveStyleFor(Element* aElement,