diff --git a/TESTING.md b/TESTING.md index 603f53e..e298629 100644 --- a/TESTING.md +++ b/TESTING.md @@ -1627,19 +1627,27 @@ mapped differences"): `image/png,image/jpeg,image/webp` plus `video/*,.mp4,.mov,.m4v,.webm,.mkv,.avi,.wmv`. A video previews in the same `#local-preview` slot as `video.local-file-preview` with native `controls`, `playsInline`, `preload="metadata"` and `muted`, never autoplays - inline, and is paused, has its `src` removed and is removed from the page on replacement - or dispose, before its URL is revoked. Each preview owns its listeners, so the page never - pins a released node. A settled video (metadata read, or failed) is then reset with - `load()` at once; one still starting is reset only once it reports metadata or an error, - because WebKit (GStreamer) blocked the page when such a pipeline was reset (`replacing a - video that just started loading never blocks the page`, `a video replaced while still - starting is reset once it settles`). A WebKit load cut off by the revoked URL usually - stalls and never settles; a fallback then only drops the waiting listeners. The viewer's - own video teardown was checked the same way (`closing the viewer on a video that just - started never blocks the page`, 15 fresh pages on WebKit) and needed no change. - Follow-up: on WebKit, a video released exactly at its `loadstart` sometimes blocks the - page, with or without any reset and also before the deferred reset (2 of 24 runs in the - pinned image), so the `loadstart` spec is skipped on WebKit until that is investigated. + inline. On replacement or dispose it is paused, has its `src` removed, is reset with + `load()` (which frees its decoder) and is removed from the page, before its URL is + revoked. Each preview owns its listeners, so the page never pins a released node. + A video replaced while still starting (a source, but neither metadata nor an error yet) + stays where it is, inert and `aria-hidden`, with the new preview inserted after it, and + the session's revoke of its URL waits; once it reports metadata or an error (or after + 15 s) it is released as above and only then is its URL revoked. Page leave and dispose + release it at once. Cause, found with native stacks of the hung WPE web process in the + pinned image: WebKit (GStreamer) deadlocks when a video leaves the page or the viewport + before its pipeline has started. The main thread changes the pipeline state and waits + for the source's streaming thread, which is itself waiting on the main thread in a + synchronous bus handler. Removing the video, hiding it, or only pushing it below the + fold with a taller preview inserted before it all hung (7 to 21 of 120 fresh pages each, + replaced at `loadstart` + 0–7 ms), and none of the earlier steps (`pause()`, removing + `src`, `load()`, revoking the URL) was the cause on its own; pushing it out after + `loadedmetadata` hung 0 of 180. Specs: `replacing a video that just started loading + never blocks the page`, and `a video replaced while still starting is reset once it + settles`, which now runs on WebKit too (15 fresh pages, replaced at `loadstart` + 0–4 + ms). The viewer's own video teardown was checked the same way (`closing the viewer on a + video that just started never blocks the page`, 15 fresh pages on WebKit) and needed no + change. - **D63** (status line): one `p.small.spaced-16` in `#local-preview`, for states the concept never shows: "Drop one file at a time.", "Folders cannot be inspected. Choose one image or video file.", "This file type cannot be previewed or inspected here. diff --git a/tests/browser/workspace-inspect.spec.mjs b/tests/browser/workspace-inspect.spec.mjs index 5923259..1502207 100644 --- a/tests/browser/workspace-inspect.spec.mjs +++ b/tests/browser/workspace-inspect.spec.mjs @@ -481,15 +481,13 @@ test('replacing a video that just started loading never blocks the page', async }); test('a video replaced while still starting is reset once it settles', async ({ context, browserName }) => { - // A WebKit (GStreamer) video released exactly at its `loadstart` sometimes blocks the page, with or without any - // reset and also with the code before the deferred reset (2 of 24 runs); tracked in TESTING.md (D62). WebKit's - // no-hang guard is `replacing a video that just started loading never blocks the page`. - test.skip(browserName === 'webkit', 'WebKit release at loadstart is a tracked follow-up (D62)'); - test.setTimeout(120000); - // The video is replaced from its own `loadstart` (readyState HAVE_NOTHING). It leaves the page with no src before - // its URL is revoked, and is reset (load() with no src, which frees its decoder) only once it reports metadata or - // an error, so the page never blocks. Fresh pages share one browser process. - for (let attempt = 0; attempt < 5; attempt++) { + test.setTimeout(180000); + // The video is replaced from its own `loadstart` (readyState HAVE_NOTHING), or a few milliseconds after it. On + // WebKit (GStreamer) a video that left the page or the viewport before its pipeline had started deadlocked the + // page (TESTING.md D62), so it stays in place, inert and still playing its URL, with the new preview after it. + // Once it reports metadata or an error it is paused, loses its src, is reset (load() with no src, which frees its + // decoder) and leaves the page, and only then is its URL revoked. Fresh pages share one browser process. + for (let attempt = 0; attempt < 15; attempt++) { const page = await context.newPage(); try { if (attempt === 0) await startCoverage(page, browserName); @@ -510,10 +508,12 @@ test('a video replaced while still starting is reset once it settles', async ({ const video = slot.querySelector('video'); if (!video) return; observer.disconnect(); - video.addEventListener('loadstart', () => { + const replace = () => { window.__starting = { node: video, url: video.getAttribute('src'), readyState: video.readyState }; input.files = window.__pngFiles; input.dispatchEvent(new Event('change', { bubbles: true })); - }, { once: true }); + window.__starting.replaced = { connected: video.isConnected, src: video.hasAttribute('src'), inert: video.inert, resets: video.__resets ?? 0 }; + }; + video.addEventListener('loadstart', () => { if (window.__delay) setTimeout(replace, window.__delay); else replace(); }, { once: true }); }).observe(slot, { childList: true }); }); const png = parityPng('f02.png'); @@ -522,13 +522,17 @@ test('a video replaced while still starting is reset once it settles', async ({ transfer.items.add(new File([new Uint8Array(bytes)], 'f02.png', { type: 'image/png' })); window.__pngFiles = transfer.files; }, [...png.buffer]); + await page.evaluate(delay => { window.__delay = delay; }, attempt % 5); await choose(page, localFixture('core-tags.webm', 'video/webm')); await expect(preview(page).locator('img.local-file-preview')).toBeVisible({ timeout: 8000 }); await expect(preview(page).locator('video')).toHaveCount(0); - // Released while starting, and never reset synchronously: the URL went once the node had no src. - expect(await page.evaluate(() => ({ readyState: window.__starting.readyState, revoked: window.__starting.revoked }))) - .toEqual({ readyState: 0, revoked: { src: false, connected: false, resets: 0 } }); - await expect.poll(() => page.evaluate(() => window.__starting.node.__resets ?? 0), { timeout: 15000 }).toBe(1); + // Replaced while starting (always so at `loadstart` itself): kept in place and untouched. A video that had + // settled is released at once. Either way it is reset once, and its URL goes last. + await expect.poll(() => page.evaluate(() => window.__starting.revoked ?? null)).toEqual({ src: false, connected: false, resets: 1 }); + const { readyState, replaced } = await page.evaluate(() => ({ readyState: window.__starting.readyState, replaced: window.__starting.replaced })); + if (attempt % 5 === 0) expect(readyState).toBe(0); + expect(replaced).toEqual(readyState === 0 ? { connected: true, src: true, inert: true, resets: 0 } : { connected: false, src: false, inert: false, resets: 1 }); + expect(await page.evaluate(() => window.__starting.node.__resets)).toBe(1); // The page still answers. await chooser(page).setInputFiles(parityPng('f02.png', 'after.png'), { timeout: 8000 }); await expect(preview(page).locator('img.local-file-preview')).toBeVisible({ timeout: 8000 }); diff --git a/tools/vendor/frontend-coverage-baseline.json b/tools/vendor/frontend-coverage-baseline.json index 56277d9..bf11d2f 100644 --- a/tools/vendor/frontend-coverage-baseline.json +++ b/tools/vendor/frontend-coverage-baseline.json @@ -296,7 +296,7 @@ "workspace/inspect.mjs": { "lines": 98, "branches": 92, - "functions": 92, + "functions": 93, "statements": 98 }, "workspace/inspection-state.mjs": { diff --git a/web/js/workspace/inspect.mjs b/web/js/workspace/inspect.mjs index cd6f2f7..39fe9ec 100644 --- a/web/js/workspace/inspect.mjs +++ b/web/js/workspace/inspect.mjs @@ -39,36 +39,24 @@ function element(tag, className, text) { // The status shown for the selection; AVI/WMV's note and an empty status keep the toast's "metadata is available". const TOASTABLE = new Set(['', LOCAL_SESSION_MESSAGES.containerOnly]); -// How long a released video still starting may take to settle before its deferred reset is dropped. -const DEFERRED_RESET_MS = 15000; +// How long a replaced video still starting may stay in place before it is released anyway. +const STARTING_VIDEO_MS = 15000; -/** - * Resets a released video once it settles. Removing `src` alone never aborts a load already under way, and WebKit - * (GStreamer) blocked the page when a pipeline still starting was reset, so load() waits for metadata or an error. - * The fallback only drops the waiting listeners (a WebKit load cut off by the revoked URL stalls and never settles), - * so the detached node is never pinned. - */ -function resetWhenSettled(node) { - const waiting = new AbortController(); - const fallback = setTimeout(() => waiting.abort(), DEFERRED_RESET_MS); - const reset = () => { waiting.abort(); clearTimeout(fallback); node.load(); }; - for (const type of ['loadedmetadata', 'error']) node.addEventListener(type, reset, { once: true, signal: waiting.signal }); +/** A video whose pipeline is still starting: it has a source but neither metadata nor an error yet. */ +function isStarting(node) { + return node instanceof HTMLMediaElement && node.hasAttribute('src') && node.readyState < HTMLMediaElement.HAVE_METADATA && node.error === null; } /** - * Stops a preview from holding its blob: URL: its own listeners end, media is paused and its src dropped before the - * session revokes the URL, and the node leaves the page. A settled video (metadata read, or failed) is then reset with - * load() to free its decoder at once; one still starting is reset as soon as it settles. + * Stops a preview from holding its blob: URL: its own listeners end, media is paused, its src dropped and it is reset + * with load() (which frees its decoder), and the node leaves the page, all before its URL is revoked. */ function releaseMedia(preview) { if (!preview) return; const { node, listeners } = preview; listeners.abort(); if (node instanceof HTMLMediaElement) { - const settled = node.readyState >= HTMLMediaElement.HAVE_METADATA || node.error !== null; - node.pause(); node.removeAttribute('src'); - if (settled) node.load(); - else resetWhenSettled(node); + node.pause(); node.removeAttribute('src'); node.load(); } else node.removeAttribute('src'); node.remove(); } @@ -108,7 +96,33 @@ export function mountInspect(main, { parentScope, api, sender }) { // The session ends after the viewer has closed and the inline media is gone, so no element holds its URL then. // The page owns page leave for the same reason: the session listens for no `pagehide` of its own. const sessionScope = createScope(); - const session = createLocalSession({ parentScope: sessionScope, pageTarget: null }); + // Replaced videos still starting, by URL. WebKit (GStreamer) deadlocked the page when a video left the page or + // the viewport before its pipeline had started, so such a video stays where it is, inert, until it reports + // metadata or an error; only then is it released and its URL, which the session asked to revoke, revoked. + const retiring = new Map(); + function finishRetiring(url) { + const entry = retiring.get(url); + if (!entry) return; + retiring.delete(url); + entry.waiting.abort(); clearTimeout(entry.timer); + releaseMedia(entry.preview); + if (entry.revoke) URL.revokeObjectURL(url); + } + function retire(preview, url) { + const waiting = new AbortController(); + const entry = { preview, waiting, revoke: false, timer: setTimeout(() => finishRetiring(url), STARTING_VIDEO_MS) }; + retiring.set(url, entry); + preview.listeners.abort(); + preview.node.inert = true; preview.node.setAttribute('aria-hidden', 'true'); + for (const type of ['loadedmetadata', 'error']) preview.node.addEventListener(type, () => finishRetiring(url), { once: true, signal: waiting.signal }); + } + const finishAllRetiring = () => { for (const url of [...retiring.keys()]) finishRetiring(url); }; + const revokeObjectURL = url => { + const entry = retiring.get(url); + if (entry) entry.revoke = true; + else URL.revokeObjectURL(url); + }; + const session = createLocalSession({ parentScope: sessionScope, pageTarget: null, revokeObjectURL }); const viewer = createMediaViewer({ api, container: main, parentScope: scope, inspectionController: session.controller, sender, adapter: localFileAdapter, onRestoreFocus: () => { if (open.isConnected) open.focus({ preventScroll: true }); } }); @@ -153,7 +167,9 @@ export function mountInspect(main, { parentScope, api, sender }) { function showMedia(state) { const url = state.preview?.url ?? null; if (url === mediaUrl) return; - releaseMedia(media); media = null; mediaUrl = url; + if (media && isStarting(media.node) && !retiring.has(mediaUrl)) retire(media, mediaUrl); + else releaseMedia(media); + media = null; mediaUrl = url; if (!url) return; const video = state.preview.media === 'video'; const node = element(video ? 'video' : 'img', 'local-file-preview'); @@ -173,7 +189,10 @@ export function mountInspect(main, { parentScope, api, sender }) { }, { once: true, signal: current.listeners.signal }); node.src = url; media = current; - preview.prepend(node); + // A video still starting keeps its place, so the new preview goes after it and never moves it. + const anchor = [...retiring.values()].map(entry => entry.preview.node).filter(item => item.parentNode === preview).at(-1); + if (anchor) anchor.after(node); + else preview.prepend(node); } scope.cleanup(session.subscribe(state => { showMedia(state); @@ -190,6 +209,7 @@ export function mountInspect(main, { parentScope, api, sender }) { // viewer (on abort), drops the media, then ends the session. scope.cleanup(() => viewer.dispose()); scope.cleanup(() => { releaseMedia(media); media = null; mediaUrl = null; }); + scope.cleanup(finishAllRetiring); scope.cleanup(() => sessionScope.dispose()); // Page leave: the viewer closes and the inline media goes before the session revokes the URL, and the chooser is @@ -198,6 +218,7 @@ export function mountInspect(main, { parentScope, api, sender }) { clearRejection(); viewer.close(); releaseMedia(media); media = null; mediaUrl = null; + finishAllRetiring(); session.clear(); chosen = null; input.value = ''; } diff --git a/web/js/workspace/local-session.mjs b/web/js/workspace/local-session.mjs index 0765ff6..646f77e 100644 --- a/web/js/workspace/local-session.mjs +++ b/web/js/workspace/local-session.mjs @@ -70,7 +70,7 @@ function chosenFile(input) { } export function createLocalSession({ - parentScope, createObjectURL = file => URL.createObjectURL(file), revokeObjectURL = url => URL.revokeObjectURL(url), + parentScope, createObjectURL = file => URL.createObjectURL(file), revokeObjectURL = URL.revokeObjectURL.bind(URL), schedulerYield = defaultSchedulerYield(), setTimer = setTimeout, clearTimer = clearTimeout, pageTarget = globalThis.window, extractFacts, pngMetadata, } = {}) {