Merge pull request #1720 from lightpanda-io/frame_scheduled_navigation

Improve frame sub-navigation

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) {
@@ -465,6 +471,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;
@@ -578,31 +585,50 @@ 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(
+    const resolved_url, const is_about_blank = blk: {
+        if (std.mem.eql(u8, request_url, "about:blank")) {
+            break :blk .{ "about:blank", true }; // navigate will handle this special case
+        }
+        const u = try URL.resolve(
             arena,
             self.base(),
             request_url,
             .{ .always_dupe = true, .encode = true },
         );
+        break :blk .{ u, false };
+    };
 
     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,
     });
 
+    // 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.* = .{
@@ -610,12 +636,11 @@ fn scheduleNavigationWithArena(self: *Page, arena: Allocator, request_url: []con
         .arena = arena,
         .url = resolved_url,
         .priority = priority,
+        .is_about_blank = is_about_blank,
     };
 
-    if (self._queued_navigation) |existing| {
-        self.arena_pool.release(existing.arena);
-    }
     self._queued_navigation = qn;
+    return session.scheduleNavigation(self);
 }
 
 // A script can have multiple competing navigation events, say it starts off
@@ -629,6 +654,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) {
@@ -637,7 +668,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
     };
@@ -764,6 +796,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);
@@ -773,9 +807,7 @@ fn notifyParentLoadComplete(self: *Page) void {
     }
 
     self._parent_notified = true;
-    if (self.parent) |p| {
+    parent.iframeCompletedLoading(self.iframe.?);
-        p.iframeCompletedLoading(self.iframe.?);
-    }
 }
 
 fn pageHeaderDoneCallback(transfer: *Http.Transfer) !bool {
@@ -955,7 +987,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 {
@@ -988,30 +1024,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: {
+    const frame_id = session.nextFrameId();
-        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();
-    };
 
     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
@@ -1024,42 +1069,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.
@@ -3036,6 +3053,7 @@ const NavigationPriority = enum {
     form,
     script,
     anchor,
+    iframe,
 };
 
 pub const QueuedNavigation = struct {
@@ -3043,6 +3061,7 @@ pub const QueuedNavigation = struct {
     url: [:0]const u8,
     opts: NavigateOpts,
     priority: NavigationPriority,
+    is_about_blank: bool,
 };
 
 pub fn triggerMouseClick(self: *Page, x: f64, y: f64) !void {

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,12 @@ const Session = @This();
 browser: *Browser,
 notification: *Notification,
 
+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,
 
@@ -70,6 +77,8 @@ pub fn init(self: *Session, browser: *Browser, notification: *Notification) !voi
         .navigation = .{ ._proto = undefined },
         .storage_shed = .{},
         .browser = browser,
+        .queued_navigation = .{},
+        .queued_queued_navigation = .{},
         .notification = notification,
         .cookie_jar = storage.Cookie.Jar.init(allocator),
     };
@@ -79,9 +88,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 +122,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 +142,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 +183,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 +239,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;
                 }
 
@@ -345,42 +355,147 @@ fn _wait(self: *Session, page: *Page, wait_ms: u32) !WaitResult {
     }
 }
 
-fn processScheduledNavigation(self: *Session, current_page: *Page) !*Page {
+pub fn scheduleNavigation(self: *Session, page: *Page) !void {
-    const browser = self.browser;
+    const list = &self.queued_navigation;
+
+    // Check if page is already queued
+    for (list.items) |existing| {
+        if (existing == page) {
+            // Already queued
+            return;
+        }
+    }
+
+    return list.append(self.arena, page);
+}
+
+fn processQueuedNavigation(self: *Session) !void {
+    const navigations = &self.queued_navigation;
+
+    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.
+        navigations.clearRetainingCapacity();
+        return self.processRootQueuedNavigation();
+    }
+
+    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;
+    const iframe = current_page.iframe.?;
+    const parent = current_page.parent.?;
 
-    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: {
+    errdefer iframe._content_window = null;
-        const page = &self.page.?;
-        const frame_id = page._frame_id;
-        const parent = page.parent;
 
-        browser.http_client.abort();
+    if (current_page._parent_notified) {
-        self.removePage();
+        // we already notified the parent that we had loaded
+        parent._pending_loads += 1;
+    }
 
-        break :blk .{ frame_id, parent };
+    const frame_id = current_page._frame_id;
-    };
+    defer current_page.deinit(true);
 
-    self.page = @as(Page, undefined);
+    const new_page = try parent.arena.create(Page);
-    const page = &self.page.?;
+    try Page.init(new_page, frame_id, self, parent);
-    try Page.init(page, frame_id, self, parent);
+    errdefer new_page.deinit(true);
 
-    // Creates a new NavigationEventTarget for this page.
+    new_page.iframe = iframe;
-    try self.navigation.onNewPage(page);
+    iframe._content_window = new_page.window;
 
-    // start JS env
+    new_page.navigate(qn.url, qn.opts) catch |err| {
-    // Inform CDP the main page has been created such that additional context for other Worlds can be created as well
+        log.err(.browser, "queued frame navigation error", .{ .err = err });
-    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;
     };
 
-    return page;
+    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.?;
+    current_page._queued_navigation = null;
+    defer self.browser.arena_pool.release(qn.arena);
+
+    self.removePage();
+    self.page = @as(Page, undefined);
+    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(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, new_page);
+
+    new_page.navigate(qn.url, qn.opts) catch |err| {
+        log.err(.browser, "queued navigation error", .{ .err = err });
+        return err;
+    };
 }
 
 pub fn nextFrameId(self: *Session) u32 {

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);
+        caught.* = try_catch.caughtOrError(local.call_arena, error.JsException);
-        return error.JSExecCallback;
+        return error.JsException;
     };
 
     if (@typeInfo(T) == .void) {

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 };
 }
 

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 @@
   }
 </script>
 
-<script id=onload>
+<script id=about_blank>
   {
     let f4 = document.createElement('iframe');
+    f4.id = 'f4';
     f4.src = "about:blank";
     document.documentElement.appendChild(f4);
 
@@ -87,8 +89,43 @@
   }
 </script>
 
+<script id=about_blank_renavigate>
+  {
+    let f5 = document.createElement('iframe');
+    f5.id = 'f5';
+    f5.src = "support/sub 1.html";
+    document.documentElement.appendChild(f5);
+    f5.src = "about:blank";
+
+    testing.eventually(() => {
+      testing.expectEqual("<html><head></head><body></body></html>", f5.contentDocument.documentElement.outerHTML);
+    });
+  }
+</script>
+
+<!--
+Need correct _target support
+<script id=link_click>
+  {
+    let f6 = document.createElement('iframe');
+    f6.id = 'f6';
+    f6.addEventListener('load', () => {
+      f6.contentDocument.querySelector('#link').click();
+    }, {once: true});
+    f6.src = "support/with_link.html";
+    document.documentElement.appendChild(f6);
+
+    testing.eventually(() => {
+      console.warn(f6.contentDocument);
+      testing.expectEqual("<html><head></head><body></body></html>", f6.contentDocument.documentElement.outerHTML);
+    });
+  }
+</script>
+ -->
+
 <script id=count>
   testing.eventually(() => {
-    testing.expectEqual(4, window.length);
+    testing.expectEqual(5, window.length);
   });
 </script>
+

src/browser/tests/frames/support/after_link.html

@@ -0,0 +1,2 @@
+<!DOCTYPE html>
+It was clicked!

src/browser/tests/frames/support/with_link.html

@@ -0,0 +1,2 @@
+<!DOCTYPE html>
+<a href="after_link.html" id=link>a link</a>

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;