diff --git a/config/scripts/mobile-web-app-terminal-render.test.mjs b/config/scripts/mobile-web-app-terminal-render.test.mjs index 56c0fbd4b3a..c92835b9323 100644 --- a/config/scripts/mobile-web-app-terminal-render.test.mjs +++ b/config/scripts/mobile-web-app-terminal-render.test.mjs @@ -201,6 +201,13 @@ describeRender( * * So the assertions are about the live DOM and the live paths, not about readiness. */ + /** The listeners the page holds with no terminal on it, which is what two mounts can differ by. */ + async function listenersWithNoTerminal(page) { + await page.evaluate(() => globalThis.__orcaTerminalProbe.setMounted(false)) + await page.locator('#terminal-container').waitFor({ state: 'detached', timeout: 30_000 }) + return page.evaluate(() => globalThis.__orcaListeners.snapshot()) + } + async function assertLiveTerminal(page, label) { await page.locator('#terminal-surface .xterm').waitFor({ state: 'attached', timeout: 30_000 }) expect( @@ -348,6 +355,7 @@ describeRender( const HOLD_MS = 20_000 let held = null const { page } = await openPage(PROBE_ROUTE, { + listeners: true, beforeNavigate: async (opened) => { await opened.route('**/*.js', async (route) => { const response = await route.fetch() @@ -385,6 +393,18 @@ describeRender( }) await openProbeTerminal(page) await assertLiveTerminal(page, 'reload-during-import') + + // And the mount the Reload abandoned has to have come to nothing. Its chunk arrives while + // the second mount is running on the same scope, so a build that resumed without re-reading + // the claim would install its listeners into this page and reset the live mount's scope, + // nulling the undo that takes the error reporter off. Read against a page that mounted once + // and disposed once: the abandoned mount is the only difference between them, so zero + // difference is the abandoned mount having touched nothing. + const afterAbandoned = await listenersWithNoTerminal(page) + const control = await openTerminal({ listeners: true }) + await openProbeTerminal(control.page) + expect(afterAbandoned).toEqual(await listenersWithNoTerminal(control.page)) + await control.page.close() await page.unrouteAll({ behavior: 'ignoreErrors' }) await page.close() }, 300_000) @@ -402,12 +422,7 @@ describeRender( // once leaks again and the two disagree. const { page } = await openTerminal({ listeners: true }) await openProbeTerminal(page) - const withNoTerminal = async () => { - await page.evaluate(() => globalThis.__orcaTerminalProbe.setMounted(false)) - await page.locator('#terminal-container').waitFor({ state: 'detached', timeout: 30_000 }) - return page.evaluate(() => globalThis.__orcaListeners.snapshot()) - } - const before = await withNoTerminal() + const before = await listenersWithNoTerminal(page) await page.evaluate(() => { globalThis.__orcaTerminalReady = false globalThis.__orcaTerminalProbe.setMounted(true) @@ -418,7 +433,7 @@ describeRender( }) await openProbeTerminal(page) const whileLive = await page.evaluate(() => globalThis.__orcaListeners.snapshot()) - const after = await withNoTerminal() + const after = await listenersWithNoTerminal(page) // The precondition: a mount that installed nothing would satisfy the equality below for // exactly the reason the case exists to refuse. diff --git a/mobile/src/terminal/terminal-web-document-mount.ts b/mobile/src/terminal/terminal-web-document-mount.ts index 5d5748b4440..bd80e3b06ae 100644 --- a/mobile/src/terminal/terminal-web-document-mount.ts +++ b/mobile/src/terminal/terminal-web-document-mount.ts @@ -35,6 +35,11 @@ export type TerminalWebDocument = { * caller that had to await the import to get a handle would have nothing to dispose while the * import was in flight. That is not a corner — a slow chunk is what the readiness watchdog is * for, and the overlay's Reload is what ruling 20 names as the way out of it. + * + * A mount disposed before its import landed resolves rather than rejecting. Nothing failed: + * the caller asked for the terminal and then asked for it to go away, and the chunk arriving + * afterwards is not an error to report. The caller learns which it got from `dispose` being + * the thing it called, not from this. */ ready: Promise } @@ -155,19 +160,23 @@ export function mountTerminalWebDocument( throw error } - const ready = buildTerminalWebDocument(host, receive).then( + const ready = buildTerminalWebDocument(host, receive, token).then( (built) => { - if (liveDocument !== token) { - // Disposed while the import was in flight. The page is already someone else's, so this - // releases nothing and starts nothing; the modules it resolved are inert until started. + if (built === null) { + // Disposed while the import was in flight, and the build stopped at the await without + // touching anything. Nothing to keep and nothing to give back. return } started = built }, (error: unknown) => { // The import failed, so nothing was started and the page has to go back — the overlay's - // Reload is a second mount and it must be allowed to make one. - release() + // Reload is a second mount and it must be allowed to make one. Only if the page is still + // this mount's: a later mount may already hold it, and emptying its host would take the + // terminal that is on the screen. + if (liveDocument === token) { + release() + } throw error } ) @@ -214,11 +223,26 @@ function teardownStartedDocument({ modules, onWindowResize }: StartedDocument) { scope.committedTerm = null } +/** + * The document, built and started — or `null` if the page stopped being this mount's. + * + * The token is read again the instant the import lands, before anything below it runs. Every + * statement after this point writes shared state: the six seams are fields on a module-singleton + * scope, `startPageDocumentModules` resets that scope and installs listeners, and the resize + * listener outlives the host. A mount disposed while its chunk was in flight owns none of it, and + * running the body anyway would plant its elements' listeners into a page a later mount is using + * and reset that mount's scope out from under it. Checking only when this resolves is too late: + * by then the writes have happened and the caller can do nothing but discard the result. + */ async function buildTerminalWebDocument( host: HTMLElement, - receive: (message: Record) => void -): Promise { + receive: (message: Record) => void, + token: symbol +): Promise { const documentModules = await import('./document/page-document-modules') + if (liveDocument !== token) { + return null + } const { scope } = documentModules // Ruling 19 reaches `window.onerror` too: the WebView's document owns its page and may take diff --git a/mobile/src/terminal/terminal-web-document-single-mount.test.ts b/mobile/src/terminal/terminal-web-document-single-mount.test.ts index 05944d65ea7..af9a371c361 100644 --- a/mobile/src/terminal/terminal-web-document-single-mount.test.ts +++ b/mobile/src/terminal/terminal-web-document-single-mount.test.ts @@ -1,5 +1,5 @@ // @vitest-environment happy-dom -import { beforeEach, describe, expect, it } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { mountTerminalWebDocument } from './terminal-web-document-mount' /** @@ -165,6 +165,63 @@ describe('the page terminal document', () => { live.dispose() }) + it('touches nothing when it is disposed before its chunk lands', async () => { + // The window the synchronous handle opened. `dispose` can now run while the import is still + // unresolved, so the build resumes on a page it no longer owns — and everything after its + // await writes shared state: the six seams are fields on one module scope, and + // `startPageDocumentModules` resets that scope and installs the document's listeners. A mount + // that checked only when it resolved would have done all of that first and then discarded the + // result, leaving the listeners behind and a later mount's scope reset out from under it. + const host = document.createElement('div') + document.body.appendChild(host) + const { scope } = await import('./document/page-document-modules') + const seamsBefore = { + postToHost: scope.postToHost, + installErrorReporter: scope.installErrorReporter, + paintDocumentBackground: scope.paintDocumentBackground, + createTerminal: scope.createTerminal, + createUnicode11Addon: scope.createUnicode11Addon, + createWebglAddon: scope.createWebglAddon + } + const generationBefore = scope.terminalGeneration + + const mounted = mountTerminalWebDocument(host, () => {}) + mounted.dispose() + // Armed after the dispose, so anything they catch is the resuming build and nothing else. + const listeners = vi.spyOn(EventTarget.prototype, 'addEventListener') + const timers = vi.spyOn(globalThis, 'setTimeout') + const frames = vi.spyOn(globalThis, 'requestAnimationFrame') + try { + // Resolves: the caller asked for the terminal and then asked for it to go away, so the + // chunk landing afterwards is not a failure to report to the error overlay. + await expect(mounted.ready).resolves.toBeUndefined() + } finally { + listeners.mockRestore() + timers.mockRestore() + frames.mockRestore() + } + + expect(listeners).not.toHaveBeenCalled() + expect(timers).not.toHaveBeenCalled() + expect(frames).not.toHaveBeenCalled() + expect({ + postToHost: scope.postToHost, + installErrorReporter: scope.installErrorReporter, + paintDocumentBackground: scope.paintDocumentBackground, + createTerminal: scope.createTerminal, + createUnicode11Addon: scope.createUnicode11Addon, + createWebglAddon: scope.createWebglAddon + }).toEqual(seamsBefore) + // `startPageDocumentModules` resets the scope, which carries this forward by one. Unchanged + // is the start sequence never having run. + expect(scope.terminalGeneration).toBe(generationBefore) + // And the page is free, which is what the overlay's Reload needs. + const remounted = mountTerminalWebDocument(host, () => {}) + await remounted.ready + expect(host.querySelector('#terminal-container')).not.toBe(null) + remounted.dispose() + }) + it('gives the page back when the mount itself fails, so Reload can try again', async () => { // The overlay's Reload path. A mount that threw holds nothing, and a flag left set would // refuse every later attempt — the document's chunk failing to load is exactly that case.