Skip to content

fix(fetch): scope subresource interception to a class set - #2704

Open
yepbro wants to merge 2 commits into
lexmount:mainfrom
yepbro:fix/fetch-interception-resource-class-set
Open

yepbro wants to merge 2 commits into
lexmount:mainfrom
yepbro:fix/fetch-interception-resource-class-set

Conversation

@yepbro

@yepbro yepbro commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Fixes #2703

Problem

Fetch.enable with heterogeneous resourceType patterns (e.g. Image + Media + Font) collapses in the protocol→renderer configuration channel to "intercept all classes" (fetch_subresource_interception_config_for_patterns early-returns (true, None)), so the renderer treats XHR as interceptable and the deliberate sync-XHR interception prohibition fires falsely: pages doing a synchronous XHR throw NetworkError: Failed to execute 'send' on 'XMLHttpRequest' and the request never reaches the server. The same collapse produces a false positive for the Script class in phase-one speculative-preload admission. Full analysis and repro in #2703.

Two implementation variants, why B

We considered two ways to fix this:

  • Variant A (narrow): keep the "one class or all" channel as is, and add a separate protocol-computed flag telling the renderer whether the XHR class is actually in interception scope; the sync-XHR ban decision reads the new flag. Smaller diff (~300–500 lines), lowest regression risk. Downside: the structurally wrong abstraction stays, the channel now carries two sources of truth that must be kept consistent through navigation seeds, workers and replay, and it only fixes the sync-XHR case — the same latent bug class (e.g. the phase-one Script false positive) remains.
  • Variant B (chosen): replace Option<SubresourceResourceType> ("one or all") with an exact set of intercepted classes — SubresourceInterceptionClasses in moli-page-types. Heterogeneous patterns scope the renderer precisely; enabled becomes constructively equivalent to !classes.is_empty() (after review the channel was collapsed to carry the bare set). Bigger mechanical diff (~37 files), but one source of truth, the whole class of false positives disappears at once, and the representation finally matches what Fetch.enable patterns actually are — arbitrary sets of resource types.

We picked B: A would have left the same latent failure mode open and duplicated state through the whole config channel; B makes the invariant constructive and matches the direction the codebase is already moving (shared admission, unified policy engines). The wider diff is mechanical (carrier plumbing), the semantic surface is smaller than A's.

What's changed

  • SubresourceInterceptionClasses (bitflags over the 12 CDP Fetch interception classes; Fetch/EventSource/XHR → FETCH_LIKE, Media/TextTrack → MEDIA, CspReport → CSP_VIOLATION_REPORT, Dictionary → OTHER, rest 1:1), built as the union of pattern classes instead of the collapse; carried bare (no separate enabled boolean) through the whole channel: fetch_owner/target_session_owner → RendererPageCommand::SetFetchSubresourceInterception { classes } → moli-core (settings, navigation engine seeds and ~10 builders) → moli-renderer-v8 (page VM, host environment, popups, dedicated/nested/shared/service workers, replay).
  • Widening to ALL preserved where the renderer must keep routing everything: type-less URL patterns (empty pattern list normalizes to one type-less pattern), auth interception (new auth_interception_can_match_any_url mirrors matches_auth_required — this also fixes a pre-existing silent auth scope cut for typed patterns), and response-stage candidates (union over patterns of both stages).
  • Sync-XHR ban (page + workers) now fires only when the XHR class is genuinely in scope; the ban itself is unchanged and still intentional (a paused sync request can never be continued on the blocked thread).
  • document.write speculative script-preload scanning and phase-one admission are now class-precise (intercepts(Script)).
  • Dead single-filter config helper removed (the Some(filter) branch became unreachable).

Observable behavior change (intentional): Fetch.enable{handleAuthRequests: true} combined with typed patterns now widens the renderer scope to ALL (auth matches by URL, not by class), so sync XHR is honestly banned for that configuration instead of silently bypassing auth interception. Noted in the commit message.

Testing

  • Unit: config construction (heterogeneous set ≠ ALL, type-less/empty → ALL, auth → ALL, negative auth branch keeps pattern classes), 17-variant mapping/ALL completeness, phase-one Script precision, negative auth-widening mirror.
  • Protocol integration: sync XHR passes with Image+Media+Font (200, zero Fetch.requestPaused) on page and workers; the ban still fires with a real XHR pattern; Fetch.disable leaves same-session Network/BiDi intercept config active.
  • Renderer: page-level sync-XHR test with both halves (heterogeneous set → honest network error surface, zero pending; ALL → the ban message) using a URL that passes the url-policy/bad-port gates; worker mirror.
  • moli-cdp-smoke fetch-font group (already the buggy heterogeneous configuration): new contract fetch_font_heterogeneous_sync_xhr — dual-engine (moli live binary + Chromium), sync XHR returns 200 with parseable body and no requestPaused.
  • Gates: cargo fmt --all, cargo clippy --workspace --all-targets --all-features -- -D warnings, cargo nextest run --no-fail-fast (workspace). Known pre-existing failures on clean main are unchanged: domains::emulation::…context_locale_override… (environment-dependent), timing flakes in moli-core/moli-renderer-v8 that pass in isolation.

Out of scope

  • True sync-XHR interception (continuation on an independent lane) — the ban stays.
  • Stylesheet/TextTrack in Fetch.enable patterns (tracked separately).

@yepbro

yepbro commented Nov 11, 2026

Copy link
Copy Markdown
Contributor Author

Heads-up on the failed check: the HEAD release build compiled and uploaded the artifact fine, but the final FinalizeArtifact call failed with ECONNRESET on the self-hosted runner (transient network drop to GitHub endpoints). The same pipeline passed for the common-ancestor build, and everything code-related is green (Format/Clippy/Nextest/CDP smoke Chromium/harness). A re-run of the failed jobs should suffice — nothing to change on the branch.

Fetch.enable patterns with heterogeneous resource types collapsed the
protocol-to-renderer channel to "intercept every class" because the
channel could only carry one SubresourceResourceType or all of them.
Synchronous XHR then hit the explicit interception ban (NetworkError
before any I/O) even though no pattern named an XHR-like class, and
phase-one script preloads were deferred for the same reason.

Replace the Option<SubresourceResourceType> channel with a
SubresourceInterceptionClasses bitset mirroring the distinct
CDP Fetch interception classes (Fetch/EventSource/XHR share the XHR
class; Audio/Video/Media/TextTrack share Media). Pattern sets now
union their classes, so Image+Media+Font routes exactly those classes
and synchronous XHR keeps its ordinary network path. Type-less
patterns, empty pattern lists, and auth-capable configurations still
widen to every class: auth matching is URL-granular and must never be
silently cut by a class scope. An empty set means nothing routes, so
the channel carries the class set alone without a separate enabled
flag. Same-class patterns (Fetch+XHR+EventSource) behave as before,
and Stylesheet/TextTrack-only patterns keep the existing
renderer-disabled behavior.

handleAuthRequests:true combined with typed patterns is now observed
to widen the class set to every class, so synchronous XHR for that
configuration reports the explicit interception failure instead of
silently missing Fetch.authRequired pauses; previously the non-named
classes simply bypassed auth interception.

The renderer's should_intercept_subresource, worker state, phase-one
and document.write script preload admission, and document fetch
context seeds all read the class set; Chromium-verified dual-engine
smoke coverage extends the fetch-font group with a synchronous XHR
contract.

Review fixes: class-precise document.write preload gate; negative
auth-widening unit test; dead single-filter config entry point
removed; (enabled, classes) tuple collapsed into the bare class set.
…terception class-set API

Upstream commits added after fix/fetch-interception-resource-class-set
branched still called set_fetch_subresource_interception(bool, Option<_>);
scope them to SubresourceInterceptionClasses (for_resource_type / ALL).
@yepbro
yepbro force-pushed the fix/fetch-interception-resource-class-set branch from 4e42d6c to d5c1d30 Compare November 12, 2026 03:46
@yepbro

yepbro commented Nov 12, 2026

Copy link
Copy Markdown
Contributor Author

Updated the branch against current main (rebased onto cbf4d0149) and added one commit, d5c1d3097 — an explanation of what and why:

What: a mechanical port of the test call-sites upstream added after this branch diverged — 6 files, 13 calls of the old set_fetch_subresource_interception(bool, Option<_>) signature (websocket handshake tests, resource_initiator fetch/cache/cancellation/orb tests, script_vm websocket tests).

Why: this PR replaces that signature with the class-set API, so the plain merge applies cleanly textually but does not compile against current main. The port maps the old arguments one-to-one onto the new API — (true, Some(T)) → SubresourceInterceptionClasses::for_resource_type(T), (true, None) → SubresourceInterceptionClasses::ALL — preserving each test's interception scope exactly. With this commit the branch builds and its relevant suites pass against the latest main (websocket/resource_initiator/synchronous-xhr suites on the renderer side, the interception suites on the protocol side), so the PR is now merge-ready as-is.

This should also give the PR a fresh CI run, replacing the previous one that failed on the artifact finalize step with a transient ECONNRESET.

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.

Synchronous XHR fails with NetworkError when Fetch.enable uses heterogeneous resourceType patterns

1 participant