Use RefPtr local variable for nextChild in insertChildrenBeforeWithou…

…tPreInsertionValidityCheck https://bugs.webkit.org/show_bug.cgi?id=268765 rdar://122122623 Reviewed by Ryosuke Niwa and Chris Dumez. This patch adds a RefPtr to hold a reference to nextChild so that the pointer stay valid through the scope of the function. In the test case, the removeChild() call (from the before() call in the js script) triggers a DOMSubtreeModified event, which eventually calls normalize. The normalize() call can destroy text elements when normalizing the content of the node if there is no one holding the reference to that node, so holding nextChild in a RefPtr prevents us from reading an invalid pointer. * LayoutTests/fast/dom/set-attribute-and-normalize-in-event-expected.txt: Added. * LayoutTests/fast/dom/set-attribute-and-normalize-in-event.html: Added. * Source/WebCore/dom/ContainerNode.cpp: (WebCore::ContainerNode::insertChildrenBeforeWithoutPreInsertionValidityCheck): Originally-landed-as: 274097.10@webkit-2024.2-embargoed (65b1fae34533). rdar://128089683 Canonical link: https://commits.webkit.org/278837@main
WebKit · May 15, 2024 · 0d0caf9 · 0d0caf9
1 parent 1b89fed
commit 0d0caf9
Show file tree

Hide file tree

Showing 3 changed files with 27 additions and 6 deletions.
diff --git a/LayoutTests/fast/dom/set-attribute-and-normalize-in-event-expected.txt b/LayoutTests/fast/dom/set-attribute-and-normalize-in-event-expected.txt
@@ -0,0 +1,3 @@
+CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded.
+CONSOLE MESSAGE: RangeError: Maximum call stack size exceeded.
+
diff --git a/LayoutTests/fast/dom/set-attribute-and-normalize-in-event.html b/LayoutTests/fast/dom/set-attribute-and-normalize-in-event.html
@@ -0,0 +1,17 @@
+<script>
+    function runTest() {
+        if (window.testRunner)
+            window.testRunner.dumpAsText();
+
+        marqueeElement.addEventListener("DOMSubtreeModified", () => {
+            try { hrElement.before(hrElement); } catch (e) { }
+            marqueeElement.normalize();
+        });
+
+        marqueeElement.setAttribute("a", "");
+    }
+</script>
+
+<body onload=runTest()>
+    <marquee id="marqueeElement">
+        <hr id="hrElement" width="1"></hr>
diff --git a/Source/WebCore/dom/ContainerNode.cpp b/Source/WebCore/dom/ContainerNode.cpp
@@ -890,10 +890,11 @@ ExceptionOr<void> ContainerNode::appendChildWithoutPreInsertionValidityCheck(Nod
 
 ExceptionOr<void> ContainerNode::insertChildrenBeforeWithoutPreInsertionValidityCheck(NodeVector&& newChildren, Node* nextChild)
 {
+    RefPtr refChild = nextChild;
     for (auto& child : newChildren) {
         if (RefPtr oldParent = child->parentNode()) {
-            if (nextChild == child.ptr())
-                nextChild = child->nextSibling();
+            if (refChild.get() == child.ptr())
+                refChild = child->nextSibling();
             if (auto result = oldParent->removeChild(child); result.hasException())
                 return result.releaseException();
         }
@@ -910,14 +911,14 @@ ExceptionOr<void> ContainerNode::insertChildrenBeforeWithoutPreInsertionValidity
 
     ChildListMutationScope mutation(*this);
     for (auto& child : newChildren) {
-        if (nextChild && nextChild->parentNode() != this) // Event listeners moved nextChild elsewhere.
+        if (refChild && refChild->parentNode() != this) // Event listeners moved nextChild elsewhere.
             break;
         if (child->parentNode()) // Event listeners inserted this child elsewhere.
             break;
-        executeNodeInsertionWithScriptAssertion(*this, child.get(), nextChild, ChildChange::Source::API, ReplacedAllChildren::No, [&] {
+        executeNodeInsertionWithScriptAssertion(*this, child.get(), refChild.get(), ChildChange::Source::API, ReplacedAllChildren::No, [&] {
             child->setTreeScopeRecursively(treeScope());
-            if (nextChild)
-                insertBeforeCommon(*nextChild, child.get());
+            if (refChild)
+                insertBeforeCommon(*refChild, child.get());
             else
                 appendChildCommon(child);
         });