From 9c7ecf221efeb2268112f31b7d826663a5ec7285 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Thu, 5 Mar 2026 14:56:07 +0800 Subject: [PATCH 01/10] Improve frame sub-navigation This makes frame sub-navigation "work" for all page navigations (click, form submit, location.top...) as well as setting the iframe.src. Fixes at least 2 WPT crashes. BUT, the implementation still isn't 100% correct, with two known issues: 1. Navigation currently happens in the context where it's called, not the context of the frame. So if Page1 accesses Frame1 and causes it to navigate, e.g. f1.contentDocument.querySelector('#link').click(), it's Page1 that will be navigated, since the JS is being executed in the Page1 context. This should be relatively easy to fix. 2. There are particularly complicated cases in WPT where a frame is navigated inside of its own load, creating an endless loop. There's some partial support for this as-is, but it doesn't work correctly and it currently is defensive and likely will not continue to navigate. This is particularly true when sub-navigation is done to about:blank within the frame's on load event. (Which is probably not a real concern, but an issue for some WPT tests) Although it shares a lot with the original navigation code, there are many more edge cases here, possibly due to being developed along side WPT tests. The source of most of the complexity is the synchronous handling of "about:blank" in page.navigate, which can result in a scheduled navigation synchronously causing more scheduled navigation. (Specifically because `self.documentIsComplete();` is called from page.navigate in that case). It might be worth seeing if something can be done about that, to simplify this new code (removing the double queue, removing the flag, simplifying pre-existing schedule checks ,...) --- src/browser/Page.zig | 144 ++++++++++-------- src/browser/Session.zig | 141 +++++++++++++---- src/browser/js/Function.zig | 4 +- src/browser/js/Local.zig | 2 +- src/browser/tests/frames/frames.html | 41 ++++- .../tests/frames/support/after_link.html | 2 + .../tests/frames/support/with_link.html | 2 + src/browser/webapi/Node.zig | 3 + 8 files changed, 242 insertions(+), 97 deletions(-) create mode 100644 src/browser/tests/frames/support/after_link.html create mode 100644 src/browser/tests/frames/support/with_link.html diff --git a/src/browser/Page.zig b/src/browser/Page.zig index aa40ab0b..a39d406b 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -323,9 +323,9 @@ pub fn init(self: *Page, frame_id: u32, session: *Session, parent: ?*Page) !void } } -pub fn deinit(self: *Page) void { +pub fn deinit(self: *Page, abort_http: bool) void { for (self.frames.items) |frame| { - frame.deinit(); + frame.deinit(abort_http); } if (comptime IS_DEBUG) { @@ -346,10 +346,16 @@ pub fn deinit(self: *Page) void { session.browser.env.destroyContext(self.js); self._script_manager.shutdown = true; + if (self.parent == null) { - // only the root frame needs to abort this. It's more efficient this way session.browser.http_client.abort(); + } else if (abort_http) { + // a small optimization, it's faster to abort _everything_ on the root + // page, so we prefer that. But if it's just the frame that's going + // away (a frame navigation) then we'll abort the frame-related requests + session.browser.http_client.abortFrame(self._frame_id); } + self._script_manager.deinit(); if (comptime IS_DEBUG) { @@ -459,6 +465,7 @@ pub fn navigate(self: *Page, request_url: [:0]const u8, opts: NavigateOpts) !voi // if the url is about:blank, we load an empty HTML document in the // page and dispatch the events. if (std.mem.eql(u8, "about:blank", request_url)) { + self.url = "about:blank"; // Assume we parsed the document. // It's important to force a reset during the following navigation. self._parse_state = .complete; @@ -572,31 +579,49 @@ pub fn scheduleNavigation(self: *Page, request_url: []const u8, opts: NavigateOp } fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []const u8, opts: NavigateOpts, priority: NavigationPriority) !void { - const resolved_url = try URL.resolve( - arena, - self.base(), - request_url, - .{ .always_dupe = true, .encode = true }, - ); + const resolved_url = blk: { + if (std.mem.eql(u8, request_url, "about:blank")) { + break :blk "about:blank"; // navigate will handle this special case + } + break :blk try URL.resolve( + arena, + self.base(), + request_url, + .{ .always_dupe = true, .encode = true }, + ); + }; const session = self._session; if (!opts.force and URL.eqlDocument(self.url, resolved_url)) { - self.arena_pool.release(arena); - self.url = try self.arena.dupeZ(u8, resolved_url); self.window._location = try Location.init(self.url, self); self.document._location = self.window._location; - return session.navigation.updateEntries(self.url, opts.kind, self, true); + if (self.parent == null) { + try session.navigation.updateEntries(self.url, opts.kind, self, true); + } + // doin't defer this, the caller, the caller is responsible for freeing + // it on error + self.arena_pool.release(arena); + return; } log.info(.browser, "schedule navigation", .{ .url = resolved_url, .reason = opts.reason, - .target = resolved_url, .type = self._type, }); - session.browser.http_client.abort(); + // This is a micro-optimization. Terminate any inflight request as early + // as we can. This will be more propery shutdown when we process the + // scheduled navigation. + if (self.parent == null) { + session.browser.http_client.abort(); + } else { + // This doesn't terminate any inflight requests for nested frames, but + // again, this is just an optimization. We'll correctly shut down all + // nested inflight requests when we process the navigation. + session.browser.http_client.abortFrame(self._frame_id); + } const qn = try arena.create(QueuedNavigation); qn.* = .{ @@ -604,12 +629,11 @@ fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []con .arena = arena, .url = resolved_url, .priority = priority, + .iframe = self.iframe, }; - if (self._queued_navigation) |existing| { - self.arena_pool.release(existing.arena); - } self._queued_navigation = qn; + return session.scheduleNavigation(qn); } // A script can have multiple competing navigation events, say it starts off @@ -623,6 +647,12 @@ fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []con // 3 - anchor clicks // Within, each category, it's last-one-wins. fn canScheduleNavigation(self: *Page, priority: NavigationPriority) bool { + if (self.parent) |parent| { + if (parent.isGoingAway()) { + return false; + } + } + const existing = self._queued_navigation orelse return true; if (existing.priority == priority) { @@ -631,7 +661,8 @@ fn canScheduleNavigation(self: *Page, priority: NavigationPriority) bool { } return switch (existing.priority) { - .anchor => true, // everything is higher priority than an anchor + .iframe => true, // everything is higher priority than iframe.src = "x" + .anchor => priority != .iframe, // an anchor is only higher priority than an iframe .form => false, // nothing is higher priority than a form .script => priority == .form, // a form is higher priority than a script }; @@ -758,6 +789,8 @@ fn _documentIsComplete(self: *Page) !void { } fn notifyParentLoadComplete(self: *Page) void { + const parent = self.parent orelse return; + if (self._parent_notified == true) { if (comptime IS_DEBUG) { std.debug.assert(false); @@ -767,9 +800,7 @@ fn notifyParentLoadComplete(self: *Page) void { } self._parent_notified = true; - if (self.parent) |p| { - p.iframeCompletedLoading(self.iframe.?); - } + parent.iframeCompletedLoading(self.iframe.?); } fn pageHeaderDoneCallback(transfer: *Http.Transfer) !bool { @@ -949,7 +980,11 @@ fn pageErrorCallback(ctx: *anyopaque, err: anyerror) void { } pub fn isGoingAway(self: *const Page) bool { - return self._queued_navigation != null; + if (self._queued_navigation != null) { + return true; + } + const parent = self.parent orelse return false; + return parent.isGoingAway(); } pub fn scriptAddedCallback(self: *Page, comptime from_parser: bool, script: *Element.Html.Script) !void { @@ -982,30 +1017,39 @@ pub fn iframeAddedCallback(self: *Page, iframe: *Element.Html.IFrame) !void { return; } + if (iframe._content_window) |cw| { + // This frame is being re-navigated. We need to do this through a + // scheduleNavigation phase. We can't navigate immediately here, for + // the same reason that a "root" page can't immediately navigate: + // we could be in the middle of a JS callback or something else that + // doesn't exit the page to just suddenly go away. + return cw._page.scheduleNavigation(src, .{ + .reason = .script, + .kind = .{ .push = null }, + }, .iframe); + } + iframe._executed = true; const session = self._session; - // A frame can be re-navigated by setting the src. - const existing_window = iframe._content_window; - const page_frame = try self.arena.create(Page); - const frame_id = blk: { - if (existing_window) |w| { - const existing_frame_id = w._page._frame_id; - session.browser.http_client.abortFrame(existing_frame_id); - break :blk existing_frame_id; - } - break :blk session.nextFrameId(); - }; + const frame_id = session.nextFrameId(); try Page.init(page_frame, frame_id, session, self); - errdefer page_frame.deinit(); + errdefer page_frame.deinit(true); self._pending_loads += 1; page_frame.iframe = iframe; iframe._content_window = page_frame.window; errdefer iframe._content_window = null; + // on first load, dispatch frame_created evnet + self._session.notification.dispatch(.page_frame_created, &.{ + .frame_id = frame_id, + .parent_id = self._frame_id, + .timestamp = timestamp(.monotonic), + }); + const url = blk: { if (std.mem.eql(u8, src, "about:blank")) { break :blk "about:blank"; // navigate will handle this special case @@ -1018,42 +1062,14 @@ pub fn iframeAddedCallback(self: *Page, iframe: *Element.Html.IFrame) !void { ); }; - if (existing_window == null) { - // on first load, dispatch frame_created evnet - self._session.notification.dispatch(.page_frame_created, &.{ - .frame_id = frame_id, - .parent_id = self._frame_id, - .timestamp = timestamp(.monotonic), - }); - } - page_frame.navigate(url, .{ .reason = .initialFrameNavigation }) catch |err| { log.warn(.page, "iframe navigate failure", .{ .url = url, .err = err }); self._pending_loads -= 1; iframe._content_window = null; - page_frame.deinit(); + page_frame.deinit(true); return error.IFrameLoadError; }; - if (existing_window) |w| { - const existing_page = w._page; - if (existing_page._parent_notified == false) { - self._pending_loads -= 1; - } - - for (self.frames.items, 0..) |p, i| { - if (p == existing_page) { - self.frames.items[i] = page_frame; - break; - } - } else { - lp.assert(false, "Existing frame not found", .{ .len = self.frames.items.len }); - } - - existing_page.deinit(); - return; - } - // window[N] is based on document order. For now we'll just append the frame // at the end of our list and set frames_sorted == false. window.getFrame // will check this flag to decide if it needs to sort the frames or not. @@ -3030,6 +3046,7 @@ const NavigationPriority = enum { form, script, anchor, + iframe, }; pub const QueuedNavigation = struct { @@ -3037,6 +3054,7 @@ pub const QueuedNavigation = struct { url: [:0]const u8, opts: NavigateOpts, priority: NavigationPriority, + iframe: ?*Element.Html.IFrame, }; pub fn triggerMouseClick(self: *Page, x: f64, y: f64) !void { diff --git a/src/browser/Session.zig b/src/browser/Session.zig index 0cf17d70..e54b7666 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -30,6 +30,7 @@ const History = @import("webapi/History.zig"); const Page = @import("Page.zig"); const Browser = @import("Browser.zig"); const Notification = @import("../Notification.zig"); +const QueuedNavigation = Page.QueuedNavigation; const Allocator = std.mem.Allocator; const IS_DEBUG = builtin.mode == .Debug; @@ -43,6 +44,14 @@ const Session = @This(); browser: *Browser, notification: *Notification, +queued_navigation: std.ArrayList(*QueuedNavigation), +// It's possible (but unlikely) that a queued navigation happens when we're +// processessing queued navigations (thank you WPT). This causes a lot of issues +// including possibly invalidating `queued_navigation` and endless loops. +// We use a double queue to avoid this. +processing_queued_navigation: bool, +queued_queued_navigation: std.ArrayList(*QueuedNavigation), + // Used to create our Inspector and in the BrowserContext. arena: Allocator, @@ -70,6 +79,9 @@ pub fn init(self: *Session, browser: *Browser, notification: *Notification) !voi .navigation = .{ ._proto = undefined }, .storage_shed = .{}, .browser = browser, + .queued_navigation = .{}, + .queued_queued_navigation = .{}, + .processing_queued_navigation = false, .notification = notification, .cookie_jar = storage.Cookie.Jar.init(allocator), }; @@ -79,9 +91,9 @@ pub fn deinit(self: *Session) void { if (self.page != null) { self.removePage(); } - const browser = self.browser; - self.cookie_jar.deinit(); + + const browser = self.browser; self.storage_shed.deinit(browser.app.allocator); browser.arena_pool.release(self.arena); } @@ -113,7 +125,7 @@ pub fn removePage(self: *Session) void { self.notification.dispatch(.page_remove, .{}); lp.assert(self.page != null, "Session.removePage - page is null", .{}); - self.page.?.deinit(); + self.page.?.deinit(false); self.page = null; self.navigation.onRemovePage(); @@ -133,7 +145,7 @@ pub fn replacePage(self: *Session) !*Page { var current = self.page.?; const frame_id = current._frame_id; const parent = current.parent; - current.deinit(); + current.deinit(false); self.browser.env.memoryPressureNotification(.moderate); @@ -174,10 +186,11 @@ pub fn wait(self: *Session, wait_ms: u32) WaitResult { switch (wait_result) { .done => { - if (page._queued_navigation == null) { + if (self.queued_navigation.items.len == 0) { return .done; } - page = self.processScheduledNavigation(page) catch return .done; + self.processQueuedNavigation() catch return .done; + page = &self.page.?; // might have changed }, else => |result| return result, } @@ -229,7 +242,7 @@ fn _wait(self: *Session, page: *Page, wait_ms: u32) !WaitResult { } }, .html, .complete => { - if (page._queued_navigation != null) { + if (self.queued_navigation.items.len != 0) { return .done; } @@ -339,42 +352,112 @@ fn _wait(self: *Session, page: *Page, wait_ms: u32) !WaitResult { } } -fn processScheduledNavigation(self: *Session, current_page: *Page) !*Page { +pub fn scheduleNavigation(self: *Session, qn: *QueuedNavigation) !void { + const iframe = qn.iframe; + const list = if (self.processing_queued_navigation) &self.queued_queued_navigation else &self.queued_navigation; + for (list.items, 0..) |existing, i| { + if (existing.iframe == iframe) { + self.browser.arena_pool.release(existing.arena); + list.items[i] = qn; + return; + } + } else { + return list.append(self.arena, qn); + } +} + +fn processQueuedNavigation(self: *Session) !void { + const navigations = &self.queued_navigation; + defer { + navigations.clearRetainingCapacity(); + const copy = navigations.*; + self.queued_navigation = self.queued_queued_navigation; + self.queued_queued_navigation = copy; + } + + if (self.page.?._queued_navigation != null) { + // This is both an optimization and a simplification of sorts. If the + // root page is navigating, then we don't need to process any other + // navigation. Also, the navigation for the root page and for a frame + // is different enough that have two distinct code blocks is, imo, + // better. Yes, there will be duplication. + return self.processRootQueuedNavigation(); + } + self.processing_queued_navigation = true; + defer self.processing_queued_navigation = false; + const browser = self.browser; + for (navigations.items) |qn| { + const iframe = qn.iframe.?; + const current_page = iframe._content_window.?._page; // Get the CURRENT page from iframe + lp.assert(current_page.parent != null, "root queued navigation", .{}); + current_page._queued_navigation = null; + defer browser.arena_pool.release(qn.arena); + + const parent = current_page.parent.?; + errdefer iframe._content_window = null; + + if (current_page._parent_notified) { + // we already notified the parent that we had loaded + parent._pending_loads += 1; + } + + const frame_id = current_page._frame_id; + defer current_page.deinit(true); + + const new_page = try parent.arena.create(Page); + try Page.init(new_page, frame_id, self, parent); + errdefer new_page.deinit(true); + + new_page.iframe = iframe; + iframe._content_window = new_page.window; + + new_page.navigate(qn.url, qn.opts) catch |err| { + log.err(.browser, "queued frame navigation error", .{ .err = err }); + return err; + }; + + for (parent.frames.items, 0..) |p, i| { + // Page.frames may or may not be sorted (depending on the + // Page.frames_sorted flag). Putting this new page at the same + // position as the one it's replacing is the simplest, safest and + // probably most efficient option. + if (p == current_page) { + parent.frames.items[i] = new_page; + break; + } + } else { + lp.assert(false, "Existing frame not found", .{ .len = parent.frames.items.len }); + } + } +} + +fn processRootQueuedNavigation(self: *Session) !void { + const current_page = &self.page.?; + const frame_id = current_page._frame_id; + + // create a copy before the page is cleared const qn = current_page._queued_navigation.?; - // take ownership of the page's queued navigation current_page._queued_navigation = null; - defer browser.arena_pool.release(qn.arena); - - const frame_id, const parent = blk: { - const page = &self.page.?; - const frame_id = page._frame_id; - const parent = page.parent; - - browser.http_client.abort(); - self.removePage(); - - break :blk .{ frame_id, parent }; - }; + defer self.browser.arena_pool.release(qn.arena); + self.removePage(); self.page = @as(Page, undefined); - const page = &self.page.?; - try Page.init(page, frame_id, self, parent); + const new_page = &self.page.?; + try Page.init(new_page, frame_id, self, null); // Creates a new NavigationEventTarget for this page. - try self.navigation.onNewPage(page); + try self.navigation.onNewPage(new_page); // start JS env // Inform CDP the main page has been created such that additional context for other Worlds can be created as well - self.notification.dispatch(.page_created, page); + self.notification.dispatch(.page_created, new_page); - page.navigate(qn.url, qn.opts) catch |err| { - log.err(.browser, "queued navigation error", .{ .err = err, .url = qn.url }); + new_page.navigate(qn.url, qn.opts) catch |err| { + log.err(.browser, "queued navigation error", .{ .err = err }); return err; }; - - return page; } pub fn nextFrameId(self: *Session) u32 { diff --git a/src/browser/js/Function.zig b/src/browser/js/Function.zig index e9bccb61..01243d35 100644 --- a/src/browser/js/Function.zig +++ b/src/browser/js/Function.zig @@ -160,8 +160,8 @@ fn _tryCallWithThis(self: *const Function, comptime T: type, this: anytype, args try_catch.rethrow(); return error.TryCatchRethrow; } - caught.* = try_catch.caughtOrError(local.call_arena, error.JSExecCallback); - return error.JSExecCallback; + caught.* = try_catch.caughtOrError(local.call_arena, error.JsException); + return error.JsException; }; if (@typeInfo(T) == .void) { diff --git a/src/browser/js/Local.zig b/src/browser/js/Local.zig index 04a65bc7..6a68b332 100644 --- a/src/browser/js/Local.zig +++ b/src/browser/js/Local.zig @@ -137,7 +137,7 @@ pub fn compileAndRun(self: *const Local, src: []const u8, name: ?[]const u8) !js ) orelse return error.CompilationError; // Run the script - const result = v8.v8__Script__Run(v8_script, self.handle) orelse return error.ExecutionError; + const result = v8.v8__Script__Run(v8_script, self.handle) orelse return error.JsException; return .{ .local = self, .handle = result }; } diff --git a/src/browser/tests/frames/frames.html b/src/browser/tests/frames/frames.html index f1519323..31e53be8 100644 --- a/src/browser/tests/frames/frames.html +++ b/src/browser/tests/frames/frames.html @@ -62,6 +62,7 @@ { let f3_load_event = false; let f3 = document.createElement('iframe'); + f3.id = 'f3'; f3.addEventListener('load', () => { f3_load_event = true; }); @@ -75,9 +76,10 @@ } - + + + + + diff --git a/src/browser/tests/frames/support/after_link.html b/src/browser/tests/frames/support/after_link.html new file mode 100644 index 00000000..0cd5fc99 --- /dev/null +++ b/src/browser/tests/frames/support/after_link.html @@ -0,0 +1,2 @@ + +It was clicked! diff --git a/src/browser/tests/frames/support/with_link.html b/src/browser/tests/frames/support/with_link.html new file mode 100644 index 00000000..2fd9ca77 --- /dev/null +++ b/src/browser/tests/frames/support/with_link.html @@ -0,0 +1,2 @@ + +a link diff --git a/src/browser/webapi/Node.zig b/src/browser/webapi/Node.zig index 4b351b6f..632d10f8 100644 --- a/src/browser/webapi/Node.zig +++ b/src/browser/webapi/Node.zig @@ -724,6 +724,9 @@ const CloneError = error{ TooManyContexts, LinkLoadError, StyleLoadError, + TypeError, + CompilationError, + JsException, }; pub fn cloneNode(self: *Node, deep_: ?bool, page: *Page) CloneError!*Node { const deep = deep_ orelse false; From 768c3a533b67504db70c328c6f3873ec60705c5a Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Thu, 5 Mar 2026 17:06:23 +0800 Subject: [PATCH 02/10] Simplify navigation logic. Must of the complexity in the previous commit had to do with the fact that about:blank is processed synchronously, meaning that we could process a scheduled navigation -> page.navigate -> scheduled navigation: ``` let iframe = document.createElement('iframe'); iframe.addEventListner('load', () => { iframe.src = "about:blank"; }); ``` This is an infinite loop which is going to be a problem no mater what, but there are different degrees of problems this can cause, e.g. looping forever vs use- after-free or other undefined behavior. The new approach does 2 passes through scheduled navigations, first processing "asynchronous" navigation (anything not "about:blank"), then processing synchronous navigation ("about:blank"). The main advantage is that if the synchronous navigation causes more synchronous navigation, it won't be processed until the next tick. PLUS, we can detect about:blank that loads about:blank and stop it (which might not be to spec, but seems right to do nonetheless). This 2-pass approach removes the need for a couple of checks and makes everything else simpler. --- src/browser/Page.zig | 13 ++-- src/browser/Session.zig | 160 ++++++++++++++++++++++++---------------- 2 files changed, 103 insertions(+), 70 deletions(-) diff --git a/src/browser/Page.zig b/src/browser/Page.zig index a39d406b..a8d063b4 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -579,16 +579,17 @@ pub fn scheduleNavigation(self: *Page, request_url: []const u8, opts: NavigateOp } fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []const u8, opts: NavigateOpts, priority: NavigationPriority) !void { - const resolved_url = blk: { + const resolved_url, const is_about_blank = blk: { if (std.mem.eql(u8, request_url, "about:blank")) { - break :blk "about:blank"; // navigate will handle this special case + break :blk .{ "about:blank", true }; // navigate will handle this special case } - break :blk try URL.resolve( + const u = try URL.resolve( arena, self.base(), request_url, .{ .always_dupe = true, .encode = true }, ); + break :blk .{ u, false }; }; const session = self._session; @@ -629,11 +630,11 @@ fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []con .arena = arena, .url = resolved_url, .priority = priority, - .iframe = self.iframe, + .is_about_blank = is_about_blank, }; self._queued_navigation = qn; - return session.scheduleNavigation(qn); + return session.scheduleNavigation(self); } // A script can have multiple competing navigation events, say it starts off @@ -3054,7 +3055,7 @@ pub const QueuedNavigation = struct { url: [:0]const u8, opts: NavigateOpts, priority: NavigationPriority, - iframe: ?*Element.Html.IFrame, + is_about_blank: bool, }; pub fn triggerMouseClick(self: *Page, x: f64, y: f64) !void { diff --git a/src/browser/Session.zig b/src/browser/Session.zig index e54b7666..61671e3f 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -44,13 +44,11 @@ const Session = @This(); browser: *Browser, notification: *Notification, -queued_navigation: std.ArrayList(*QueuedNavigation), -// It's possible (but unlikely) that a queued navigation happens when we're -// processessing queued navigations (thank you WPT). This causes a lot of issues -// including possibly invalidating `queued_navigation` and endless loops. -// We use a double queue to avoid this. -processing_queued_navigation: bool, -queued_queued_navigation: std.ArrayList(*QueuedNavigation), +queued_navigation: std.ArrayList(*Page), +// Temporary buffer for about:blank navigations during processing. +// We process async navigations first (safe from re-entrance), then sync +// about:blank navigations (which may add to queued_navigation). +queued_queued_navigation: std.ArrayList(*Page), // Used to create our Inspector and in the BrowserContext. arena: Allocator, @@ -81,7 +79,6 @@ pub fn init(self: *Session, browser: *Browser, notification: *Notification) !voi .browser = browser, .queued_navigation = .{}, .queued_queued_navigation = .{}, - .processing_queued_navigation = false, .notification = notification, .cookie_jar = storage.Cookie.Jar.init(allocator), }; @@ -352,28 +349,22 @@ fn _wait(self: *Session, page: *Page, wait_ms: u32) !WaitResult { } } -pub fn scheduleNavigation(self: *Session, qn: *QueuedNavigation) !void { - const iframe = qn.iframe; - const list = if (self.processing_queued_navigation) &self.queued_queued_navigation else &self.queued_navigation; - for (list.items, 0..) |existing, i| { - if (existing.iframe == iframe) { - self.browser.arena_pool.release(existing.arena); - list.items[i] = qn; +pub fn scheduleNavigation(self: *Session, page: *Page) !void { + const list = &self.queued_navigation; + + // Check if page is already queued + for (list.items) |existing| { + if (existing == page) { + // Already queued return; } - } else { - return list.append(self.arena, qn); } + + return list.append(self.arena, page); } fn processQueuedNavigation(self: *Session) !void { const navigations = &self.queued_navigation; - defer { - navigations.clearRetainingCapacity(); - const copy = navigations.*; - self.queued_navigation = self.queued_queued_navigation; - self.queued_queued_navigation = copy; - } if (self.page.?._queued_navigation != null) { // This is both an optimization and a simplification of sorts. If the @@ -381,55 +372,96 @@ fn processQueuedNavigation(self: *Session) !void { // navigation. Also, the navigation for the root page and for a frame // is different enough that have two distinct code blocks is, imo, // better. Yes, there will be duplication. + navigations.clearRetainingCapacity(); return self.processRootQueuedNavigation(); } - self.processing_queued_navigation = true; - defer self.processing_queued_navigation = false; + + const about_blank_queue = &self.queued_queued_navigation; + defer about_blank_queue.clearRetainingCapacity(); + + // First pass: process async navigations (non-about:blank) + // These cannot cause re-entrant navigation scheduling + for (navigations.items) |page| { + const qn = page._queued_navigation.?; + + if (qn.is_about_blank) { + // Defer about:blank to second pass + try about_blank_queue.append(self.arena, page); + continue; + } + + try self.processFrameNavigation(page, qn); + } + + // Clear the queue after first pass + navigations.clearRetainingCapacity(); + + // Second pass: process synchronous navigations (about:blank) + // These may trigger new navigations which go into queued_navigation + for (about_blank_queue.items) |page| { + const qn = page._queued_navigation.?; + try self.processFrameNavigation(page, qn); + } + + // Safety: Remove any about:blank navigations that were queued during the + // second pass to prevent infinite loops + var i: usize = 0; + while (i < navigations.items.len) { + const page = navigations.items[i]; + if (page._queued_navigation) |qn| { + if (qn.is_about_blank) { + log.warn(.page, "recursive about blank", .{}); + _ = navigations.swapRemove(i); + continue; + } + } + i += 1; + } +} + +fn processFrameNavigation(self: *Session, current_page: *Page, qn: *QueuedNavigation) !void { + lp.assert(current_page.parent != null, "root queued navigation", .{}); const browser = self.browser; - for (navigations.items) |qn| { - const iframe = qn.iframe.?; - const current_page = iframe._content_window.?._page; // Get the CURRENT page from iframe - lp.assert(current_page.parent != null, "root queued navigation", .{}); + const iframe = current_page.iframe.?; + const parent = current_page.parent.?; - current_page._queued_navigation = null; - defer browser.arena_pool.release(qn.arena); + current_page._queued_navigation = null; + defer browser.arena_pool.release(qn.arena); - const parent = current_page.parent.?; - errdefer iframe._content_window = null; + errdefer iframe._content_window = null; - if (current_page._parent_notified) { - // we already notified the parent that we had loaded - parent._pending_loads += 1; - } - - const frame_id = current_page._frame_id; - defer current_page.deinit(true); - - const new_page = try parent.arena.create(Page); - try Page.init(new_page, frame_id, self, parent); - errdefer new_page.deinit(true); - - new_page.iframe = iframe; - iframe._content_window = new_page.window; - - new_page.navigate(qn.url, qn.opts) catch |err| { - log.err(.browser, "queued frame navigation error", .{ .err = err }); - return err; - }; - - for (parent.frames.items, 0..) |p, i| { - // Page.frames may or may not be sorted (depending on the - // Page.frames_sorted flag). Putting this new page at the same - // position as the one it's replacing is the simplest, safest and - // probably most efficient option. - if (p == current_page) { - parent.frames.items[i] = new_page; - break; - } - } else { - lp.assert(false, "Existing frame not found", .{ .len = parent.frames.items.len }); + if (current_page._parent_notified) { + // we already notified the parent that we had loaded + parent._pending_loads += 1; + } + + const frame_id = current_page._frame_id; + defer current_page.deinit(true); + + const new_page = try parent.arena.create(Page); + try Page.init(new_page, frame_id, self, parent); + errdefer new_page.deinit(true); + + new_page.iframe = iframe; + iframe._content_window = new_page.window; + + new_page.navigate(qn.url, qn.opts) catch |err| { + log.err(.browser, "queued frame navigation error", .{ .err = err }); + return err; + }; + + for (parent.frames.items, 0..) |p, i| { + // Page.frames may or may not be sorted (depending on the + // Page.frames_sorted flag). Putting this new page at the same + // position as the one it's replacing is the simplest, safest and + // probably most efficient option. + if (p == current_page) { + parent.frames.items[i] = new_page; + break; } + } else { + lp.assert(false, "Existing frame not found", .{ .len = parent.frames.items.len }); } } From 679e703754336eb99b91329030d82e84e16197b7 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 6 Mar 2026 09:12:58 +0800 Subject: [PATCH 03/10] Release KeyboardEvent if it isn't used --- src/browser/Page.zig | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/browser/Page.zig b/src/browser/Page.zig index aa40ab0b..1d481eeb 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -3114,7 +3114,11 @@ pub fn handleClick(self: *Page, target: *Node) !void { pub fn triggerKeyboard(self: *Page, keyboard_event: *KeyboardEvent) !void { const event = keyboard_event.asEvent(); - const element = self.window._document._active_element orelse return; + const element = self.window._document._active_element orelse { + keyboard_event.deinit(false, self); + return; + }; + if (comptime IS_DEBUG) { log.debug(.page, "page keydown", .{ .url = self.url, From 9332b1355ee75e5b24607bd2c7ef1667a8a4ae82 Mon Sep 17 00:00:00 2001 From: egrs Date: Fri, 6 Mar 2026 09:37:55 +0100 Subject: [PATCH 04/10] initialize all App fields after allocator.create Same pattern as 3dea554e (mcp/Server.zig): allocator.create returns undefined memory, and struct field defaults (shutdown: bool = false) are not applied when fields are set individually. Use self.* = .{...} to ensure all fields including defaults are initialized. --- src/App.zig | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/App.zig b/src/App.zig index bb797ec5..2d930fd6 100644 --- a/src/App.zig +++ b/src/App.zig @@ -47,10 +47,17 @@ pub fn init(allocator: Allocator, config: *const Config) !*App { const app = try allocator.create(App); errdefer allocator.destroy(app); - app.config = config; - app.allocator = allocator; - - app.robots = RobotStore.init(allocator); + app.* = .{ + .config = config, + .allocator = allocator, + .robots = RobotStore.init(allocator), + .http = undefined, + .platform = undefined, + .snapshot = undefined, + .app_dir_path = undefined, + .telemetry = undefined, + .arena_pool = undefined, + }; app.http = try Http.init(allocator, &app.robots, config); errdefer app.http.deinit(); From bfe2065b9f93b180343d8b9e79608eb5dd7dec66 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 6 Mar 2026 16:54:16 +0800 Subject: [PATCH 05/10] Add target-aware(ish) navigation All inner navigations have an originator and a target. Consider this: ```js aframe.contentDocument.querySelector('#link').click(); ``` The originator is the context in which this JavaScript is called, the target is `aframe. Importantly, relative URLs are resolved based on the originator. This commit adds that. This is only a first step, there are other aspect to this relationship that isn't addressed yet, like differences in behavior if the originator and target are on different origins, and specific target targetting via the things like the "target" attribute. What this commit does though is address the normal / common case. It builds on top of https://github.com/lightpanda-io/browser/pull/1720 --- src/browser/Page.zig | 119 +++++++++++------- src/browser/Session.zig | 45 +++---- src/browser/js/Context.zig | 2 +- src/browser/tests/frames/frames.html | 43 +++---- .../tests/frames/support/with_link.html | 2 +- src/browser/webapi/HTMLDocument.zig | 4 +- src/browser/webapi/Location.zig | 8 +- src/browser/webapi/Window.zig | 4 +- src/browser/webapi/element/html/IFrame.zig | 6 +- src/browser/webapi/navigation/Navigation.zig | 8 +- 10 files changed, 129 insertions(+), 112 deletions(-) diff --git a/src/browser/Page.zig b/src/browser/Page.zig index a8d063b4..7c6bcac4 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -69,6 +69,7 @@ const ArenaPool = App.ArenaPool; const timestamp = @import("../datetime.zig").timestamp; const milliTimestamp = @import("../datetime.zig").milliTimestamp; +const IFrame = Element.Html.IFrame; const WebApiURL = @import("webapi/URL.zig"); const GlobalEventHandlersLookup = @import("webapi/global_event_handlers.zig").Lookup; @@ -223,7 +224,7 @@ _arena_pool_leak_track: (if (IS_DEBUG) std.AutoHashMapUnmanaged(usize, struct { parent: ?*Page, window: *Window, document: *Document, -iframe: ?*Element.Html.IFrame = null, +iframe: ?*IFrame = null, frames: std.ArrayList(*Page) = .{}, frames_sorted: bool = true, @@ -566,62 +567,76 @@ pub fn navigate(self: *Page, request_url: [:0]const u8, opts: NavigateOpts) !voi }; } -// We cannot navigate immediately as navigating will delete the DOM tree, -// which holds this event's node. -// As such we schedule the function to be called as soon as possible. -pub fn scheduleNavigation(self: *Page, request_url: []const u8, opts: NavigateOpts, priority: NavigationPriority) !void { - if (self.canScheduleNavigation(priority) == false) { +// Navigation can happen in many places, such as executing a - - - + diff --git a/src/browser/tests/frames/support/with_link.html b/src/browser/tests/frames/support/with_link.html index 2fd9ca77..bc31b190 100644 --- a/src/browser/tests/frames/support/with_link.html +++ b/src/browser/tests/frames/support/with_link.html @@ -1,2 +1,2 @@ -a link +a link diff --git a/src/browser/webapi/HTMLDocument.zig b/src/browser/webapi/HTMLDocument.zig index 9efde318..15ba610b 100644 --- a/src/browser/webapi/HTMLDocument.zig +++ b/src/browser/webapi/HTMLDocument.zig @@ -180,8 +180,8 @@ pub fn getLocation(self: *const HTMLDocument) ?*@import("Location.zig") { return self._proto._location; } -pub fn setLocation(_: *const HTMLDocument, url: [:0]const u8, page: *Page) !void { - return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .script); +pub fn setLocation(self: *HTMLDocument, url: [:0]const u8, page: *Page) !void { + return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .{ .script = self._proto._page }); } pub fn getAll(self: *HTMLDocument, page: *Page) !*collections.HTMLAllCollection { diff --git a/src/browser/webapi/Location.zig b/src/browser/webapi/Location.zig index dbda1e51..9055abbb 100644 --- a/src/browser/webapi/Location.zig +++ b/src/browser/webapi/Location.zig @@ -83,19 +83,19 @@ pub fn setHash(_: *const Location, hash: []const u8, page: *Page) !void { return page.scheduleNavigation(normalized_hash, .{ .reason = .script, .kind = .{ .replace = null }, - }, .script); + }, .{ .script = page }); } pub fn assign(_: *const Location, url: [:0]const u8, page: *Page) !void { - return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .script); + return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .{ .script = page }); } pub fn replace(_: *const Location, url: [:0]const u8, page: *Page) !void { - return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .replace = null } }, .script); + return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .replace = null } }, .{ .script = page }); } pub fn reload(_: *const Location, page: *Page) !void { - return page.scheduleNavigation(page.url, .{ .reason = .script, .kind = .reload }, .script); + return page.scheduleNavigation(page.url, .{ .reason = .script, .kind = .reload }, .{ .script = page }); } pub fn toString(self: *const Location, page: *const Page) ![:0]const u8 { diff --git a/src/browser/webapi/Window.zig b/src/browser/webapi/Window.zig index 91f55b60..2c3e3cd0 100644 --- a/src/browser/webapi/Window.zig +++ b/src/browser/webapi/Window.zig @@ -160,8 +160,8 @@ pub fn getSelection(self: *const Window) *Selection { return &self._document._selection; } -pub fn setLocation(_: *const Window, url: [:0]const u8, page: *Page) !void { - return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .script); +pub fn setLocation(self: *Window, url: [:0]const u8, page: *Page) !void { + return page.scheduleNavigation(url, .{ .reason = .script, .kind = .{ .push = null } }, .{ .script = self._page }); } pub fn getHistory(_: *Window, page: *Page) *History { diff --git a/src/browser/webapi/element/html/IFrame.zig b/src/browser/webapi/element/html/IFrame.zig index e9cffc6e..d912dd41 100644 --- a/src/browser/webapi/element/html/IFrame.zig +++ b/src/browser/webapi/element/html/IFrame.zig @@ -30,7 +30,7 @@ const IFrame = @This(); _proto: *HtmlElement, _src: []const u8 = "", _executed: bool = false, -_content_window: ?*Window = null, +_window: ?*Window = null, pub fn asElement(self: *IFrame) *Element { return self._proto._proto; @@ -40,11 +40,11 @@ pub fn asNode(self: *IFrame) *Node { } pub fn getContentWindow(self: *const IFrame) ?*Window { - return self._content_window; + return self._window; } pub fn getContentDocument(self: *const IFrame) ?*Document { - const window = self._content_window orelse return null; + const window = self._window orelse return null; return window._document; } diff --git a/src/browser/webapi/navigation/Navigation.zig b/src/browser/webapi/navigation/Navigation.zig index e81ff46e..447f5e00 100644 --- a/src/browser/webapi/navigation/Navigation.zig +++ b/src/browser/webapi/navigation/Navigation.zig @@ -308,7 +308,7 @@ pub fn navigateInner( _ = try self.pushEntry(url, .{ .source = .navigation, .value = state }, page, true); } else { - try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .script); + try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .{ .script = page }); } }, .replace => |state| { @@ -321,7 +321,7 @@ pub fn navigateInner( _ = try self.replaceEntry(url, .{ .source = .navigation, .value = state }, page, true); } else { - try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .script); + try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .{ .script = page }); } }, .traverse => |index| { @@ -334,11 +334,11 @@ pub fn navigateInner( // todo: Fire navigate event finished.resolve("navigation traverse", {}); } else { - try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .script); + try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .{ .script = page }); } }, .reload => { - try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .script); + try page.scheduleNavigation(url, .{ .reason = .navigation, .kind = kind }, .{ .script = page }); }, } From cabd62b48f71414b1ebd9678b1d57548125c7d9e Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 6 Mar 2026 11:29:43 +0800 Subject: [PATCH 06/10] Optimize about:blank loading in general and for frames specifically Instead of going through the parser, just create / append the 3 elements. iframe without a src automatically loads about:blank. This is important, because the following is valid: ```js const iframe = document.createElement('iframe'); document.documentElement.appendChild(iframe); // documentElement should exist and should be the HTML of the blank page. iframe.contentDocument.documentElement.appendChild(...); ``` Builds on top of https://github.com/lightpanda-io/browser/pull/1720 --- src/browser/Page.zig | 19 ++++-------- src/browser/tests/frames/frames.html | 45 ++++++++++++++++++---------- src/browser/webapi/Document.zig | 28 +++++++++++++++++ 3 files changed, 63 insertions(+), 29 deletions(-) diff --git a/src/browser/Page.zig b/src/browser/Page.zig index 7c6bcac4..88068f0c 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -471,12 +471,10 @@ pub fn navigate(self: *Page, request_url: [:0]const u8, opts: NavigateOpts) !voi // It's important to force a reset during the following navigation. self._parse_state = .complete; - { - const parse_arena = try self.getArena(.{ .debug = "about:blank parse" }); - defer self.releaseArena(parse_arena); - var parser = Parser.init(parse_arena, self.document.asNode(), self); - parser.parse(""); - } + self.document.injectBlank(self) catch |err| { + log.err(.browser, "inject blank", .{ .err = err }); + return error.InjectBlankFailed; + }; self.documentIsComplete(); session.notification.dispatch(.page_navigate, &.{ @@ -1029,9 +1027,9 @@ pub fn iframeAddedCallback(self: *Page, iframe: *IFrame) !void { return; } - const src = iframe.asElement().getAttributeSafe(comptime .wrap("src")) orelse return; + var src = iframe.asElement().getAttributeSafe(comptime .wrap("src")) orelse ""; if (src.len == 0) { - return; + src = "about:blank"; } if (iframe._window != null) { @@ -2914,11 +2912,6 @@ fn nodeIsReady(self: *Page, comptime from_parser: bool, node: *Node) !void { return err; }; } else if (node.is(IFrame)) |iframe| { - if ((comptime from_parser == false) and iframe._src.len == 0) { - // iframe was added via JavaScript, but without a src - return; - } - self.iframeAddedCallback(iframe) catch |err| { log.err(.page, "page.nodeIsReady", .{ .err = err, .element = "iframe", .type = self._type, .url = self.url }); return err; diff --git a/src/browser/tests/frames/frames.html b/src/browser/tests/frames/frames.html index 10a22021..4e614de9 100644 --- a/src/browser/tests/frames/frames.html +++ b/src/browser/tests/frames/frames.html @@ -7,46 +7,59 @@ } - + + + + diff --git a/src/browser/webapi/Document.zig b/src/browser/webapi/Document.zig index 10af05fc..74a260e8 100644 --- a/src/browser/webapi/Document.zig +++ b/src/browser/webapi/Document.zig @@ -40,6 +40,8 @@ const Selection = @import("Selection.zig"); pub const XMLDocument = @import("XMLDocument.zig"); pub const HTMLDocument = @import("HTMLDocument.zig"); +const IS_DEBUG = @import("builtin").mode == .Debug; + const Document = @This(); _type: Type, @@ -937,6 +939,32 @@ fn validateElementName(name: []const u8) !void { } } +// When a page or frame's URL is about:blank, or as soon as a frame is +// programmatically created, it has this default "blank" content +pub fn injectBlank(self: *Document, page: *Page) error{InjectBlankError}!void { + self._injectBlank(page) catch |err| { + // we wrap _injectBlank like this so that injectBlank can only return an + // InjectBlankError. injectBlank is used in when nodes are inserted + // as since it inserts node itself, Zig can't infer the error set. + log.err(.browser, "inject blank", .{ .err = err }); + return error.InjectBlankError; + }; +} + +fn _injectBlank(self: *Document, page: *Page) !void { + if (comptime IS_DEBUG) { + // should only be called on an empty document + std.debug.assert(self.asNode()._children == null); + } + + const html = try page.createElementNS(.html, "html", null); + const head = try page.createElementNS(.html, "head", null); + const body = try page.createElementNS(.html, "body", null); + try page.appendNode(html, head, .{}); + try page.appendNode(html, body, .{}); + try page.appendNode(self.asNode(), html, .{}); +} + const ReadyState = enum { loading, interactive, From 3271e1464ea0ec56f4fca2016f9e15c4288c75a3 Mon Sep 17 00:00:00 2001 From: Pierre Tachoire Date: Fri, 6 Mar 2026 10:21:07 +0100 Subject: [PATCH 07/10] Revert pool arena usage w/ ReadableStream Revert "update ref counting for new ReadableStream usages" This reverts commit c64500dd85bda8f5b55f80b5070374ec2c01b0f5. Revert "add reference counting for ReadableStream" This reverts commit 812ad3f49e29d1e4ffb6ba8b9cd4d32ddcf38edb. Revert "use a pool arena with ReadableStream" This reverts commit 8e8a1a7541e33e2bd33905fd2fe3b407538f8683. --- .../webapi/encoding/TextDecoderStream.zig | 10 ---- .../webapi/encoding/TextEncoderStream.zig | 10 ---- src/browser/webapi/streams/ReadableStream.zig | 46 +------------------ .../ReadableStreamDefaultController.zig | 24 +++------- .../streams/ReadableStreamDefaultReader.zig | 19 -------- .../webapi/streams/TransformStream.zig | 20 -------- 6 files changed, 9 insertions(+), 120 deletions(-) diff --git a/src/browser/webapi/encoding/TextDecoderStream.zig b/src/browser/webapi/encoding/TextDecoderStream.zig index b0b7e45c..a5b2dc9a 100644 --- a/src/browser/webapi/encoding/TextDecoderStream.zig +++ b/src/browser/webapi/encoding/TextDecoderStream.zig @@ -72,14 +72,6 @@ pub fn init(label_: ?[]const u8, opts_: ?InitOpts, page: *Page) !TextDecoderStre }; } -pub fn acquireRef(self: *TextDecoderStream) void { - self._transform.acquireRef(); -} - -pub fn deinit(self: *TextDecoderStream, shutdown: bool, page: *Page) void { - self._transform.deinit(shutdown, page); -} - fn decodeTransform(controller: *TransformStream.DefaultController, chunk: js.Value, ignoreBOM: bool) !void { // chunk should be a Uint8Array; decode it as UTF-8 string const typed_array = try chunk.toZig(js.TypedArray(u8)); @@ -119,8 +111,6 @@ pub const JsApi = struct { pub const name = "TextDecoderStream"; pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; - pub const weak = true; - pub const finalizer = bridge.finalizer(TextDecoderStream.deinit); }; pub const constructor = bridge.constructor(TextDecoderStream.init, .{}); diff --git a/src/browser/webapi/encoding/TextEncoderStream.zig b/src/browser/webapi/encoding/TextEncoderStream.zig index a7ae5e2e..b2526637 100644 --- a/src/browser/webapi/encoding/TextEncoderStream.zig +++ b/src/browser/webapi/encoding/TextEncoderStream.zig @@ -34,14 +34,6 @@ pub fn init(page: *Page) !TextEncoderStream { }; } -pub fn acquireRef(self: *TextEncoderStream) void { - self._transform.acquireRef(); -} - -pub fn deinit(self: *TextEncoderStream, shutdown: bool, page: *Page) void { - self._transform.deinit(shutdown, page); -} - fn encodeTransform(controller: *TransformStream.DefaultController, chunk: js.Value) !void { // chunk should be a JS string; encode it as UTF-8 bytes (Uint8Array) const str = chunk.isString() orelse return error.InvalidChunk; @@ -64,8 +56,6 @@ pub const JsApi = struct { pub const name = "TextEncoderStream"; pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; - pub const weak = true; - pub const finalizer = bridge.finalizer(TextEncoderStream.deinit); }; pub const constructor = bridge.constructor(TextEncoderStream.init, .{}); diff --git a/src/browser/webapi/streams/ReadableStream.zig b/src/browser/webapi/streams/ReadableStream.zig index 7d99a0c1..e4e5d0f9 100644 --- a/src/browser/webapi/streams/ReadableStream.zig +++ b/src/browser/webapi/streams/ReadableStream.zig @@ -52,8 +52,6 @@ _pull_fn: ?js.Function.Global = null, _pulling: bool = false, _pull_again: bool = false, _cancel: ?Cancel = null, -_arena: std.mem.Allocator, -_rc: usize = 0, const UnderlyingSource = struct { start: ?js.Function = null, @@ -70,18 +68,13 @@ const QueueingStrategy = struct { pub fn init(src_: ?UnderlyingSource, strategy_: ?QueueingStrategy, page: *Page) !*ReadableStream { const strategy: QueueingStrategy = strategy_ orelse .{}; - const arena = try page.getArena(.{ .debug = "ReadableStream" }); - errdefer page.releaseArena(arena); - - const self = try arena.create(ReadableStream); - self.* = .{ + const self = try page._factory.create(ReadableStream{ ._page = page, ._state = .readable, - ._arena = arena, ._reader = null, ._controller = undefined, ._stored_error = null, - }; + }); self._controller = try ReadableStreamDefaultController.init(self, strategy.highWaterMark, page); @@ -115,23 +108,6 @@ pub fn initWithData(data: []const u8, page: *Page) !*ReadableStream { return stream; } -pub fn deinit(self: *ReadableStream, _: bool, page: *Page) void { - const rc = self._rc; - if (comptime IS_DEBUG) { - std.debug.assert(rc != 0); - } - - if (rc == 1) { - page.releaseArena(self._arena); - } else { - self._rc = rc - 1; - } -} - -pub fn acquireRef(self: *ReadableStream) void { - self._rc += 1; -} - pub fn getReader(self: *ReadableStream, page: *Page) !*ReadableStreamDefaultReader { if (self.getLocked()) { return error.ReaderLocked; @@ -144,12 +120,6 @@ pub fn getReader(self: *ReadableStream, page: *Page) !*ReadableStreamDefaultRead pub fn releaseReader(self: *ReadableStream) void { self._reader = null; - - const rc = self._rc; - if (comptime IS_DEBUG) { - std.debug.assert(rc != 0); - } - self._rc = rc - 1; } pub fn getAsyncIterator(self: *ReadableStream, page: *Page) !*AsyncIterator { @@ -397,8 +367,6 @@ pub const JsApi = struct { pub const name = "ReadableStream"; pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; - pub const weak = true; - pub const finalizer = bridge.finalizer(ReadableStream.deinit); }; pub const constructor = bridge.constructor(ReadableStream.init, .{}); @@ -422,14 +390,6 @@ pub const AsyncIterator = struct { }); } - pub fn acquireRef(self: *AsyncIterator) void { - self._stream.acquireRef(); - } - - pub fn deinit(self: *AsyncIterator, shutdown: bool, page: *Page) void { - self._stream.deinit(shutdown, page); - } - pub fn next(self: *AsyncIterator, page: *Page) !js.Promise { return self._reader.read(page); } @@ -446,8 +406,6 @@ pub const AsyncIterator = struct { pub const name = "ReadableStreamAsyncIterator"; pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; - pub const weak = true; - pub const finalizer = bridge.finalizer(AsyncIterator.deinit); }; pub const next = bridge.function(ReadableStream.AsyncIterator.next, .{}); diff --git a/src/browser/webapi/streams/ReadableStreamDefaultController.zig b/src/browser/webapi/streams/ReadableStreamDefaultController.zig index cd6c6352..18228475 100644 --- a/src/browser/webapi/streams/ReadableStreamDefaultController.zig +++ b/src/browser/webapi/streams/ReadableStreamDefaultController.zig @@ -27,8 +27,6 @@ const ReadableStreamDefaultReader = @import("ReadableStreamDefaultReader.zig"); const IS_DEBUG = @import("builtin").mode == .Debug; -/// ReadableStreamDefaultController uses ReadableStream's arena to make -/// allocation. Indeed, the controller is owned by its ReadableStream. const ReadableStreamDefaultController = @This(); pub const Chunk = union(enum) { @@ -48,6 +46,7 @@ pub const Chunk = union(enum) { _page: *Page, _stream: *ReadableStream, +_arena: std.mem.Allocator, _queue: std.ArrayList(Chunk), _pending_reads: std.ArrayList(js.PromiseResolver.Global), _high_water_mark: u32, @@ -57,22 +56,15 @@ pub fn init(stream: *ReadableStream, high_water_mark: u32, page: *Page) !*Readab ._page = page, ._queue = .empty, ._stream = stream, + ._arena = page.arena, ._pending_reads = .empty, ._high_water_mark = high_water_mark, }); } -pub fn acquireRef(self: *ReadableStreamDefaultController) void { - self._stream.acquireRef(); -} - -pub fn deinit(self: *ReadableStreamDefaultController, shutdown: bool, page: *Page) void { - self._stream.deinit(shutdown, page); -} - pub fn addPendingRead(self: *ReadableStreamDefaultController, page: *Page) !js.Promise { const resolver = page.js.local.?.createPromiseResolver(); - try self._pending_reads.append(self._stream._arena, try resolver.persist()); + try self._pending_reads.append(self._arena, try resolver.persist()); return resolver.promise(); } @@ -82,8 +74,8 @@ pub fn enqueue(self: *ReadableStreamDefaultController, chunk: Chunk) !void { } if (self._pending_reads.items.len == 0) { - const chunk_copy = try chunk.dupe(self._stream._arena); - return self._queue.append(self._stream._arena, chunk_copy); + const chunk_copy = try chunk.dupe(self._page.arena); + return self._queue.append(self._arena, chunk_copy); } // I know, this is ouch! But we expect to have very few (if any) @@ -117,7 +109,7 @@ pub fn enqueueValue(self: *ReadableStreamDefaultController, value: js.Value) !vo if (self._pending_reads.items.len == 0) { const persisted = try value.persist(); - try self._queue.append(self._stream._arena, .{ .js_value = persisted }); + try self._queue.append(self._arena, .{ .js_value = persisted }); return; } @@ -178,7 +170,7 @@ pub fn doError(self: *ReadableStreamDefaultController, err: []const u8) !void { } self._stream._state = .errored; - self._stream._stored_error = try self._stream._arena.dupe(u8, err); + self._stream._stored_error = try self._page.arena.dupe(u8, err); // Reject all pending reads for (self._pending_reads.items) |resolver| { @@ -218,8 +210,6 @@ pub const JsApi = struct { pub const name = "ReadableStreamDefaultController"; pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; - pub const weak = true; - pub const finalizer = bridge.finalizer(ReadableStreamDefaultController.deinit); }; pub const enqueue = bridge.function(ReadableStreamDefaultController.enqueueValue, .{}); diff --git a/src/browser/webapi/streams/ReadableStreamDefaultReader.zig b/src/browser/webapi/streams/ReadableStreamDefaultReader.zig index e8c639ce..2d3c5bbe 100644 --- a/src/browser/webapi/streams/ReadableStreamDefaultReader.zig +++ b/src/browser/webapi/streams/ReadableStreamDefaultReader.zig @@ -19,8 +19,6 @@ const std = @import("std"); const js = @import("../../js/js.zig"); -const IS_DEBUG = @import("builtin").mode == .Debug; - const Page = @import("../../Page.zig"); const ReadableStream = @import("ReadableStream.zig"); const ReadableStreamDefaultController = @import("ReadableStreamDefaultController.zig"); @@ -37,21 +35,6 @@ pub fn init(stream: *ReadableStream, page: *Page) !*ReadableStreamDefaultReader }); } -pub fn acquireRef(self: *ReadableStreamDefaultReader) void { - const stream = self._stream orelse { - if (comptime IS_DEBUG) { - std.debug.assert(false); - } - return; - }; - stream.acquireRef(); -} - -pub fn deinit(self: *ReadableStreamDefaultReader, shutdown: bool, page: *Page) void { - const stream = self._stream orelse return; - stream.deinit(shutdown, page); -} - pub const ReadResult = struct { done: bool, value: Chunk, @@ -127,8 +110,6 @@ pub const JsApi = struct { pub const name = "ReadableStreamDefaultReader"; pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; - pub const weak = true; - pub const finalizer = bridge.finalizer(ReadableStreamDefaultReader.deinit); }; pub const read = bridge.function(ReadableStreamDefaultReader.read, .{}); diff --git a/src/browser/webapi/streams/TransformStream.zig b/src/browser/webapi/streams/TransformStream.zig index bfed7330..e1f42029 100644 --- a/src/browser/webapi/streams/TransformStream.zig +++ b/src/browser/webapi/streams/TransformStream.zig @@ -85,14 +85,6 @@ pub fn initWithZigTransform(zig_transform: ZigTransformFn, page: *Page) !*Transf return self; } -pub fn acquireRef(self: *TransformStream) void { - self._readable.acquireRef(); -} - -pub fn deinit(self: *TransformStream, shutdown: bool, page: *Page) void { - self._readable.deinit(shutdown, page); -} - pub fn transformWrite(self: *TransformStream, chunk: js.Value, page: *Page) !void { if (self._controller._zig_transform_fn) |zig_fn| { // Zig-level transform (used by TextEncoderStream etc.) @@ -138,8 +130,6 @@ pub const JsApi = struct { pub const name = "TransformStream"; pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; - pub const weak = true; - pub const finalizer = bridge.finalizer(TransformStream.deinit); }; pub const constructor = bridge.constructor(TransformStream.init, .{}); @@ -175,14 +165,6 @@ pub const TransformStreamDefaultController = struct { }); } - pub fn acquireRef(self: *TransformStreamDefaultController) void { - self._stream.acquireRef(); - } - - pub fn deinit(self: *TransformStreamDefaultController, shutdown: bool, page: *Page) void { - self._stream.deinit(shutdown, page); - } - pub fn enqueue(self: *TransformStreamDefaultController, chunk: ReadableStreamDefaultController.Chunk) !void { try self._stream._readable._controller.enqueue(chunk); } @@ -207,8 +189,6 @@ pub const TransformStreamDefaultController = struct { pub const name = "TransformStreamDefaultController"; pub const prototype_chain = bridge.prototypeChain(); pub var class_id: bridge.ClassId = undefined; - pub const weak = true; - pub const finalizer = bridge.finalizer(TransformStreamDefaultController.deinit); }; pub const enqueue = bridge.function(TransformStreamDefaultController.enqueueValue, .{}); From 74c0d55a6cf01d8c58372b04fdcb217d228ddc27 Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 6 Mar 2026 17:38:16 +0800 Subject: [PATCH 08/10] Fix cloning custom element with constructor which attaches the element This is a follow up to ca0f77bdee340f058438621c06ef2f30009d15cf that applies the same fix to all places where cloneNode is used and then the cloned element is inserted. A helper was added more of a means of documentation than to DRY up the code. --- .../tests/custom_elements/constructor.html | 56 +++++++++++++++++++ src/browser/webapi/DocumentFragment.zig | 5 +- src/browser/webapi/Element.zig | 18 ++---- src/browser/webapi/Node.zig | 23 ++++++++ src/browser/webapi/Range.zig | 13 +++-- 5 files changed, 95 insertions(+), 20 deletions(-) diff --git a/src/browser/tests/custom_elements/constructor.html b/src/browser/tests/custom_elements/constructor.html index 0241b242..c99639a6 100644 --- a/src/browser/tests/custom_elements/constructor.html +++ b/src/browser/tests/custom_elements/constructor.html @@ -72,3 +72,59 @@ testing.expectEqual(2, calls); } + +
+ + + +
+ + diff --git a/src/browser/webapi/DocumentFragment.zig b/src/browser/webapi/DocumentFragment.zig index f804b08e..004ee916 100644 --- a/src/browser/webapi/DocumentFragment.zig +++ b/src/browser/webapi/DocumentFragment.zig @@ -195,8 +195,9 @@ pub fn cloneFragment(self: *DocumentFragment, deep: bool, page: *Page) !*Node { var child_it = node.childrenIterator(); while (child_it.next()) |child| { - const cloned_child = try child.cloneNode(true, page); - try page.appendNode(fragment_node, cloned_child, .{ .child_already_connected = self_is_connected }); + if (try child.cloneNodeForAppending(true, page)) |cloned_child| { + try page.appendNode(fragment_node, cloned_child, .{ .child_already_connected = self_is_connected }); + } } } diff --git a/src/browser/webapi/Element.zig b/src/browser/webapi/Element.zig index ef2386da..7a6598a3 100644 --- a/src/browser/webapi/Element.zig +++ b/src/browser/webapi/Element.zig @@ -1328,20 +1328,12 @@ pub fn clone(self: *Element, deep: bool, page: *Page) !*Node { if (deep) { var child_it = self.asNode().childrenIterator(); while (child_it.next()) |child| { - const cloned_child = try child.cloneNode(true, page); - if (cloned_child._parent != null) { - // This is almost always false, the only case where a cloned - // node would already have a parent is with a custom element - // that has a constructor (which is called during cloning) which - // inserts it somewhere. In that case, whatever parent was set - // in the constructor should not be changed. - continue; + if (try child.cloneNodeForAppending(true, page)) |cloned_child| { + // We pass `true` to `child_already_connected` as a hacky optimization + // We _know_ this child isn't connected (Because the parent isn't connected) + // setting this to `true` skips all connection checks. + try page.appendNode(node, cloned_child, .{ .child_already_connected = true }); } - - // We pass `true` to `child_already_connected` as a hacky optimization - // We _know_ this child isn't connected (Because the parent isn't connected) - // setting this to `true` skips all connection checks. - try page.appendNode(node, cloned_child, .{ .child_already_connected = true }); } } diff --git a/src/browser/webapi/Node.zig b/src/browser/webapi/Node.zig index 4b351b6f..e93cc1f7 100644 --- a/src/browser/webapi/Node.zig +++ b/src/browser/webapi/Node.zig @@ -751,6 +751,29 @@ pub fn cloneNode(self: *Node, deep_: ?bool, page: *Page) CloneError!*Node { } } +/// Clone a node for the purpose of appending to a parent. +/// Returns null if the cloned node was already attached somewhere by a custom element +/// constructor, indicating that the constructor's decision should be respected. +/// +/// This helper is used when iterating over children to clone them. The typical pattern is: +/// while (child_it.next()) |child| { +/// if (try child.cloneNodeForAppending(true, page)) |cloned| { +/// try page.appendNode(parent, cloned, opts); +/// } +/// } +/// +/// The only case where a cloned node would already have a parent is when a custom element +/// constructor (which runs during cloning per the HTML spec) explicitly attaches the element +/// somewhere. In that case, we respect the constructor's decision and return null to signal +/// that the cloned node should not be appended to our intended parent. +pub fn cloneNodeForAppending(self: *Node, deep: bool, page: *Page) CloneError!?*Node { + const cloned = try self.cloneNode(deep, page); + if (cloned._parent != null) { + return null; + } + return cloned; +} + pub fn compareDocumentPosition(self: *Node, other: *Node) u16 { const DISCONNECTED: u16 = 0x01; const PRECEDING: u16 = 0x02; diff --git a/src/browser/webapi/Range.zig b/src/browser/webapi/Range.zig index 840fa227..21a3ce12 100644 --- a/src/browser/webapi/Range.zig +++ b/src/browser/webapi/Range.zig @@ -446,8 +446,9 @@ pub fn cloneContents(self: *const Range, page: *Page) !*DocumentFragment { var offset = self._proto._start_offset; while (offset < self._proto._end_offset) : (offset += 1) { if (self._proto._start_container.getChildAt(offset)) |child| { - const cloned = try child.cloneNode(true, page); - _ = try fragment.asNode().appendChild(cloned, page); + if (try child.cloneNodeForAppending(true, page)) |cloned| { + _ = try fragment.asNode().appendChild(cloned, page); + } } } } @@ -468,9 +469,11 @@ pub fn cloneContents(self: *const Range, page: *Page) !*DocumentFragment { if (self._proto._start_container.parentNode() == self._proto._end_container.parentNode()) { var current = self._proto._start_container.nextSibling(); while (current != null and current != self._proto._end_container) { - const cloned = try current.?.cloneNode(true, page); - _ = try fragment.asNode().appendChild(cloned, page); - current = current.?.nextSibling(); + const next = current.?.nextSibling(); + if (try current.?.cloneNodeForAppending(true, page)) |cloned| { + _ = try fragment.asNode().appendChild(cloned, page); + } + current = next; } } From 4a26cd8d68ed580649f491254d4cf364c0c2183d Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Fri, 6 Mar 2026 09:31:51 +0800 Subject: [PATCH 09/10] Halt tests (@panic) on ArenaLeak or double-free These are too hard to see during a full test run. --- src/browser/Page.zig | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/browser/Page.zig b/src/browser/Page.zig index aa40ab0b..0376b0f3 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -357,6 +357,9 @@ pub fn deinit(self: *Page) void { while (it.next()) |value_ptr| { if (value_ptr.count > 0) { log.err(.bug, "ArenaPool Leak", .{ .owner = value_ptr.owner, .type = self._type, .url = self.url }); + if (comptime builtin.is_test) { + @panic("ArenaPool Leak"); + } } } } @@ -429,6 +432,9 @@ pub fn releaseArena(self: *Page, allocator: Allocator) void { const found = self._arena_pool_leak_track.getPtr(@intFromPtr(allocator.ptr)).?; if (found.count != 1) { log.err(.bug, "ArenaPool Double Free", .{ .owner = found.owner, .count = found.count, .type = self._type, .url = self.url }); + if (comptime builtin.is_test) { + @panic("ArenaPool Double Free"); + } return; } found.count = 0; From 52250ed10e4b75decf4d57e3db576f7e49fe0cde Mon Sep 17 00:00:00 2001 From: Pierre Tachoire Date: Fri, 6 Mar 2026 09:47:01 +0100 Subject: [PATCH 10/10] wpt: increase concurrency --- .github/workflows/wpt.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/wpt.yml b/.github/workflows/wpt.yml index 03313ea8..306f1457 100644 --- a/.github/workflows/wpt.yml +++ b/.github/workflows/wpt.yml @@ -5,6 +5,7 @@ env: AWS_SECRET_ACCESS_KEY: ${{ secrets.LPD_PERF_AWS_SECRET_ACCESS_KEY }} AWS_BUCKET: ${{ vars.LPD_PERF_AWS_BUCKET }} AWS_REGION: ${{ vars.LPD_PERF_AWS_REGION }} + AWS_CF_DISTRIBUTION: ${{ vars.AWS_CF_DISTRIBUTION }} LIGHTPANDA_DISABLE_TELEMETRY: true on: @@ -73,7 +74,7 @@ jobs: # use a self host runner. runs-on: lpd-bench-hetzner - timeout-minutes: 120 + timeout-minutes: 180 steps: - uses: actions/checkout@v6 @@ -107,7 +108,7 @@ jobs: run: | ./wpt serve 2> /dev/null & echo $! > WPT.pid sleep 10s - ./wptrunner -lpd-path ./lightpanda -json -concurrency 3 > wpt.json + ./wptrunner -lpd-path ./lightpanda -json -concurrency 6 > wpt.json kill `cat WPT.pid` - name: write commit