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

@@ -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(
+    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,
     });
 
+    // 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| {
+    parent.iframeCompletedLoading(self.iframe.?);
-        p.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: {
+    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
@@ -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 {

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 browser = self.browser;
+    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", .{});
 
-    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 parent = current_page.parent.?;
-        const page = &self.page.?;
+        errdefer iframe._content_window = null;
-        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;