fix(browsercontext): set closedStatus to closed after close - #41806
Conversation
_didCloseInternal guarded on status === 'closed' but never set that value, so concurrent browserClosed and close both re-ran cleanup and emitted Close twice. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
Dmitry Gozman (dgozman)
left a comment
There was a problem hiding this comment.
I don't think this test brings any value, but the fix itself is nice. Could you please remove the test?
dgozman: the fix is fine; the test does not add value.
Removed the test in 8842642. The one-line |
Test results for "MCP"3 failed 7757 passed, 1249 skipped Merge workflow run. |
Test results for "tests 1"1 flaky49910 passed, 1158 skipped Merge workflow run. |
Dmitry Gozman (dgozman)
left a comment
There was a problem hiding this comment.
Thank you for the PR, looks good.
7a94de6
into
microsoft:main
Summary
BrowserContext._didCloseInternalguards on_closedStatus === 'closed', but that status was never set. ConcurrentbrowserClosed()andclose()(documented in the comment) both ran cleanup and emittedClosetwice (tracing.abort()twice, double fulfill of the close promise).Fix
Set
_closedStatus = 'closed'at the start of_didCloseInternalafter the early return, so a second entry is a no-op.Evidence (red / green)
Test plan
browserClosedemits servercloseoncebrowsercontext-basicsmoke still passes