Skip to content

Share the Chrome launcher and retarget CdpClient onto deno-cri - #14835

Draft
cwickham wants to merge 6 commits into
feat/axe-scan-commandfrom
refactor/shared-chrome-launcher
Draft

Share the Chrome launcher and retarget CdpClient onto deno-cri#14835
cwickham wants to merge 6 commits into
feat/axe-scan-commandfrom
refactor/shared-chrome-launcher

Conversation

@cwickham

@cwickham cwickham commented Aug 31, 2026

Copy link
Copy Markdown
Member

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.

  1. Launcher — start the process: flags, profile dir, wait for the CDP
    port, kill on exit.
  2. Transport — a generic pipe to the running browser. It sends any CDP
    command, awaits the result, subscribes to events, and closes.
  3. Task logic — which commands to send, in what order, with what
    recovery. For mermaid that is the cri.ts facade. For the scanner it is
    scanCell.

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.ts is a third, puppeteer-based
launch path — withHeadlessBrowser, reached through
withPuppeteerBrowserAndPage and inPuppeteer. Nothing outside that file
enters 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 new src/core/cri/launch.ts starts headless Chrome
for both callers. It owns the flag set, the QUARTO_CHROMIUM_HEADLESS_MODE
escape hatch, the optional throwaway --user-data-dir, stderr draining, exit
cleanup, and the wait for the CDP endpoint.

The two old launchers differed in six ways. Three are now unified and three
are passed in:

Difference Was Now
--renderer-process-limit=1 cri only passed in by cri
--hide-scrollbars scanner only passed in by the scanner
throwaway --user-data-dir scanner only passed in by the scanner
stdout cri piped it and never read it "null" for both
stderr cri read it once after a failure, the scanner drained it drained for both
exit cleanup cri used registerForExitCleanup, the scanner used onCleanup registerForExitCleanup for both

The three unifications each fix something in the failure path:

  • An unread stdout pipe is a way for Chrome to block on a full buffer.
  • cri's old error path could hang. It awaited cmd.status for a Chrome that
    was running happily but had not opened the port, and it asserted that one
    read had drained the whole stderr pipe.
  • onCleanup handlers cannot be unregistered, so a mermaid-heavy render grew
    the 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 installed
the handler that kills its own registry — only execProcess() did. A command
that spawns a browser and never shells out could therefore register a process
and still orphan it on Ctrl-C. quarto call axe is exactly that command,
which is why it used onCleanup directly. registerForExitCleanup() now
installs 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

CdpClient in scan.ts kept its own WebSocket internals: message framing, an
id counter, a pending map, a listener registry. That was the third copy of
that machinery in the tree, after deno-cri and whatever Chrome runs on the
other end. Those internals are now the vendored deno-cri
(src/core/cri/deno-cri/), the same client cri.ts connects 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 can
    stop caring about a load event that belongs to it.
  • Close rejects everything still in flight.

deno-cri does not do the third one. It notices a dropped socket and emits
disconnect, then leaves every in-flight command unsettled forever. So
CdpClient tracks in-flight sends itself and rejects them on close or on
disconnect. A crashed tab fails its own cell inside --timeout instead of
hanging the scan.

Target discovery went with it: deno-cri picks the page target, and creates
one if the browser has none (#4653), so scan.ts's own /json/list polling
is gone. The launcher's wait for the endpoint is the only wait left.
Connecting retries five times at 100ms, the interval cri.ts measured its way
to.

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-cri
parses inside the socket's onmessage handler, so an unparseable frame exits
the process. Chrome does not send such frames, and guarding it would mean
patching deno-cri/chrome.js for a case never observed, so it is left alone.

Known cost: connecting through deno-cri fetches /json/protocol to
build its domain shorthands. That is roughly 1 MB of JSON, once per scan, on a
run that already takes minutes. cri.ts has paid it per render for years.

Explicitly out of scope

  • cri.ts exporting a typed transport as a core surface. That is the cleaner
    end state, and it waits on the scanner's needs settling — concurrent tabs in
    particular.
  • Any change to mermaid's facade behaviour.
  • Any change to the vendored deno-cri sources.

Verification

  • 147 axe unit tests and 9 tests/smoke/axe/ smoke tests pass, at
    every commit that touches them.

  • 3 mermaid smoke tests pass (mermaid-svg-docx, mermaid-gfm-svg,
    mermaid-multi-diagram).

  • A mermaid-format: png render produces a byte-identical PNG before and
    after (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, and
    all 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.md already
    documents.

  • 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 command
    the 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 as hung,
    so a transport that never settles fails rather than passes. Mutation-checked:
    reverting abandonPending to deno-cri's own behaviour fails it with
    hung: nothing settled within 15000ms. Runs in ~0.7s.

  • deno check src/quarto.ts and deno lint are clean on every changed file.

Checklist

I have (if applicable):

  • referenced the GitHub issue this PR closes — no issue. This answers a
    review thread on Add quarto call axe, a hidden experimental site accessibility scanner #14815 (linked above)
  • updated the appropriate changelog in the PR — no user-visible change
  • ensured the present test suite passes
  • added new tests — axe-transport-failclosed.test.ts, covering the half
    a stubbed client cannot: that a real CdpClient rejects at all when the
    connection goes
  • created a separate documentation PR

llm-docs/axe-scan-architecture.md is updated: its scan-stage section
described two launchers and a hand-rolled WebSocket, with both of these
changes listed as future work.

AI-assisted PR
  • AI tool used: Claude Code
  • Codebase grounding: local clone
  • Human review: I have reviewed, tested, and verified the AI-generated content before submitting.

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.
@posit-snyk-bot

posit-snyk-bot commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 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
cwickham force-pushed the refactor/shared-chrome-launcher branch from 5d608be to 142f0f9 Compare August 31, 2026 22:50
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.
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.

2 participants