diff --git a/src/browser/Page.zig b/src/browser/Page.zig index 597c0ba47..e8046b739 100644 --- a/src/browser/Page.zig +++ b/src/browser/Page.zig @@ -140,6 +140,13 @@ replaces: ?*Page = null, // code. The two are kept in sync. replacement: ?*Page = null, +// Prevents double entry in session._page_destruction_queue. Can happen since +// various paths can enter this, and there isn't always a single clear owner +// of who should errdefer, e.g. if this happens before a navigation's +// transfer.submit(), then the caller needs to handle the failure. If it happens +// after, then frameErrorCallback does. +destroying: bool = false, + // The viewport every consumer should read. The runtime override (set via // Emulation.setDeviceMetricsOverride) is stored on the Browser so it persists // across page navigations; delegate to it here, keeping a single read path for diff --git a/src/browser/Session.zig b/src/browser/Session.zig index 9b318e217..be8fe3aae 100644 --- a/src/browser/Session.zig +++ b/src/browser/Session.zig @@ -284,6 +284,11 @@ fn allocatePage(self: *Session, frame_id: u32) !*Page { // Tear down and free a Page allocated via allocatePage. fn queuePageDestruction(self: *Session, page: *Page) void { + if (page.destroying) { + // see Page.destroying field for why this is necessary. + return; + } + page.destroying = true; self._page_destruction_queue.append(self.arena.allocator(), page) catch @panic("OOM"); } @@ -292,6 +297,10 @@ fn retire(self: *Session, page: *Page) void { // page is being destroyed, if it was replacing a page, then update that // page's replacement to keep replaces<->replacement consistent. live.replacement = null; + // Might have retired while transfer was active and the Frame's + // header/error callback might still happen. Setting this null prevents + // those callbacks from using this page post-retirement + page.replaces = null; } if (page.replacement) |replacement| { @@ -389,10 +398,9 @@ pub fn closePage(self: *Session, frame_id: u32) void { // We queue then process so that there is a single place (processDestroyQueues) // where pages get destroyed. pub fn closeAllPages(self: *Session) void { - self._page_destruction_queue.ensureUnusedCapacity(self.arena.allocator(), self.pages.items.len) catch @panic("OOM"); for (self.pages.items) |page| { page.frame.abortTransfers(); - self._page_destruction_queue.appendAssumeCapacity(page); + self.queuePageDestruction(page); } self.pages.clearRetainingCapacity(); self.processDestroyQueues(); @@ -845,7 +853,8 @@ pub fn initiateRootNavigation(self: *Session, frame_id: u32, url: [:0]const u8, errdefer live.replacement = null; try self.pages.append(self.arena.allocator(), page); - errdefer _ = self.pages.pop(); + // don't just pop, frame.navitate() can itself remove this page from the pages list + errdefer self.removePageFromList(page); if (comptime IS_DEBUG) { log.debug(.browser, "initiate root navigation", .{ .url = url }); @@ -976,3 +985,36 @@ pub const PageHandle = struct { return f.navigate(request_url, opts); } }; + +const testing = @import("../testing.zig"); +test "Session: retiring a pending page destroys it once" { + const session = testing.test_session; + defer session.closeAllPages(); + + const handle = try session.createPage(); + const live = handle.page().?; + + // A pending root navigation, set up like initiateRootNavigation does. + const pending = try session.allocatePage(live.frame._frame_id); + pending.replaces = live; + live.replacement = pending; + try session.pages.append(session.arena.allocator(), pending); + + // Its transfer failed inside submit(): the error_callback discards the + // pending page... + session.discardPendingPage(pending); + try testing.expectEqual(null, pending.replaces); + try testing.expectEqual(null, live.replacement); + + // ...and then submit's error unwinds initiateRootNavigation's errdefers + // over a page that is already off the list and already queued. + session.removePageFromList(pending); + session.queuePageDestruction(pending); + + try testing.expectEqual(1, session.pages.items.len); + try testing.expectEqual(live, session.pages.items[0]); + try testing.expectEqual(1, session._page_destruction_queue.items.len); + + // Would deinit `pending` twice if it had been queued twice. + session.processDestroyQueues(); +}