From b50bf862f059737eab9400fae53ce5f8bc6bb361 Mon Sep 17 00:00:00 2001 From: Vito Date: Fri, 23 Oct 2026 03:51:38 -0700 Subject: [PATCH] fix(inspect): hold a route change until a starting video settles (#294) * fix(inspect): keep a replaced video in place until its pipeline has started 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. * fix(inspect): hold a route change until a starting video settles A route change within the workspace detaches the Inspect page, and WebKit (GStreamer) deadlocks when a video still starting is detached. Route changes at loadstart + 0-7 ms hung 4-5 of 120 fresh pages (14 of 120 with no page code releasing anything, so the detach itself is the trigger). Inspect now exposes leaveGuard(): a promise while a video on the page is still starting (current or replaced). The shell renders the next route once it settles (metadata, error, or the 15 s bound); the latest route wins. Route changes at loadstart + 0-7 ms now hang 0 of 120. Not changed, with numbers in TESTING.md D62: - A replaced video still shows above the new preview until it settles. Same-slot stacking changes the video's own box and hung again (absolute 5/120, absolute + opacity 0 8/120, one-cell grid 4/120). - A real page leave cannot wait. Navigating away hung 8/120 and reload 22/120 even with no page-side release, so it stays a known limitation. --- TESTING.md | 21 +++++++++-- tests/browser/workspace-inspect.spec.mjs | 48 ++++++++++++++++++++++++ web/js/workspace/index.mjs | 16 +++++++- web/js/workspace/inspect.mjs | 27 +++++++++++-- 4 files changed, 104 insertions(+), 8 deletions(-) diff --git a/TESTING.md b/TESTING.md index 6566eb2..cab865a 100644 --- a/TESTING.md +++ b/TESTING.md @@ -1684,8 +1684,20 @@ mapped differences"): 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 + 15 s) it is released as above and only then is its URL revoked. For that moment (the + time to metadata, or at most 15 s for a video that never settles) the old video and the + new preview both show, the new one below it: a known, brief overlap. Stacking them in one + slot instead hung again, because it changes the video's own box: the old video + absolutely positioned in its own box hung 5 of 120 pages (8 of 120 with `opacity: 0`), + and a one-cell grid 4 of 120. Only `opacity: 0` on the video, left in the flow, did not + hang (0 of 120), and alone it would leave an empty hole above the new preview. A route + change within the workspace waits the same way: the shell renders the next route once a + video still starting on Inspect settles (`leaveGuard`; the latest route wins). Route + changes at `loadstart` + 0–7 ms hung 4–5 of 120 pages before and 0 of 120 after. + A real page leave cannot wait. With no page code releasing anything, navigating + away hung 8 of 120 pages and reload 22 of 120 (3 and 8 of 120 with the page's own + release), so that WebKit teardown stays a known limitation; dispose still releases 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 @@ -1696,8 +1708,9 @@ mapped differences"): `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 + ms), and `leaving for another route while a video starts waits for it to settle`. 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 diff --git a/tests/browser/workspace-inspect.spec.mjs b/tests/browser/workspace-inspect.spec.mjs index 5d19793..2640cff 100644 --- a/tests/browser/workspace-inspect.spec.mjs +++ b/tests/browser/workspace-inspect.spec.mjs @@ -577,6 +577,54 @@ test('a video replaced while still starting is reset once it settles', async ({ } }); +test('leaving for another route while a video starts waits for it to settle', async ({ context, browserName }) => { + test.setTimeout(180000); + // A WebKit (GStreamer) video detached before its pipeline had started deadlocked the page (TESTING.md D62); a + // route change detaches the whole page, so the shell renders the next route once the video reports metadata or an + // error. The route changes from the video's own `loadstart`, or a few milliseconds after it. + for (let attempt = 0; attempt < 15; attempt++) { + const page = await context.newPage(); + try { + if (attempt === 0) await startCoverage(page, browserName); + await page.addInitScript(() => { + const load = HTMLMediaElement.prototype.load; + HTMLMediaElement.prototype.load = function () { + if (!this.hasAttribute('src') && window.__starting?.node === this) window.__starting.reset = { readyState: this.readyState, failed: this.error !== null, connected: this.isConnected }; + return load.call(this); + }; + }); + await openInspect(page); + await page.evaluate(delay => { + const slot = document.querySelector('#local-preview'); + new MutationObserver((records, observer) => { + const video = slot.querySelector('video'); + if (!video) return; + observer.disconnect(); + const leave = () => { + window.__starting = { node: video, readyState: video.readyState }; + location.hash = '#/overview'; + }; + video.addEventListener('loadstart', () => { if (delay) setTimeout(leave, delay); else leave(); }, { once: true }); + }).observe(slot, { childList: true }); + }, attempt % 5); + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(main(page).getByRole('heading', { level: 1, name: 'Overview', exact: true })).toBeVisible({ timeout: 8000 }); + const { readyState, reset } = await page.evaluate(() => ({ readyState: window.__starting.readyState, reset: window.__starting.reset })); + if (attempt % 5 === 0) expect(readyState).toBe(0); + // The video was reset and left the page only once it had settled. + expect(reset.readyState >= 1 || reset.failed).toBe(true); + expect(reset.connected).toBe(true); + // The page still answers. + await page.locator('aside.sidebar nav').getByRole('link', { name: 'Inspect a file', exact: true }).click(); + await chooser(page).setInputFiles(parityPng('f02.png'), { timeout: 8000 }); + await expect(preview(page).locator('img.local-file-preview')).toBeVisible({ timeout: 8000 }); + if (attempt === 0) await saveCoverage(page, browserName); + } finally { + await page.close(); + } + } +}); + test('closing the viewer on a video that just started never blocks the page', async ({ context, browserName }) => { test.setTimeout(180000); // The viewer's video teardown resets its element; on WebKit a reset of a still-starting pipeline blocked the page. diff --git a/web/js/workspace/index.mjs b/web/js/workspace/index.mjs index 5e29f04..70adf1a 100644 --- a/web/js/workspace/index.mjs +++ b/web/js/workspace/index.mjs @@ -132,9 +132,23 @@ function mountWorkspace() { refreshCounts(NAV_COUNT_ROUTES); let viewScope; let firstRender = true; + // D62: a page whose teardown must wait (Inspect, while a video is still starting) holds the next route until it + // settles; the latest route wins. + let leaveGuard = null, renderToken = 0; scope.cleanup(() => viewScope?.dispose()); function render(route) { + const token = ++renderToken; + const hold = leaveGuard?.(); + if (hold) { + hold.then(() => { if (token === renderToken && !scope.signal.aborted) renderNow(route); }); + return; + } + renderNow(route); + } + + function renderNow(route) { + leaveGuard = null; // A shared page dialog never stays open over the next route. closeWorkspaceDialog(); viewScope?.dispose(); @@ -240,7 +254,7 @@ function mountWorkspace() { if (route.id === 'inspect') { // The concept's Inspect a file (app.js renderMetadata): a local file, read only in this browser. description.textContent = route.description; - mountInspect(main, { parentScope: view, api, sender: createPromptSender({ scope: view }) }); + ({ leaveGuard } = mountInspect(main, { parentScope: view, api, sender: createPromptSender({ scope: view }) })); return; } if (isHandoff(route)) { diff --git a/web/js/workspace/inspect.mjs b/web/js/workspace/inspect.mjs index 719b067..c514d02 100644 --- a/web/js/workspace/inspect.mjs +++ b/web/js/workspace/inspect.mjs @@ -64,8 +64,10 @@ function releaseMedia(preview) { /** * Mounts the page into `main` below the heading the shell rendered. `sender` is the shared prompt sender; `api` is - * only handed to the viewer (the local adapter never calls it). Returns `{dispose}`; the page also ends with - * `parentScope`. + * only handed to the viewer (the local adapter never calls it). Returns `{dispose, leaveGuard}` (`{dispose}` when + * `parentScope` has already ended); the page also ends with `parentScope`. `leaveGuard()` is a promise while a video + * on the page is still starting (D62): the shell renders the next route once it settles, since detaching it then + * deadlocked WebKit. */ export function mountInspect(main, { parentScope, api, sender }) { const scope = createScope(); @@ -110,10 +112,12 @@ export function mountInspect(main, { parentScope, api, sender }) { entry.waiting.abort(); clearTimeout(entry.timer); releaseMedia(entry.preview); if (entry.revoke) URL.revokeObjectURL(url); + entry.done(); } function retire(preview, url) { const waiting = new AbortController(); const entry = { preview, waiting, revoke: false, timer: setTimeout(() => finishRetiring(url), STARTING_VIDEO_MS) }; + entry.finished = new Promise(resolve => { entry.done = resolve; }); retiring.set(url, entry); preview.listeners.abort(); preview.node.inert = true; preview.node.setAttribute('aria-hidden', 'true'); @@ -262,5 +266,22 @@ export function mountInspect(main, { parentScope, api, sender }) { if (open.getAttribute('aria-disabled') === 'true' || !entry || session.state.phase !== 'ready') return; viewer.open({ items: [entry], selectedKey: entry.key, trigger: open }); }); - return { dispose }; + /** Settles when no video on the page is still starting; `null` when none is. */ + function leaveGuard() { + if (scope.signal.aborted) return null; + const waits = []; + for (const entry of retiring.values()) waits.push(entry.finished); + if (media && isStarting(media.node)) { + const { node } = media; + waits.push(new Promise(resolve => { + const settled = new AbortController(); + const done = () => { settled.abort(); clearTimeout(timer); resolve(); }; + const timer = setTimeout(done, STARTING_VIDEO_MS); + for (const type of ['loadedmetadata', 'error']) node.addEventListener(type, done, { once: true, signal: settled.signal }); + scope.signal.addEventListener('abort', done, { once: true, signal: settled.signal }); + })); + } + return waits.length ? Promise.all(waits) : null; + } + return { dispose, leaveGuard }; }