Merge pull request #1668 from lightpanda-io/selector_list_arena

Add RC support to NodeList

src/browser/tests/document/query_selector_all.html

@@ -442,3 +442,29 @@
 }
 </script>
 
+<script id=iterator_list_lifetime>
+  // This test is intended to ensure that a list remains alive as long as it
+  // must, i.e. as long as any iterator referencing the list is alive.
+  // This test depends on being able to force the v8 GC to cleanup, which
+  // we have no way of controlling. At worst, the test will pass without
+  // actually testing correct lifetime. But it was at least manually verified
+  // for me that this triggers plenty of GCs.
+  const expected = Array.from(document.querySelectorAll('*')).length;
+  {
+    let keys = [];
+
+    // Phase 1: Create many lists+iterators to fill up the arena pool
+    for (let i = 0; i < 1000; i++) {
+      let list = document.querySelectorAll('*');
+      keys.push(list.keys());
+
+      // Create an Event every iteration to compete for arenas
+      new Event('arena_compete');
+    }
+
+    for (let k of keys) {
+      const result = Array.from(k);
+      testing.expectEqual(expected, result.length);
+    }
+  }
+</script>

src/browser/webapi/collections/ChildNodes.zig

@@ -26,6 +26,7 @@ const GenericIterator = @import("iterator.zig").Entry;
 // No need to go through a TreeWalker or add any filtering.
 const ChildNodes = @This();
 
+_arena: std.mem.Allocator,
 _last_index: usize,
 _last_length: ?u32,
 _last_node: ?*std.DoublyLinkedList.Node,
@@ -37,13 +38,23 @@ pub const ValueIterator = GenericIterator(Iterator, "1");
 pub const EntryIterator = GenericIterator(Iterator, null);
 
 pub fn init(node: *Node, page: *Page) !*ChildNodes {
-    return page._factory.create(ChildNodes{
+    const arena = try page.getArena(.{ .debug = "ChildNodes" });
+    errdefer page.releaseArena(arena);
+
+    const self = try arena.create(ChildNodes);
+    self.* = .{
         ._node = node,
+        ._arena = arena,
         ._last_index = 0,
         ._last_node = null,
         ._last_length = null,
         ._cached_version = page.version,
-    });
+    };
+    return self;
+}
+
+pub fn deinit(self: *const ChildNodes, page: *Page) void {
+    page.releaseArena(self._arena);
 }
 
 pub fn length(self: *ChildNodes, page: *Page) !u32 {
@@ -115,7 +126,7 @@ fn versionCheck(self: *ChildNodes, page: *Page) bool {
 
 const NodeList = @import("NodeList.zig");
 pub fn runtimeGenericWrap(self: *ChildNodes, page: *Page) !*NodeList {
-    return page._factory.create(NodeList{ .data = .{ .child_nodes = self } });
+    return page._factory.create(NodeList{ ._data = .{ .child_nodes = self } });
 }
 
 const Iterator = struct {

src/browser/webapi/collections/HTMLFormControlsCollection.zig

@@ -85,7 +85,7 @@ pub fn namedItem(self: *HTMLFormControlsCollection, name: []const u8, page: *Pag
                     ._name = try page.dupeString(name),
                 });
 
-                radio_node_list._proto = try page._factory.create(NodeList{ .data = .{ .radio_node_list = radio_node_list } });
+                radio_node_list._proto = try page._factory.create(NodeList{ ._data = .{ .radio_node_list = radio_node_list } });
 
                 return .{ .radio_node_list = radio_node_list };
             }

src/browser/webapi/collections/NodeList.zig

@@ -28,24 +28,36 @@ const RadioNodeList = @import("RadioNodeList.zig");
 const SelectorList = @import("../selector/List.zig");
 const NodeLive = @import("node_live.zig").NodeLive;
 
-const Mode = enum {
-    child_nodes,
-    selector_list,
-    radio_node_list,
-    name,
-};
-
 const NodeList = @This();
 
-data: union(Mode) {
+_data: union(enum) {
     child_nodes: *ChildNodes,
     selector_list: *SelectorList,
     radio_node_list: *RadioNodeList,
     name: NodeLive(.name),
 },
+_rc: usize = 0,
+
+pub fn deinit(self: *NodeList, _: bool, page: *Page) void {
+    const rc = self._rc;
+    if (rc > 1) {
+        self._rc = rc - 1;
+        return;
+    }
+
+    switch (self._data) {
+        .selector_list => |list| list.deinit(page),
+        .child_nodes => |cn| cn.deinit(page),
+        else => {},
+    }
+}
+
+pub fn acquireRef(self: *NodeList) void {
+    self._rc += 1;
+}
 
 pub fn length(self: *NodeList, page: *Page) !u32 {
-    return switch (self.data) {
+    return switch (self._data) {
         .child_nodes => |impl| impl.length(page),
         .selector_list => |impl| @intCast(impl.getLength()),
         .radio_node_list => |impl| impl.getLength(),
@@ -58,7 +70,7 @@ pub fn indexedGet(self: *NodeList, index: usize, page: *Page) !*Node {
 }
 
 pub fn getAtIndex(self: *NodeList, index: usize, page: *Page) !?*Node {
-    return switch (self.data) {
+    return switch (self._data) {
         .child_nodes => |impl| impl.getAtIndex(index, page),
         .selector_list => |impl| impl.getAtIndex(index),
         .radio_node_list => |impl| impl.getAtIndex(index, page),
@@ -106,6 +118,14 @@ const Iterator = struct {
 
     const Entry = struct { u32, *Node };
 
+    pub fn deinit(self: *Iterator, shutdown: bool, page: *Page) void {
+        self.list.deinit(shutdown, page);
+    }
+
+    pub fn acquireRef(self: *Iterator) void {
+        self.list.acquireRef();
+    }
+
     pub fn next(self: *Iterator, page: *Page) !?Entry {
         const index = self.index;
         const node = try self.list.getAtIndex(index, page) orelse return null;
@@ -122,6 +142,8 @@ pub const JsApi = struct {
         pub const prototype_chain = bridge.prototypeChain();
         pub var class_id: bridge.ClassId = undefined;
         pub const enumerable = false;
+        pub const weak = true;
+        pub const finalizer = bridge.finalizer(NodeList.deinit);
     };
 
     pub const length = bridge.accessor(NodeList.length, null, .{});

src/browser/webapi/collections/iterator.zig

@@ -21,8 +21,7 @@ const js = @import("../../js/js.zig");
 const Page = @import("../../Page.zig");
 
 pub fn Entry(comptime Inner: type, comptime field: ?[]const u8) type {
-    const InnerStruct = Inner;
-    const R = reflect(InnerStruct, field);
+    const R = reflect(Inner, field);
 
     return struct {
         inner: Inner,
@@ -40,6 +39,18 @@ pub fn Entry(comptime Inner: type, comptime field: ?[]const u8) type {
             return page._factory.create(Self{ .inner = inner });
         }
 
+        pub fn deinit(self: *Self, shutdown: bool, page: *Page) void {
+            if (@hasDecl(Inner, "deinit")) {
+                self.inner.deinit(shutdown, page);
+            }
+        }
+
+        pub fn acquireRef(self: *Self) void {
+            if (@hasDecl(Inner, "acquireRef")) {
+                self.inner.acquireRef();
+            }
+        }
+
         pub fn next(self: *Self, page: *Page) if (R.has_error_return) anyerror!Result else Result {
             const entry = (if (comptime R.has_error_return) try self.inner.next(page) else self.inner.next(page)) orelse {
                 return .{ .done = true, .value = null };
@@ -61,6 +72,8 @@ pub fn Entry(comptime Inner: type, comptime field: ?[]const u8) type {
             pub const Meta = struct {
                 pub const prototype_chain = bridge.prototypeChain();
                 pub var class_id: bridge.ClassId = undefined;
+                pub const weak = true;
+                pub const finalizer = bridge.finalizer(Self.deinit);
             };
 
             pub const next = bridge.function(Self.next, .{ .null_as_undefined = true });

src/browser/webapi/collections/node_live.zig

@@ -349,7 +349,7 @@ pub fn NodeLive(comptime mode: Mode) type {
 
         pub fn runtimeGenericWrap(self: Self, page: *Page) !if (mode == .name) *NodeList else *HTMLCollection {
             const collection = switch (mode) {
-                .name => return page._factory.create(NodeList{ .data = .{ .name = self } }),
+                .name => return page._factory.create(NodeList{ ._data = .{ .name = self } }),
                 .tag => HTMLCollection{ ._data = .{ .tag = self } },
                 .tag_name => HTMLCollection{ ._data = .{ .tag_name = self } },
                 .tag_name_ns => HTMLCollection{ ._data = .{ .tag_name_ns = self } },

src/browser/webapi/selector/List.zig

@@ -40,6 +40,10 @@ pub const EntryIterator = GenericIterator(Iterator, null);
 pub const KeyIterator = GenericIterator(Iterator, "0");
 pub const ValueIterator = GenericIterator(Iterator, "1");
 
+pub fn deinit(self: *const List, page: *Page) void {
+    page.releaseArena(self._arena);
+}
+
 pub fn collect(
     allocator: std.mem.Allocator,
     root: *Node,
@@ -207,8 +211,12 @@ pub fn getAtIndex(self: *const List, index: usize) !?*Node {
 }
 
 const NodeList = @import("../collections/NodeList.zig");
-pub fn runtimeGenericWrap(self: *List, page: *Page) !*NodeList {
-    return page._factory.create(NodeList{ .data = .{ .selector_list = self } });
+pub fn runtimeGenericWrap(self: *List, _: *const Page) !*NodeList {
+    const nl = try self._arena.create(NodeList);
+    nl.* = .{
+        ._data = .{ .selector_list = self },
+    };
+    return nl;
 }
 
 const IdAnchor = struct {

src/browser/webapi/selector/Selector.zig

@@ -62,7 +62,9 @@ pub fn querySelectorAll(root: *Node, input: []const u8, page: *Page) !*List {
         return error.SyntaxError;
     }
 
-    const arena = page.arena;
+    const arena = try page.getArena(.{ .debug = "querySelectorAll" });
+    errdefer page.releaseArena(arena);
+
     var nodes: std.AutoArrayHashMapUnmanaged(*Node, void) = .empty;
 
     const selectors = try Parser.parseList(arena, input, page);
@@ -70,10 +72,12 @@ pub fn querySelectorAll(root: *Node, input: []const u8, page: *Page) !*List {
         try List.collect(arena, root, selector, &nodes, page);
     }
 
-    return page._factory.create(List{
+    const list = try arena.create(List);
+    list.* = .{
         ._arena = arena,
         ._nodes = nodes.keys(),
-    });
+    };
+    return list;
 }
 
 pub fn matches(el: *Node.Element, input: []const u8, page: *Page) !bool {

src/cdp/Node.zig

@@ -400,20 +400,32 @@ test "cdp Node: search list" {
         defer page._session.removePage();
         var doc = page.window._document;
 
-        const s1 = try search_list.create((try doc.querySelectorAll(.wrap("a"), page))._nodes);
-        try testing.expectEqual("1", s1.name);
-        try testing.expectEqualSlices(u32, &.{ 1, 2 }, s1.node_ids);
+        {
+            const l1 = try doc.querySelectorAll(.wrap("a"), page);
+            defer l1.deinit(page);
+            const s1 = try search_list.create(l1._nodes);
+            try testing.expectEqual("1", s1.name);
+            try testing.expectEqualSlices(u32, &.{ 1, 2 }, s1.node_ids);
+        }
 
         try testing.expectEqual(2, registry.lookup_by_id.count());
         try testing.expectEqual(2, registry.lookup_by_node.count());
 
-        const s2 = try search_list.create((try doc.querySelectorAll(.wrap("#a1"), page))._nodes);
-        try testing.expectEqual("2", s2.name);
-        try testing.expectEqualSlices(u32, &.{1}, s2.node_ids);
+        {
+            const l2 = try doc.querySelectorAll(.wrap("#a1"), page);
+            defer l2.deinit(page);
+            const s2 = try search_list.create(l2._nodes);
+            try testing.expectEqual("2", s2.name);
+            try testing.expectEqualSlices(u32, &.{1}, s2.node_ids);
+        }
 
-        const s3 = try search_list.create((try doc.querySelectorAll(.wrap("#a2"), page))._nodes);
-        try testing.expectEqual("3", s3.name);
-        try testing.expectEqualSlices(u32, &.{2}, s3.node_ids);
+        {
+            const l3 = try doc.querySelectorAll(.wrap("#a2"), page);
+            defer l3.deinit(page);
+            const s3 = try search_list.create(l3._nodes);
+            try testing.expectEqual("3", s3.name);
+            try testing.expectEqualSlices(u32, &.{2}, s3.node_ids);
+        }
 
         try testing.expectEqual(2, registry.lookup_by_id.count());
         try testing.expectEqual(2, registry.lookup_by_node.count());

src/cdp/domains/dom.zig

@@ -98,6 +98,8 @@ fn performSearch(cmd: anytype) !void {
     const bc = cmd.browser_context orelse return error.BrowserContextNotLoaded;
     const page = bc.session.currentPage() orelse return error.PageNotLoaded;
     const list = try Selector.querySelectorAll(page.window._document.asNode(), params.query, page);
+    defer list.deinit(page);
+
     const search = try bc.node_search_list.create(list._nodes);
 
     // dispatch setChildNodesEvents to inform the client of the subpart of node
@@ -247,6 +249,8 @@ fn querySelectorAll(cmd: anytype) !void {
     };
 
     const selected_nodes = try Selector.querySelectorAll(node.dom, params.selector, page);
+    defer selected_nodes.deinit(page);
+
     const nodes = selected_nodes._nodes;
 
     const node_ids = try cmd.arena.alloc(Node.Id, nodes.len);