Clear slot's assigned nodes if the slot assignment is dirty when the slot is removed

If we don't clear slot's assigned nodes when detaching a slot, if the slot assignment recalc flag is dirty (the actual condition is more complex, see the code), *cycle* would happen in detaching (after attaching a slot again) because a series of DOM mutations can create a tree of trees as such: - one of the slot's formerly assigned node, call it |A|, is a shadow-including ancestor of the slot. - |A| is already attached. In this case, detaching the slot will try to lazy-reattach |A|, and lazy-reattaching |A| doesn't return early (because |A| doesn't need Attach). As a result, detaching the slot *cycles* and causes an infinite loop (... -> |A| -> .. -> host -> shadow root -> .. -> slot -> |A| -> ...). See the test (or bug 849599) for a concrete example. Bug: 849599 Change-Id: I1c1ddb06ca9777af0052260aa721c2438da3c62b Reviewed-on: https://chromium-review.googlesource.com/1090420Reviewed-by: Takayoshi Kochi <kochi@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Commit-Queue: Hayato Ito <hayato@chromium.org> Cr-Commit-Position: refs/heads/master@{#565323}

Clear slot's assigned nodes if the slot assignment is dirty when the slot is removed
If we don't clear slot's assigned nodes when detaching a slot, if the slot assignment recalc flag is dirty (the actual condition is more complex, see the code), *cycle* would happen in detaching (after attaching a slot again) because a series of DOM mutations can create a tree of trees as such: - one of the slot's formerly assigned node, call it |A|, is a shadow-including ancestor of the slot. - |A| is already attached. In this case, detaching the slot will try to lazy-reattach |A|, and lazy-reattaching |A| doesn't return early (because |A| doesn't need Attach). As a result, detaching the slot *cycles* and causes an infinite loop (... -> |A| -> .. -> host -> shadow root -> .. -> slot -> |A| -> ...). See the test (or bug 849599) for a concrete example. Bug: 849599 Change-Id: I1c1ddb06ca9777af0052260aa721c2438da3c62b Reviewed-on: https://chromium-review.googlesource.com/1090420Reviewed-by: Takayoshi Kochi <kochi@chromium.org> Reviewed-by: Rune Lillesveen <futhark@chromium.org> Commit-Queue: Hayato Ito <hayato@chromium.org> Cr-Commit-Position: refs/heads/master@{#565323}
4b04a57e · Hayato Ito · Commit Bot · b0b361c5 · 4b04a57e · 4b04a57e
Commit 4b04a57e authored Jun 07, 2018 by Hayato Ito Committed by Commit Bot Jun 07, 2018
2 changed files
--- a/third_party/WebKit/LayoutTests/shadow-dom/crashes/cyclic-detach-crash.html
+++ b/third_party/WebKit/LayoutTests/shadow-dom/crashes/cyclic-detach-crash.html
+<!DOCTYPE html>
+<script src='../../resources/testharness.js'></script>
+<script src='../../resources/testharnessreport.js'></script>
+<div id="a"><div id="host"><div id="child"></div></div></div>
+<script>
+test(() => {
+  const a = document.querySelector('#a');
+  const child = document.querySelector('#child');
+  const host = document.querySelector('#host');
+  const sr = host.attachShadow({ mode: 'open' });
+  sr.appendChild(document.createElement('slot'));
+  document.body.offsetLeft;
+  child.remove();
+  host.remove();
+  a.appendChild(child);
+  document.body.offsetLeft;
+  child.appendChild(host);
+  a.remove();
+}, "crbug.com/849599. Cycling detach shoul not happen.");
+</script>
--- a/third_party/blink/renderer/core/html/html_slot_element.cc
+++ b/third_party/blink/renderer/core/html/html_slot_element.cc
@@ -442,8 +442,30 @@ void HTMLSlotElement::RemovedFrom(ContainerNode* insertion_point) {
  // ContainingShadowRoot() is okay to use here because 1) It doesn't use
  // kIsInShadowTreeFlag flag, and 2) TreeScope has been alreay updated for the
  // slot.
-  if (insertion_point->IsInV1ShadowTree() && !ContainingShadowRoot()) {
+  if (ShadowRoot* shadow_root = ContainingShadowRoot()) {
-    // This slot was in a shadow tree and got disconnected from the shadow tree
+    // In this case, the shadow host (or it's shadow-inclusive ancestor) was
+    // removed orginally. In the above example, (this slot == s2) and
+    // (shadow_root == sr2). The shadow tree (sr2)'s structure didn't change at
+    // all.
+    if (RuntimeEnabledFeatures::IncrementalShadowDOMEnabled()) {
+      if (shadow_root->NeedsSlotAssignmentRecalc()) {
+        // Need to clear assigned_nodes here.
+        // Without that, a cyclic detach would happen, if one of the
+        // assinged_nodes_ becomes a shadow-including ancestor of this slot by a
+        // series of DOM mutations. See http://crbug.com/849599 for details.
+        ClearAssignedNodesAndFlatTreeChildren();
+      } else {
+        // We don't need to clear assigned_nodes here. That's an important
+        // optimization.
+      }
+    } else {
+      ClearDistribution();
+    }
+  } else if (insertion_point->IsInV1ShadowTree()) {
+    // This slot was in a shadow tree and got disconnected from the shadow
+    // tree.
+    // In the above examle, (this slot == s1), (insertion point == d)
+    // and (insertion_point->ContainingShadowRoot() == sr1)
    insertion_point->ContainingShadowRoot()->GetSlotAssignment().DidRemoveSlot(
        *this);
    if (RuntimeEnabledFeatures::IncrementalShadowDOMEnabled()) {
@@ -451,6 +473,8 @@ void HTMLSlotElement::RemovedFrom(ContainerNode* insertion_point) {
    } else {
      ClearDistribution();
    }
+  } else {
+    DCHECK(assigned_nodes_.IsEmpty());
  }
  HTMLElement::RemovedFrom(insertion_point);