Merge pull request #2172 from lightpanda-io/replaceChild_safety

Improve safety of Node.replaceChild and Element.replaceWith

src/browser/tests/element/replace_with.html

@@ -332,3 +332,34 @@
   testing.expectEqual(new13, document.getElementById('new13'));
   testing.expectEqual(l4, new13.parentElement);
 </script>
+
+<!-- Test 14: Callback removes ref_node during replaceWith -->
+<script id=test14-callback-removes-ref-node>
+  // Custom element whose connectedCallback removes the ref_node
+  // This tests that replaceWith handles mid-operation DOM mutations
+  class RefRemover extends HTMLElement {
+    connectedCallback() {
+      // Remove the next sibling (which is the ref_node being replaced)
+      const sibling = this.nextSibling;
+      if (sibling) {
+        sibling.remove();
+      }
+    }
+  }
+  customElements.define('ref-remover', RefRemover);
+
+  const parent14 = document.createElement('div');
+  const oldChild14 = document.createElement('div');
+  parent14.appendChild(oldChild14);
+  document.body.appendChild(parent14);
+
+  const newChild14 = document.createElement('ref-remover');
+
+  // replaceWith inserts newChild14 before oldChild14, then connectedCallback
+  // fires and removes oldChild14. replaceWith should not crash when it
+  // tries to remove the already-removed oldChild14.
+  oldChild14.replaceWith(newChild14);
+
+  testing.expectEqual(newChild14, parent14.firstChild);
+  testing.expectEqual(null, oldChild14.parentNode);
+</script>

src/browser/tests/node/replace_child.html

@@ -40,3 +40,34 @@
   testing.expectEqual(c3, d1.replaceChild(c3, c3));
   assertChildren([c3, c4], d1)
 </script>
+
+<script id=replaceChild_callback_removes_old_child>
+  // Custom element whose connectedCallback removes the old_child
+  // This tests that replaceChild handles mid-operation DOM mutations
+  class RemoverElement extends HTMLElement {
+    connectedCallback() {
+      // Remove the sibling that replaceChild is about to remove
+      const sibling = this.nextSibling;
+      if (sibling) {
+        sibling.remove();
+      }
+    }
+  }
+  customElements.define('remover-element', RemoverElement);
+
+  const parent = document.createElement('div');
+  const oldChild = document.createElement('div');
+  parent.appendChild(oldChild);
+  document.body.appendChild(parent);
+
+  const newChild = document.createElement('remover-element');
+
+  // insertBefore inserts newChild before oldChild, then connectedCallback
+  // fires and removes oldChild. replaceChild should not crash when it
+  // tries to remove the already-removed oldChild.
+  parent.replaceChild(newChild, oldChild);
+
+  testing.expectEqual(newChild, parent.firstChild);
+  testing.expectEqual(null, parent.lastChild.nextSibling);
+  testing.expectEqual(null, oldChild.parentNode);
+</script>

src/browser/webapi/Element.zig

@@ -872,7 +872,9 @@ pub fn replaceWith(self: *Element, nodes: []const Node.NodeOrText, page: *Page)
         );
     }
 
-    if (rm_ref_node) {
+    // Re-check parent after insertNodeRelative since callbacks (e.g. connectedCallback)
+    // could have already removed ref_node from parent.
+    if (rm_ref_node and ref_node._parent == parent) {
         page.removeNode(parent, ref_node, .{ .will_be_reconnected = false });
     }
 }

src/browser/webapi/Node.zig

@@ -624,7 +624,9 @@ pub fn replaceChild(self: *Node, new_child: *Node, old_child: *Node, page: *Page
 
     // Special case: if we replace a node by itself, we don't remove it.
     // insertBefore is an noop in this case.
-    if (new_child != old_child) {
+    // Re-check parent after insertBefore since callbacks (e.g. connectedCallback)
+    // could have already removed old_child from self.
+    if (new_child != old_child and old_child._parent == self) {
         page.removeNode(self, old_child, .{ .will_be_reconnected = false });
     }