From becbd54f4ee4ded6120d2a3e3ed4a01a0a440de3 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Tue, 5 May 2026 18:30:56 +0800 Subject: [PATCH] Track DOM version based on the node's owning frame This fixes 2 separate issues, driven by the [very] broken /dom/ranges/Range-insertNode.html WPT test. The first, and simplest, is that when a script element is cloned, its executing _executed and _force_async flags need to be copied. This prevents double- execution. The second is more complicated: dom version tracking (as a caching mechanism used in various live collection) has to be against the node's owning frame, not the frame that's executing the JavaScript. This includes both the version checking and version change (on dom mutation). This introduces a relatively expensive node -> document -> frame on every mutation and cache read. This is potentially worth optimizing somehow, in a follow up PR. Two options come to mind: 1 - Track a single dom version on the Page. Mutation in frame1 invalidates frame2. 2 - Optimize for the frame-less case. If a page has no frame, then the calling context should be equal to the owning document's frame. --- src/browser/Frame.zig | 4 +- .../html/script/clone_already_started.html | 30 ++++++++++ .../tests/frames/cross_realm_collection.html | 58 +++++++++++++++++++ .../support/cross_realm_collection.html | 3 + src/browser/webapi/Document.zig | 6 +- src/browser/webapi/DocumentFragment.zig | 2 +- src/browser/webapi/Element.zig | 9 ++- src/browser/webapi/Node.zig | 29 ++++++++-- src/browser/webapi/Range.zig | 2 +- src/browser/webapi/collections/ChildNodes.zig | 24 +++++--- .../webapi/collections/HTMLAllCollection.zig | 12 ++-- src/browser/webapi/collections/node_live.zig | 9 ++- src/browser/webapi/element/Attribute.zig | 4 +- src/browser/webapi/element/Html.zig | 2 +- src/browser/webapi/element/html/Option.zig | 2 +- src/browser/webapi/element/html/Script.zig | 9 +++ 16 files changed, 168 insertions(+), 37 deletions(-) create mode 100644 src/browser/tests/element/html/script/clone_already_started.html create mode 100644 src/browser/tests/frames/cross_realm_collection.html create mode 100644 src/browser/tests/frames/support/cross_realm_collection.html diff --git a/src/browser/Frame.zig b/src/browser/Frame.zig index 422fa955..51c6254b 100644 --- a/src/browser/Frame.zig +++ b/src/browser/Frame.zig @@ -2976,7 +2976,7 @@ pub fn appendNode(self: *Frame, parent: *Node, child: *Node, opts: InsertNodeOpt } pub fn appendAllChildren(self: *Frame, parent: *Node, target: *Node) !void { - self.domChanged(); + target.bumpDomVersion(self); const dest_connected = target.isConnected(); // Use firstChild() instead of iterator to handle cases where callbacks @@ -2991,7 +2991,7 @@ pub fn appendAllChildren(self: *Frame, parent: *Node, target: *Node) !void { } pub fn insertAllChildrenBefore(self: *Frame, fragment: *Node, parent: *Node, ref_node: *Node) !void { - self.domChanged(); + parent.bumpDomVersion(self); const dest_connected = parent.isConnected(); // Use firstChild() instead of iterator to handle cases where callbacks diff --git a/src/browser/tests/element/html/script/clone_already_started.html b/src/browser/tests/element/html/script/clone_already_started.html new file mode 100644 index 00000000..5748459d --- /dev/null +++ b/src/browser/tests/element/html/script/clone_already_started.html @@ -0,0 +1,30 @@ + + + + + + + + diff --git a/src/browser/tests/frames/cross_realm_collection.html b/src/browser/tests/frames/cross_realm_collection.html new file mode 100644 index 00000000..9d14b1b9 --- /dev/null +++ b/src/browser/tests/frames/cross_realm_collection.html @@ -0,0 +1,58 @@ + + + + + + + + + + + + + diff --git a/src/browser/tests/frames/support/cross_realm_collection.html b/src/browser/tests/frames/support/cross_realm_collection.html new file mode 100644 index 00000000..f247803c --- /dev/null +++ b/src/browser/tests/frames/support/cross_realm_collection.html @@ -0,0 +1,3 @@ + + +

p0

p1

p2

diff --git a/src/browser/webapi/Document.zig b/src/browser/webapi/Document.zig index 8ffec5ce..b18211f5 100644 --- a/src/browser/webapi/Document.zig +++ b/src/browser/webapi/Document.zig @@ -488,8 +488,8 @@ pub fn importNode(_: *const Document, node: *Node, deep_: ?bool, frame: *Frame) pub fn append(self: *Document, nodes: []const Node.NodeOrText, frame: *Frame) !void { try validateDocumentNodes(self, nodes, false); - frame.domChanged(); const parent = self.asNode(); + parent.bumpDomVersion(frame); const parent_is_connected = parent.isConnected(); for (nodes) |node_or_text| { @@ -513,8 +513,8 @@ pub fn append(self: *Document, nodes: []const Node.NodeOrText, frame: *Frame) !v pub fn prepend(self: *Document, nodes: []const Node.NodeOrText, frame: *Frame) !void { try validateDocumentNodes(self, nodes, false); - frame.domChanged(); const parent = self.asNode(); + parent.bumpDomVersion(frame); const parent_is_connected = parent.isConnected(); var i = nodes.len; @@ -736,7 +736,7 @@ fn writeInternal(self: *Document, text: []const []const u8, append_newline: bool insert_after = child; } - frame.domChanged(); + parent.bumpDomVersion(frame); self._write_insertion_point = children_to_insert.getLast(); } diff --git a/src/browser/webapi/DocumentFragment.zig b/src/browser/webapi/DocumentFragment.zig index 186bc68a..106cb427 100644 --- a/src/browser/webapi/DocumentFragment.zig +++ b/src/browser/webapi/DocumentFragment.zig @@ -154,7 +154,7 @@ pub fn getInnerHTML(self: *DocumentFragment, writer: *std.Io.Writer, frame: *Fra pub fn setInnerHTML(self: *DocumentFragment, html: []const u8, frame: *Frame) !void { const parent = self.asNode(); - frame.domChanged(); + parent.bumpDomVersion(frame); var it = parent.childrenIterator(); while (it.next()) |child| { frame.removeNode(parent, child, .{ .will_be_reconnected = false }); diff --git a/src/browser/webapi/Element.zig b/src/browser/webapi/Element.zig index a5aa19d5..91f47758 100644 --- a/src/browser/webapi/Element.zig +++ b/src/browser/webapi/Element.zig @@ -467,7 +467,7 @@ pub fn setOuterHTML(self: *Element, html: []const u8, frame: *Frame) !void { const node = self.asNode(); const parent = node._parent orelse return; - frame.domChanged(); + parent.bumpDomVersion(frame); if (html.len > 0) { const fragment = (try Node.DocumentFragment.init(frame)).asNode(); try frame.parseHtmlAsChildren(fragment, html); @@ -493,7 +493,7 @@ pub fn setInnerHTML(self: *Element, html: []const u8, frame: *Frame) !void { // fires disconnectedCallback for custom elements, which can mutate // the child list and dangle any cached next-pointer the iterator // would otherwise hold. - frame.domChanged(); + parent.bumpDomVersion(frame); while (parent.firstChild()) |child| { frame.removeNode(parent, child, .{ .will_be_reconnected = false }); } @@ -879,10 +879,9 @@ pub fn replaceChildren(self: *Element, nodes: []const Node.NodeOrText, frame: *F } pub fn replaceWith(self: *Element, nodes: []const Node.NodeOrText, frame: *Frame) !void { - frame.domChanged(); - const ref_node = self.asNode(); const parent = ref_node._parent orelse return; + parent.bumpDomVersion(frame); const parent_is_connected = parent.isConnected(); @@ -919,9 +918,9 @@ pub fn replaceWith(self: *Element, nodes: []const Node.NodeOrText, frame: *Frame } pub fn remove(self: *Element, frame: *Frame) void { - frame.domChanged(); const node = self.asNode(); const parent = node._parent orelse return; + parent.bumpDomVersion(frame); frame.removeNode(parent, node, .{ .will_be_reconnected = false }); } diff --git a/src/browser/webapi/Node.zig b/src/browser/webapi/Node.zig index fafa32c6..718c065a 100644 --- a/src/browser/webapi/Node.zig +++ b/src/browser/webapi/Node.zig @@ -226,7 +226,7 @@ pub fn appendChild(self: *Node, child: *Node, frame: *Frame) !*Node { try validateNodeInsertion(self, child); - frame.domChanged(); + self.bumpDomVersion(frame); // If the child is currently connected, and if its new parent is connected, // then we can remove + add a bit more efficiently (we don't have to fully @@ -512,11 +512,30 @@ pub fn ownerDocument(self: *const Node, frame: *const Frame) ?*Document { return frame.document; } +// Returns the Frame that owns this node's tree. Used to tie cached state of +// "live" collections (NodeList, HTMLCollection, etc.) to the right frame's DOM +// version: cross-realm callers must invalidate based on mutations through the +// node's owning frame, not the caller's frame. +// +// Falls back to `default` when the node has no associated document yet (e.g., +// freshly created and detached) or its document has no frame. pub fn ownerFrame(self: *const Node, default: *Frame) *Frame { + if (self._type == .document) { + return self._type.document._frame orelse default; + } const doc = self.ownerDocument(default) orelse return default; return doc._frame orelse default; } +// Tells the owning frame that this node's subtree has changed. Use this from +// mutation paths instead of `frame.domChanged()` so cross-realm mutations +// (e.g., parent JS mutating an iframe's DOM) bump the iframe's version, not +// the caller's. Otherwise, live collections that key off the owning frame's +// version won't see the change and will return stale cached state. +pub fn bumpDomVersion(self: *const Node, default: *Frame) void { + self.ownerFrame(default).domChanged(); +} + pub const ResolveURLOpts = struct { allocator: ?Allocator = null, }; @@ -548,7 +567,7 @@ pub fn removeChild(self: *Node, child: *Node, frame: *Frame) !*Node { var it = self.childrenIterator(); while (it.next()) |n| { if (n == child) { - frame.domChanged(); + self.bumpDomVersion(frame); frame.removeNode(self, child, .{ .will_be_reconnected = false }); return child; } @@ -563,7 +582,7 @@ pub fn insertBefore(self: *Node, new_node: *Node, ref_node_: ?*Node, frame: *Fra // special case: if nodes are the same, ignore the change. if (new_node == ref_node_) { - frame.domChanged(); + self.bumpDomVersion(frame); if (frame.hasMutationObservers()) { const parent = new_node._parent.?; @@ -594,7 +613,7 @@ pub fn insertBefore(self: *Node, new_node: *Node, ref_node_: ?*Node, frame: *Fra const parent_owner = self.ownerDocument(frame) orelse self.as(Document); const adopting_to_new_document = child_owner != null and child_owner.? != parent_owner; - frame.domChanged(); + self.bumpDomVersion(frame); const will_be_reconnected = self.isConnected() and !adopting_to_new_document; if (new_node._parent) |parent| { frame.removeNode(parent, new_node, .{ .will_be_reconnected = will_be_reconnected }); @@ -1049,7 +1068,7 @@ pub fn replaceChildren(self: *Node, nodes: []const NodeOrText, frame: *Frame) !v } } - frame.domChanged(); + self.bumpDomVersion(frame); // Remove all existing children var it = self.childrenIterator(); diff --git a/src/browser/webapi/Range.zig b/src/browser/webapi/Range.zig index c1eb3fde..686c69df 100644 --- a/src/browser/webapi/Range.zig +++ b/src/browser/webapi/Range.zig @@ -374,7 +374,7 @@ pub fn deleteContents(self: *Range, frame: *Frame) !void { if (self._proto.getCollapsed()) { return; } - frame.domChanged(); + self._proto._start_container.bumpDomVersion(frame); // Simple case: same container if (self._proto._start_container == self._proto._end_container) { diff --git a/src/browser/webapi/collections/ChildNodes.zig b/src/browser/webapi/collections/ChildNodes.zig index 1fa3b180..c94e7e53 100644 --- a/src/browser/webapi/collections/ChildNodes.zig +++ b/src/browser/webapi/collections/ChildNodes.zig @@ -34,8 +34,11 @@ _arena: std.mem.Allocator, _last_index: usize, _last_length: ?u32, _last_node: ?*std.DoublyLinkedList.Node, +// Version observed on `_owning_frame` the last time we refreshed our cache. +// Compare against `_owning_frame.version` to detect DOM mutations. _cached_version: usize, _node: *Node, +_owning_frame: *Frame, pub const KeyIterator = GenericIterator(Iterator, "0"); pub const ValueIterator = GenericIterator(Iterator, "1"); @@ -45,6 +48,8 @@ pub fn init(node: *Node, frame: *Frame) !*ChildNodes { const arena = try frame.getArena(.small, "ChildNodes"); errdefer frame.releaseArena(arena); + const owning_frame = node.ownerFrame(frame); + const self = try arena.create(ChildNodes); self.* = .{ ._node = node, @@ -52,7 +57,8 @@ pub fn init(node: *Node, frame: *Frame) !*ChildNodes { ._last_index = 0, ._last_node = null, ._last_length = null, - ._cached_version = frame.version, + ._cached_version = owning_frame.version, + ._owning_frame = owning_frame, }; return self; } @@ -61,8 +67,8 @@ pub fn deinit(self: *const ChildNodes, page: *Page) void { page.releaseArena(self._arena); } -pub fn length(self: *ChildNodes, frame: *Frame) !u32 { - if (self.versionCheck(frame)) { +pub fn length(self: *ChildNodes, _: *Frame) !u32 { + if (self.versionCheck()) { if (self._last_length) |cached_length| { return cached_length; } @@ -75,16 +81,16 @@ pub fn length(self: *ChildNodes, frame: *Frame) !u32 { return len; } -pub fn getAtIndex(self: *ChildNodes, index: usize, frame: *Frame) !?*Node { - _ = self.versionCheck(frame); +pub fn getAtIndex(self: *ChildNodes, index: usize, _: *Frame) !?*Node { + _ = self.versionCheck(); var current = self._last_index; var node: ?*std.DoublyLinkedList.Node = null; - if (index < current) { + if (index < current or self._last_node == null) { current = 0; node = self.first() orelse return null; } else { - node = self._last_node orelse self.first() orelse return null; + node = self._last_node; } defer self._last_index = current; @@ -116,8 +122,8 @@ pub fn entries(self: *ChildNodes, frame: *Frame) !*EntryIterator { return .init(.{ .list = self }, frame); } -fn versionCheck(self: *ChildNodes, frame: *Frame) bool { - const current = frame.version; +fn versionCheck(self: *ChildNodes) bool { + const current = self._owning_frame.version; if (current == self._cached_version) { return true; } diff --git a/src/browser/webapi/collections/HTMLAllCollection.zig b/src/browser/webapi/collections/HTMLAllCollection.zig index 835de18c..7e9758b7 100644 --- a/src/browser/webapi/collections/HTMLAllCollection.zig +++ b/src/browser/webapi/collections/HTMLAllCollection.zig @@ -32,19 +32,23 @@ _tw: TreeWalker.FullExcludeSelf, _last_index: usize, _last_length: ?u32, _cached_version: usize, +_owning_frame: *Frame, pub fn init(root: *Node, frame: *Frame) HTMLAllCollection { + const owning_frame = root.ownerFrame(frame); return .{ ._last_index = 0, ._last_length = null, ._tw = TreeWalker.FullExcludeSelf.init(root, .{}), - ._cached_version = frame.version, + ._cached_version = owning_frame.version, + ._owning_frame = owning_frame, }; } -fn versionCheck(self: *HTMLAllCollection, frame: *const Frame) bool { - if (self._cached_version != frame.version) { - self._cached_version = frame.version; +fn versionCheck(self: *HTMLAllCollection, _: *const Frame) bool { + const current = self._owning_frame.version; + if (self._cached_version != current) { + self._cached_version = current; self._last_index = 0; self._last_length = null; self._tw.reset(); diff --git a/src/browser/webapi/collections/node_live.zig b/src/browser/webapi/collections/node_live.zig index 60e2e40c..33962d8e 100644 --- a/src/browser/webapi/collections/node_live.zig +++ b/src/browser/webapi/collections/node_live.zig @@ -99,16 +99,19 @@ pub fn NodeLive(comptime mode: Mode) type { _last_index: usize, _last_length: ?u32, _cached_version: usize, + _owning_frame: *Frame, const Self = @This(); pub fn init(root: *Node, filter: Filter, frame: *Frame) Self { + const owning_frame = root.ownerFrame(frame); return .{ ._last_index = 0, ._last_length = null, ._filter = filter, ._tw = TW.init(root, .{}), - ._cached_version = frame.version, + ._cached_version = owning_frame.version, + ._owning_frame = owning_frame, }; } @@ -342,8 +345,8 @@ pub fn NodeLive(comptime mode: Mode) type { }; } - fn versionCheck(self: *Self, frame: *const Frame) bool { - const current = frame.version; + fn versionCheck(self: *Self, _: *const Frame) bool { + const current = self._owning_frame.version; if (current == self._cached_version) { return true; } diff --git a/src/browser/webapi/element/Attribute.zig b/src/browser/webapi/element/Attribute.zig index c0d4d053..24b6477c 100644 --- a/src/browser/webapi/element/Attribute.zig +++ b/src/browser/webapi/element/Attribute.zig @@ -229,7 +229,7 @@ pub const List = struct { }; try frame.addElementId(parent, element, entry._value.str()); } - frame.domChanged(); + element.asNode().bumpDomVersion(frame); frame.attributeChange(element, result.normalized, entry._value, old_value); return entry; } @@ -292,7 +292,7 @@ pub const List = struct { frame.removeElementId(element, entry._value.str()); } - frame.domChanged(); + element.asNode().bumpDomVersion(frame); frame.attributeRemove(element, result.normalized, old_value); _ = frame._attribute_lookup.remove(@intFromPtr(entry)); self._list.remove(&entry._node); diff --git a/src/browser/webapi/element/Html.zig b/src/browser/webapi/element/Html.zig index 3ed96f57..ffaba715 100644 --- a/src/browser/webapi/element/Html.zig +++ b/src/browser/webapi/element/Html.zig @@ -273,7 +273,7 @@ pub fn setInnerText(self: *HtmlElement, text: []const u8, frame: *Frame) !void { const parent = self.asElement().asNode(); // Remove all existing children - frame.domChanged(); + parent.bumpDomVersion(frame); var it = parent.childrenIterator(); while (it.next()) |child| { frame.removeNode(parent, child, .{ .will_be_reconnected = false }); diff --git a/src/browser/webapi/element/html/Option.zig b/src/browser/webapi/element/html/Option.zig index 9ca53f25..494573cf 100644 --- a/src/browser/webapi/element/html/Option.zig +++ b/src/browser/webapi/element/html/Option.zig @@ -80,7 +80,7 @@ pub fn setSelected(self: *Option, selected: bool, frame: *Frame) !void { // TODO: When setting selected=true, may need to unselect other options // in the parent