Share the Chrome launcher and retarget CdpClient onto deno-cri - #14835
Draft
cwickham wants to merge 6 commits into
Draft
Share the Chrome launcher and retarget CdpClient onto deno-cri#14835cwickham wants to merge 6 commits into
cwickham wants to merge 6 commits into
Conversation
Two subsystems launch Chrome over CDP — criClient (mermaid) and the axe scanner — and their launch halves had drifted into near-duplicates of each other: the same headless-mode escape hatch, the same flag set, the same "Chrome never exits on its own" kill, the same wait-for-the-CDP-port poll. Two copies of "how quarto starts Chrome" is one copy too many; the cri.ts comment warning readers to sync flag changes by hand was the interim fix. launchChrome() in src/core/cri/launch.ts is now the single launcher. It owns the flags, QUARTO_CHROMIUM_HEADLESS_MODE, the optional throwaway profile dir, stderr draining, exit cleanup, and the wait loop; callers pass in only what they genuinely disagree about (--renderer-process-limit=1 for mermaid, --hide-scrollbars and an isolated profile for the scanner). No caller uses it yet — the two switches follow. registerForExitCleanup() now installs the handler that actually kills the registry. It never did: only execProcess() installed it, so a command that spawns a browser and never shells out could register a process and still orphan it on Ctrl-C. The axe scanner is exactly that command, and it used onCleanup() directly to work around this.
criClient keeps its mermaid-shaped facade (navigate / querySelector / screenshot) and its deno-cri connection; only the spawn half moves out to launchChrome(). --renderer-process-limit=1 is passed in, since one diagram is rendered at a time. Three things change as a side effect of using the shared code path, all in the failure direction only: - stdout is no longer piped. Nothing ever read it, and an unread pipe is a way for Chrome to block on a full buffer. - stderr is drained to the debug log as it arrives, instead of being read once after a failed wait. The old path could hang: it awaited `cmd.status` for a Chrome that was running happily but had not opened the port, and asserted that a single read had drained the whole pipe. - the wait for the CDP endpoint goes from 3s to 15s (the shared default). This only lengthens how long a genuinely broken launch takes to report; a healthy Chrome still returns as soon as the endpoint answers. Verified: a mermaid-format: png render produces a byte-identical PNG before and after (sha256 9ee2aef2...).
launchScanBrowser drops its own copy of the launch half — flags, headless mode, temp profile, stderr drain, kill-on-exit, wait loop — and calls launchChrome() instead. What stays here is what is genuinely scanner-specific: --hide-scrollbars, an isolated profile, and connecting the CDP client. Behaviour is unchanged, with one deliberate substitution: exit cleanup now goes through registerForExitCleanup() rather than onCleanup() directly, so the kill handler is unregistered once the browser has been closed cleanly instead of staying on the cleanup list for the life of the process. Tests: all 147 axe unit tests and all 8 tests/smoke/axe/ smoke tests pass.
Collaborator
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
CdpClient kept its own WebSocket internals — message framing, id counter, pending map, listener registry — which is the third copy of that machinery in the tree after deno-cri and whatever Chrome does on the other end. The typed interface stays exactly as it was (send<T>, cancellable once, close); only what sits under it changes, to the same deno-cri client cri.ts already uses. deno-cri does not do the one thing fail-closed cells depend on: it notices a dropped socket but leaves the commands that were in flight unsettled forever. So this tracks in-flight sends itself and rejects them on close or disconnect — a crashed tab fails its own cell inside --timeout rather than hanging the scan. The unit tests that stub this client cover exactly that behaviour and are unchanged. Target discovery goes with it: deno-cri picks the page target (and creates one if the browser has none), so scan.ts's own /json/list polling is gone, and the launcher's wait for the CDP endpoint is the only wait left. Connecting retries 5x100ms, the interval cri.ts measured its way to. One behaviour is not carried over: the old client logged and ignored a frame that did not parse as JSON, where deno-cri parses inside the socket's onmessage handler and an unparseable frame therefore exits the process. Chrome does not send such frames, and guarding it would mean patching vendored code for a case never observed, so it is left alone. Tests: 147 axe unit tests and 8 tests/smoke/axe/ smoke tests pass.
The scan-stage section still described two launchers and a hand-rolled WebSocket, with the shared launcher as follow-up work and the deno-cri retarget as a "plausible future". Both are done, so rewrite the section around the three layers the PR discussion settled on — launcher, transport, task logic — and say which are shared and which never will be. Both the section and launch.ts's own header say "the one place quarto's CDP drivers start headless Chrome", not "the one place quarto starts headless Chrome": src/core/puppeteer.ts launches through puppeteer instead (withHeadlessBrowser, reached through withPuppeteerBrowserAndPage and inPuppeteer). Nothing outside that file enters it today, but a maintainer chasing browser-launch behaviour should not be told it doesn't exist.
cwickham
force-pushed
the
refactor/shared-chrome-launcher
branch
from
August 31, 2026 22:50
5d608be to
142f0f9
Compare
The unit tests stub the CDP client, so they cover what scanCell does with a rejected send but not whether a real client rejects at all. That half now matters more than it did: rejecting in-flight commands used to fall out of owning the WebSocket, and is now CdpClient's own contribution on top of deno-cri, which notices a dropped socket and leaves those commands unsettled forever. Three cases against a real browser, each with a command the browser can never answer in flight: closing the client, sending after close, and the connection dropping from the far end (Browser.close, as the portable stand-in for a tab or process dying). Every wait has a deadline and a blown deadline reports as `hung`, so a transport that never settles fails the assertion instead of passing it. Checked by mutation: reverting abandonPending to deno-cri's own behaviour fails the test with `hung: nothing settled within 15000ms`. Runs in ~0.7s.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #14815 (
feat/axe-scan-command).Description
This is the follow-up promised in
#14815 (comment),
where @cderv asked why the axe scanner does not reuse
cri.ts.The short answer from that thread: a Chrome driver has three layers, and the
answer differs at each one.
port, kill on exit.
command, awaits the result, subscribes to events, and closes.
recovery. For mermaid that is the
cri.tsfacade. For the scanner it isscanCell.Layers 1 and 2 were duplicated. This PR removes both copies. Layer 3 stays
separate, because mermaid's commands and the scanner's commands are different
jobs. After this PR, no launcher code and no protocol code exists twice among
quarto's CDP drivers. (
src/core/puppeteer.tsis a third, puppeteer-basedlaunch path —
withHeadlessBrowser, reached throughwithPuppeteerBrowserAndPageandinPuppeteer. Nothing outside that fileenters it, so I left it alone and only made sure the new comments do not
claim it away.)
No user-visible change, so no changelog entry.
Part 1: one launcher
launchChrome()in the newsrc/core/cri/launch.tsstarts headless Chromefor both callers. It owns the flag set, the
QUARTO_CHROMIUM_HEADLESS_MODEescape hatch, the optional throwaway
--user-data-dir, stderr draining, exitcleanup, and the wait for the CDP endpoint.
The two old launchers differed in six ways. Three are now unified and three
are passed in:
--renderer-process-limit=1--hide-scrollbars--user-data-dir"null"for bothregisterForExitCleanup, the scanner usedonCleanupregisterForExitCleanupfor bothThe three unifications each fix something in the failure path:
cmd.statusfor a Chrome thatwas running happily but had not opened the port, and it asserted that one
read had drained the whole stderr pipe.
onCleanuphandlers cannot be unregistered, so a mermaid-heavy render grewthe cleanup list by one dead closure per file.
I left the throwaway profile dir as the scanner's own, and it is the one
decision I am least sure of. Giving mermaid an isolated profile is arguably
correct for the same reason it is correct here — a Chrome the user already
has running can otherwise short-circuit the launch. But mermaid renders sit
in the hot path of ordinary user renders, a fresh profile costs Chrome's
first-run work every time, and this PR is meant to remove duplication rather
than change mermaid. Happy to unify it if you disagree.
One more change came out of this.
registerForExitCleanup()never installedthe handler that kills its own registry — only
execProcess()did. A commandthat spawns a browser and never shells out could therefore register a process
and still orphan it on Ctrl-C.
quarto call axeis exactly that command,which is why it used
onCleanupdirectly.registerForExitCleanup()nowinstalls the handler itself.
One timing changed, in the failure direction only. cri's wait for the CDP
endpoint goes from 3s to the shared 15s, which only lengthens how long a
genuinely broken launch takes to report. A healthy Chrome still returns as
soon as the endpoint answers.
Part 2: one transport
CdpClientinscan.tskept its own WebSocket internals: message framing, anid counter, a pending map, a listener registry. That was the third copy of
that machinery in the tree, after
deno-criand whatever Chrome runs on theother end. Those internals are now the vendored
deno-cri(
src/core/cri/deno-cri/), the same clientcri.tsconnects with.The typed interface is unchanged, because the unit tests stub it and three
behaviours are what fail-closed cells depend on (all three now also covered
against a real browser — see Verification):
send<T>resolves with the command result, or rejects.once(method)returns{ event, cancel }, so a cell that timed out canstop caring about a load event that belongs to it.
deno-cridoes not do the third one. It notices a dropped socket and emitsdisconnect, then leaves every in-flight command unsettled forever. SoCdpClienttracks in-flight sends itself and rejects them on close or ondisconnect. A crashed tab fails its own cell inside
--timeoutinstead ofhanging the scan.
Target discovery went with it:
deno-cripicks the page target, and createsone if the browser has none (#4653), so
scan.ts's own/json/listpollingis gone. The launcher's wait for the endpoint is the only wait left.
Connecting retries five times at 100ms, the interval
cri.tsmeasured its wayto.
No vendored code is touched. One behaviour is not carried over: the old
client logged and ignored a frame that did not parse as JSON, where
deno-criparses inside the socket's
onmessagehandler, so an unparseable frame exitsthe process. Chrome does not send such frames, and guarding it would mean
patching
deno-cri/chrome.jsfor a case never observed, so it is left alone.Known cost: connecting through
deno-crifetches/json/protocoltobuild its domain shorthands. That is roughly 1 MB of JSON, once per scan, on a
run that already takes minutes.
cri.tshas paid it per render for years.Explicitly out of scope
cri.tsexporting a typed transport as a core surface. That is the cleanerend state, and it waits on the scanner's needs settling — concurrent tabs in
particular.
deno-crisources.Verification
147 axe unit tests and 9
tests/smoke/axe/smoke tests pass, atevery commit that touches them.
3 mermaid smoke tests pass (
mermaid-svg-docx,mermaid-gfm-svg,mermaid-multi-diagram).A
mermaid-format: pngrender produces a byte-identical PNG before andafter (sha256
9ee2aef2…).A full quarto-web scan: 416 pages found, 373 scanned, 1504 cells,
before and after. Identical finding set (the same 300 signature ids), the
same
{total: 300, new: 300, baselined: 0}, the same 12 not-ok cells, andall 1504 cell statuses identical. Timing is unchanged: 639s of cell time
before, 643s after, median cell 355ms in both.
What is not byte-identical is which cells six occurrences were attributed
to, on OJS tables, a leaflet map and a headroom navbar — all client-side
rendering. I ran the unchanged code a second time as a control, and those
two runs of the same code differ from each other in 21 attributions. So
the after-run sits inside the existing run-to-run noise, and this is the
client-side render race
llm-docs/axe-scan-architecture.mdalreadydocuments.
Fail-closed against a real browser is now a test rather than a manual
check:
tests/smoke/axe/axe-transport-failclosed.test.ts. With a commandthe browser can never answer in flight, closing the client, sending after
close, and the connection dropping from the far end all reject with
CDP connection closed. Every wait has a deadline that reports ashung,so a transport that never settles fails rather than passes. Mutation-checked:
reverting
abandonPendingto deno-cri's own behaviour fails it withhung: nothing settled within 15000ms. Runs in ~0.7s.deno check src/quarto.tsanddeno lintare clean on every changed file.Checklist
I have (if applicable):
review thread on Add
quarto call axe, a hidden experimental site accessibility scanner #14815 (linked above)axe-transport-failclosed.test.ts, covering the halfa stubbed client cannot: that a real
CdpClientrejects at all when theconnection goes
llm-docs/axe-scan-architecture.mdis updated: its scan-stage sectiondescribed two launchers and a hand-rolled WebSocket, with both of these
changes listed as future work.
AI-assisted PR