Call Element::rebuildLayoutTree from Document::updateStyle directly

third_party/WebKit/Source/core/dom/Element.cpp

Index: third_party/WebKit/Source/core/dom/Element.cpp
diff --git a/third_party/WebKit/Source/core/dom/Element.cpp b/third_party/WebKit/Source/core/dom/Element.cpp
index 2fde2c526802721659102806ad693fc66f3f70b6..3d45bf5ab298471e5b0f13b3193ab3818819a9d2 100644
--- a/third_party/WebKit/Source/core/dom/Element.cpp
+++ b/third_party/WebKit/Source/core/dom/Element.cpp
@@ -1884,7 +1884,6 @@ PassRefPtr<ComputedStyle> Element::originalStyleForLayoutObject() {
 void Element::recalcStyle(StyleRecalcChange change, Text* nextTextSibling) {
   DCHECK(document().inStyleRecalc());
   DCHECK(!document().lifecycle().inDetach());
-  DCHECK(!parentOrShadowHostNode()->needsStyleRecalc());

[esprehn, 2017/02/16 05:49:25]

ditto, entering into here with a dirty style is a serious bug, it means you're
inherting from a dirty parent.

[nainar, 2017/02/16 06:47:54]

This isn't true anymore in case we need this information in
reattachLayoutTree(). Can bring it back in the case needsReattachLayoutTree is
false. 

[esprehn, 2017/02/16 06:54:12]

Hmm? We should have cleared all the bits by calling attach() which then computes
the style in order. Can you give a short example?

[nainar, 2017/02/16 07:13:20]

Test case:

<div style="display:none">
  <input type="image" id="image" list="dl1">
</div>

   DCHECK(inActiveDocument());
 
   if (hasCustomStyleCallbacks())
@@ -1901,16 +1900,20 @@ void Element::recalcStyle(StyleRecalcChange change, Text* nextTextSibling) {
           elementAnimations->setAnimationStyleChange(false);
       }
     }
-    if (parentComputedStyle())
+    if (parentComputedStyle()) {
       change = recalcOwnStyle(change, nextTextSibling);
-    clearNeedsStyleRecalc();
-    clearNeedsReattachLayoutTree();
+    } else {
+      // In case we don't perform recalcOwnStyle we will never clear the
+      // NeedsReattachLayoutTree flag which is set on the creation of each
+      // Node. Clear that here.
+      clearNeedsReattachLayoutTree();

[esprehn, 2017/02/16 05:49:25]

why would this be true? We should still follow the childNeeds bits in the next
phase and eventually get here and clear it? The entry won't be in the map
though.

How do we get here with the NeedsReattachLayoutTree bit set, but not the
ancestor bits set? I don't think you need this code.

[nainar, 2017/02/16 06:47:54]

Done.

+    }
   }
 
-  // If we reattached we don't need to recalc the style of our descendants
-  // anymore.
-  if ((change >= UpdatePseudoElements && change < Reattach) ||
-      childNeedsStyleRecalc()) {
+  // If we are going to reattach we don't need to recalc the style of
+  // our descendants anymore.
+  if (change < Reattach &&
+      (change >= UpdatePseudoElements || childNeedsStyleRecalc())) {
     SelectorFilterParentScope filterScope(*this);
     StyleSharingDepthScope sharingScope(*this);
 
@@ -1936,9 +1939,11 @@ void Element::recalcStyle(StyleRecalcChange change, Text* nextTextSibling) {
                         childNeedsStyleRecalc() ? Force : change);
 
     clearChildNeedsStyleRecalc();
-    clearChildNeedsReattachLayoutTree();
   }
 
+  if (!needsReattachLayoutTree() && !childNeedsReattachLayoutTree())
+    clearNeedsStyleRecalc();
+
   if (hasCustomStyleCallbacks())
     didRecalcStyle();
 }
@@ -1968,7 +1973,6 @@ PassRefPtr<ComputedStyle> Element::propagateInheritedProperties(
 StyleRecalcChange Element::recalcOwnStyle(StyleRecalcChange change,
                                           Text* nextTextSibling) {
   DCHECK(document().inStyleRecalc());
-  DCHECK(!parentOrShadowHostNode()->needsStyleRecalc());

[esprehn, 2017/02/16 05:49:25]

Add this back, entering into recalcOwnStyle with a dirty style on the parent is
a bug.

[nainar, 2017/02/16 06:47:54]

This isn't true anymore in case we need this information in
reattachLayoutTree(). Can bring it back in the case needsReattachLayoutTree is
false. 

[esprehn, 2017/02/16 06:54:12]

This should be true, we're only leaving the style bits behind on the attach
roots, and we should never traverse down into recalcOwnStyle on anything under
an attach root. Can you give a short markup example?

[nainar, 2017/02/16 07:13:20]

Same test case as above.

I see what you mean here though. Debugging this. 

   DCHECK(change >= IndependentInherit || needsStyleRecalc());
   DCHECK(parentComputedStyle());
 
@@ -1996,7 +2000,7 @@ StyleRecalcChange Element::recalcOwnStyle(StyleRecalcChange change,
     styleReattachData.nextTextSibling = nextTextSibling;
     document().addStyleReattachData(*this, styleReattachData);
     setNeedsReattachLayoutTree();
-    return rebuildLayoutTree();
+    return Reattach;
   }
 
   DCHECK(oldStyle);
@@ -2037,23 +2041,32 @@ StyleRecalcChange Element::recalcOwnStyle(StyleRecalcChange change,
   return localChange;
 }
 
-StyleRecalcChange Element::rebuildLayoutTree() {
+void Element::rebuildLayoutTree() {
   DCHECK(inActiveDocument());
+  DCHECK(parentNode());
+
   StyleReattachData styleReattachData = document().getStyleReattachData(*this);
   AttachContext reattachContext;
   reattachContext.resolvedStyle = styleReattachData.computedStyle.get();
   bool layoutObjectWillChange = needsAttach() || layoutObject();
 
-  // We are calling Element::rebuildLayoutTree() from inside
-  // Element::recalcOwnStyle where we set the NeedsReattachLayoutTree
-  // flag - so needsReattachLayoutTree() should always be true.
-  DCHECK(parentNode());
-  DCHECK(parentNode()->childNeedsReattachLayoutTree());
-  DCHECK(needsReattachLayoutTree());
-  reattachLayoutTree(reattachContext);
-  // Since needsReattachLayoutTree() is always true we go into
-  // reattachLayoutTree() which reattaches all the descendant
-  // sub-trees. At this point no child should need reattaching.
+  if (needsReattachLayoutTree()) {
+    reattachLayoutTree(reattachContext);
+  } else if (childNeedsReattachLayoutTree()) {
+    DCHECK(!needsReattachLayoutTree());
+    SelectorFilterParentScope filterScope(*this);
+    StyleSharingDepthScope sharingScope(*this);
+    reattachPseudoElementLayoutTree(PseudoIdBefore);
+    rebuildShadowRootLayoutTree();
+    rebuildChildrenLayoutTrees();
+    reattachPseudoElementLayoutTree(PseudoIdAfter);
+    reattachPseudoElementLayoutTree(PseudoIdBackdrop);
+    reattachPseudoElementLayoutTree(PseudoIdFirstLetter);
+    DCHECK(!needsStyleRecalc());

[esprehn, 2017/02/16 05:49:25]

remove, you check it below

[nainar, 2017/02/16 06:47:54]

Done.

+  }
+  DCHECK(!needsStyleRecalc());
+  DCHECK(!childNeedsStyleRecalc());
+  DCHECK(!needsReattachLayoutTree());
   DCHECK(!childNeedsReattachLayoutTree());
 
   if (layoutObjectWillChange || layoutObject()) {
@@ -2062,9 +2075,27 @@ StyleRecalcChange Element::rebuildLayoutTree() {
     // or store it.
     // The choice is between increased time and increased memory complexity.
     reattachWhitespaceSiblingsIfNeeded(styleReattachData.nextTextSibling);
-    return Reattach;
+  } else {
+    clearNeedsStyleRecalc();

[esprehn, 2017/02/16 05:49:25]

You DCHECK(!needsStyleRecalc()); right above this, so this code can be removed

[nainar, 2017/02/16 06:47:54]

Done.

+  }
+}
+
+void Element::rebuildShadowRootLayoutTree() {
+  for (ShadowRoot* root = youngestShadowRoot(); root;
+       root = root->olderShadowRoot()) {
+    if (root->needsReattachLayoutTree() || root->childNeedsReattachLayoutTree())
+      root->rebuildLayoutTree();
+  }
+}
+
+void Element::reattachPseudoElementLayoutTree(PseudoId pseudoId) {
+  if (PseudoElement* element = pseudoElement(pseudoId)) {
+    if (element->needsReattachLayoutTree() ||
+        element->childNeedsReattachLayoutTree())
+      element->rebuildLayoutTree();
+  } else {
+    createPseudoElementIfNeeded(pseudoId);
   }
-  return ReattachNoLayoutObject;
 }
 
 void Element::updateCallbackSelectors(const ComputedStyle* oldStyle,
@@ -3214,7 +3245,6 @@ void Element::cancelFocusAppearanceUpdate() {
 }
 
 void Element::updatePseudoElement(PseudoId pseudoId, StyleRecalcChange change) {
-  DCHECK(!needsStyleRecalc());
   PseudoElement* element = pseudoElement(pseudoId);
 
   if (element && (change == UpdatePseudoElements ||