Fix frame-scoped ref resolution (playwright-core ≥1.62), SPA navigation stall, and Chrome process leaks on crash - #144
Conversation
…hrome leaks
Three bugs found while integrating browserclaw into an agent loop, all
reproduced end-to-end and fixed:
1. Default aria snapshot mode broken with playwright-core >= 1.62.
Since 1.62, the main frame emits frame-scoped refs (f1e5) after any
navigation past the first — and launch() always navigates. The snapshot
parser only recognized [ref=eN], so it appended duplicate generated
markers ([ref=e1] [ref=f1e5]) and actions resolved via aria-ref=e1,
matching nothing and timing out after 8s. Ref parsing, resolution, and
ref counters now accept both shapes; refs are preserved verbatim.
2. First navigation could stall SPA pages indefinitely. The SSRF
navigation guard installs page.route('**') and removed it right after
goto(waitUntil: 'commit') resolves — while the document is still
streaming. Unrouting at that point strands requests paused at the CDP
Fetch layer; a parser-blocking script never completes, the document
freezes mid-body, and load never fires (reproduced 5/6 runs on
demo.playwright.dev/todomvc; 0/6 after the fix). The route teardown is
now deferred until the page reaches load (or a bounded grace period),
without delaying the navigation result.
3. Crashed runs leaked headless Chrome processes and isolated profiles.
launch()-owned Chrome is now registered with a process-exit hook
(plus SIGINT/SIGTERM/SIGHUP handlers that defer to host-app handlers)
so it dies with the Node process. New keepAliveOnExit launch option
opts out for keep-alive workflows. connect()-attached browsers are
never killed.
Also: acting on a ref absent from the latest aria snapshot now throws the
typed StaleRefError immediately instead of a generic 8s timeout, and
toAIFriendlyError passes typed errors through instead of rewrapping them.
The lockfile bump to playwright-core 1.62.1 makes CI exercise the version
that a fresh install of the declared ^1.50.0 range actually resolves to;
the suite still passes against 1.58.2.
Co-Authored-By: Claude Fable 5 <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3de6479f16
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }, | ||
| "engines": { | ||
| "node": ">=18" | ||
| "node": ">=20" |
There was a problem hiding this comment.
With this lockfile entry, the repo now installs [email protected], whose own engine requires Node >=20, while the root package.json still advertises support for Node >=18. In Node 18 environments that honor package engines (for example with engine-strict enabled), npm ci will reject the dependency even though this package claims that runtime is supported; either raise the package/CI engine to Node 20 or lock a Playwright version that still supports Node 18.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 91501f7. The lockfile now pins playwright-core 1.61.1, the newest version whose engines (>=18) match this package's advertised Node >=18 (the 1.62 line requires Node >=20). The frame-scoped ref format this PR fixes landed in 1.62, so it's covered by unit tests with 1.62-format fixtures, and the full suite + live-browser E2E also pass against 1.62.1 locally. One thing for the maintainer to consider separately: a fresh npm install browserclaw on Node 18 will still resolve ^1.50.0 to 1.62.x (npm only warns on engine mismatch), so a range cap like <1.62 — or an engines bump to >=20 — may be worth a follow-up decision.
| running.proc.once('exit', () => { | ||
| liveLaunchedChromes.delete(running); | ||
| }); |
There was a problem hiding this comment.
When an isolated Chrome exits or crashes before the caller reaches stop() and the Node process later exits, this listener removes the instance from liveLaunchedChromes, so killLaunchedChromesSync() no longer sees it and never removes running.userDataDir. That leaves isolated temp profiles behind despite the new default exit cleanup; either clean isolated profiles in this proc exit path or keep a separate cleanup registry for them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 91501f7 — the proc.once('exit') listener now removes the isolated profile directory itself when Chrome dies on its own, so the directory is cleaned at the earliest correct moment instead of relying on the process-exit hook that can no longer see the instance. Redundant with stopChrome()'s cleanup on the normal path (rmSync with force is a no-op on an already-removed directory). Unit test added.
| // Defer the teardown until the page reaches `load` (or a bounded grace | ||
| // period), without delaying the navigation result. | ||
| void opts.page | ||
| .waitForLoadState('load', { timeout: Math.max(opts.timeoutMs, 10_000) }) |
There was a problem hiding this comment.
Because the broad page.route('**') handler now remains registered after goto() has already returned at commit, any request that fires before load (or until this timeout on pages that never load) is still handled by browserclaw's route.continue(). Playwright gives the most recently registered matching route precedence and continue() does not fall through to older handlers, so callers that have their own page/context routes for XHRs or other resources can see those routes silently bypassed after navigate() returns; use fallback() for requests the guard does not own, or otherwise avoid keeping the terminal continue() handler installed beyond the guarded navigation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 91501f7 — the guard handler now calls route.fallback() instead of route.continue() for requests it doesn't block, so handlers registered by the embedding application are deferred to rather than finalized past. When no other handler exists, Playwright continues the request, so the stall fix is unaffected — re-verified 6/6 SPA mounts and that the SSRF abort path still blocks metadata/private addresses end-to-end.
…oute.fallback() - Pin the lockfile to playwright-core 1.61.1 — the newest version whose engines (>=18) match this package's advertised Node >=18. The 1.62 line requires Node >=20; its frame-scoped ref format remains covered by unit tests, and the full suite also passes against 1.62.1 locally. The declared ^1.50.0 range is unchanged. - When an isolated Chrome exits on its own before stop(), remove its profile directory in the proc exit listener — previously the instance left the exit-hook registry at that point and the directory leaked. - Use route.fallback() instead of route.continue() in the navigation guard so handlers registered by the embedding application are deferred to rather than silently bypassed, now that the guard route stays installed until the page reaches load. - interaction.ts: replace locator.elementHandle() + null check with locator.evaluate() — elementHandle()'s return type is nullable in playwright-core <1.62 and non-null in 1.62+, so no single null check lints clean against both; evaluate() is equivalent and version-stable. Co-Authored-By: Claude Fable 5 <[email protected]>
1 participant
Found while embedding browserclaw in a real agent loop. Three independent bugs, each reproduced end-to-end before fixing and verified after. Happy to split into separate PRs if you prefer.
1. Default
ariamode is broken with playwright-core ≥ 1.62 (what a fresh install gets)^1.50.0resolves to 1.62.x today. Since 1.62, Playwright's AI snapshot emits frame-scoped refs for the main frame (f1e5) after any navigation past the first — andlaunch()always navigates (about:blank→ target URL), so this hits immediately.Repro (plain playwright-core over CDP, no browserclaw):
buildRoleSnapshotFromAiSnapshotonly recognized[ref=eN], so with 1.62 every line got a duplicate generated marker (link "Docs" [ref=e1] [ref=f1e3]), and acting one1resolved viaaria-ref=e1— which matches nothing → every action failed with an 8s timeout.Fix: ref parsing/resolution/counters accept both
eNandfMeN(aria-ref=f1e5has been a valid selector since well before 1.50); Playwright refs are preserved verbatim. Unit tests cover both shapes; the suite passes against both 1.58.2 and 1.62.1. The lockfile bump makes CI exercise what the declared range actually installs.2. SSRF navigation guard can freeze SPA pages mid-load
gotoPageWithNavigationGuardinstallspage.route('**'), awaitsgoto(waitUntil: 'commit'), then immediately unroutes — while the document is still streaming and subresources are in flight. Unrouting at that point strands requests paused at the CDP Fetch layer: a parser-blocking script never completes, the document freezes mid-body (readyStatestuck onloading), andloadnever fires.Isolated repro on
demo.playwright.dev/todomvc(headless, 6 rounds each):**) + goto(commit) + immediate unrouteloadFix: navigation still resolves at
commit(unchanged semantics), but the route teardown is deferred in the background until the page reachesloador a bounded grace period. Permanent routing was rejected because Playwright disables the HTTP cache while interception is active. Verified 0/6 stalls through the public API after the fix, and the SSRF block itself still works (metadata endpoint navigation still refused,navigation.secure-defaulttests pass).3. Crashed runs leak headless Chrome processes + isolated profiles
If the Node process dies before
stop()— an uncaught exception is enough — the launched Chrome stays alive forever (I accumulated several invisible headless Chromes within an hour of normal development).Fix:
launch()-owned Chromes are registered with aprocess.once('exit')hook (SIGKILL — exit handlers must be synchronous; stale singleton locks are already handled by the existing stale-lock recovery) plus SIGINT/SIGTERM/SIGHUP handlers that only act when no other listener exists, so host apps that own their shutdown sequence are not preempted. Isolated profile dirs are removed best-effort. NewkeepAliveOnExit: truelaunch option opts out for keep-alive workflows;connect()-attached browsers are never touched.Verified end-to-end: child process crash → Chrome dead; SIGINT → Chrome dead;
keepAliveOnExit: true→ Chrome survives.Also included
StaleRefErrorimmediately instead of a generic 8s timeout (toAIFriendlyErrornow passes typed errors through instead of rewrapping them). This also means the README's error-handling example (catch StaleRefError → re-snapshot → retry) actually works in aria mode.interaction.ts: dropped a null check that playwright-core 1.62's types flag (Locator.elementHandle()is non-nullable now) — lint was failing on it with the updated lockfile.Testing
npm test: 633 passing (628 before; new coverage for frame-scoped ref parsing/resolution, ref counters, exit cleanup) — green on playwright-core 1.62.1 and 1.58.2npm run lint,typecheck,build,check-exports: cleanf1eNrefs, 6/6 SPA mounts, SSRF still enforced, and the three process-leak scenarios above🤖 Generated with Claude Code