mirror/browser
1
0
Fork 0
mirror of https://github.com/lightpanda-io/browser.git synced 2026-06-11 09:35:59 -04:00
Code Issues Packages Projects Releases Wiki Activity

Fix more DOM assumptions that callback and document.write can break

Browse Source 
As we've seen before, both document.write and custom elements callbacks can
introduce otherwise impossible states. See https://github.com/lightpanda-io/browser/pull/2172
for just one example.

The issue with both is that they can trigger code during DOM manipulation that
renders the state of that DOM manipulation invalid. This commit started with
a fairly easy case to understand (one that actually happened in production). It
goes something like:

```
<div id=x>
  <script>
    document.write('<script>x.innerHTML = "";</script>');
  </script>
</div>
```
Here we see that the written script deletes the node which is in the process
of being rendered.

These issues are almost always easy to fix, but they're hard to predict. After
fixing this issue, I asked Claude to check for other cases, and it was able to
find / reproduce / fix two more involving custom elements callback.
This commit is contained in:
Karl Seguin
2026-04-29 09:09:07 +08:00
parent e133b26e9b
commit 9d271ce4ae
5 changed files with 144 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
src/browser/Frame.zig
View File

@@ -2885,12 +2885,24 @@ pub fn insertAllChildrenBefore(self: *Frame, fragment: *Node, parent: *Node, ref
// 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 });
try self.insertNodeRelative(
parent,
child,
.{ .before = ref_node },
.{ .child_already_connected = child_was_connected },
);
// A callback fired by a previous iteration's insert (e.g. a custom
// element's connectedCallback) may have detached ref_node from
// parent. In that case, fall back to append so the remaining
// children still land in `parent` in source order.
if (ref_node._parent == parent) {
try self.insertNodeRelative(
parent,
child,
.{ .before = ref_node },
.{ .child_already_connected = child_was_connected },
);
} else {
try self.appendNode(
parent,
child,
.{ .child_already_connected = child_was_connected },
);
}
}
}

86
src/browser/tests/custom_elements/mutation_during_callback.html Normal file
View File

@@ -0,0 +1,86 @@
<!DOCTYPE html>
<body>
<script src="../testing.js"></script>
<!-- Regression: Frame.insertAllChildrenBefore (used by Element.outerHTML
setter) loops over fragment children and inserts each one .before
ref_node. _insertNodeRelative invokes connectedCallback for custom
elements, and that callback can detach ref_node from parent. The
next iteration must not panic on `ref_node._parent.? == parent`. -->
<script id="outerHTML_callback_detaches_ref_node">
{
class DetachTarget extends HTMLElement {
connectedCallback() {
const target = document.getElementById('outer_target');
if (target && target.parentNode) {
target.parentNode.removeChild(target);
}
}
}
customElements.define('detach-target-1', DetachTarget);
const wrap = document.createElement('div');
document.body.appendChild(wrap);
const target = document.createElement('span');
target.id = 'outer_target';
wrap.appendChild(target);
// Two siblings: the first one's connectedCallback removes `target`
// (which is the ref_node passed to insertAllChildrenBefore). The
// second one then tries to insert before a parentless ref_node.
target.outerHTML = '<detach-target-1></detach-target-1><b id="second">x</b>';
// We don't strictly care about the final shape, just that we got
// here without crashing. Sanity-check the first child landed.
testing.expectTrue(wrap.querySelector('detach-target-1') !== null);
}
</script>
<!-- Regression: Element.setInnerHTML removes existing children using
`childrenIterator()`. removeNode fires disconnectedCallback for
custom elements, and that callback can mutate the host's child
list, leaving the iterator advancing through a node that no longer
belongs to the parent. -->
<script id="innerHTML_disconnected_mutates_siblings">
{
let host;
let removeNextOnDisconnect = false;
class SiblingZapper extends HTMLElement {
disconnectedCallback() {
if (!removeNextOnDisconnect) return;
removeNextOnDisconnect = false;
// Reach into the parent (captured via closure) to remove a
// sibling that the outer setInnerHTML loop is about to visit.
// The element being removed here is what the iterator's
// current node still points its `next` pointer at.
if (host.children.length > 0) {
host.removeChild(host.children[0]);
}
}
}
customElements.define('sibling-zapper-2', SiblingZapper);
host = document.createElement('div');
document.body.appendChild(host);
const a = document.createElement('sibling-zapper-2');
const b = document.createElement('sibling-zapper-2');
const c = document.createElement('span');
host.appendChild(a);
host.appendChild(b);
host.appendChild(c);
removeNextOnDisconnect = true;
// Triggers the removal loop in setInnerHTML. Removing `a` fires its
// disconnectedCallback, which removes `b` (the iterator's next
// target). The loop must not visit a node whose `_parent` is null.
host.innerHTML = '<i>after</i>';
testing.expectEqual(1, host.children.length);
testing.expectEqual('I', host.children[0].tagName);
}
</script>
</body>
</html>

22
src/browser/tests/document/write.html
View File

@@ -120,6 +120,28 @@
}
</script>
<!-- Regression: a script written by document.write can detach the
currently-executing script during fragment parsing (here by
replacing the parent's children via innerHTML). document.write
must not crash when its `_current_script` ends up parentless. -->
<div id=dw_detach_container>
<script id=write_when_detached_during_write>
document.write('<script>document.getElementById("dw_detach_container").innerHTML = "";<\/script>');
// All we care is that the above didn't crash.
testing.expectEqual(true, true);
</script>
</div>
<script id=verify_dw_detach>
{
// The previous script *not crashing* is the real assertion. We also
// sanity-check that the inner script's innerHTML="" took effect by
// verifying the only element children are gone.
const container = document.getElementById('dw_detach_container');
testing.expectEqual(0, container.children.length);
}
</script>
<!-- Phase 3 Tests: document.open/close would go here -->
<!-- Note: Testing document.open/close requires async/setTimeout which doesn't -->
<!-- work well with the test isolation. The implementation is tested manually. -->

9
src/browser/webapi/Document.zig
View File

@@ -713,13 +713,20 @@ fn writeInternal(self: *Document, text: []const []const u8, append_newline: bool
// Determine insertion point:
// - If _write_insertion_point is set and still parented correctly, continue from there
// - Otherwise, start after the script (first write, or previous insertion point was removed)
// parseFragment above can synchronously execute a parser-blocking script
// (e.g. <script src=...> with from_parser=true). That script's side
// effects can detach `script` from `parent` — for instance, by writing
// to parent.innerHTML — leaving us nowhere sensible to splice in.
var insert_after: ?*Node = blk: {
if (self._write_insertion_point) |wip| {
if (wip._parent == parent) {
break :blk wip;
}
}
break :blk script.asNode();
if (script.asNode()._parent == parent) {
break :blk script.asNode();
}
return;
};
for (children_to_insert.items) |child| {

14
src/browser/webapi/Element.zig
View File

@@ -474,7 +474,11 @@ pub fn setOuterHTML(self: *Element, html: []const u8, frame: *Frame) !void {
try frame.insertAllChildrenBefore(fragment, parent, node);
}
frame.removeNode(parent, node, .{ .will_be_reconnected = false });
// A custom element callback fired during insertAllChildrenBefore may
// have already detached `node`; only remove it if it's still here.
if (node._parent == parent) {
frame.removeNode(parent, node, .{ .will_be_reconnected = false });
}
}
pub fn getInnerHTML(self: *Element, writer: *std.Io.Writer, frame: *Frame) !void {
@@ -485,10 +489,12 @@ pub fn getInnerHTML(self: *Element, writer: *std.Io.Writer, frame: *Frame) !void
pub fn setInnerHTML(self: *Element, html: []const u8, frame: *Frame) !void {
const parent = self.asNode();
// Remove all existing children
// Remove all existing children. Drain via firstChild(): removeNode
// fires disconnectedCallback for custom elements, which can mutate
// the child list and dangle any cached next-pointer the iterator
// would otherwise hold.
frame.domChanged();
var it = parent.childrenIterator();
while (it.next()) |child| {
while (parent.firstChild()) |child| {
frame.removeNode(parent, child, .{ .will_be_reconnected = false });
}