Reland "Fix several crashes when nested slots are removed from a ShadowRoot"

This is a reland of dbfed21f

--> Patchset 2 contains the fix, just a missing initializer on an
int in the test.


Original change's description:
> Fix several crashes when nested slots are removed from a ShadowRoot
>
> There were some crashes caused by nested slots (e.g.
> <slot><slot>Content</slot></slot>) being removed from the tree.
> These crashes were triggered by [1], which removed Shadow DOM v0, but
> my theory is that due to the old V0 shadow root code, more calls were
> being made to SlotAssignment::RecalcAssignment(). Now that the V0 code
> is gone, it has exposed some missing code.
>
> Three issues are being fixed here:
>  1. In Node::CheckSlotChange(), while removing the inner nested slot,
>     the parent_slot will have already been removed from the tree, so we
>     only need to call DidSlotChange if not. This used to be a DCHECK.
>  2. In TreeOrderedMap::Get(), while removing a key that previously had
>     more than one element, we may walk the tree and find that none of
>     the pre-existing elements are present. I.e. we're in a RemoveScope.
>     In this case, the key should be removed from the map.
>  3. In SlotAssignment::DidRemoveSlotInternal(), given #2 above, we can
>     just early-out if the slot isn't present in the map.
>
> I added a test for the crash conditions (variations on removing nested
> named and unnamed slots), plus I added a test for the TreeOrderedMap
> class, since there was none previously. The last test in the set
> documents the new Get() behavior. I also tried to improve some of the
> comments along the way. Finally, this CL rolls back a mitigation [2]
> previously landed for this crash.
>
> [1] https://chromium-review.googlesource.com/c/chromium/src/+/2586019
> [2] https://chromium-review.googlesource.com/c/chromium/src/+/2595967
>
> Bug: 1159328, 1159727
> Change-Id: I47fbf33b2313b9ae2efe229443af6e8c9a1920a9
> Cq-Do-Not-Cancel-Tryjobs: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2597040
> Commit-Queue: Mason Freed <masonfreed@chromium.org>
> Reviewed-by: Yu Han <yuzhehan@chromium.org>
> Reviewed-by: Joey Arhar <jarhar@chromium.org>
> Auto-Submit: Mason Freed <masonfreed@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#838974}

Bug: 1159328
Bug: 1159727
Change-Id: I0025c0f00d6b3876de8f40a60fdc34f726ddc85c
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2601051
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Reviewed-by: Yu Han <yuzhehan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839148}

third_party/blink/renderer/core/BUILD.gn

@@ -1572,7 +1572,7 @@ source_set("unit_tests") {
     "xml/xpath_functions_test.cc",
   ]
 
-  # If you create a new subdirectory 'foo' that contains unit tets, list them in
+  # If you create a new subdirectory 'foo' that contains unit tests, list them in
   # 'foo/build.gni' to define blink_core_tests_foo, and add any dependencies in
   # the deps section below.
   sources += rebase_path(blink_core_tests_accessibility, "", "accessibility")

third_party/blink/renderer/core/dom/build.gni

@@ -317,6 +317,7 @@ blink_core_tests_dom = [
   "space_split_string_test.cc",
   "static_range_test.cc",
   "text_test.cc",
+  "tree_ordered_map_test.cc",
   "tree_scope_adopter_test.cc",
   "tree_scope_test.cc",
   "weak_identifier_map_test.cc",

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

@@ -3162,7 +3162,9 @@ void Node::SetCustomElementState(CustomElementState new_state) {
 
 void Node::CheckSlotChange(SlotChangeType slot_change_type) {
   // Common check logic is used in both cases, "after inserted" and "before
-  // removed".
+  // removed". This function calls DidSlotChange() on the appropriate nodes,
+  // e.g. the assigned slot for this node, or the parent slot for a slot's
+  // fallback content.
 
   // Relevant DOM Standard:
   // https://dom.spec.whatwg.org/#concept-node-insert
@@ -3187,11 +3189,16 @@ void Node::CheckSlotChange(SlotChangeType slot_change_type) {
   } else if (IsInShadowTree()) {
     // Checking for fallback content if the node is in a shadow tree.
     if (auto* parent_slot = DynamicTo<HTMLSlotElement>(parentElement())) {
-      DCHECK(parent_slot->SupportsAssignment());
       // The parent_slot's assigned nodes might not be calculated because they
       // are lazy evaluated later at UpdateDistribution() so we have to check it
-      // here.
-      if (!parent_slot->HasAssignedNodesSlow())
+      // here. Also, parent_slot may have already been removed, if this was the
+      // removal of nested slots, e.g.
+      //   <slot name=parent-slot><slot name=this-slot>fallback</slot></slot>.
+      // In that case, parent-slot has already been removed, so parent_slot->
+      // SupportsAssignment() is false, but this-slot is still in the process
+      // of being removed, so IsInShadowTree() is still true.
+      if (parent_slot->SupportsAssignment() &&
+          !parent_slot->HasAssignedNodesSlow())
         parent_slot->DidSlotChange(slot_change_type);
     }
   }

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

@@ -77,7 +77,6 @@ void SlotAssignment::DidRemoveSlot(HTMLSlotElement& slot) {
     return;
   }
 
-  DCHECK(GetCachedFirstSlotWithoutAccessingNodeTree(slot.GetName()));
   DidRemoveSlotInternal(slot, slot.GetName(), SlotMutationType::kRemoved);
   // Ensures that TreeOrderedMap has a cache if there is a slot for the name.
   DCHECK(!slot_map_->Contains(slot.GetName()) ||
@@ -97,7 +96,7 @@ void SlotAssignment::DidAddSlotInternal(HTMLSlotElement& slot) {
 
   // At this timing, we can't use FindSlotByName because what we are interested
   // in is the first slot *before* |slot| was inserted. Here, |slot| was already
-  // disconnected from the tree. Thus, we can't use on FindBySlotName because
+  // connected to the tree. Thus, we can't use on FindBySlotName because
   // it might scan the current tree and return a wrong result.
   HTMLSlotElement* old_active =
       GetCachedFirstSlotWithoutAccessingNodeTree(slot_name);
@@ -146,11 +145,18 @@ void SlotAssignment::DidRemoveSlotInternal(
 
   // At this timing, we can't use FindSlotByName because what we are interested
   // in is the first slot *before* |slot| was removed. Here, |slot| was already
-  // connected to the tree. Thus, we can't use FindBySlotName because
+  // disconnected from the tree. Thus, we can't use FindBySlotName because
   // it might scan the current tree and return a wrong result.
   HTMLSlotElement* old_active =
       GetCachedFirstSlotWithoutAccessingNodeTree(slot_name);
-  DCHECK(old_active);
+
+  // If we don't have a cached slot for this slot name, then we're
+  // likely removing a nested identically named slot, e.g.
+  // <slot id=removed><slot></slot</slot>, and this is the inner
+  // slot. It has already been removed from the map, so return.
+  if (!old_active)
+    return;
+
   slot_map_->Remove(slot_name, slot);
   // This also ensures that TreeOrderedMap has a cache for the first element.
   HTMLSlotElement* new_active = FindSlotByName(slot_name);

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

@@ -138,6 +138,9 @@ inline Element* TreeOrderedMap::Get(const AtomicString& key,
 #if DCHECK_IS_ON()
   DCHECK(g_remove_scope_level);
 #endif
+  // Since we didn't find any elements for this key, remove the key from the
+  // map here.
+  map_.erase(key);
   return nullptr;
 }
 

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

@@ -32,6 +32,7 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_DOM_TREE_ORDERED_MAP_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_DOM_TREE_ORDERED_MAP_H_
 
+#include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/platform/heap/handle.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
@@ -46,7 +47,12 @@ class Element;
 class HTMLSlotElement;
 class TreeScope;
 
-class TreeOrderedMap : public GarbageCollected<TreeOrderedMap> {
+// TreeOrderedMap is a map from keys to |Element|s, which allows multiple values
+// per key, maintained in tree order per key. Tree walks are avoided when
+// possible by retaining a cached, ordered array of matching nodes. Adding or
+// removing an element for a given key often clears the cache, forcing a tree
+// walk upon the next access.
+class CORE_EXPORT TreeOrderedMap : public GarbageCollected<TreeOrderedMap> {
  public:
   TreeOrderedMap();
 
@@ -73,7 +79,7 @@ class TreeOrderedMap : public GarbageCollected<TreeOrderedMap> {
   // Rare trees, but ID lookups may legitimately fail across such removals;
   // this scope object informs TreeOrderedMaps about the transitory state of the
   // underlying tree.
-  class RemoveScope {
+  class CORE_EXPORT RemoveScope {
     STACK_ALLOCATED();
 
    public:
@@ -81,7 +87,7 @@ class TreeOrderedMap : public GarbageCollected<TreeOrderedMap> {
     ~RemoveScope();
   };
 #else
-  class RemoveScope {
+  class CORE_EXPORT RemoveScope {
     STACK_ALLOCATED();
 
    public:

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

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/dom/tree_ordered_map.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/renderer/core/dom/document.h"
+#include "third_party/blink/renderer/core/dom/element.h"
+#include "third_party/blink/renderer/core/editing/testing/editing_test_base.h"
+#include "third_party/blink/renderer/core/html/html_div_element.h"
+#include "third_party/blink/renderer/core/html/html_slot_element.h"
+#include "third_party/blink/renderer/platform/heap/heap.h"
+
+namespace blink {
+
+class TreeOrderedMapTest : public EditingTestBase {
+ protected:
+  void SetUp() override {
+    EditingTestBase::SetUp();
+    root_ = MakeGarbageCollected<HTMLDivElement>(GetDocument());
+    root_->setAttribute(html_names::kIdAttr, AtomicString("ROOT"));
+    GetDocument().body()->appendChild(root_);
+  }
+
+  Element* AddElement(AtomicString slot_name) {
+    auto* slot = MakeGarbageCollected<HTMLSlotElement>(GetDocument());
+    slot->setAttribute(html_names::kNameAttr, slot_name);
+    std::string id = "SLOT_" + base::NumberToString(++element_num);
+    slot->setAttribute(html_names::kIdAttr, AtomicString(id.c_str()));
+    root_->appendChild(slot);
+    return static_cast<Element*>(slot);
+  }
+  TreeScope& GetTreeScope() { return root_->GetTreeScope(); }
+
+ private:
+  int element_num{0};
+  Persistent<HTMLDivElement> root_;
+};
+
+TEST_F(TreeOrderedMapTest, Basic) {
+  auto* map = MakeGarbageCollected<TreeOrderedMap>();
+  AtomicString key = "test";
+  auto& element = *AddElement(key);
+  map->Add(key, element);
+  EXPECT_TRUE(map->Contains(key));
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key), element);
+  map->Remove(key, element);
+  EXPECT_FALSE(map->Contains(key));
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key), nullptr);
+}
+
+TEST_F(TreeOrderedMapTest, DuplicateKeys) {
+  auto* map = MakeGarbageCollected<TreeOrderedMap>();
+  AtomicString key = "test";
+  auto& element1 = *AddElement(key);
+  auto& element2 = *AddElement(key);
+  map->Add(key, element1);
+  EXPECT_TRUE(map->Contains(key));
+  EXPECT_FALSE(map->ContainsMultiple(key));
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key), element1);
+  map->Add(key, element2);
+  EXPECT_TRUE(map->Contains(key));
+  EXPECT_TRUE(map->ContainsMultiple(key));
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key), nullptr)
+      << "No tree walk yet";
+  EXPECT_EQ(map->GetSlotByName(key, GetTreeScope()), element1);
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key), element1)
+      << "Tree walk forced by GetSlotByName";
+  element1.remove();  // Remove it from the tree also.
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key), element1)
+      << "Make sure we don't touch the tree";
+  map->Remove(key, element1);
+  EXPECT_TRUE(map->Contains(key));
+  EXPECT_FALSE(map->ContainsMultiple(key));
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key), nullptr);
+  EXPECT_EQ(map->GetSlotByName(key, GetTreeScope()), element2);
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key), element2);
+  map->Remove(key, element2);
+  EXPECT_FALSE(map->Contains(key));
+  EXPECT_FALSE(map->ContainsMultiple(key));
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key), nullptr);
+  EXPECT_EQ(map->GetSlotByName(key, GetTreeScope()), nullptr)
+      << "nullptr even though we never removed element2 from the tree";
+}
+
+TEST_F(TreeOrderedMapTest, ManyKeys) {
+  auto* map = MakeGarbageCollected<TreeOrderedMap>();
+  AtomicString key1 = "test1";
+  AtomicString key2 = "";  // Empty should be handled as a unique key
+  auto& element1 = *AddElement(key1);
+  auto& element2 = *AddElement(key1);
+  auto& element3 = *AddElement(key2);
+  auto& element4 = *AddElement(key2);
+  map->Add(key1, element1);
+  map->Add(key1, element2);
+  map->Add(key2, element3);
+  map->Add(key2, element4);
+  EXPECT_TRUE(map->Contains(key1));
+  EXPECT_TRUE(map->Contains(key2));
+  EXPECT_TRUE(map->ContainsMultiple(key1));
+  EXPECT_TRUE(map->ContainsMultiple(key2));
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key1), nullptr);
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key2), nullptr);
+  EXPECT_EQ(map->GetSlotByName(key1, GetTreeScope()), element1);
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key1), element1);
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key2), nullptr);
+  EXPECT_EQ(map->GetSlotByName(key2, GetTreeScope()), element3);
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key2), element3);
+  map->Remove(key1, element2);
+  map->Remove(key1, element1);
+  map->Remove(key2, element3);
+  element3.remove();
+  EXPECT_FALSE(map->Contains(key1));
+  EXPECT_TRUE(map->Contains(key2));
+  EXPECT_FALSE(map->ContainsMultiple(key2));
+  EXPECT_EQ(map->GetCachedFirstElementWithoutAccessingNodeTree(key2), nullptr);
+  EXPECT_EQ(map->GetSlotByName(key2, GetTreeScope()), element4);
+}
+
+TEST_F(TreeOrderedMapTest, RemovedDuplicateKeys) {
+  auto* map = MakeGarbageCollected<TreeOrderedMap>();
+  AtomicString key = "test";
+  auto& outer = *AddElement(key);
+  auto& inner = *AddElement(key);
+  outer.appendChild(&inner);
+  map->Add(key, outer);
+  map->Add(key, inner);
+  EXPECT_EQ(map->GetSlotByName(key, GetTreeScope()), outer);
+  EXPECT_TRUE(map->ContainsMultiple(key));
+  outer.remove();  // This removes both elements from the tree
+  EXPECT_TRUE(map->ContainsMultiple(key)) << "We haven't touched the map yet";
+  TreeOrderedMap::RemoveScope tree_remove_scope;
+  map->Remove(key, outer);
+  EXPECT_TRUE(map->Contains(key))
+      << "The map will still contain the entry for inner at this point";
+  EXPECT_FALSE(map->ContainsMultiple(key));
+  EXPECT_EQ(map->GetSlotByName(key, GetTreeScope()), nullptr);
+  EXPECT_FALSE(map->Contains(key))
+      << "The call to GetSlotByName should have cleared the key entirely";
+}
+
+}  // namespace blink

third_party/blink/renderer/core/html/html_slot_element.cc

@@ -419,13 +419,14 @@ void HTMLSlotElement::RemovedFrom(ContainerNode& insertion_point) {
   // `removedFrom` is called after the node is removed from the tree.
   // That means:
   // 1. If this slot is still in a tree scope, it means the slot has been in a
-  // shadow tree. An inclusive shadow-including ancestor of the shadow host was
-  // originally removed from its parent.
+  //    shadow tree. An inclusive shadow-including ancestor of the shadow host
+  //    was originally removed from its parent. See slot s2 below.
   // 2. Or (this slot is not in a tree scope), this slot's inclusive
-  // ancestor was orginally removed from its parent (== insertion point). This
-  // slot and the originally removed node was in the same tree before removal.
+  //    ancestor was orginally removed from its parent (== insertion point).
+  //    This slot and the originally removed node was in the same tree before
+  //    removal. See slot s1 below.
 
-  // For exmaple, given the following trees, (srN: = shadow root, sN: = slot)
+  // For example, given the following trees, (srN: = shadow root, sN: = slot)
   // a
   // |- b --sr1
   // |- c   |--d
@@ -699,7 +700,7 @@ void HTMLSlotElement::CheckFallbackAfterInsertedIntoShadowTree() {
   DCHECK(SupportsAssignment());
   if (HasSlotableChild()) {
     // We use kSuppress here because a slotchange event shouldn't be
-    // dispatched if a slot being inserted don't get any assigned
+    // dispatched if a slot being inserted doesn't get any assigned
     // node, but has a slotable child, according to DOM Standard.
     DidSlotChange(SlotChangeType::kSuppressSlotChangeEvent);
   }
@@ -738,10 +739,7 @@ void HTMLSlotElement::EnqueueSlotChangeEvent() {
 
 bool HTMLSlotElement::HasAssignedNodesSlow() const {
   ShadowRoot* root = ContainingShadowRoot();
-  if (!root) {
-    NOTREACHED() << "We shouldn't get here - see crbug.com/1159328";
-    return false;
-  }
+  DCHECK(root) << "This should only be called on slots inside a shadow tree";
   SlotAssignment& assignment = root->GetSlotAssignment();
   if (assignment.FindSlotByName(GetName()) != this)
     return false;

third_party/blink/web_tests/external/wpt/shadow-dom/nested-slot-remove-crash.html

+<!DOCTYPE html>
+<meta charset="utf-8">
+<link rel="author" title="Mason Freed" href="mailto:masonfreed@chromium.org">
+<link rel="help" href="https://crbug.com/1159328">
+<meta name="assert" content="The renderer should not crash.">
+
+<div id="host"></div>
+<span>This test passes if the renderer does not crash.</span>
+
+<script>
+  const root = host.attachShadow({mode:"open"});
+  root.innerHTML = `
+    <slot id=outer1 name=outer>
+      <slot id=inner1 name=inner>Fallback</slot>
+    </slot>
+  `;
+  document.body.offsetTop;
+  // The renderer should not crash here:
+  root.querySelector("#outer1").remove();
+
+  root.innerHTML = `
+    <slot id=outer2>
+      <slot id=inner2>Fallback</slot>
+    </slot>
+  `;
+  document.body.offsetTop;
+  // The renderer should not crash here:
+  root.querySelector("#outer2").remove();
+
+  const root = host.attachShadow({mode:"open"});
+  root.innerHTML = `
+    <slot id=outer3>
+      <slot id=inner3>
+        <slot id=way-inner3>Fallback</slot>
+      </slot>
+    </slot>
+  `;
+  document.body.offsetTop;
+  // The renderer should not crash here:
+  root.querySelector("#outer3").remove();
+</script>