From 1bffde88081a10a9ea0727204f9431cb65eb73be Mon Sep 17 00:00:00 2001
From: Karl Seguin <k@openmymind.io>
Date: Thu, 16 Apr 2026 16:01:27 +0800
Subject: [PATCH] Improve safety of Node.replaceChild and Element.replaceWith

We've seen this sort of thing before - an assumption we make about the state of
the DOM through a transition is broken by CustomElement callbacks.

Here we see replaceChild which (a) inserts the new nodes and (b) removes the
old one. As part of removing, our page.removeNode has an assertion that the
child belongs to the parent. This is a guarantee that the Page is asking the
caller (Node.replaceChild in this case) to make. But, if the node being inserted
is a custom element, it can have a callback so step (a) can cause changes to
the document, including removing/moving the node being replaced.

TL;DR - CustomElement callbacks means that we have to check that the child to
be replaced is still a child of the parent after our insert.
---
 src/browser/tests/element/replace_with.html | 31 +++++++++++++++++++++
 src/browser/tests/node/replace_child.html   | 31 +++++++++++++++++++++
 src/browser/webapi/Element.zig              |  4 ++-
 src/browser/webapi/Node.zig                 |  4 ++-
 4 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/src/browser/tests/element/replace_with.html b/src/browser/tests/element/replace_with.html
index 14940f4d..0597b9b1 100644
--- a/src/browser/tests/element/replace_with.html
+++ b/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>
diff --git a/src/browser/tests/node/replace_child.html b/src/browser/tests/node/replace_child.html
index 51b0a173..b45a3d51 100644
--- a/src/browser/tests/node/replace_child.html
+++ b/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>
diff --git a/src/browser/webapi/Element.zig b/src/browser/webapi/Element.zig
index 655a4e9d..a26f1f80 100644
--- a/src/browser/webapi/Element.zig
+++ b/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 });
     }
 }
diff --git a/src/browser/webapi/Node.zig b/src/browser/webapi/Node.zig
index 5871abee..d6d73fee 100644
--- a/src/browser/webapi/Node.zig
+++ b/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 });
     }