keep the existing page active until the pending one is loaded by krichprollsch · Pull Request #2297 · lightpanda-io/browser

krichprollsch · 2026-04-28T07:20:30Z

During a root navigation, we keep the existing page active until we get
the headers callback from the pending page. Then
Session.commitPendingPage makes the switch.

It delays the deinit of CPD execution context to handle JS execution in
the meantime.

Now session has an array of two pages, _active_idx points to the main
page.

Both active and pending pages share the same frame_id, it must remains
stable. So this PR adds a Request.protect_from_abort to avoid removing
the request form the pending page when deinit the previous active page.

fix #2187

krichprollsch · 2026-04-28T07:25:28Z

I have to add a new e2e test
EDIT: lightpanda-io/demo#169

krichprollsch · 2026-04-28T09:11:33Z

The branch works on my own test case, but fails on #2187 (comment) script.

EDIT: it works now ✔️

krichprollsch · 2026-04-29T14:28:36Z

+    // Refresh self.frame from session. In case of pending page, we want to
+    // take its state while loading. If we use only the current frame, we will
+    // return a .done result immediately.
+    self.frame = self.session.currentOrPendingFrame() orelse return .done;


I'm not sure about that... But I don't really have a better solution :/

karlseguin · 2026-04-30T04:44:36Z


    self.* = .{
-        .page = null,
+        ._pages = .{ null, null },


I'm guilty of doing this too, but you both set default value sin the fields, then set those same defaults on init...

I removed from init, but I'm not sure it it's better this way or to remove default from fields?

I think either is better than both :)

field defaults have the advantage of letting you create an empty struct without an init. But, when that's not an option (like here), the practical different is small. I do think field defaults might better improve documentation / discoverability. If I want to understand a field, I'll go scroll up to where it's defined, hope for a comment, and the default might provide more insight too.

karlseguin · 2026-04-30T05:03:20Z

+// available for the next pending allocation.
+//
+// Convention: a slot is "occupied" iff its `?Page` is non-null.
+_pages: [2]?Page = .{ null, null },


did you consider going with two fields and allocating page?

active: ?*Page, pending: ?*Page,

I did, and I disliked it 🤔 But I didn't remember exactly why. I'm trying again.
You think it's better to allocate to avoid these empty structs in the stack?

I think it's minor. A few places that are currently 2 lines ((1) get the index (2) get the page) become 1. That's pretty much it.

pub fn currentPage(self: *Session) ?*Page { const idx = self._active_idx orelse return null; return &self._pages[idx].?; }

becomes

pub fn currentPage(self: *Session) ?*Page { return self.active; }

Or, if you look at the start ofinitRootNavigation, it becomes:

self.discardPendingPage(); // can do const page = self.pending orelse return; const page = try self.pageInit(frame_id); errdefer page.deinit(); page._state = .pending; self.pending = page; errdefer self.pending = null ..

(again, quite minor)

ok, it's simpler after all. But where should I allocate the pages? Claude suggests to use app.allocator directly, but not sure about that. WDYT?

Ya, or a dedicated std.heap.MemoryPool(Page) on the session if you think we'll create a lot. Or on the Browser if you think we'll create a lot of Sessions...

karlseguin · 2026-04-30T05:13:24Z

    current_frame._queued_navigation = null;

+    // Synthetic navigations (about:blank, blob:) commit instantly — no HTTP,
+    // so there is no in-flight window to worry about. Use the legacy


The fact that it's "legacy" isn't relevant. We normally say something is legacy to explain why a behavior is the way it is. But here, replaceRootImmediate doesn't exist for some "legacy" reason..it's just a more optimized / simpler (maybe more correct?) path to take for these cases. Or am I misunderstanding and replaceRootImmediate exists only to keep an pre-existing behavior?

you are correct.

karlseguin · 2026-04-30T05:16:13Z

+    // Synthetic navigations (about:blank, blob:) commit instantly — no HTTP,
+    // so there is no in-flight window to worry about. Use the legacy
+    // immediate-swap path for them.
+    const is_synthetic = std.mem.eql(u8, qn.url, "about:blank") or


qn.is_about_blank

We need to check this when QueuedNavigation is created (in Frame.scheduleNavigationWithArena) and I know it was checked again during actual navigation, so I figured might as well save the boolean.

karlseguin · 2026-04-30T05:20:50Z

+pub fn commitPendingPage(self: *Session) !void {
+    const pending_idx = self._pending_idx orelse {
+        lp.assert(false, "Session.commitPendingPage - no pending page", .{});
+        return error.NoPendingPage;


unreachable.

The intent here is unclear. Do you want the assert only in debug mode? (this pattern happens 3 or so times in this file, I think they should all be cleaned up).

good, point, Claude did see that too and I didn't fix it 🤦

karlseguin · 2026-04-30T05:22:11Z

+        lp.assert(false, "Session.commitPendingPage - no pending page", .{});
+        return error.NoPendingPage;
+    };
+    const old_idx = self._active_idx orelse {


old_active_idx? just a big more clear later on when you do:

self._pages[old_active_idx].?.deinit(); self.freeSlot(old_active_idx);

(pretty minor, I'd probably have named it old_idx too!)

good catch, I did move that after a rebase... It was closer to the deinit at first.

karlseguin · 2026-04-30T06:27:52Z

+    // with this flag so the callback chain we are sitting inside isn't killed
+    // mid-flight. Session.discardPendingPage uses .full scope to override
+    // the flag in failure paths.
+    protect_from_abort: bool = false,


This is clever, and it makes sense. BUT, I think if we do give HttpClient a major cleanup (e.g. merging Request and Transfer), I'd try to come up with a better way to track ownership of frame <-> request.

Yeah, this trick is only because I must share the same frame_id between actually 2 different frames...
And I dislike it...

During a root navigation, we keep the existing page active until we get the headers callback from the pending page. Then Session.commitPendingPage makes the switch. It delays the deinit of CPD execution context to handle JS execution in the meantime. Now session has an array of two pages, _active_idx points to the main page. Both active and pending pages share the same frame_id, it must remains stable. So this PR adds a Request.protect_from_abort to avoid removing the request form the pending page when deinit the previous active page.

So the CDP can remove/reset context and re-create isolated world accordingly

We want to take in account the pending frame in Runner._tick to continue to process. If we use only the current frame, we will immediately return in case of navigation.

Use distinct key for laoder id and request id based captured response.

Co-authored-by: Karl Seguin <karlseguin@users.noreply.github.com>

Remove unreachable code and use `comptime IS_DEBUG` check when need.

And fix comment

krichprollsch force-pushed the pending-page branch from c54e9e8 to 03ea72c Compare April 28, 2026 07:51

krichprollsch self-assigned this Apr 28, 2026

krichprollsch marked this pull request as draft April 28, 2026 10:35

krichprollsch marked this pull request as ready for review April 28, 2026 16:32

krichprollsch force-pushed the pending-page branch from 50b9ef1 to 5381eb6 Compare April 28, 2026 17:00

krichprollsch mentioned this pull request Apr 28, 2026

add a test for pending page lightpanda-io/demo#169

Merged

krichprollsch force-pushed the pending-page branch 2 times, most recently from 7a59be4 to b730739 Compare April 29, 2026 09:22

krichprollsch commented Apr 29, 2026

View reviewed changes

krichprollsch force-pushed the pending-page branch 2 times, most recently from 55efd0e to d4765fc Compare April 29, 2026 15:40

krichprollsch requested a review from karlseguin April 29, 2026 15:41

karlseguin reviewed Apr 30, 2026

View reviewed changes

krichprollsch force-pushed the pending-page branch 5 times, most recently from b8ed5b3 to 0b374b0 Compare May 1, 2026 15:03

krichprollsch force-pushed the pending-page branch from 0b374b0 to 7fabccb Compare May 4, 2026 06:51

krichprollsch added 8 commits May 4, 2026 08:59

always deinit http of frame/page/worker by frame id

6e1b8f6

dispatch frame_remove and new_frame events from sesion.replacePage

f7ac258

So the CDP can remove/reset context and re-create isolated world accordingly

cdp: remove replacePage and use Session.initiateRootNavigation

c251f0c

Get the pending frame from the Runner

84246c3

We want to take in account the pending frame in Runner._tick to continue to process. If we use only the current frame, we will immediately return in case of navigation.

cdp: use loader_id as captured response key for documents

11172a3

discardPendingPage on Session.initiateRootNavigation

e33018f

update comments according to abortFrame change

b2c53f4

cdp: avoid request id conflict between LID- and REQ-

cddabe6

Use distinct key for laoder id and request id based captured response.

krichprollsch and others added 8 commits May 4, 2026 08:59

refacto Session install and teardown active page

ddbdaaf

typo fix

e3eb8eb

rename pendingOrCurrentFrame

38169fd

Co-authored-by: Karl Seguin <karlseguin@users.noreply.github.com>

rename var old_idx into old_active_idx for clarity

3fc5e6d

fix lp.assert usage

35cecf6

Remove unreachable code and use `comptime IS_DEBUG` check when need.

Don't re-init fields w/ default values

6e7398d

use qn.is_about_blank for check instead of string comparison

f9cdc12

And fix comment

replace pages array from Session to allocated active and pending pages

14743e4

krichprollsch force-pushed the pending-page branch from 7fabccb to 14743e4 Compare May 4, 2026 07:00

karlseguin approved these changes May 4, 2026

View reviewed changes

krichprollsch merged commit 65308f0 into main May 4, 2026
90 of 95 checks passed

krichprollsch deleted the pending-page branch May 4, 2026 07:42

github-actions Bot locked and limited conversation to collaborators May 4, 2026

Conversation

krichprollsch commented Apr 28, 2026

Uh oh!

krichprollsch commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krichprollsch commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krichprollsch Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

krichprollsch commented Apr 28, 2026 •

edited

Loading

krichprollsch commented Apr 28, 2026 •

edited

Loading

krichprollsch Apr 30, 2026 •

edited

Loading