From acc689825543942939bbef14ccd2844b7f13298d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miguel=20=C3=81ngel?= Date: Tue, 25 Aug 2026 05:25:24 +0000 Subject: [PATCH] =?UTF-8?q?fix(core):=20address=20review=20=E2=80=94=20gat?= =?UTF-8?q?e=20early=20diagnostic,=20fix=20empty=20crossOrigin,=20document?= =?UTF-8?q?=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/core/package-subpaths.json | 6 ++ packages/core/package.json | 10 +++ packages/core/src/runtime/init.ts | 26 ++++++-- .../core/src/runtime/webAudioRoute.test.ts | 60 ++++++++++++++++++ packages/core/src/runtime/webAudioRoute.ts | 61 ++++++++++++++++++- .../src/components/sidebar/AudioRow.tsx | 15 ++++- 6 files changed, 168 insertions(+), 10 deletions(-) diff --git a/packages/core/package-subpaths.json b/packages/core/package-subpaths.json index b6acfeeef..8c135bcc8 100644 --- a/packages/core/package-subpaths.json +++ b/packages/core/package-subpaths.json @@ -236,6 +236,12 @@ "types": "./dist/runtime/stackingContext.d.ts", "environments": ["browser", "bun", "node"] }, + "./runtime/web-audio-route": { + "source": "./src/runtime/webAudioRoute.ts", + "runtime": "./dist/runtime/webAudioRoute.js", + "types": "./dist/runtime/webAudioRoute.d.ts", + "environments": ["browser", "bun", "node"] + }, "./compiler/html-document": { "source": "./src/compiler/htmlDocument.ts", "runtime": "./dist/compiler/htmlDocument.js", diff --git a/packages/core/package.json b/packages/core/package.json index ad3e657e7..40b264a6c 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -245,6 +245,12 @@ "import": "./src/runtime/stackingContext.ts", "types": "./src/runtime/stackingContext.ts" }, + "./runtime/web-audio-route": { + "bun": "./src/runtime/webAudioRoute.ts", + "node": "./dist/runtime/webAudioRoute.js", + "import": "./src/runtime/webAudioRoute.ts", + "types": "./src/runtime/webAudioRoute.ts" + }, "./compiler/html-document": { "bun": "./src/compiler/htmlDocument.ts", "node": "./dist/compiler/htmlDocument.js", @@ -539,6 +545,10 @@ "import": "./dist/runtime/stackingContext.js", "types": "./dist/runtime/stackingContext.d.ts" }, + "./runtime/web-audio-route": { + "import": "./dist/runtime/webAudioRoute.js", + "types": "./dist/runtime/webAudioRoute.d.ts" + }, "./compiler/html-document": { "import": "./dist/compiler/htmlDocument.js", "types": "./dist/compiler/htmlDocument.d.ts" diff --git a/packages/core/src/runtime/init.ts b/packages/core/src/runtime/init.ts index f371be926..eccd69dd4 100644 --- a/packages/core/src/runtime/init.ts +++ b/packages/core/src/runtime/init.ts @@ -42,7 +42,11 @@ import { applyVariableBindings } from "./applyVariableBindings"; import { createColorGradingRuntime, type RuntimeColorGradingApi } from "./colorGrading"; import { TransportClock } from "./clock"; import { WebAudioTransport } from "./webAudioTransport"; -import { classifyWebAudioMediaRoute, reportWebAudioMediaRoute } from "./webAudioRoute.js"; +import { + classifyWebAudioMediaRoute, + isRouteSelectionSettled, + reportWebAudioMediaRoute, +} from "./webAudioRoute.js"; import { ensureAudioGroupInertStyle, HF_AUDIO_GROUP_TAG, @@ -1840,6 +1844,13 @@ export function initSandboxRuntimeModular(): void { // were. const reportWebAudioRoute = (mediaEl: HTMLMediaElement) => { if (!(mediaEl instanceof HTMLAudioElement)) return; + // Before resource selection settles, the verdict is built from `` + // children the browser might still pass over — good enough for the + // schedule path's conservative withhold, not good enough to put in front + // of a human as a diagnostic. Skip; the `loadedmetadata` call to this same + // function (see below) always has a settled `currentSrc` and will report + // for real once the guess would no longer be one. + if (!isRouteSelectionSettled(mediaEl)) return; reportWebAudioMediaRoute(mediaEl, classifyWebAudioMediaRoute(mediaEl)); }; @@ -1875,10 +1886,15 @@ export function initSandboxRuntimeModular(): void { // schedule time. `hyperframes check` seeks, it never calls play(), so a // diagnostic raised from the transport would be invisible to the one // gate whose job is to surface exactly this class of silent failure. - // Bound twice on purpose: now, so a composition that never plays still - // reports, and again at `loadedmetadata`, when `currentSrc` is finally - // authoritative and the early read may have been guessing from - // `` children. `reportWebAudioMediaRoute` latches per element. + // Bound twice on purpose: now, for a `src`/committed-`currentSrc` + // element so a composition that never plays still reports promptly, and + // again at `loadedmetadata`, when `currentSrc` is unconditionally + // authoritative. `reportWebAudioRoute` itself skips the "now" call when + // selection hasn't settled (see `isRouteSelectionSettled`) — with only + // `` children to go on, the browser could still pick a + // different one than the classifier just judged, and a diagnostic is a + // claim of fact, not a guess. `reportWebAudioMediaRoute` latches per + // element, so the deferred-to-`loadedmetadata` case still reports once. mediaEl.addEventListener("loadedmetadata", onMediaLoadedMetadataForRoute); reportWebAudioRoute(mediaEl); // Reactive (zero-videoWidth) + tertiary (error event) proxy-fallback diff --git a/packages/core/src/runtime/webAudioRoute.test.ts b/packages/core/src/runtime/webAudioRoute.test.ts index 1725d4b25..406963098 100644 --- a/packages/core/src/runtime/webAudioRoute.test.ts +++ b/packages/core/src/runtime/webAudioRoute.test.ts @@ -1,6 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { classifyWebAudioMediaRoute, + isRouteSelectionSettled, nativeUnexpressibleProcessing, reportWebAudioMediaRoute, } from "./webAudioRoute"; @@ -21,6 +22,18 @@ function withCurrentSrc(el: HTMLAudioElement, currentSrc: string): HTMLAudioElem return el; } +/** + * Shadows the IDL `crossOrigin` accessor with a plain data property, so the + * value is visible ONLY via the property — unlike `el.crossOrigin = value` + * (which jsdom, like real browsers, reflects straight back to the + * `crossorigin` attribute), this simulates a host whose IDL property is + * genuinely decoupled from the attribute, per `hasCorsOptIn`'s secondary read. + */ +function withUnreflectedCrossOrigin(el: HTMLAudioElement, value: string): HTMLAudioElement { + Object.defineProperty(el, "crossOrigin", { value, configurable: true }); + return el; +} + describe("classifyWebAudioMediaRoute", () => { it("routes same-origin media through Web Audio", () => { expect(classifyWebAudioMediaRoute(audio({ src: "/assets/vo.mp3" }))).toEqual({ @@ -97,6 +110,53 @@ describe("classifyWebAudioMediaRoute", () => { it("routes an element with no resolvable source through Web Audio", () => { expect(classifyWebAudioMediaRoute(audio())).toEqual({ kind: "web-audio" }); }); + + it("treats an unreflected empty-string crossOrigin IDL property as opt-in", () => { + // The IDL fallback for `crossorigin=""` / bare `crossorigin` is the empty + // string. A host whose property setter doesn't reflect to the attribute + // (unlike jsdom's own accessor, which does) must not have that empty + // string misread as "no opt-in" — `Boolean("")` is false, which is + // exactly the fail-open this test guards against. + const el = withUnreflectedCrossOrigin(audio({ src: `${CROSS_ORIGIN}/track.mp3` }), ""); + expect(el.getAttribute("crossorigin")).toBeNull(); // confirms it's genuinely unreflected + + expect(classifyWebAudioMediaRoute(el)).toEqual({ kind: "web-audio" }); + }); + + it("does not treat an untouched crossOrigin IDL property as opt-in", () => { + // The other direction of the same risk: a host must not default + // `crossOrigin` to a truthy/string value for elements that never opted + // in, or the cross-origin check would be permanently disabled. + const el = audio({ src: `${CROSS_ORIGIN}/track.mp3` }); + expect(el.crossOrigin).toBeNull(); + + expect(classifyWebAudioMediaRoute(el)).toEqual({ + kind: "decode-only", + reason: "cross_origin_no_cors", + asset: `${CROSS_ORIGIN}/track.mp3`, + }); + }); +}); + +describe("isRouteSelectionSettled", () => { + it("is unsettled for an element with only children", () => { + const el = audio(); + el.innerHTML = ``; + expect(isRouteSelectionSettled(el)).toBe(false); + }); + + it("is settled once a src attribute is definitive, even before load", () => { + expect(isRouteSelectionSettled(audio({ src: "/assets/vo.mp3" }))).toBe(true); + }); + + it("is settled once currentSrc has committed", () => { + const el = withCurrentSrc(audio(), `${SAME_ORIGIN}/assets/vo.mp3`); + expect(isRouteSelectionSettled(el)).toBe(true); + }); + + it("is unsettled for an element with no source at all", () => { + expect(isRouteSelectionSettled(audio())).toBe(false); + }); }); describe("nativeUnexpressibleProcessing", () => { diff --git a/packages/core/src/runtime/webAudioRoute.ts b/packages/core/src/runtime/webAudioRoute.ts index 8692fb6a0..f1eeb6e04 100644 --- a/packages/core/src/runtime/webAudioRoute.ts +++ b/packages/core/src/runtime/webAudioRoute.ts @@ -21,6 +21,19 @@ import type { RuntimeJson } from "./types"; * a pure classifier, so the same verdict can be reached at media-discovery time * (to emit a diagnostic) and at schedule time (to actually withhold the node) * without those two ever drifting apart. + * + * That guarantee holds for every caller that routes through this classifier — + * it is NOT a runtime-wide interception of `createMediaElementSource`. The + * timeline transport (`webAudioTransport.ts`, via `init.ts`) always goes + * through it; a UI surface that builds its own throwaway `AudioContext` for + * an unrelated purpose (e.g. the asset sidebar's preview player, + * `AudioRow.tsx`) has to call it too, and is expected to. Known gap: an + * element playing a `MediaStream` via `srcObject` instead of `src`/`` + * has no origin for this module to judge — `routeCandidates` only reads + * `src`-shaped attributes, so a `srcObject` element always reads as + * `web-audio` here, correctly or not. Nothing in this codebase feeds + * `createMediaElementSource` from a `srcObject` element today, so this is + * recorded as a boundary rather than fixed. */ export type WebAudioMediaRoute = /** Same-origin, CORS-opted-in, or a scheme the check doesn't apply to. */ @@ -50,11 +63,23 @@ function hasAttr(el: HTMLMediaElement, name: string): boolean { * `anonymous`, so PRESENCE is the opt-in — `crossorigin=""` and even * `crossorigin="garbage"` both make the fetch a CORS request. Comparing the * value against `"anonymous"` would wrongly block those. + * + * Two independent reads, because a spec-faithful host and a permissive one + * disagree about where the truth lives: + * - `getAttribute` is the primary read and covers every real browser: the + * markup is unambiguous regardless of what the IDL getter does with it. + * - `el.crossOrigin` is a secondary read for a host that sets the IDL + * property without reflecting it back to the attribute — some + * jsdom-style test/preview hosts do this. The check is `!= null` + * (covers both `null` and `undefined`), not a truthiness check, ON + * PURPOSE: `crossorigin=""` is a valid, common opt-in (see above), and + * its IDL fallback value is the empty string — a falsy value that + * `Boolean(el.crossOrigin)` would silently misread as "not opted in", + * reintroducing the exact silent-audio bug this module exists to close. */ function hasCorsOptIn(el: HTMLMediaElement): boolean { if (hasAttr(el, "crossorigin")) return true; - // Secondary read for a host that set the IDL property without reflecting it. - return typeof el.crossOrigin === "string"; + return el.crossOrigin != null; } function baseUri(el: HTMLMediaElement): string { @@ -108,6 +133,26 @@ function isCorsSilenced(rawUrl: string, el: HTMLMediaElement): boolean { return !hasCorsOptIn(el); } +/** + * Whether resource selection has settled enough for a verdict to be a FACT + * rather than a guess. `currentSrc`/`src` are both definitive per the HTML + * resource-selection algorithm (see `routeCandidates` above); before either + * is set, a verdict can only be built from `` children, any of which + * the browser may still pass over before committing. + * + * `classifyWebAudioMediaRoute` itself stays unsettled-tolerant on purpose — + * the schedule path needs *a* verdict even before selection settles, and + * conservatively withholding the node there costs nothing but a decode-only + * fallback. This predicate exists for the one caller that must NOT act on a + * guess: the discovery-time diagnostic, which drops a message in a human's + * lap and only gets to say it once (see `reportWebAudioMediaRoute`'s latch). + */ +export function isRouteSelectionSettled(el: HTMLMediaElement): boolean { + const current = typeof el.currentSrc === "string" ? el.currentSrc : ""; + if (current) return true; + return hasAttr(el, "src"); +} + /** * Pure — no node creation, no diagnostics, no element mutation. Called from * both the schedule path (where it withholds the node) and the discovery path @@ -163,7 +208,17 @@ export function nativeUnexpressibleProcessing(el: HTMLMediaElement): string[] { * only `