Clear style recalc root removed from flat tree.

We are not allowed to have style recalc roots outside the flat tree with
FlatTreeStyleRecalc enabled. We already made sure we don't add any, but
did not clear any when the were removed outside the flat tree as a
result of a slot re-assignment or by attaching a shadow root.

Update the style recalc root when we call Node::RemovedFromFlatTree().

We now clear the style dirty bits as part of DetachLayoutTree() when
not performing a reattach to be able to figure out if the recalc root
was part of the subtree removed from the flat tree as the is no flag
similar to isConnected() that we can use to check this. Clearing the
dirty flags should be correct in this case anyway.

Bug: 1027829
Change-Id: Ibd30ba5c16c9ccb8390e5bfa5935649d1997425a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1949788
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Anders Hartvoll Ruud <andruud@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722022}

third_party/blink/renderer/core/css/style_engine.h

@@ -304,6 +304,9 @@ class CORE_EXPORT StyleEngine final : public GarbageCollected<StyleEngine>,
 
   void NodeWillBeRemoved(Node&);
   void ChildrenRemoved(ContainerNode& parent);
+  void RemovedFromFlatTree(Node& node) {
+    style_recalc_root_.RemovedFromFlatTree(node);
+  }
   void PseudoElementRemoved(Element& originating_element) {
     layout_tree_rebuild_root_.ChildrenRemoved(originating_element);
   }

third_party/blink/renderer/core/css/style_engine_test.cc

@@ -26,6 +26,7 @@
 #include "third_party/blink/renderer/core/dom/node_computed_style.h"
 #include "third_party/blink/renderer/core/dom/shadow_root.h"
 #include "third_party/blink/renderer/core/dom/shadow_root_init.h"
+#include "third_party/blink/renderer/core/dom/slot_assignment_engine.h"
 #include "third_party/blink/renderer/core/dom/text.h"
 #include "third_party/blink/renderer/core/frame/frame_test_helpers.h"
 #include "third_party/blink/renderer/core/frame/local_frame_view.h"
@@ -2447,4 +2448,40 @@ TEST_F(StyleEngineTest, StyleRecalcRootOutsideFlatTree) {
   UpdateAllLifecyclePhases();
 }
 
+TEST_F(StyleEngineTest, RemoveStyleRecalcRootFromFlatTree) {
+  ScopedFlatTreeStyleRecalcForTest scope(true);
+
+  GetDocument().body()->SetInnerHTMLFromString(R"HTML(
+    <div id=host><span></span></div>
+  )HTML");
+
+  auto* host = GetDocument().getElementById("host");
+  auto* span = To<Element>(host->firstChild());
+
+  ShadowRoot& shadow_root =
+      host->AttachShadowRootInternal(ShadowRootType::kOpen);
+  shadow_root.SetInnerHTMLFromString("<div><slot></slot></div>");
+
+  UpdateAllLifecyclePhases();
+
+  // Make the span style dirty.
+  span->setAttribute("style", "color:green");
+
+  EXPECT_TRUE(GetDocument().NeedsLayoutTreeUpdate());
+  EXPECT_EQ(span, GetStyleRecalcRoot());
+
+  auto* div = shadow_root.firstChild();
+  auto* slot = To<Element>(div->firstChild());
+
+  slot->setAttribute("name", "x");
+  GetDocument().GetSlotAssignmentEngine().RecalcSlotAssignments();
+
+  // Make sure shadow tree div and slot have their ChildNeedsStyleRecalc()
+  // cleared.
+  EXPECT_FALSE(div->ChildNeedsStyleRecalc());
+  EXPECT_FALSE(slot->ChildNeedsStyleRecalc());
+  EXPECT_FALSE(span->NeedsStyleRecalc());
+  EXPECT_FALSE(GetStyleRecalcRoot());
+}
+
 }  // namespace blink

third_party/blink/renderer/core/css/style_recalc_root.cc

@@ -110,4 +110,24 @@ void StyleRecalcRoot::RootRemoved(ContainerNode& parent) {
   Clear();
 }
 
+void StyleRecalcRoot::RemovedFromFlatTree(const Node& node) {
+  if (!RuntimeEnabledFeatures::FlatTreeStyleRecalcEnabled())
+    return;
+  if (!GetRootNode())
+    return;
+  if (GetRootNode()->IsDocumentNode())
+    return;
+  // If the recalc root is the removed node, or if it's a descendant of the root
+  // node, the recalc flags will be cleared in DetachLayoutTree() since
+  // performing_reattach=false. If that's the case, call RootRemoved() below to
+  // make sure we don't have a recalc root outside the flat tree, which is not
+  // allowed with FlatTreeStyleRecalc enabled.
+  if (GetRootNode()->NeedsStyleRecalc() ||
+      GetRootNode()->ChildNeedsStyleRecalc()) {
+    return;
+  }
+  DCHECK(node.parentElement());
+  RootRemoved(*node.parentElement());
+}
+
 }  // namespace blink

third_party/blink/renderer/core/css/style_recalc_root.h

@@ -14,6 +14,7 @@ class CORE_EXPORT StyleRecalcRoot : public StyleTraversalRoot {
 
  public:
   Element& RootElement() const;
+  void RemovedFromFlatTree(const Node& node);
 
  private:
 #if DCHECK_IS_ON()

third_party/blink/renderer/core/dom/node.cc

@@ -1661,6 +1661,16 @@ void Node::DetachLayoutTree(bool performing_reattach) {
   if (GetLayoutObject())
     GetLayoutObject()->DestroyAndCleanupAnonymousWrappers();
   SetLayoutObject(nullptr);
+  if (!performing_reattach) {
+    // We are clearing the ComputedStyle for elements, which means we should not
+    // need to recalc style. Also, this way we can detect if we need to remove
+    // this Node as a StyleRecalcRoot if this detach is because the node is
+    // removed from the flat tree. That is necessary because we are not allowed
+    // to have a style recalc root outside the flat tree when traversing the
+    // flat tree for style recalc (see StyleRecalcRoot::RemovedFromFlatTree()).
+    ClearNeedsStyleRecalc();
+    ClearChildNeedsStyleRecalc();
+  }
 }
 
 const ComputedStyle* Node::VirtualEnsureComputedStyle(
@@ -3279,6 +3289,14 @@ void Node::FlatTreeParentChanged() {
   SetForceReattachLayoutTree();
 }
 
+void Node::RemovedFromFlatTree() {
+  // This node was previously part of the flat tree, but due to slot re-
+  // assignment it no longer is. We need to detach the layout tree and notify
+  // the StyleEngine in case the StyleRecalcRoot is removed from the flat tree.
+  DetachLayoutTree();
+  GetDocument().GetStyleEngine().RemovedFromFlatTree(*this);
+}
+
 Node* Node::TrustedTypesCheckForScriptNode(
     Node* child,
     ExceptionState& exception_state) const {

third_party/blink/renderer/core/dom/node.h

@@ -888,11 +888,7 @@ class CORE_EXPORT Node : public EventTarget {
   }
 
   void FlatTreeParentChanged();
-  void RemovedFromFlatTree() {
-    // This node was previously part of the flat tree, but due to slot re-
-    // assignment it no longer is. We need to detach the layout tree.
-    DetachLayoutTree();
-  }
+  void RemovedFromFlatTree();
 
   void SetHasDuplicateAttributes() { SetFlag(kHasDuplicateAttributes); }
   bool HasDuplicateAttribute() const {

third_party/blink/renderer/core/dom/node_test.cc

@@ -511,12 +511,16 @@ TEST_F(NodeTest, UpdateChildDirtyAfterSlottingDirtyNode) {
   auto* host = GetDocument().getElementById("host");
   auto* span = To<Element>(host->firstChild());
 
+  ShadowRoot& shadow_root =
+      host->AttachShadowRootInternal(ShadowRootType::kOpen);
+  shadow_root.SetInnerHTMLFromString("<div><slot name=x></slot></div>");
+  UpdateAllLifecyclePhasesForTest();
+
   // Make sure the span is style dirty.
   span->setAttribute("style", "color:green");
 
-  ShadowRoot& shadow_root =
-      host->AttachShadowRootInternal(ShadowRootType::kOpen);
-  shadow_root.SetInnerHTMLFromString("<div><slot></slot></div>");
+  // Assign span to slot.
+  span->setAttribute("slot", "x");
 
   GetDocument().GetSlotAssignmentEngine().RecalcSlotAssignments();
 
@@ -525,6 +529,9 @@ TEST_F(NodeTest, UpdateChildDirtyAfterSlottingDirtyNode) {
   EXPECT_TRUE(shadow_root.firstChild()->ChildNeedsStyleRecalc());
   EXPECT_TRUE(shadow_root.firstChild()->firstChild()->ChildNeedsStyleRecalc());
   EXPECT_TRUE(span->NeedsStyleRecalc());
+
+  // This used to call a DCHECK failure. Make sure we don't regress.
+  UpdateAllLifecyclePhasesForTest();
 }
 
 TEST_F(NodeTest, ChildDirtyNeedsV0Distribution) {