From 6e6082119fbc0a5b8d237df7f774cbf39cc9bb5c Mon Sep 17 00:00:00 2001 From: Karl Seguin Date: Tue, 17 Feb 2026 18:16:35 +0800 Subject: [PATCH] Remove session.transfer_arena This no longer works with frames. Multiple frames could have a scheduled navigation, so a single arena no longer has a clear lifecycle. Instead an arena from the pool is used per navigation event, thus the queued_navigation is self- contained. This required having libcurl copy the body. Unfortunate. Currently we free the arena as soon as the navigation begins. This is clean. But it means the body is immediately freed (thus we need libcurl to copy it). As an alternative, each page could maintain an optional transfer_arena, which it could free on httpDone/Error. --- src/browser/Page.zig | 57 +++++++++++++-------------------- src/browser/Session.zig | 70 ++++++++++++++++------------------------- src/http/Http.zig | 2 +- 3 files changed, 50 insertions(+), 79 deletions(-) diff --git a/src/browser/Page.zig b/src/browser/Page.zig index 52a1b07f..6d6828bc 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -181,7 +181,7 @@ _notified_network_almost_idle: IdleNotification = .init, // A navigation event that happens from a script gets scheduled to run on the // next tick. -_queued_navigation: ?QueuedNavigation = null, +_queued_navigation: ?*QueuedNavigation = null, // The URL of the current page url: [:0]const u8 = "about:blank", @@ -542,23 +542,27 @@ 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. -// The page.arena is safe to use here, but the transfer_arena exists -// specifically for this type of lifetime. pub fn scheduleNavigation(self: *Page, request_url: []const u8, opts: NavigateOpts, priority: NavigationPriority) !void { if (self.canScheduleNavigation(priority) == false) { return; } + const arena = try self.arena_pool.acquire(); + errdefer self.arena_pool.release(arena); + return self.scheduleNavigationWithArena(arena, request_url, opts, priority); +} - const session = self._session; - +fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []const u8, opts: NavigateOpts, priority: NavigationPriority) !void { const resolved_url = try URL.resolve( - session.transfer_arena, + arena, self.base(), request_url, .{ .always_dupe = 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; @@ -572,13 +576,16 @@ pub fn scheduleNavigation(self: *Page, request_url: []const u8, opts: NavigateOp .type = self._type, }); - self._session.browser.http_client.abort(); + session.browser.http_client.abort(); - self._queued_navigation = .{ + const qn = try arena.create(QueuedNavigation); + qn.* = .{ .opts = opts, + .arena = arena, .url = resolved_url, .priority = priority, }; + self._queued_navigation = qn; } // A script can have multiple competing navigation events, say it starts off @@ -837,8 +844,6 @@ fn pageDoneCallback(ctx: *anyopaque) !void { log.debug(.page, "navigate done", .{ .type = self._type }); } - self.clearTransferArena(); - //We need to handle different navigation types differently. try self._session.navigation.commitNavigation(self); @@ -925,24 +930,6 @@ fn pageErrorCallback(ctx: *anyopaque, err: anyerror) void { }; } -// The transfer arena is useful and interesting, but has a weird lifetime. -// When we're transferring from one page to another (via delayed navigation) -// we need things in memory: like the URL that we're navigating to and -// optionally the body to POST. That cannot exist in the page.arena, because -// the page that we have is going to be destroyed and a new page is going -// to be created. If we used the page.arena, we'd wouldn't be able to reset -// it between navigation. -// So the transfer arena is meant to exist between a navigation event. It's -// freed when the main html navigation is complete, either in pageDoneCallback -// or pageErrorCallback. It needs to exist for this long because, if we set -// a body, CURLOPT_POSTFIELDS does not copy the body (it optionally can, but -// why would we want to) and requires the body to live until the transfer -// is complete. -fn clearTransferArena(self: *Page) void { - self.arena_pool.reset(self._session.transfer_arena, 4 * 1024); -} - -<<<<<<< HEAD pub fn wait(self: *Page, wait_ms: u32) Session.WaitResult { return self._wait(wait_ms) catch |err| { switch (err) { @@ -1174,8 +1161,6 @@ fn printWaitAnalysis(self: *Page) void { } } -======= ->>>>>>> 3bd80eb3 (Move page.wait to session.wait) pub fn isGoingAway(self: *const Page) bool { return self._queued_navigation != null; } @@ -3150,7 +3135,8 @@ const NavigationPriority = enum { anchor, }; -const QueuedNavigation = struct { +pub const QueuedNavigation = struct { + arena: Allocator, url: [:0]const u8, opts: NavigateOpts, priority: NavigationPriority, @@ -3333,11 +3319,12 @@ pub fn submitForm(self: *Page, submitter_: ?*Element, form_: ?*Element.Html.Form // I don't think this is technically correct, but FormData handles it ok const form_data = try FormData.init(form, submitter_, self); - const transfer_arena = self._session.transfer_arena; + const arena = try self.arena_pool.acquire(); + errdefer self.arena_pool.release(arena); const encoding = form_element.getAttributeSafe(comptime .wrap("enctype")); - var buf = std.Io.Writer.Allocating.init(transfer_arena); + var buf = std.Io.Writer.Allocating.init(arena); try form_data.write(encoding, &buf.writer); const method = form_element.getAttributeSafe(comptime .wrap("method")) orelse ""; @@ -3353,9 +3340,9 @@ pub fn submitForm(self: *Page, submitter_: ?*Element, form_: ?*Element.Html.Form // form_data.write currently only supports this encoding, so we know this has to be the content type opts.header = "Content-Type: application/x-www-form-urlencoded"; } else { - action = try URL.concatQueryString(transfer_arena, action, buf.written()); + action = try URL.concatQueryString(arena, action, buf.written()); } - return self.scheduleNavigation(action, opts, .form); + return self.scheduleNavigationWithArena(arena, action, opts, .form); } // insertText is a shortcut to insert text into the active element. diff --git a/src/browser/Session.zig b/src/browser/Session.zig index 1fef35c9..c61cd4f7 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -46,16 +46,6 @@ notification: *Notification, // Used to create our Inspector and in the BrowserContext. arena: Allocator, -// The page's arena is unsuitable for data that has to existing while -// navigating from one page to another. For example, if we're clicking -// on an HREF, the URL exists in the original page (where the click -// originated) but also has to exist in the new page. -// While we could use the Session's arena, this could accumulate a lot of -// memory if we do many navigation events. The `transfer_arena` is meant to -// bridge the gap: existing long enough to store any data needed to end one -// page and start another. -transfer_arena: Allocator, - cookie_jar: storage.Cookie.Jar, storage_shed: storage.Shed, @@ -71,9 +61,6 @@ pub fn init(self: *Session, browser: *Browser, notification: *Notification) !voi const arena = try browser.arena_pool.acquire(); errdefer browser.arena_pool.release(arena); - const transfer_arena = try browser.arena_pool.acquire(); - errdefer browser.arena_pool.release(transfer_arena); - self.* = .{ .page = null, .arena = arena, @@ -84,7 +71,6 @@ pub fn init(self: *Session, browser: *Browser, notification: *Notification) !voi .storage_shed = .{}, .browser = browser, .notification = notification, - .transfer_arena = transfer_arena, .cookie_jar = storage.Cookie.Jar.init(allocator), }; } @@ -97,7 +83,6 @@ pub fn deinit(self: *Session) void { self.cookie_jar.deinit(); self.storage_shed.deinit(browser.app.allocator); - browser.arena_pool.release(self.transfer_arena); browser.arena_pool.release(self.arena); } @@ -182,17 +167,17 @@ pub fn wait(self: *Session, wait_ms: u32) WaitResult { const wait_result = self._wait(page, wait_ms) catch |err| { switch (err) { error.JsError => {}, // already logged (with hopefully more context) - else => log.err(.browser, "session wait", .{ .err = err, }), + else => log.err(.browser, "session wait", .{ + .err = err, + }), } return .done; }; switch (wait_result) { .done => { - if (page._queued_navigation == null) { - return .done; - } - page = self.processScheduledNavigation() catch return .done; + const qn = page._queued_navigation orelse return .done; + page = self.processScheduledNavigation(qn) catch return .done; }, else => |result| return result, } @@ -337,35 +322,34 @@ fn _wait(self: *Session, page: *Page, wait_ms: u32) !WaitResult { } } -fn processScheduledNavigation(self: *Session) !*Page { - defer self.browser.arena_pool.reset(self.transfer_arena, 4 * 1024); - const url, const opts, const page_id = blk: { - const page = self.page.?; - const qn = page._queued_navigation.?; - // qn might not be safe to use after self.removePage is called, hence - // this block; - const url = qn.url; - const opts = qn.opts; +fn processScheduledNavigation(self: *Session, qn: *Page.QueuedNavigation) !*Page { + const browser = self.browser; + defer browser.arena_pool.release(qn.arena); - // This was already aborted on the page, but it would be pretty - // bad if old requests went to the new page, so let's make double sure - self.browser.http_client.abort(); + const page_id, const parent = blk: { + const page = &self.page.?; + const page_id = page.id; + const parent = page._parent; + + browser.http_client.abort(); self.removePage(); - break :blk .{ url, opts, page.id }; + break :blk .{page_id, parent}; }; - const page = self.createPage() catch |err| { - log.err(.browser, "queued navigation page error", .{ - .err = err, - .url = url, - }); - return err; - }; - page.id = page_id; + self.page = @as(Page, undefined); + const page = &self.page.?; + try Page.init(page, page_id, self, parent); - page.navigate(url, opts) catch |err| { - log.err(.browser, "queued navigation error", .{ .err = err, .url = url }); + // Creates a new NavigationEventTarget for this page. + try self.navigation.onNewPage(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); + + page.navigate(qn.url, qn.opts) catch |err| { + log.err(.browser, "queued navigation error", .{ .err = err, .url = qn.url }); return err; }; diff --git a/src/http/Http.zig b/src/http/Http.zig index 0c20faa8..553c1c0e 100644 --- a/src/http/Http.zig +++ b/src/http/Http.zig @@ -211,7 +211,7 @@ pub const Connection = struct { const easy = self.easy; try errorCheck(c.curl_easy_setopt(easy, c.CURLOPT_HTTPPOST, @as(c_long, 1))); try errorCheck(c.curl_easy_setopt(easy, c.CURLOPT_POSTFIELDSIZE, @as(c_long, @intCast(body.len)))); - try errorCheck(c.curl_easy_setopt(easy, c.CURLOPT_POSTFIELDS, body.ptr)); + try errorCheck(c.curl_easy_setopt(easy, c.CURLOPT_COPYPOSTFIELDS, body.ptr)); } // These are headers that may not be send to the users for inteception.