From 7a4b6eebb217c56bbc0b89e8c6bcb0e30eb93a6a Mon Sep 17 00:00:00 2001 From: Vito Date: Fri, 23 Oct 2026 00:08:43 -0700 Subject: [PATCH 1/2] test(shell): split the shell concept comparison per route and motion (#297) "shell geometry, tokens and controls match the concept" compared all three routes (Images, Library, Tags) under both motions in one test per size and theme: 12 page loads, 12 captures and 36 hovers under the 30 s default budget. Over the last 40 Viewer.js runs it took 22 s median and up to 33 s on CI WebKit, and timed out 6 times (main run 39886719087 among them). The CI traces show steady progress with no stalled step (the longest one is a 0.8 s navigation), so the cause is the amount of work in one test, not a hang or a race. Each route and motion is now its own test, still covering both sizes and both themes, with the same assertions and captures. Under run-pw-ci.sh on WebKit at 2 CPUs, --repeat-each=10: before 18/40 timed out (median 26.9 s, max 32.5 s); after 0/240 (median 6.5 s, p90 9.8 s, max 17.2 s). --- tests/browser/shell-concept.spec.mjs | 41 ++++++++++++++-------------- 1 file changed, 21 insertions(+), 20 deletions(-) diff --git a/tests/browser/shell-concept.spec.mjs b/tests/browser/shell-concept.spec.mjs index a4076d7a..14976317 100644 --- a/tests/browser/shell-concept.spec.mjs +++ b/tests/browser/shell-concept.spec.mjs @@ -178,27 +178,28 @@ async function captureFooter(page,testInfo,name) { test.beforeEach(async({request})=>{await seedConcept(request);}); -for(const [width,height] of SIZES)for(const theme of THEMES)test(`shell geometry, tokens and controls match the concept ${width}x${height} ${theme}`,async({page},testInfo)=>{ +// One test per route, size, theme and motion. All of them in one test per size and theme (12 page loads, 12 captures, +// 36 hovers) steadily took 22 s median and up to 33 s on CI WebKit, past the 30 s budget with no step stalled; one +// route in one motion is a sixth of that work under the same budget. +for(const [route,breadcrumb] of ROUTES)for(const [width,height] of SIZES)for(const theme of THEMES)for(const motion of MOTIONS)test(`shell geometry, tokens and controls match the concept on ${route} ${width}x${height} ${theme} ${motion}`,async({page},testInfo)=>{ await page.setViewportSize({width,height});const result={}; - for(const motion of MOTIONS)for(const [route,breadcrumb] of ROUTES) { - await openConcept(page,route,theme,motion);await page.mouse.move(0,0); - const reference=await inventoryOf(page,'#main'); - if(motion==='no-preference')await captureCanonicalState(page,testInfo,`concept-shell-${route}-${width}-${theme}`); - const referenceRings=FULL_HEADING.has(route)?await ringsOf(page,'#main'):null; - const referenceHovers=await hoversOf(page); - if(motion==='no-preference')await captureFooter(page,testInfo,`concept-footer-${route}-${width}-${theme}`); - await openProduction(page,route,theme,motion);await page.mouse.move(0,0); - const actual=await inventoryOf(page,'#workspace-main'); - if(motion==='no-preference')await captureCanonicalState(page,testInfo,`production-shell-${route}-${width}-${theme}`); - const actualRings=FULL_HEADING.has(route)?await ringsOf(page,'#workspace-main'):null; - const actualHovers=await hoversOf(page); - if(motion==='no-preference')await captureFooter(page,testInfo,`production-footer-${route}-${width}-${theme}`); - result[`${motion} ${route}`]={route,breadcrumb,reference,actual,diff:compare(route,reference,actual),referenceRings,actualRings, - referenceHovers,actualHovers}; - } - const path=testInfo.outputPath('shell-geometry.json');await writeFile(path,JSON.stringify(result,null,2)); - await testInfo.attach('shell-geometry',{path,contentType:'application/json'}); - for(const [key,{route,breadcrumb,reference,actual,diff,referenceRings,actualRings,referenceHovers,actualHovers}] of Object.entries(result)) { + await openConcept(page,route,theme,motion);await page.mouse.move(0,0); + const reference=await inventoryOf(page,'#main'); + if(motion==='no-preference')await captureCanonicalState(page,testInfo,`concept-shell-${route}-${width}-${theme}`); + const referenceRings=FULL_HEADING.has(route)?await ringsOf(page,'#main'):null; + const referenceHovers=await hoversOf(page); + if(motion==='no-preference')await captureFooter(page,testInfo,`concept-footer-${route}-${width}-${theme}`); + await openProduction(page,route,theme,motion);await page.mouse.move(0,0); + const actual=await inventoryOf(page,'#workspace-main'); + if(motion==='no-preference')await captureCanonicalState(page,testInfo,`production-shell-${route}-${width}-${theme}`); + const actualRings=FULL_HEADING.has(route)?await ringsOf(page,'#workspace-main'):null; + const actualHovers=await hoversOf(page); + if(motion==='no-preference')await captureFooter(page,testInfo,`production-footer-${route}-${width}-${theme}`); + result[`${motion} ${route}`]={reference,actual,diff:compare(route,reference,actual),referenceRings,actualRings, + referenceHovers,actualHovers}; + const path=testInfo.outputPath(`shell-geometry-${route}-${motion}.json`);await writeFile(path,JSON.stringify(result,null,2)); + await testInfo.attach(`shell-geometry-${route}-${motion}`,{path,contentType:'application/json'}); + for(const [key,{reference,actual,diff,referenceRings,actualRings,referenceHovers,actualHovers}] of Object.entries(result)) { // The concept itself is what this comparison assumes: its sample counts and its shell controls. expect.soft(reference.chrome.navs[0].rows.map(row=>[row.text,row.count?.text??null]),`${key} concept WORKSPACE rows`) .toEqual([['Images','24'],['Prompt library','6'],['Tags & categories','15'],['Inspect a file',null]]); From fd747a0ffc9526ea5ff87b1745354932b4d0fd39 Mon Sep 17 00:00:00 2001 From: Vito Date: Fri, 23 Oct 2026 00:31:37 -0700 Subject: [PATCH 2/2] fix(inspect): keep a replaced video in place until its pipeline has started (#291) WebKit (GStreamer) deadlocks the page 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 waits on the main thread in a synchronous bus handler (native stacks of the hung WPE web process in the pinned image). Removing, hiding or merely pushing the video below the fold all hung; none of pause(), removing src, load() or the revoke was the cause on its own. A video replaced while still starting now 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 paused, loses its src, is reset with load() and removed, and only then is its URL revoked. Every released video is reset with load(), so no decoder is kept per replaced video. The loadstart replacement spec now runs on WebKit too (15 fresh pages, replaced at loadstart + 0-4 ms). TESTING.md D62 records the cause. --- TESTING.md | 34 ++++++---- tests/browser/workspace-inspect.spec.mjs | 34 +++++----- tools/vendor/frontend-coverage-baseline.json | 2 +- web/js/workspace/inspect.mjs | 67 +++++++++++++------- web/js/workspace/local-session.mjs | 2 +- 5 files changed, 86 insertions(+), 53 deletions(-) diff --git a/TESTING.md b/TESTING.md index 603f53e7..e2986297 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 5923259c..15022078 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 56277d9c..bf11d2f0 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 cd6f2f7b..39fe9ecf 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 0765ff68..646f77e8 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, } = {}) {