From 433c03c709211acb7ea673e92c7afb21903c6d44 Mon Sep 17 00:00:00 2001
From: Karl Seguin <k@openmymind.io>
Date: Mon, 23 Mar 2026 21:16:11 +0800
Subject: [PATCH] Handle `appendAllChildren` mutating the list of children

`appendAllChildren` iterates through the children, but when a child is appended
it can mutate the DOM (only via a custom element connected callback AFAIK) which
can render the iterator invalid. Constantly get parent.firstChild() as the
target.
---
 src/browser/Page.zig                     | 12 +++++---
 src/browser/tests/node/append_child.html | 37 ++++++++++++++++++++++++
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/src/browser/Page.zig b/src/browser/Page.zig
index bb51be90..c817cf45 100644
--- a/src/browser/Page.zig
+++ b/src/browser/Page.zig
@@ -2617,8 +2617,10 @@ pub fn appendAllChildren(self: *Page, parent: *Node, target: *Node) !void {
     self.domChanged();
     const dest_connected = target.isConnected();
 
-    var it = parent.childrenIterator();
-    while (it.next()) |child| {
+    // Use firstChild() instead of iterator to handle cases where callbacks
+    // (like custom element connectedCallback) modify the parent during iteration.
+    // The iterator captures "next" pointers that can become stale.
+    while (parent.firstChild()) |child| {
         // Check if child was connected BEFORE removing it from parent
         const child_was_connected = child.isConnected();
         self.removeNode(parent, child, .{ .will_be_reconnected = dest_connected });
@@ -2630,8 +2632,10 @@ pub fn insertAllChildrenBefore(self: *Page, fragment: *Node, parent: *Node, ref_
     self.domChanged();
     const dest_connected = parent.isConnected();
 
-    var it = fragment.childrenIterator();
-    while (it.next()) |child| {
+    // Use firstChild() instead of iterator to handle cases where callbacks
+    // (like custom element connectedCallback) modify the fragment during iteration.
+    // The iterator captures "next" pointers that can become stale.
+    while (fragment.firstChild()) |child| {
         // Check if child was connected BEFORE removing it from fragment
         const child_was_connected = child.isConnected();
         self.removeNode(fragment, child, .{ .will_be_reconnected = dest_connected });
diff --git a/src/browser/tests/node/append_child.html b/src/browser/tests/node/append_child.html
index 0736fa03..151815b9 100644
--- a/src/browser/tests/node/append_child.html
+++ b/src/browser/tests/node/append_child.html
@@ -28,3 +28,40 @@
   d1.appendChild(p2);
   assertChildren(['p1', 'p2'], d1);
 </script>
+
+<div id=d3></div>
+<script id=appendChild_fragment_mutation>
+  // Test that appendChild with DocumentFragment handles synchronous callbacks
+  // (like custom element connectedCallback) that modify the fragment during iteration.
+  // This reproduces a bug where the iterator captures "next" node pointers
+  // before processing, but callbacks can remove those nodes from the fragment.
+  const d3 = $('#d3');
+  const fragment = document.createDocumentFragment();
+
+  // Create custom element whose connectedCallback modifies the fragment
+  let bElement = null;
+  class ModifyingElement extends HTMLElement {
+    connectedCallback() {
+      // When this element is connected, remove 'b' from the fragment
+      if (bElement && bElement.parentNode === fragment) {
+        fragment.removeChild(bElement);
+      }
+    }
+  }
+  customElements.define('modifying-element', ModifyingElement);
+
+  const a = document.createElement('modifying-element');
+  a.id = 'a';
+  const b = document.createElement('span');
+  b.id = 'b';
+  bElement = b;
+  fragment.appendChild(a);
+  fragment.appendChild(b);
+
+  // This should not crash - appendChild should handle the modification gracefully
+  d3.appendChild(fragment);
+
+  // 'a' should be in d3, 'b' was removed by connectedCallback and is now detached
+  assertChildren(['a'], d3);
+  testing.expectEqual(null, b.parentNode);
+</script>