diff --git a/TESTING.md b/TESTING.md index 7b1cb16d..96683564 100644 --- a/TESTING.md +++ b/TESTING.md @@ -2869,6 +2869,12 @@ mapped differences"): 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. + The shared viewer's own video gets the same guard (#483): a new selection while the + viewer D155 opened is still starting its video leaves the viewer open, untouched, until + that video reports metadata or an error (or 15 s), then closes it; the URL is revoked + after that, and a viewer asked for meanwhile (the new file's D155 open) opens then. The + merge queue hung once without it (run 40903238387: the PNG selection 12 ms after the + inline video showed, with the viewer's new video still starting). 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 diff --git a/tests/browser/workspace-inspect.spec.mjs b/tests/browser/workspace-inspect.spec.mjs index 3d77f112..81795e44 100644 --- a/tests/browser/workspace-inspect.spec.mjs +++ b/tests/browser/workspace-inspect.spec.mjs @@ -704,6 +704,147 @@ test('a video replaced while still starting is reset once it settles', async ({ } }); +test('a new selection while the viewer\'s own video is still starting leaves it until it settles (#483)', async ({ context, browserName }) => { + test.setTimeout(180000); + // D155 opens the viewer once the inline preview has settled; the viewer then starts a video of its own. A new + // selection from that video's own `loadstart` (readyState HAVE_NOTHING), or a few milliseconds after it, must not + // reset it or take it off the page while it starts (WebKit, GStreamer: TESTING.md D62; merge-queue run + // 40903238387). The viewer stays open until it reports metadata or an error, then closes; the URL goes last. + for (let attempt = 0; attempt < 10; 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')) { this.__resets = (this.__resets ?? 0) + 1; this.__resetReadyState ??= this.readyState; } return load.call(this); }; + const revoke = URL.revokeObjectURL; + URL.revokeObjectURL = url => { + const starting = window.__starting; + if (url === starting?.url) starting.revoked = { src: starting.node.hasAttribute('src'), connected: starting.node.isConnected, resets: starting.node.__resets ?? 0 }; + return revoke(url); + }; + }); + await openInspect(page); + await page.evaluate(() => { + const input = document.querySelector('#local-file'); + new MutationObserver((records, observer) => { + const video = document.querySelector('#viewer-dialog video'); + if (!video) return; + observer.disconnect(); + const replace = () => { + window.__starting = { node: video, url: video.getAttribute('src'), readyState: video.readyState }; + input.files = window.__pngFiles; input.dispatchEvent(new Event('change', { bubbles: true })); + window.__starting.replaced = { connected: video.isConnected, src: video.hasAttribute('src'), resets: video.__resets ?? 0 }; + }; + video.addEventListener('loadstart', () => { if (window.__delay) setTimeout(replace, window.__delay); else replace(); }, { once: true }); + }).observe(document.body, { childList: true, subtree: true }); + }); + const png = parityPng('f02.png'); + await page.evaluate(bytes => { + const transfer = new DataTransfer(); + 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.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); + // Starting when replaced: still on the page with its URL, never reset; settled: released at once. + expect(replaced).toEqual(readyState === 0 ? { connected: true, src: true, resets: 0 } : { connected: false, src: false, resets: 1 }); + expect(await page.evaluate(() => window.__starting.node.__resets)).toBe(1); + // Its one reset came after it had metadata, never while it was starting. + expect(await page.evaluate(() => window.__starting.node.__resetReadyState)).toBeGreaterThanOrEqual(1); + // The new file's viewer opens after the old one closed, and the page still answers. + await expect(viewer(page).getByRole('img', { name: 'f02.png', exact: true })).toBeVisible({ timeout: 8000 }); + await expect(viewer(page).locator('video')).toHaveCount(0); + if (attempt === 0) await saveCoverage(page, browserName); + } finally { + await page.close(); + } + } +}); + +/** #483: holds the viewer's video in HAVE_NOTHING (its `loadedmetadata` never fires) so a hold can be driven by hand. */ +async function stallViewerVideo(page) { + await page.addInitScript(() => { + const describe = Object.getOwnPropertyDescriptor(HTMLMediaElement.prototype, 'readyState'); + Object.defineProperty(HTMLMediaElement.prototype, 'readyState', { configurable: true, get() { + return this.closest?.('#viewer-dialog') && window.__stallViewer ? 0 : describe.get.call(this); + } }); + window.addEventListener('loadedmetadata', event => { + if (window.__stallViewer && event.target.closest?.('#viewer-dialog')) event.stopImmediatePropagation(); + }, true); + }); +} + +test('a held viewer closed by hand, or replaced twice, opens only the latest file\'s viewer at once (#483)', async ({ page }) => { + await stallViewerVideo(page); + await openInspect(page); + await page.evaluate(() => { window.__stallViewer = true; }); + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(viewer(page).locator('video')).toBeAttached(); + // Two selections while the viewer's video is (made to look) still starting: the viewer stays, untouched. + await chooser(page).setInputFiles(parityPng('f02.png', 'first.png')); + await chooser(page).setInputFiles(parityPng('f02.png', 'second.png')); + await expect(preview(page).locator('img.local-file-preview')).toBeVisible(); + await expect(viewer(page).locator('video')).toBeAttached(); + // Open file metadata pressed meanwhile (the page behind the viewer is inert, so by script): the latest request. + await action(page).evaluate(node => node.click()); + // Closing it by hand ends the hold at once: the latest file's viewer opens, once, well before D62's 15 s. + await page.keyboard.press('Escape'); + await expect(viewer(page).getByRole('img', { name: 'second.png', exact: true })).toBeVisible({ timeout: 5000 }); + await expect(viewer(page).getByRole('img', { name: 'first.png', exact: true })).toHaveCount(0); + await expect(viewer(page).locator('video')).toHaveCount(0); + await expect(page.locator('dialog[open]')).toHaveCount(1); +}); + +test('a held viewer revokes its URL only once it is gone, drops a stale open, and ends on page leave (#483)', async ({ page }) => { + await stallViewerVideo(page); + await openInspect(page); + await trackReleases(page); + await page.evaluate(() => { window.__stallViewer = true; }); + const videoUrl = () => page.evaluate(() => document.querySelector('#viewer-dialog video')?.getAttribute('src') ?? null); + const released = url => page.evaluate(url => window.__released.filter(item => item.url === url), url); + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(viewer(page).locator('video')).toBeAttached(); + const first = await videoUrl(); + await chooser(page).setInputFiles(parityPng('f02.png', 'held.png')); + await expect(preview(page).locator('img.local-file-preview')).toBeVisible(); + // The session asked to revoke the video's URL; it waits while the held viewer still plays it. + expect(await released(first)).toEqual([]); + // An Open asked for during the hold, then an AVI (which never opens the viewer by itself): the open for the earlier + // file is stale and is dropped when the hold ends. + await action(page).evaluate(node => node.click()); + const avi = { name: 'clip.avi', mimeType: 'video/x-msvideo', buffer: Buffer.concat([Buffer.from('RIFF'), Buffer.alloc(4), Buffer.from('AVI LIST'), Buffer.alloc(64)]) }; + await chooser(page).setInputFiles(avi); + await expect(page.locator('#workspace-toast')).toHaveText(TOAST); + await page.keyboard.press('Escape'); + await expect(page.locator('dialog[open]')).toHaveCount(0); + await expect.poll(() => released(first)).toEqual([{ url: first, live: 0 }]); + // Page leave while held: the viewer goes and the deferred URL is revoked with nothing holding it. + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(viewer(page).locator('video')).toBeAttached(); + const second = await videoUrl(); + await chooser(page).setInputFiles(parityPng('f02.png', 'leave.png')); + await expect(viewer(page).locator('video')).toBeAttached(); + expect(await released(second)).toEqual([]); + await page.evaluate(() => window.dispatchEvent(new PageTransitionEvent('pagehide', { persisted: true }))); + await expect(page.locator('dialog[open]')).toHaveCount(0); + await expect.poll(() => released(second)).toEqual([{ url: second, live: 0 }]); + // Another route while held: the page's dispose ends the hold and revokes the URL with nothing holding it. + await page.evaluate(() => window.dispatchEvent(new PageTransitionEvent('pageshow', { persisted: true }))); + await choose(page, localFixture('core-tags.webm', 'video/webm')); + await expect(viewer(page).locator('video')).toBeAttached(); + const third = await videoUrl(); + await chooser(page).setInputFiles(parityPng('f02.png', 'route.png')); + await expect(viewer(page).locator('video')).toBeAttached(); + await page.evaluate(() => { window.location.hash = '#/library'; }); + await expect(main(page).getByRole('heading', { level: 1, name: 'Prompt library', exact: true })).toBeVisible(); + await expect.poll(() => released(third)).toEqual([{ url: third, live: 0 }]); +}); + 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 diff --git a/web/js/workspace/inspect.mjs b/web/js/workspace/inspect.mjs index ee67d7ec..2a2ef612 100644 --- a/web/js/workspace/inspect.mjs +++ b/web/js/workspace/inspect.mjs @@ -115,9 +115,39 @@ export function mountInspect(main, { parentScope, api, sender }) { retiring.delete(url); entry.waiting.abort(); clearTimeout(entry.timer); releaseMedia(entry.preview); - if (entry.revoke) URL.revokeObjectURL(url); + if (entry.revoke) revokeObjectURL(url); entry.done(); } + // #483 (D62): the viewer's own video, when a new selection closes the viewer while that video is still starting. + // The viewer stays open until the video reports metadata or an error (or D62's 15 s), then closes; its URL, which + // the session asked to revoke, goes after that. A viewer asked for meanwhile opens then: only the latest request, + // and only if its selection is still current (an automatic open waits for its own inline video again). + let held = null; + function closeViewer() { + const node = document.querySelector('#viewer-dialog video'); + if (!node || !isStarting(node)) { viewer.close(); return; } + const waiting = new AbortController(); + held = { url: node.getAttribute('src'), revoke: false, waiting, pending: null, timer: setTimeout(finishHeld, STARTING_VIDEO_MS) }; + for (const type of ['loadedmetadata', 'error']) node.addEventListener(type, finishHeld, { once: true, signal: waiting.signal }); + } + function endHeld() { + const entry = held; + if (!entry) return null; + held = null; + entry.waiting.abort(); clearTimeout(entry.timer); + return entry; + } + function finishHeld() { + const entry = endHeld(); + if (!entry) return; + viewer.close(); + // Through the session's hook, so an inline video still starting on the same URL keeps it until it settles. + if (entry.revoke) revokeObjectURL(entry.url); + const request = entry.pending; + if (!request || scope.signal.aborted || session.entry !== request.entry || session.state.phase !== 'ready') return; + if (request.auto) autoOpen(request.entry); + else openViewer(request.trigger); + } function retire(preview, url) { const waiting = new AbortController(); const entry = { preview, waiting, revoke: false, timer: setTimeout(() => finishRetiring(url), STARTING_VIDEO_MS) }; @@ -129,6 +159,7 @@ export function mountInspect(main, { parentScope, api, sender }) { } const finishAllRetiring = () => { for (const url of [...retiring.keys()]) finishRetiring(url); }; const revokeObjectURL = url => { + if (held?.url === url) { held.revoke = true; return; } const entry = retiring.get(url); if (entry) entry.revoke = true; else URL.revokeObjectURL(url); @@ -136,7 +167,8 @@ export function mountInspect(main, { parentScope, api, sender }) { 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 }); }, - onClose: () => { viewerKey = null; } }); + // A held viewer closed by hand (Escape, Close) ends its hold at once: its video is gone. + onClose: () => { viewerKey = null; if (held) finishHeld(); } }); let viewerKey = null, media = null, mediaUrl = null, statusLine = null, rejectionLine = null, rejectionTimer = 0, announced = null, chosen = null; function statusElement(message) { @@ -209,7 +241,7 @@ export function mountInspect(main, { parentScope, api, sender }) { scope.cleanup(session.subscribe(state => { // D155: a new selection can start while the viewer shows the previous one; it closes first, so no element holds // the old URL when the session revokes it (the new file's viewer opens once it is ready). - if (viewerKey !== null && (state.phase !== 'ready' || session.entry?.key !== viewerKey)) { viewer.close(); viewerKey = null; } + if (viewerKey !== null && (state.phase !== 'ready' || session.entry?.key !== viewerKey)) { closeViewer(); viewerKey = null; } showMedia(state); showStatus(state.message); const entry = session.entry; @@ -220,17 +252,14 @@ export function mountInspect(main, { parentScope, api, sender }) { // while this selection is still the current one and no other dialog is open (the viewer is replaced). D62: // a video opens it once the inline preview has settled, so a replacement while it starts never tears down a // second starting video in the viewer (that deadlocked WebKit). - if (AUTO_OPEN.has(state.message)) whenSettled(media, () => { - if (scope.signal.aborted || session.entry !== entry || session.state.phase !== 'ready' || document.querySelector('dialog[open]:not(#viewer-dialog)')) return; - openViewer(input); - }); + if (AUTO_OPEN.has(state.message)) autoOpen(entry); // An unreadable or failed preview says so in its alert; the toast never claims metadata is available. else if (TOASTABLE.has(state.message)) showWorkspaceToast(INSPECT_COPY.toast); } })); // A replacement or page leave revokes the URL after this subscriber dropped the media; a dispose closes the // viewer (on abort), drops the media, then ends the session. - scope.cleanup(() => viewer.dispose()); + scope.cleanup(() => { const entry = endHeld(); viewer.dispose(); if (entry?.revoke) revokeObjectURL(entry.url); }); scope.cleanup(() => { releaseMedia(media); media = null; mediaUrl = null; }); scope.cleanup(finishAllRetiring); scope.cleanup(() => sessionScope.dispose()); @@ -239,9 +268,11 @@ export function mountInspect(main, { parentScope, api, sender }) { // cleared, so a page restored from the back-forward cache never names a file beside the empty selection. function leave() { clearRejection(); + const entry = endHeld(); viewer.close(); releaseMedia(media); media = null; mediaUrl = null; finishAllRetiring(); + if (entry?.revoke) revokeObjectURL(entry.url); session.clear(); chosen = null; input.value = ''; } @@ -287,10 +318,20 @@ export function mountInspect(main, { parentScope, api, sender }) { for (const type of ['loadedmetadata', 'error']) preview.node.addEventListener(type, done, { once: true, signal: settled.signal }); scope.cleanup(done); } + /** D155: the viewer opens on `entry` once its inline preview has settled, while it is still the current selection. */ + function autoOpen(entry) { + whenSettled(media, () => { + if (scope.signal.aborted || session.entry !== entry || session.state.phase !== 'ready' || document.querySelector('dialog[open]:not(#viewer-dialog)')) return; + // #483: the previous viewer is still closing around its starting video; this one opens once it has. + if (held) { held.pending = { entry, auto: true }; return; } + openViewer(input); + }); + } /** The viewer on the current selection, details showing (D155 amends D97); focus returns to `trigger` on close. */ function openViewer(trigger) { const entry = session.entry; if (!entry || session.state.phase !== 'ready') return; + if (held) { held.pending = { entry, trigger, auto: false }; return; } viewer.open({ items: [entry], selectedKey: entry.key, trigger, details: 'shown' }); viewerKey = entry.key; }