Changeset 282505 in webkit

Timestamp:

Sep 16, 2021, 1:26:42 AM (4 years ago)

Author:

Adrian Perez de Castro

Message:

Merge r276010 - REGRESSION(r272900): Nullptr crash in ComposedTreeIterator::traverseNextInShadowTree() via ShadowRoot::hostChildElementDidChange
​https://bugs.webkit.org/show_bug.cgi?id=222720

Patch by Carlos Garcia Campos <​cgarcia@igalia.com> on 2021-04-15
Reviewed by Ryosuke Niwa.

This patch reverts r274064 to apply a different fix. Instead of null-checking the nodes returned by
SlotAssignment::assignedNodesForSlot(), assigned nodes are removed from the list when they are about to be
removed from the parent. That ensures we never return nullptr nodes nor nodes with a nullptr parent from the
assigned nodes vector.

• dom/ComposedTreeIterator.cpp:

(WebCore::ComposedTreeIterator::traverseNextInShadowTree):
(WebCore::ComposedTreeIterator::advanceInSlot):

• dom/ContainerNode.cpp:

(WebCore::ContainerNode::removeBetween):

• dom/Node.h:

(WebCore::Node::hasShadowRootContainingSlots const):
(WebCore::Node::setHasShadowRootContainingSlots):

• dom/ShadowRoot.h:
• dom/SlotAssignment.cpp:

(WebCore::SlotAssignment::addSlotElementByName):
(WebCore::SlotAssignment::removeSlotElementByName):
(WebCore::SlotAssignment::willRemoveAssignedNode):

• dom/SlotAssignment.h:

(WebCore::ShadowRoot::willRemoveAssignedNode):

Location:

releases/WebKitGTK/webkit-2.32/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/ComposedTreeIterator.cpp (modified) (2 diffs)
• dom/ContainerNode.cpp (modified) (1 diff)
• dom/Node.h (modified) (2 diffs)
• dom/ShadowRoot.h (modified) (1 diff)
• dom/SlotAssignment.cpp (modified) (3 diffs)
• dom/SlotAssignment.h (modified) (3 diffs)

releases/WebKitGTK/webkit-2.32/Source/WebCore/ChangeLog

@@ +1 @@
+2021-04-15  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        REGRESSION(r272900): Nullptr crash in ComposedTreeIterator::traverseNextInShadowTree() via ShadowRoot::hostChildElementDidChange
+        https://bugs.webkit.org/show_bug.cgi?id=222720
+
+        Reviewed by Ryosuke Niwa.
+
+        This patch reverts r274064 to apply a different fix. Instead of null-checking the nodes returned by
+        SlotAssignment::assignedNodesForSlot(), assigned nodes are removed from the list when they are about to be
+        removed from the parent. That ensures we never return nullptr nodes nor nodes with a nullptr parent from the
+        assigned nodes vector.
+
+        * dom/ComposedTreeIterator.cpp:
+        (WebCore::ComposedTreeIterator::traverseNextInShadowTree):
+        (WebCore::ComposedTreeIterator::advanceInSlot):
+        * dom/ContainerNode.cpp:
+        (WebCore::ContainerNode::removeBetween):
+        * dom/Node.h:
+        (WebCore::Node::hasShadowRootContainingSlots const):
+        (WebCore::Node::setHasShadowRootContainingSlots):
+        * dom/ShadowRoot.h:
+        * dom/SlotAssignment.cpp:
+        (WebCore::SlotAssignment::addSlotElementByName):
+        (WebCore::SlotAssignment::removeSlotElementByName):
+        (WebCore::SlotAssignment::willRemoveAssignedNode):
+        * dom/SlotAssignment.h:
+        (WebCore::ShadowRoot::willRemoveAssignedNode):
+
 2021-04-14  Ryosuke Niwa  <rniwa@webkit.org>
 

releases/WebKitGTK/webkit-2.32/Source/WebCore/dom/ComposedTreeIterator.cpp

@@ -163 +163 @@
         auto& slot = downcast<HTMLSlotElement>(current());
         if (auto* assignedNodes = slot.assignedNodes()) {
-            if (auto assignedNode = assignedNodes->at(0)) {
-                context().slotNodeIndex = 0;
-                m_contextStack.append(Context(*assignedNode->parentElement(), *assignedNode, Context::Slotted));
-                return;
-            }
+            context().slotNodeIndex = 0;
+            auto* assignedNode = assignedNodes->at(0).get();
+            ASSERT(assignedNode);
+            ASSERT(assignedNode->parentElement());
+            m_contextStack.append(Context(*assignedNode->parentElement(), *assignedNode, Context::Slotted));
+            return;
         }
     }
@@ -200 +201 @@
 
     auto* slotNode = assignedNodes.at(context().slotNodeIndex).get();
-    if (!slotNode)
-        return false;
+    ASSERT(slotNode);
+    ASSERT(slotNode->parentElement());
     m_contextStack.append(Context(*slotNode->parentElement(), *slotNode, Context::Slotted));
     return true;

releases/WebKitGTK/webkit-2.32/Source/WebCore/dom/ContainerNode.cpp

@@ -631 +631 @@
     destroyRenderTreeIfNeeded(oldChild);
 
+    if (UNLIKELY(hasShadowRootContainingSlots()))
+        shadowRoot()->willRemoveAssignedNode(oldChild);
+
     if (nextChild) {
         nextChild->setPreviousSibling(previousChild);

releases/WebKitGTK/webkit-2.32/Source/WebCore/dom/Node.h

@@ -223 +223 @@
     void setHasSyntheticAttrChildNodes(bool flag) { setNodeFlag(NodeFlag::HasSyntheticAttrChildNodes, flag); }
 
+    bool hasShadowRootContainingSlots() const { return hasNodeFlag(NodeFlag::HasShadowRootContainingSlots); }
+    void setHasShadowRootContainingSlots(bool flag) { setNodeFlag(NodeFlag::HasShadowRootContainingSlots, flag); }
+
     // If this node is in a shadow tree, returns its shadow host. Otherwise, returns null.
     WEBCORE_EXPORT Element* shadowHost() const;
@@ -567 +570 @@
         InclusiveAncestorStateForForm = 1 << 28,
         InclusiveAncestorStateForCanvas = 1 << 29,
-        // Bits 30-31 are free.
+        HasShadowRootContainingSlots = 1 << 30,
+        // Bit 31 is free.
     };
 

releases/WebKitGTK/webkit-2.32/Source/WebCore/dom/ShadowRoot.h

@@ -94 +94 @@
     void resolveSlotsBeforeNodeInsertionOrRemoval();
     void willRemoveAllChildren(ContainerNode&);
+    void willRemoveAssignedNode(const Node&);
 
     void didRemoveAllChildrenOfShadowHost();

releases/WebKitGTK/webkit-2.32/Source/WebCore/dom/SlotAssignment.cpp

@@ -125 +125 @@
         assignSlots(shadowRoot);
 
+    shadowRoot.host()->setHasShadowRootContainingSlots(true);
+    m_slotElementCount++;
+
     bool needsSlotchangeEvent = shadowRoot.shouldFireSlotchangeEvent() && hasAssignedNodes(shadowRoot, slot);
 
@@ -152 +155 @@
 #endif
 
-    if (auto* host = shadowRoot.host()) // FIXME: We should be able to do a targeted reconstruction.
+    ASSERT(m_slotElementCount > 0);
+    m_slotElementCount--;
+
+    if (auto host = makeRefPtr(shadowRoot.host())) {
+        // FIXME: We should be able to do a targeted reconstruction.
         host->invalidateStyleAndRenderersForSubtree();
+        if (!m_slotElementCount)
+            host->setHasShadowRootContainingSlots(false);
+    }
 
     auto* slot = m_slots.get(slotNameFromAttributeValue(name));
@@ -338 +348 @@
 }
 
+void SlotAssignment::willRemoveAssignedNode(const Node& node)
+{
+    if (!m_slotAssignmentsIsValid)
+        return;
+
+    if (!is<Text>(node) && !is<Element>(node))
+        return;
+
+    auto* slot = m_slots.get(slotNameForHostChild(node));
+    if (!slot || slot->assignedNodes.isEmpty())
+        return;
+
+    slot->assignedNodes.removeFirstMatching([&node](const auto& item) {
+        return item.get() == &node;
+    });
+}
+
 const AtomString& SlotAssignment::slotNameForHostChild(const Node& child) const
 {

releases/WebKitGTK/webkit-2.32/Source/WebCore/dom/SlotAssignment.h

@@ -61 +61 @@
 
     const Vector<WeakPtr<Node>>* assignedNodesForSlot(const HTMLSlotElement&, ShadowRoot&);
+    void willRemoveAssignedNode(const Node&);
 
     virtual void hostChildElementDidChange(const Element&, ShadowRoot&);
@@ -104 +105 @@
     unsigned m_slotMutationVersion { 0 };
     unsigned m_slotResolutionVersion { 0 };
+    unsigned m_slotElementCount { 0 };
 };
 
@@ -145 +147 @@
 }
 
+inline void ShadowRoot::willRemoveAssignedNode(const Node& node)
+{
+    if (m_slotAssignment)
+        m_slotAssignment->willRemoveAssignedNode(node);
+}
+
 } // namespace WebCore