Commit 233a440f authored by Takayoshi Kochi's avatar Takayoshi Kochi Committed by Commit Bot

Clear slot's assigned nodes when it's inserted and assigned nodes are dirty

Between when a host element with its shadow root gets orphaned and
when it gets connected again, its slot assignment can become dirty
by some DOM mutation of its host children, but slots still hold stale
assigned nodes.

When a detached host child becomes an ancestor of the shadow host,
unless those cached assigned nodes are cleared, it can make a cycle
during DetachLayoutTree() traverses down to those cached nodes.

See the case (cyclic-detach-crash2.html) for details.

Bug: 847056, 845770
Change-Id: I44d3c118c9810ad3847fa24d630b7ddb9f9d2e50
Reviewed-on: https://chromium-review.googlesource.com/1100718Reviewed-by: default avatarHayato Ito <hayato@chromium.org>
Reviewed-by: default avatarRune Lillesveen <futhark@chromium.org>
Commit-Queue: Takayoshi Kochi <kochi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567636}
parent 6d218406
...@@ -17,5 +17,5 @@ test(() => { ...@@ -17,5 +17,5 @@ test(() => {
document.body.offsetLeft; document.body.offsetLeft;
child.appendChild(host); child.appendChild(host);
a.remove(); a.remove();
}, "crbug.com/849599. Cycling detach shoul not happen."); }, "crbug.com/849599. Cyclic detach should not happen.");
</script> </script>
<!doctype html>
<script src='../../resources/testharness.js'></script>
<script src='../../resources/testharnessreport.js'></script>
<div id="div1"><div id="div2"></div></div>
<script>
test(() => {
const div1 = document.getElementById('div1');
const div2 = document.getElementById('div2');
// slot1 (#div2 is assigned)
div1.attachShadow({mode: 'open'}).innerHTML='<slot></slot>';
// slot2
div2.attachShadow({mode: 'open'}).innerHTML='<slot></slot>';
document.body.offsetTop;
// All shadow roots' assignment recalc flags are clean.
div1.remove();
const span = document.createElement('span');
// #div2's shadow root's assignment recalc flag becomes dirty.
div2.appendChild(span);
// #div1's shadow root's assignment recalc flag becomes dirty.
document.body.appendChild(div2);
// slot2's assignment is recalculated.
document.body.offsetTop;
span.appendChild(div1);
// #div1's shadow root's assignment recalc flag is still dirty.
span.remove();
}, "crbug.com/847056. Cyclic detach should not happen.");
</script>
...@@ -412,6 +412,17 @@ Node::InsertionNotificationRequest HTMLSlotElement::InsertedInto( ...@@ -412,6 +412,17 @@ Node::InsertionNotificationRequest HTMLSlotElement::InsertedInto(
if (root == insertion_point->ContainingShadowRoot()) { if (root == insertion_point->ContainingShadowRoot()) {
// This slot is inserted into the same tree of |insertion_point| // This slot is inserted into the same tree of |insertion_point|
root->DidAddSlot(*this); root->DidAddSlot(*this);
} else if (RuntimeEnabledFeatures::IncrementalShadowDOMEnabled() &&
insertion_point->isConnected() &&
root->NeedsSlotAssignmentRecalc()) {
// Even when a slot and its containing shadow root is removed together
// and inserted together again, the slot's cached assigned nodes can be
// stale if the NeedsSlotAssignmentRecalc flag is set, and it may cause
// infinite recursion in DetachLayoutTree() when one of the stale node
// is a shadow-including ancestor of this slot by making a circular
// reference. Clear the cache here to avoid the situation.
// See http://crbug.com/849599 for details.
ClearAssignedNodesAndFlatTreeChildren();
} }
} }
return kInsertionDone; return kInsertionDone;
...@@ -440,32 +451,30 @@ void HTMLSlotElement::RemovedFrom(ContainerNode* insertion_point) { ...@@ -440,32 +451,30 @@ void HTMLSlotElement::RemovedFrom(ContainerNode* insertion_point) {
// - For slot s2, s2.removedFrom(d) is called. // - For slot s2, s2.removedFrom(d) is called.
// ContainingShadowRoot() is okay to use here because 1) It doesn't use // ContainingShadowRoot() is okay to use here because 1) It doesn't use
// kIsInShadowTreeFlag flag, and 2) TreeScope has been alreay updated for the // kIsInShadowTreeFlag flag, and 2) TreeScope has been already updated for the
// slot. // slot.
if (ShadowRoot* shadow_root = ContainingShadowRoot()) { if (ShadowRoot* shadow_root = ContainingShadowRoot()) {
// In this case, the shadow host (or it's shadow-inclusive ancestor) was // In this case, the shadow host (or its shadow-inclusive ancestor) was
// removed orginally. In the above example, (this slot == s2) and // removed originally. In the above example, (this slot == s2) and
// (shadow_root == sr2). The shadow tree (sr2)'s structure didn't change at // (shadow_root == sr2). The shadow tree (sr2)'s structure didn't change at
// all. // all.
if (RuntimeEnabledFeatures::IncrementalShadowDOMEnabled()) { if (RuntimeEnabledFeatures::IncrementalShadowDOMEnabled()) {
if (shadow_root->NeedsSlotAssignmentRecalc()) { if (shadow_root->NeedsSlotAssignmentRecalc()) {
// Need to clear assigned_nodes here. // Clear |assigned_nodes_| here, so that the referenced node can get
// Without that, a cyclic detach would happen, if one of the // garbage collected if they no longer needed. See also InsertedInto()'s
// assinged_nodes_ becomes a shadow-including ancestor of this slot by a // comment for cases that stale |assigned_nodes| can be problematic.
// series of DOM mutations. See http://crbug.com/849599 for details.
ClearAssignedNodesAndFlatTreeChildren(); ClearAssignedNodesAndFlatTreeChildren();
} else { } else {
// We don't need to clear assigned_nodes here. That's an important // We don't need to clear |assigned_nodes_| here. That's an important
// optimization. // optimization.
} }
} else { } else {
ClearDistribution(); ClearDistribution();
} }
} else if (insertion_point->IsInV1ShadowTree()) { } else if (insertion_point->IsInV1ShadowTree()) {
// This slot was in a shadow tree and got disconnected from the shadow // This slot was in a shadow tree and got disconnected from the shadow tree.
// tree. // In the above example, (this slot == s1), (insertion point == d)
// In the above examle, (this slot == s1), (insertion point == d) // and (insertion_point->ContainingShadowRoot == sr1).
// and (insertion_point->ContainingShadowRoot() == sr1)
insertion_point->ContainingShadowRoot()->GetSlotAssignment().DidRemoveSlot( insertion_point->ContainingShadowRoot()->GetSlotAssignment().DidRemoveSlot(
*this); *this);
if (RuntimeEnabledFeatures::IncrementalShadowDOMEnabled()) { if (RuntimeEnabledFeatures::IncrementalShadowDOMEnabled()) {
......
...@@ -79,7 +79,7 @@ class CORE_EXPORT HTMLSlotElement final : public HTMLElement { ...@@ -79,7 +79,7 @@ class CORE_EXPORT HTMLSlotElement final : public HTMLElement {
AtomicString GetName() const; AtomicString GetName() const;
// This method can be slow because this has to traverse the children of a // This method can be slow because this has to traverse the children of a
// shadow host. This method should be used only when m_assignedNodes is // shadow host. This method should be used only when |assigned_nodes_| is
// dirty. e.g. To detect a slotchange event in DOM mutations. // dirty. e.g. To detect a slotchange event in DOM mutations.
bool HasAssignedNodesSlow() const; bool HasAssignedNodesSlow() const;
bool FindHostChildWithSameSlotName() const; bool FindHostChildWithSameSlotName() const;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment