From 063b0bbc91c06b40702247daf2ba5c0c74e82d0d Mon Sep 17 00:00:00 2001
From: Qiaochu Hu <110803307+hobostay@users.noreply.github.com>
Date: Wed, 22 Jul 2026 15:56:35 +0800
Subject: [PATCH] fix(extension): pass frame loaderId when waiting for
 navigation commit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

handleNavigate's `Page.frameNavigated` handler called
`acceptLifecycleEvent("commit", frameId)` without the frame's loaderId.
Once `Page.navigate` resolves, the loader guard always carries the new
loaderId, so `eventLoaderIsRelevant(undefined, guard)` rejected the
commit event outright.

The navigation still completed in the common case only because the
readyState probe short-circuited first. On cross-origin navigations the
probe returns null (the old execution context has been destroyed), so
the wait fell back to `frameNavigated` — which was then rejected — and
`wait_until="commit"` timed out for the full `timeout_ms` instead of
resolving on commit.

Pass `p.frame?.loaderId` through, matching the sibling `lifecycleEvent`
branches. Adds a regression test that makes the readyState probe fail
and asserts commit is reached from the `frameNavigated` event; it fails
(reached "timeout") on the previous code.

Co-Authored-By: Claude <noreply@anthropic.com>
---
 .../src/tools/__tests__/navigation.test.ts    | 44 +++++++++++++++++--
 apps/extension/src/tools/navigation.ts        |  4 +-
 2 files changed, 43 insertions(+), 5 deletions(-)

diff --git a/apps/extension/src/tools/__tests__/navigation.test.ts b/apps/extension/src/tools/__tests__/navigation.test.ts
index d59a8dd..f2a6bbe 100644
--- a/apps/extension/src/tools/__tests__/navigation.test.ts
+++ b/apps/extension/src/tools/__tests__/navigation.test.ts
@@ -49,6 +49,8 @@ function makeFakeCdp(opts?: {
   beforeLoaderId?: string;
   readyState?: string;
   readyStateSequence?: string[];
+  /** Make the readyState probe's `Runtime.evaluate` throw (returns null). */
+  evaluateThrows?: boolean;
 }) {
   const readyStates = opts?.readyStateSequence ?? (opts?.readyState ? [opts.readyState] : []);
   let readyStateIdx = 0;
@@ -94,7 +96,10 @@ function makeFakeCdp(opts?: {
       return {};
     },
     "Runtime.enable": () => ({}),
-    "Runtime.evaluate": () => ({ result: { value: nextReadyState() } }),
+    "Runtime.evaluate": () => {
+      if (opts?.evaluateThrows) throw new Error("Cannot execute in a destroyed execution context");
+      return { result: { value: nextReadyState() } };
+    },
     "Page.getNavigationHistory": () => ({
       currentIndex: opts?.historyIndex ?? 1,
       entries: opts?.historyEntries ?? [
@@ -168,8 +173,13 @@ function makeFakeCdp(opts?: {
       const payload = { name, frameId, loaderId };
       for (const listener of [...events]) listener({ tabId: 4 }, "Page.lifecycleEvent", payload);
     },
-    fireFrameNavigated(frameId = opts?.navigateFrameId ?? "frame-1") {
-      const payload = { frame: { id: frameId, url: opts?.finalUrl ?? "https://example.com/" } };
+    fireFrameNavigated(
+      frameId = opts?.navigateFrameId ?? "frame-1",
+      loaderId = opts?.navigateLoaderId ?? "loader-after",
+    ) {
+      const payload = {
+        frame: { id: frameId, loaderId, url: opts?.finalUrl ?? "https://example.com/" },
+      };
       for (const listener of [...events]) listener({ tabId: 4 }, "Page.frameNavigated", payload);
     },
     listeners: events,
@@ -457,6 +467,34 @@ describe("handleNavigate", () => {
     expect(res.reached).toBe("commit");
     expect(fake.listeners.length).toBe(0);
   });
+
+  it("resolves wait_until=commit from Page.frameNavigated when the readyState probe fails", async () => {
+    const sm = new SessionManager({ agentWindow: fakeAgentWindow([100]) });
+    await sm.start("aa11");
+    // Cross-origin-style navigation: the readyState probe can't read the
+    // (already destroyed) old execution context, so commit must come from
+    // the Page.frameNavigated event. Before the fix this timed out because
+    // the handler dropped the frame's loaderId and the loader guard then
+    // rejected the event.
+    const fake = makeFakeCdp({ evaluateThrows: true });
+    const navP = handleNavigate(
+      sm,
+      {
+        session_id: "aa11",
+        url: "https://example.com/",
+        wait_until: "commit",
+        timeout_ms: 1_000,
+      },
+      { cdp: fake.cdp, tabsApi: fake.tabsApi },
+    );
+
+    await new Promise((r) => setTimeout(r, 5));
+    fake.fireFrameNavigated("frame-1", "loader-after");
+    const res = await navP;
+    if ("code" in res) throw new Error(`unexpected error: ${JSON.stringify(res)}`);
+    expect(res.reached).toBe("commit");
+    expect(fake.listeners.length).toBe(0);
+  });
 });
 
 describe("handleNavigateBack / Forward", () => {
diff --git a/apps/extension/src/tools/navigation.ts b/apps/extension/src/tools/navigation.ts
index f12e389..5032e48 100644
--- a/apps/extension/src/tools/navigation.ts
+++ b/apps/extension/src/tools/navigation.ts
@@ -325,14 +325,14 @@ function startLifecycleWait(
           if (source.tabId !== expectedTabId) return;
 
           if (targetName === "commit" && method === "Page.frameNavigated") {
-            const p = params as { frame?: { id?: string; parentId?: string } };
+            const p = params as { frame?: { id?: string; parentId?: string; loaderId?: string } };
             const expectedFrameId = currentFrameId();
             if (expectedFrameId.length > 0) {
               if (p.frame?.id !== expectedFrameId) return;
             } else if (p.frame?.parentId) {
               return;
             }
-            if (!acceptLifecycleEvent("commit", p.frame?.id)) return;
+            if (!acceptLifecycleEvent("commit", p.frame?.id, p.frame?.loaderId)) return;
             finish({ reached: "match", lastLifecycle: "commit" });
             return;
           }