diff --git a/config/scripts/mobile-web-app-terminal-render-fixture.mjs b/config/scripts/mobile-web-app-terminal-render-fixture.mjs index 375f8bb06ce..a8fba22d3e5 100644 --- a/config/scripts/mobile-web-app-terminal-render-fixture.mjs +++ b/config/scripts/mobile-web-app-terminal-render-fixture.mjs @@ -56,11 +56,29 @@ const SHELL_HOST = { * the file finishes keeps the vitest worker alive after its last test has reported. */ async function closeTerminalRenderFixture({ browser, scratch, server }) { - await browser?.close() - if (server) { - await new Promise((resolve) => server.close(resolve)) + // Each one is asked independently, because stopping at the first refusal is how the socket and + // the scratch tree survived in the first place: a browser that will not close would take the + // other two down with it. The first failure is what comes back, after all three have been tried. + const failures = [] + const attempt = async (close) => { + try { + await close() + } catch (error) { + failures.push(error) + } + } + await attempt(() => browser?.close()) + await attempt( + () => + server && + new Promise((resolve, reject) => { + server.close((error) => (error ? reject(error) : resolve())) + }) + ) + await attempt(() => rm(scratch, { recursive: true, force: true })) + if (failures.length > 0) { + throw failures[0] } - await rm(scratch, { recursive: true, force: true }) } /** @@ -106,7 +124,10 @@ export async function startTerminalRenderFixture() { ...(executablePath ? { executablePath } : {}) }) } catch (error) { - await closeTerminalRenderFixture({ browser, scratch, server: served?.server }) + // Swallowed on purpose: what the caller needs is the reason the setup failed, and a cleanup + // that also refuses would replace it with something about a socket. The rollback is + // best-effort; the original error is the contract. + await closeTerminalRenderFixture({ browser, scratch, server: served?.server }).catch(() => {}) throw error } const { origin } = served diff --git a/mobile/src/terminal/terminal-web-document-mount-rejection.test.ts b/mobile/src/terminal/terminal-web-document-mount-rejection.test.ts index 81b1c18ba98..e874851fc24 100644 --- a/mobile/src/terminal/terminal-web-document-mount-rejection.test.ts +++ b/mobile/src/terminal/terminal-web-document-mount-rejection.test.ts @@ -47,34 +47,39 @@ describe('a page mount whose document chunk failed', () => { realRemove(...removed) } - const abandoned = mountTerminalWebDocument(host, () => {}) - abandoned.dispose() - // The same element, as React hands it back on the overlay's Reload. - const live = mountTerminalWebDocument(host, () => {}) - // The message is the mocking layer's, not the one thrown, so the two counters are what say - // which import did what: the abandoned mount's failed, and the live mount's did not. - await expect(abandoned.ready).rejects.toThrow() - expect(chunk.failures, 'the abandoned mount is the one whose chunk failed').toBe(1) + // Put back whatever happens, as the sibling case does: a failure part way through would + // otherwise leave the patched functions on `window` for everything that runs after it. + try { + const abandoned = mountTerminalWebDocument(host, () => {}) + abandoned.dispose() + // The same element, as React hands it back on the overlay's Reload. + const live = mountTerminalWebDocument(host, () => {}) + // The message is the mocking layer's, not the one thrown, so the two counters are what say + // which import did what: the abandoned mount's failed, and the live mount's did not. + await expect(abandoned.ready).rejects.toThrow() + expect(chunk.failures, 'the abandoned mount is the one whose chunk failed').toBe(1) - expect(host.querySelector('#terminal-container')).not.toBe(null) - expect(host.classList.contains(HOST_CLASS)).toBe(true) - // Still claimed, so the release did not hand the page back either. - expect(() => mountTerminalWebDocument(host, () => {})).toThrow( - 'the terminal document is already mounted on this page' - ) + expect(host.querySelector('#terminal-container')).not.toBe(null) + expect(host.classList.contains(HOST_CLASS)).toBe(true) + // Still claimed, so the release did not hand the page back either. + expect(() => mountTerminalWebDocument(host, () => {})).toThrow( + 'the terminal document is already mounted on this page' + ) - await live.ready - // The other half of the precondition: the mount that replaced it is a real started document, - // not a second casualty. Its resize listener is the one the start sequence adds. - expect(resizeListeners, 'the live mount started its document').toBe(1) - // And disposing the abandoned handle a second time changes nothing. - abandoned.dispose() - expect(host.querySelector('#terminal-container')).not.toBe(null) - expect(host.classList.contains(HOST_CLASS)).toBe(true) - live.dispose() - window.addEventListener = realAdd - window.removeEventListener = realRemove - expect(host.querySelector('#terminal-container')).toBe(null) - expect(resizeListeners, 'and it took its listener back on the way out').toBe(0) + await live.ready + // The other half of the precondition: the mount that replaced it is a real started document, + // not a second casualty. Its resize listener is the one the start sequence adds. + expect(resizeListeners, 'the live mount started its document').toBe(1) + // And disposing the abandoned handle a second time changes nothing. + abandoned.dispose() + expect(host.querySelector('#terminal-container')).not.toBe(null) + expect(host.classList.contains(HOST_CLASS)).toBe(true) + live.dispose() + expect(host.querySelector('#terminal-container')).toBe(null) + expect(resizeListeners, 'and it took its listener back on the way out').toBe(0) + } finally { + window.addEventListener = realAdd + window.removeEventListener = realRemove + } }) }) diff --git a/mobile/src/terminal/terminal-web-document-mount.ts b/mobile/src/terminal/terminal-web-document-mount.ts index 93d16e4f2bb..2482c263bc6 100644 --- a/mobile/src/terminal/terminal-web-document-mount.ts +++ b/mobile/src/terminal/terminal-web-document-mount.ts @@ -200,6 +200,10 @@ export function mountTerminalWebDocument( if (started) { teardownStartedDocument(started) } + // Dropped, not just torn down. `send` reads this, and the modules it names are the page's one + // singleton — so a handle that kept them would route a command into whatever document is + // live next, which is the mount that replaced this one. + started = null host.innerHTML = '' host.classList.remove(HOST_CLASS) }, 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 aff1446ec5b..9c034a7a803 100644 --- a/mobile/src/terminal/terminal-web-document-single-mount.test.ts +++ b/mobile/src/terminal/terminal-web-document-single-mount.test.ts @@ -232,12 +232,14 @@ describe('the page terminal document', () => { const host = document.createElement('div') document.body.appendChild(host) const listeners = new Set() + let adds = 0 const realAdd = window.addEventListener.bind(window) const realRemove = window.removeEventListener.bind(window) // Parameters taken from the bound original, so the wrapper carries the real signature rather // than three implicit `any`s the tests typecheck refuses. window.addEventListener = (...added: Parameters) => { if (added[0] === 'resize') { + adds += 1 listeners.add(added[1]) } realAdd(...added) @@ -260,11 +262,10 @@ describe('the page terminal document', () => { } // The precondition: the document did start, so there was something to tear down. A build that - // bailed at the ownership check would add no listener and satisfy the emptiness below for the - // one reason this case exists to refuse. - expect( - listeners.size + Number(host.querySelector('#terminal-container') === null) - ).toBeGreaterThan(0) + // returned right after its ownership check would add nothing and satisfy the emptiness below + // for the one reason this case exists to refuse. Counted on the way in rather than read off + // the host afterwards — dispose empties the host on every path, so that told us nothing. + expect(adds, 'the document started and added its resize listener').toBe(1) expect(listeners.size, 'the resize listener the started document added').toBe(0) const { scope } = await import('./document/page-document-modules') expect(scope.term).toBe(null) @@ -274,6 +275,31 @@ describe('the page terminal document', () => { remounted.dispose() }) + it('routes nothing into the document that replaced it', async () => { + // `send` reads what the mount adopted, and what it adopted names the page's one set of + // document modules. A handle that kept them after its dispose would hand a host command to + // whichever document is live next: same modules, same scope, a terminal that is not its own. + // `ping` is the cheapest way to see it, because the document answers it by posting through the + // scope's `postToHost` seam — which by then belongs to the mount that replaced this one. + const host = document.createElement('div') + document.body.appendChild(host) + const stale = mountTerminalWebDocument(host, () => {}) + await stale.ready + stale.dispose() + + const posts: unknown[] = [] + const live = mountTerminalWebDocument(host, (message) => posts.push(message.type)) + await live.ready + stale.send({ id: 4242, type: 'ping' }) + expect(posts).toEqual([]) + + // The precondition: the live document does answer a ping, so the silence above is the stale + // handle declining to speak rather than the command doing nothing. + live.send({ id: 4243, type: 'ping' }) + expect(posts).toEqual(['pong']) + live.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.