fix(mobile): drop what a disposed page mount adopted, and close the fixture's three resources apart

Round 7's five items.

1. The queued-dispose case's precondition was vacuous. It read the host for a
missing container, which dispose empties on every path, so a build that returned
straight after its ownership check satisfied it. The wrapper now counts resize
adds and the case asserts exactly one, which is the document having started. Red
under that mutation, on the count.

2. The render fixture's rollback awaited its cleanup unguarded, so a cleanup that
also refused replaced the error the caller needs — the reason the setup failed.
The rollback is best-effort now and the original error is what comes back.

3. That cleanup stopped at the first throw, so a browser refusing to close took
the socket and the scratch tree with it, which is the leak the rollback exists to
prevent. Each of the three is asked independently and the first failure is
rethrown after all three have been tried.

4. The rejection case restores its `window` patch in a `finally`, as its sibling
does, so a failure part way through no longer leaves the patched functions behind
for everything that runs after it.

5. `dispose` left `started` set. `send` reads it, and what it holds names the
page's one set of document modules, so a stale handle could route a host command
into whichever document is live next. Nulled, and pinned: the stale handle pings,
and with the old code the *live* mount's `receive` answers `pong`, because the
scope's seam belongs to it by then. The precondition is the live handle's own ping
being answered, so the silence is the stale handle declining rather than the
command doing nothing.

Items 2 and 3 have no pin of their own. Both are failure paths of the cleanup
itself, reachable only by making a browser or a socket refuse to close, and
standing something in front of Playwright to do it is what the anti-slop gate
refuses in this file's suffix. The rollback's own pin still covers the path that
matters, and both changes are read by it.

Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
This commit is contained in:
Jinwoo-H
2026-09-20 18:20:08 -04:00
parent 0f09a1f556
commit 21a8f7fc4e
4 changed files with 93 additions and 37 deletions
@@ -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
@@ -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
}
})
})
@@ -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)
},
@@ -232,12 +232,14 @@ describe('the page terminal document', () => {
const host = document.createElement('div')
document.body.appendChild(host)
const listeners = new Set<unknown>()
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<typeof realAdd>) => {
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.