Repository navigation
fix(frames): reject context waits when page closes - #43650
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| if (this._page.delegate.noUtilityWorld?.()) | ||
| world = 'main'; | ||
| return this._contextData.get(world)!.contextPromise.then(contextOrDestroyedReason => { | ||
| return this._page.openScope.race(this._contextData.get(world)!.contextPromise.then(contextOrDestroyedReason => { |
There was a problem hiding this comment.
I think this is the wrong place. Instead, when frame detaches, it should reject all contextPomise instances on itself. Similarly, when the page closes, it should detach all frames (including the main frame) and thus invalidate the context.
|
Hi, I'm the Playwright bot. I started to address the review comments on Linux: agent run |
#43650 (comment): Reuse frame-detach context invalidation for all frames on page disposal, preserve the close reason, and remove the context-acquisition race and its retry exception.
This comment has been minimized.
This comment has been minimized.
🔴 The Firefox close-reason failure is caused by this PRHi, I'm the Playwright bot. I compared the failing test on the CI merge and its base: 5/5 repetitions failed with this PR; 5/5 passed without it. DetailsThe latest merged report has one failed test and one flaky test. The Firefox failure is a regression in the revised frame-disposal change; the video flake is pre-existing. Caused by this PR
Pre-existing flake / infra
Two MCP jobs are still running; this verdict covers the reported failures and completed checks. Triaged by the Playwright bot - agent run |
This comment has been minimized.
This comment has been minimized.
Dmitry Gozman (dgozman)
left a comment
There was a problem hiding this comment.
This is now failing a test, please check.
| if (contentOrError instanceof Error) | ||
| expect(contentOrError.message).toContain(kTargetClosedErrorMessage); | ||
| else | ||
| expect(contentOrError).toBe('<html><head></head><body></body></html>'); |
There was a problem hiding this comment.
Why can this resolve with content? The iframe never loads, so this is very strange.
|
Hi, I'm the Playwright bot. I started to address the review comments on Linux: agent run |
|
Hi, I'm the Playwright bot. I started to address the review comments on Linux: agent run |
|
Hi, I'm the Playwright bot. I started to address the review comments on Linux: agent run |
#43650 (comment): Keep detaching all frames on page disposal and preserve the original error during context invalidation. #43650 (review): Fix the reproduced Firefox EX-243 failure by preserving TargetClosedError so assertions report the custom page close reason. #43650 (comment): Firefox can read the initial about:blank document before navigation commits; use a never-resolving iframe evaluation and require a target-closed error on every browser.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Dmitry Gozman (dgozman)
left a comment
There was a problem hiding this comment.
This needs a rebase now.
|
Hi, I'm the Playwright bot. I started to address the review comments on Linux: agent run |
#43650 (review): Rebased on 1d5572d; resolved the conflict by keeping detached-request cleanup on frame removal rather than page disposal, preserving page-close request errors. #43650 (comment): Already addressed; retained frame detachment and context invalidation on page disposal. #43650 (review): Retained original close errors; EX-243 and PA-051 pass across all three browsers after the rebase correction. #43650 (comment): Already addressed; retained the pending iframe evaluation and required target-closed error.
e4c04ea to
ff487bf
Compare
Test results for "MCP"5194 passed, 616 skipped Merge workflow run. |
Test results for "tests 1"1 flaky23598 passed, 705 skipped Merge workflow run. |
An uncommitted frame can wait indefinitely for an execution context after its page closes. Race context acquisition against the page's close scope and propagate target-closed errors without retrying or replacing them.
Extend the existing page-close promise-cancellation test with an unanswered iframe navigation. Verified on Linux across Chromium, Firefox and WebKit: 232 tests passed, 2 existing skips; the regression also passed in separate driver-process mode. An external DevTools target-close variant hangs without the fix and passes with it. Build and
flintpassed.Fixes: #43647
Prepared by the triage workflow.