fix(client): report a failed BrowserContext.close() instead of resolving

[skvortsov-dev]

Fixes #42069.

The problem

BrowserContext.close() sets _closingStatus = 'closing' before its first await and never restores it if one of the four awaited steps throws. Since isClosed() reports 'closing' as closed, the entry guard becomes permanent: after one failed close the context can never be closed again, and every later close() — from anywhere in the process — resolves as success while doing nothing.

The context stays fully usable meanwhile (navigates, screenshots, opens new pages) and keeps being listed in browser.contexts(), so nothing in the public API tells the caller it is still there.

Reproduction (public API only, no monkey-patching):

const context = await browser.newContext({ recordHar: { path: os.tmpdir() } }); // unwritable HAR path
await (await context.newPage()).goto('about:blank');
await context.close().catch(e => console.log('rejected:', e.message));  // rejected: EISDIR ...
console.log(context.isClosed());          // true  ← wrong, the context is live
console.log(browser.contexts().length);   // 1
await context.close();                    // resolves, closes nothing — forever

The change

Remember the failure, restore the status so isClosed() stays truthful, and rethrow it on subsequent calls, so close() never claims success for a context it did not close.

	before	after
first close()	rejected	rejected (unchanged)
isClosed() after the failure	true	false
later close() calls	resolved, closed nothing	rejected with the original error
healthy context: close() twice	ok, no-op	ok, no-op (unchanged)
browser.close() afterwards	ok	ok

The added test in browsercontext-basic.spec.ts covers the first three rows; the existing close() should be callable twice covers the fourth.

Why the failure is cached rather than retried

I first tried the smaller change of only restoring _closingStatus, letting every call retry the close for real. The first two attempts then rejected correctly, but the third never settled (still pending after 60 s), so the server-side close path does not appear to survive repeated attempts once a HAR export has failed. Caching the failure keeps the client honest without depending on that.

If you would rather have real retries — which is the nicer semantic — the server side likely needs a fix underneath, and I am happy to look into that instead; just let me know which direction you prefer.

[skvortsov-dev]

@microsoft-github-policy-service agree

[skvortsov-dev]

Ran the checks from CONTRIBUTING.md locally on macOS (Chromium):

• npm run ctest -- tests/library/browsercontext-basic.spec.ts tests/library/har.spec.ts — 92 passed, 1 skipped
• npm run tsc, npx eslint on both changed files, npm run lint-tests — clean

The new test also fails as expected without the client change, on expect(context.isClosed()).toBe(false) — the pre-fix behaviour where the second close() resolves and the context keeps reporting itself as closed.

One note on portability: the test makes close() fail by pointing recordHar.path at an existing directory. HAR writing goes through fs.promises.writeFile, so the write is refused on all three platforms (EISDIR on macOS/Linux, EPERM/EISDIR on Windows) — but I could only verify macOS locally, since the workflow runs are still awaiting approval.

[pavelfeldman]

Not rejecting the safety try/catch outright, but the issue is with the har path not being validated for writing.

[skvortsov-dev]

@pavelfeldman Fair point on the HAR path — validating it upfront makes sense regardless.

The HAR was just how I triggered it, though: the shortest way to make close() reject through the public API. The poisoning doesn't care what threw — close() awaits request.dispose, runBeforeCloseBrowserContext, _exportAllHars and _channel.close, and a rejection in any of them leaves the context in a state where isClosed() says closed, the context stays fully alive, and every later close() resolves without doing anything.

I also owe you an honest update on the "we hit this in our suite" part. While building tripwires to catch which await was failing on our CI, I found that our own leaks had a different cause entirely — our harness registered its cleanup afterEach at the top level of a shared module, so module caching silently dropped it for every spec file after the worker's first. Our bug, fixed on our side.

But the tripwire work is exactly why I still think this one is worth fixing: to tell "close failed" from "close never ran" from "close succeeded", I had to instrument everything, because a rejected close() and a successful one look identical to the caller from the second call on. I also reproduced the impact end-to-end in a real @playwright/test suite: poison one context's close, let the test pass, and the poisoned context's page becomes the next failing test's test-failed-1.png — a screenshot from a different test presented as failure evidence.

So: validating the HAR path removes one trigger; keeping close() from reporting success on a context it didn't close removes the class. The patch in this PR does the latter and holds up on the repro and the library tests.

[pavelfeldman]

Closing as agentic slop - it caches close error for no good reason and masks underlying issues.