Skip to content

fix(extension): resolve wait_until=commit when the readyState probe fails#34

Open
hobostay wants to merge 1 commit into
Tencent:mainfrom
hobostay:fix/navigate-commit-frame-loader-id
Open

fix(extension): resolve wait_until=commit when the readyState probe fails#34
hobostay wants to merge 1 commit into
Tencent:mainfrom
hobostay:fix/navigate-commit-frame-loader-id

Conversation

@hobostay

Copy link
Copy Markdown

Problem

handleNavigate with wait_until: "commit" can time out for the full timeout_ms instead of resolving on commit, on cross-origin navigations.

The Page.frameNavigated handler called acceptLifecycleEvent without the frame's loaderId:

if (targetName === "commit" && method === "Page.frameNavigated") {
  const p = params as { frame?: { id?: string; parentId?: string } };
  ...
  if (!acceptLifecycleEvent("commit", p.frame?.id)) return;   // no loaderId
  ...
}

Once Page.navigate resolves, the loader guard always carries the new loaderId, so eventLoaderIsRelevant(undefined, guard) hits the expected.length > 0 && !eventLoaderId branch and rejects the commit event outright (every time).

This was masked in the common case because the document.readyState probe short-circuits first (lifecycleAlreadyReached("loading" | "interactive", "commit") is true). But on a cross-origin navigation the probe returns null — the old execution context is destroyed, so the Runtime.evaluate throws — and the wait falls back to Page.frameNavigated, which is then rejected. Result: the commit wait runs to timeout_ms (default 30s).

Fix

Pass the frame's loaderId through, exactly as the sibling Page.lifecycleEvent branches already do (navigation.ts lines that call acceptLifecycleEvent(p.name, p.frameId, p.loaderId)):

const p = params as { frame?: { id?: string; parentId?: string; loaderId?: string } };
...
if (!acceptLifecycleEvent("commit", p.frame?.id, p.frame?.loaderId)) return;

Test

Adds a regression test that makes the readyState probe throw (returns null, simulating the destroyed old execution context of a cross-origin nav) and asserts wait_until="commit" is reached from the Page.frameNavigated event. This test fails on the previous code (reached: "timeout") and passes with the fix. The existing commit test is unaffected (it still resolves via the probe).

Validation

Frontend CI gates run locally against this branch:

  • pnpm ext:test — 388 passed ✅
  • pnpm --filter @browser-skill/extension compile (tsc --noEmit) ✅
  • pnpm lint (biome + stylelint) ✅
  • pnpm ext:build

🤖 Generated with Claude Code

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant