Skip to content

Bug 2065546 - [firefox-devtools-mcp] navigate should accept an option to wait for load - #167

Open
shoemoney wants to merge 1 commit into
mozilla:mainfrom
shoemoney:bug-2065546-navigate-wait
Open

Bug 2065546 - [firefox-devtools-mcp] navigate should accept an option to wait for load#167
shoemoney wants to merge 1 commit into
mozilla:mainfrom
shoemoney:bug-2065546-navigate-wait

Conversation

@shoemoney

Copy link
Copy Markdown
Contributor

navigate derives its readiness state from the URL scheme and gives the caller no say:

// Default wait time is "interactive" (DOMContentLoaded).
// All uncommon schemes use wait time "none"
const wait = isCommonScheme(url) ? 'interactive' : 'none';

So an agent cannot tell a page that has finished loading from one still fetching subresources — which is exactly the distinction Bug 2065546 needs for "when is it safe to stop the performance recording".

This adds an optional wait to navigate_page and new_page, exposing the three BiDi readiness states. Omitting it keeps today's behaviour exactly, so nothing changes for existing callers.

{ "url": "https://example.com", "wait": "complete" }

Two decisions worth flagging

An explicit wait is honoured for every scheme, not just common ones. The scheme heuristic only applies when no override is given. Silently downgrading complete to none because the URL happened to be moz-extension: would discard what the caller asked for with no way to tell — the same failure mode this bug is about. There is a test for it.

An unknown value is rejected, not ignored. wait: "load" is the obvious guess (it is the DOM event name, and the wire value is complete), and on main an unrecognised argument is silently dropped and navigate_page reports success:

[0] → https://example.com          ← main: "load" ignored, looks like it worked
wait must be one of none, interactive, complete (got "load")   ← this PR

The tool response also echoes the state back ([0] → https://example.com (waited for: complete)) but only when one was explicitly requested, so the default output is unchanged.

new_page gets the same argument because createNewPage calls navigate directly — an agent opening a tab has the same need. Happy to drop that half if you would rather keep the change to navigate_page alone.

Verified against real Firefox

Local server, document commits immediately, <img> subresource stalls 3s. Median of 5 runs after 2 warm-up loads:

wait time document.readyState
omitted (default) 661ms interactive
'none' 524ms interactive
'interactive' 327ms interactive
'complete' 3220ms complete

complete blocks on the stalled subresource; the default is unchanged. Separating none from interactive needs a streamed document rather than a stalled subresource — with a flushed 64KB first chunk and the body trickling for 3s, none returns at 1251ms (commit) while interactive waits 3044ms for DOMContentLoaded. Worth knowing that none returns on navigation commit, not instantly.

A limit you should know about before taking this

wait: "complete" is capped at 10 seconds by the global BiDi command timeout at src/firefox/bidi.ts:61. On a page whose load fires later than that, navigate throws rather than waiting:

wait:interactive  OK after 1092ms   readyState=interactive
wait:complete     THREW after 10004ms  -> BiDi command timeout: browsingContext.navigate

(Measured with a 12s-stalled subresource.) This is pre-existing and applies to every BiDi command, but complete will hit it far more often than interactive does, and heavy real-world pages — ads, third-party trackers, video — routinely take longer than 10s to fire load.

I have not changed that timeout. Making it configurable, or special-casing navigate, is a decision about the BiDi layer that affects every command, and it is not mine to make inside a feature PR. Happy to follow up in whichever shape you prefer — a per-command override, a longer default for navigate, or leaving it and documenting the ceiling.

Tests

12 new unit tests across tests/firefox/pages.test.ts and tests/tools/pages.test.ts. All 12 fail on unmodified main:

× isReadinessState > accepts the three BiDi readiness states
× isReadinessState > rejects anything else, including the DOM event name "load"
× PageManagement > navigate > honours an explicit wait for common schemes
× PageManagement > navigate > honours an explicit wait for uncommon schemes rather than downgrading it
× PageManagement > navigate > allows an explicit wait to opt out of waiting on a common scheme
× PageManagement > createNewPage > forwards an explicit wait override to navigate
× navigatePageTool should expose an optional wait enum
× newPageTool should expose an optional wait enum
× passes an explicit wait through to navigate
× leaves the default in place when wait is omitted
× passes an explicit wait through to new_page
× rejects an unknown wait value instead of silently ignoring it

No integration test, deliberately. I wrote one, and it passed on its own and alongside the other integration files — but under the full vitest run it intermittently failed with BiDi command timeout: browsingContext.navigate, because wait: "complete" holds a BiDi call open for the length of the stall and the suite was already contending for resources. Any stall long enough to reliably separate complete from interactive is long enough to risk that timeout under load. Given Bug 2036018 is already open about intermittent Firefox CI failures, adding a new source of flake seemed clearly worse than proving the behaviour with deterministic unit tests plus the measurements above. Say the word if you want it anyway and I will add it back.

npm run lint            clean
npm run format:check    clean
npm run typecheck       clean
npm run typecheck:tests clean
npm run build           clean
npx vitest run          50 files, 662 tests passed

… to wait for load

navigate always used the scheme-derived readiness state: "interactive"
(DOMContentLoaded) for http/https/data/blob/file, "none" for everything
else. An agent had no way to wait for the load event, so it could not tell
a page that had finished loading from one still fetching subresources —
which matters when deciding it is safe to stop a performance recording.

Add an optional `wait` argument to navigate_page and new_page, exposing the
three WebDriver BiDi readiness states. Omitting it keeps the existing
scheme-derived default, so nothing changes for callers that do not pass it.

An explicit value is honoured for every scheme rather than being downgraded
for uncommon ones, and an unknown value is rejected instead of ignored —
silently falling back to the default would leave the caller believing it
waited for something it did not.
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