Reland "Menulist SELECT: Avoid updating UA shadow nodes in InsertedInto() and RemovedFrom()"

This CL relands crrev.com/c/2074840. A part of it was landed as
crrev.com/746315, and this CL is the remaining.

> HTMLOptionElement's InsertedInto() and RemovedFrom() called
> MenuListSelectType::UpdateTextStyleAndContent(), which updates a Text
> node in a UA shadow tree.  It triggered a DCHECK failure about
> IsSlotAssignmentRecalcForbidden().  We should not update the DOM tree
> in these functions.
>
> This CL moves the logic of the InsertedInto() and the RemovedFrom() to
> HTMLSelectElement::ChildrenChanged() and |HTMLOptGroupElement::
> ChildrenChanged()|.
>
> As for kAllChildrenRemoved such as |select.innerHTML=''|,
> ChildrenChanged() had no ways to know removed nodes. This CL adds
> ChildrenChangedAllChildrenRemovedNeedsList flag, and ContainerNode
> provides a list of removed nodes if the flag is true.
>
> This CL also updates comments on Node::InsertedInto(),
> Node::RemovedFrom(), and ContainerNode::ChildrenChanged().

Bug: 1056094
Change-Id: Icef4c9f180333a5b39d38b1950f9c5c2660c66c5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2086451Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746659}

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

@@ -810,6 +810,11 @@ void ContainerNode::RemoveChildren(SubtreeModificationAction action) {
     GetDocument().NodeChildrenWillBeRemoved(*this);
   }
 
+  HeapVector<Member<Node>>* removed_nodes = nullptr;
+  if (ChildrenChangedAllChildrenRemovedNeedsList()) {
+    removed_nodes =
+        MakeGarbageCollected<HeapVector<Member<Node>>>(CountChildren());
+  }
   {
     HTMLFrameOwnerElement::PluginDisposeSuspendScope suspend_plugin_dispose;
     TreeOrderedMap::RemoveScope tree_remove_scope;
@@ -822,6 +827,8 @@ void ContainerNode::RemoveChildren(SubtreeModificationAction action) {
       while (Node* child = first_child_) {
         RemoveBetween(nullptr, child->nextSibling(), *child);
         NotifyNodeRemoved(*child);
+        if (removed_nodes)
+          removed_nodes->push_back(child);
       }
     }
 
@@ -830,7 +837,7 @@ void ContainerNode::RemoveChildren(SubtreeModificationAction action) {
                              nullptr,
                              nullptr,
                              nullptr,
-                             nullptr};
+                             removed_nodes};
     ChildrenChanged(change);
   }
 
@@ -1036,6 +1043,10 @@ void ContainerNode::ChildrenChanged(const ChildrenChange& change) {
     inserted_node->SetStyleChangeOnInsertion();
 }
 
+bool ContainerNode::ChildrenChangedAllChildrenRemovedNeedsList() const {
+  return false;
+}
+
 void ContainerNode::CloneChildNodesFrom(const ContainerNode& node) {
   for (const Node& child : NodeTraversal::ChildrenOf(node))
     AppendChild(child.Clone(GetDocument(), CloneChildrenFlag::kClone));

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

@@ -366,15 +366,23 @@ class CORE_EXPORT ContainerNode : public Node {
     //  - siblingChanged.nextSibling after single node insertion
     //  - nextSibling of the last inserted node after multiple node insertion.
     Node* sibling_after_change = nullptr;
-    // TODO(crbug.com/1056094): This field is not used yet.
+    // List of removed nodes for ChildrenChangeType::kAllChildrenRemoved.
+    // This is available only if ChildrenChangedAllChildrenRemovedNeedsList()
+    // returns true.
     HeapVector<Member<Node>>* removed_nodes;
   };
 
   // Notifies the node that it's list of children have changed (either by adding
   // or removing child nodes), or a child node that is of the type
-  // CDATA_SECTION_NODE, TEXT_NODE or COMMENT_NODE has changed its value.
+  // kCdataSectionNode, kTextNode or kCommentNode has changed its value.
+  //
+  // ChildrenChanged() implementations may modify the DOM tree, and may dispatch
+  // synchronous events.
   virtual void ChildrenChanged(const ChildrenChange&);
 
+  // Provides ChildrenChange::removed_nodes for kAllChildrenRemoved.
+  virtual bool ChildrenChangedAllChildrenRemovedNeedsList() const;
+
   virtual bool ChildrenCanHaveStyle() const { return true; }
 
   void Trace(Visitor*) override;

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

@@ -767,16 +767,20 @@ class CORE_EXPORT Node : public EventTarget {
   //
   // Blink notifies this callback regardless if the subtree of the node is a
   // document tree or a floating subtree.  Implementation can determine the type
-  // of subtree by seeing insertionPoint->isConnected().  For a performance
+  // of subtree by seeing insertion_point->isConnected().  For a performance
   // reason, notifications are delivered only to ContainerNode subclasses if the
-  // insertionPoint is out of document.
+  // insertion_point is out of document.
   //
-  // There are another callback named didNotifySubtreeInsertionsToDocument(),
+  // There are another callback named DidNotifySubtreeInsertionsToDocument(),
   // which is called after all the descendant is notified, if this node was
   // inserted into the document tree. Only a few subclasses actually need
   // this. To utilize this, the node should return
-  // InsertionShouldCallDidNotifySubtreeInsertions from insertedInto().
+  // kInsertionShouldCallDidNotifySubtreeInsertions from InsertedInto().
   //
+  // InsertedInto() implementations must not modify the DOM tree, and must not
+  // dispatch synchronous events. On the other hand,
+  // DidNotifySubtreeInsertionsToDocument() may modify the DOM tree, and may
+  // dispatch synchronous events.
   enum InsertionNotificationRequest {
     kInsertionDone,
     kInsertionShouldCallDidNotifySubtreeInsertions
@@ -788,10 +792,12 @@ class CORE_EXPORT Node : public EventTarget {
 
   // Notifies the node that it is no longer part of the tree.
   //
-  // This is a dual of insertedInto(), and is similar to the
+  // This is a dual of InsertedInto(), and is similar to the
   // DOMNodeRemovedFromDocument DOM event, but does not require the overhead of
   // event dispatching, and is called _after_ the node is removed from the tree.
   //
+  // RemovedFrom() implementations must not modify the DOM tree, and must not
+  // dispatch synchronous events.
   virtual void RemovedFrom(ContainerNode& insertion_point);
 
   // FIXME(dominicc): This method is not debug-only--it is used by

third_party/blink/renderer/core/html/forms/html_opt_group_element.cc

@@ -84,6 +84,30 @@ bool HTMLOptGroupElement::MatchesEnabledPseudoClass() const {
   return !IsDisabledFormControl();
 }
 
+void HTMLOptGroupElement::ChildrenChanged(const ChildrenChange& change) {
+  HTMLElement::ChildrenChanged(change);
+  auto* select = OwnerSelectElement();
+  if (!select)
+    return;
+  if (change.type == ChildrenChangeType::kElementInserted) {
+    if (auto* option = DynamicTo<HTMLOptionElement>(change.sibling_changed))
+      select->OptionInserted(*option, option->Selected());
+  } else if (change.type == ChildrenChangeType::kElementRemoved) {
+    if (auto* option = DynamicTo<HTMLOptionElement>(change.sibling_changed))
+      select->OptionRemoved(*option);
+  } else if (change.type == ChildrenChangeType::kAllChildrenRemoved) {
+    DCHECK(change.removed_nodes);
+    for (Node* node : *change.removed_nodes) {
+      if (auto* option = DynamicTo<HTMLOptionElement>(node))
+        select->OptionRemoved(*option);
+    }
+  }
+}
+
+bool HTMLOptGroupElement::ChildrenChangedAllChildrenRemovedNeedsList() const {
+  return true;
+}
+
 Node::InsertionNotificationRequest HTMLOptGroupElement::InsertedInto(
     ContainerNode& insertion_point) {
   HTMLElement::InsertedInto(insertion_point);

third_party/blink/renderer/core/html/forms/html_opt_group_element.h

@@ -52,6 +52,8 @@ class CORE_EXPORT HTMLOptGroupElement final : public HTMLElement {
   ~HTMLOptGroupElement() override;
 
   bool SupportsFocus() const override;
+  void ChildrenChanged(const ChildrenChange& change) override;
+  bool ChildrenChangedAllChildrenRemovedNeedsList() const override;
   void ParseAttribute(const AttributeModificationParams&) override;
   void AccessKeyAction(bool send_mouse_events) override;
   void DidAddUserAgentShadowRoot(ShadowRoot&) override;

third_party/blink/renderer/core/html/forms/html_option_element.cc

@@ -339,30 +339,6 @@ String HTMLOptionElement::DefaultToolTip() const {
   return String();
 }
 
-Node::InsertionNotificationRequest HTMLOptionElement::InsertedInto(
-    ContainerNode& insertion_point) {
-  HTMLElement::InsertedInto(insertion_point);
-  if (HTMLSelectElement* select = OwnerSelectElement()) {
-    if (&insertion_point == select ||
-        (IsA<HTMLOptGroupElement>(insertion_point) &&
-         insertion_point.parentNode() == select))
-      select->OptionInserted(*this, is_selected_);
-  }
-  return kInsertionDone;
-}
-
-void HTMLOptionElement::RemovedFrom(ContainerNode& insertion_point) {
-  if (auto* select = DynamicTo<HTMLSelectElement>(insertion_point)) {
-    if (!parentNode() || IsA<HTMLOptGroupElement>(*parentNode()))
-      select->OptionRemoved(*this);
-  } else if (IsA<HTMLOptGroupElement>(insertion_point)) {
-    select = DynamicTo<HTMLSelectElement>(insertion_point.parentNode());
-    if (select)
-      select->OptionRemoved(*this);
-  }
-  HTMLElement::RemovedFrom(insertion_point);
-}
-
 String HTMLOptionElement::CollectOptionInnerText() const {
   StringBuilder text;
   for (Node* node = firstChild(); node;) {

third_party/blink/renderer/core/html/forms/html_option_element.h

@@ -101,8 +101,6 @@ class CORE_EXPORT HTMLOptionElement final : public HTMLElement {
   bool MatchesDefaultPseudoClass() const override;
   bool MatchesEnabledPseudoClass() const override;
   void ParseAttribute(const AttributeModificationParams&) override;
-  InsertionNotificationRequest InsertedInto(ContainerNode&) override;
-  void RemovedFrom(ContainerNode&) override;
   void AccessKeyAction(bool) override;
   void ChildrenChanged(const ChildrenChange&) override;
 

third_party/blink/renderer/core/html/forms/html_select_element.cc

@@ -866,6 +866,37 @@ void HTMLSelectElement::OptionSelectionStateChanged(HTMLOptionElement* option,
     ResetToDefaultSelection();
 }
 
+void HTMLSelectElement::ChildrenChanged(const ChildrenChange& change) {
+  HTMLFormControlElementWithState::ChildrenChanged(change);
+  if (change.type == ChildrenChangeType::kElementInserted) {
+    if (auto* option = DynamicTo<HTMLOptionElement>(change.sibling_changed)) {
+      OptionInserted(*option, option->Selected());
+    } else if (auto* optgroup =
+                   DynamicTo<HTMLOptGroupElement>(change.sibling_changed)) {
+      for (auto& option : Traversal<HTMLOptionElement>::ChildrenOf(*optgroup))
+        OptionInserted(option, option.Selected());
+    }
+  } else if (change.type == ChildrenChangeType::kElementRemoved) {
+    if (auto* option = DynamicTo<HTMLOptionElement>(change.sibling_changed)) {
+      OptionRemoved(*option);
+    } else if (auto* optgroup =
+                   DynamicTo<HTMLOptGroupElement>(change.sibling_changed)) {
+      for (auto& option : Traversal<HTMLOptionElement>::ChildrenOf(*optgroup))
+        OptionRemoved(option);
+    }
+  } else if (change.type == ChildrenChangeType::kAllChildrenRemoved) {
+    DCHECK(change.removed_nodes);
+    for (Node* node : *change.removed_nodes) {
+      if (auto* option = DynamicTo<HTMLOptionElement>(node))
+        OptionRemoved(*option);
+    }
+  }
+}
+
+bool HTMLSelectElement::ChildrenChangedAllChildrenRemovedNeedsList() const {
+  return true;
+}
+
 void HTMLSelectElement::OptionInserted(HTMLOptionElement& option,
                                        bool option_is_selected) {
   DCHECK_EQ(option.OwnerSelectElement(), this);

third_party/blink/renderer/core/html/forms/html_select_element.h

@@ -212,6 +212,8 @@ class CORE_EXPORT HTMLSelectElement final
   FormControlState SaveFormControlState() const override;
   void RestoreFormControlState(const FormControlState&) override;
 
+  void ChildrenChanged(const ChildrenChange& change) override;
+  bool ChildrenChangedAllChildrenRemovedNeedsList() const override;
   void ParseAttribute(const AttributeModificationParams&) override;
   bool IsPresentationAttribute(const QualifiedName&) const override;
 

third_party/blink/renderer/core/html/forms/html_select_element_test.cc

@@ -459,4 +459,14 @@ TEST_F(HTMLSelectElementTest, CrashOnAttachingMenuList) {
   ASSERT_TRUE(select->GetLayoutObject());
 }
 
+TEST_F(HTMLSelectElementTest, SlotAssignmentRecalcDuringOptionRemoval) {
+  // crbug.com/1056094
+  // This test passes if no CHECK failure about IsSlotAssignmentRecalcForbidden.
+  SetHtmlInnerHTML("<div dir=auto><select><option>option0");
+  auto* select = GetDocument().body()->firstChild()->firstChild();
+  auto* option = select->firstChild();
+  select->appendChild(option);
+  option->remove();
+}
+
 }  // namespace blink