fix(chromium): do not hang on pages without a renderer when connecting over CDP - #42936
Merged
Merged
Conversation
…g over CDP When connecting to a browser where a tab has no renderer, e.g. because it was discarded by the Memory Saver or its renderer crashed, renderer-bound initialization commands are never answered and connectOverCDP hangs. - Send Inspector.enable during page initialization, so that Chromium reports Inspector.targetCrashed for such a page. - Ignore responses to commands that were already rejected by the crash, instead of asserting on them. - Do not report a page that crashed before it was initialized. Fixes microsoft#41714.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Send Inspector.enable and skip crashed pages only for pages attached while connecting to an existing browser, tracked by BrowserContext._skipCrashedPages on the default context. A new page might not have a renderer yet, e.g. an Electron window before its first navigation, which is not a crash. Inspector.enable is now the first command sent to such a page, so that the crash is reported as early as possible. The discard test compares against the actual url of the discards page, which Edge rewrites to edge://discards/.
Dmitry Gozman (dgozman)
force-pushed
the
fix-41714
branch
from
September 29, 2026 11:37
e35f62b to
6458ae7
Compare
Contributor
Test results for "tests 1"4 flaky52396 passed, 1243 skipped Merge workflow run. |
Contributor
Test results for "MCP"1 failed 8828 passed, 1480 skipped Merge workflow run. |
Contributor
|
Hi, I'm the Playwright bot and I took a first look at the CI failure here. 🟢 The one failure is a pre-existing flake, unrelated to this PR
DetailsThe latest merged report for Pre-existing flake / infra
Triaged by the Playwright bot - agent run |
Contributor
Test results for "tests 2"4 failed 45 flaky107752 passed, 4646 skipped Merge workflow run. |
Contributor
|
Hi, I'm the Playwright bot and I took a first look at the latest "tests 2" failures. 🟡 Three failures are known flakes; one Android failure is unproven but likely unrelatedThe two Firefox failures and DetailsThe PR's product changes only run while connecting over CDP. The new code checks Pre-existing flake / infra
Uncertain
Triaged by the Playwright bot - agent run |
Yury Semikhatsky (yury-s)
approved these changes
Sep 29, 2026
| if (this._lifecycle === 'crashed' && this.browserContext._skipCrashedPages) { | ||
| // When connecting, any crashed/discarded/unloaded page is not reported to the client at all. | ||
| // This is not a default behavior to avoid false positives when a newly created | ||
| // page is navigating slowly for whatever reason. TODO: fix in chromium upstream. |
There was a problem hiding this comment.
If it is just navigating slowly, it should not be marked as 'crashed', no?
Collaborator
Author
There was a problem hiding this comment.
Yes, indeed. That's the upstream issue we should fix.
| for (const initScript of this._crPage._page.allInitScripts()) | ||
| promises.push(this._evaluateOnNewDocument(initScript, 'main', true /* runImmediately */)); | ||
| } | ||
| if (inspectorEnabled) |
There was a problem hiding this comment.
Do we actually get a response if the tab is unloaded?
Collaborator
Author
There was a problem hiding this comment.
Yes, we get it from the browser process.
Dmitry Gozman (dgozman)
merged commit Sep 30, 2026
132be89
into
microsoft:main
70 of 75 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When connecting to a browser where a tab has no renderer, e.g. because it was discarded by the Memory Saver or its renderer crashed, renderer-bound initialization commands are never answered and
connectOverCDPhangs.Inspector.enablefor pages attached while connecting to an existing browser, so that Chromium reportsInspector.targetCrashedfor such a page. Chromium already does this in response toInspector.enable, but we never sent it. This is the first command sent to such a page.Inspector.targetCrashedbefore the response toInspector.enable, so by the time the response arrives its callback is gone.Both behaviors are limited to pages attached while connecting, tracked by
BrowserContext._skipCrashedPageson the default context. A new page might not have a renderer yet, e.g. an Electron window before its first navigation, and must not be treated as crashed.Tests: one crashes a page through
chrome://crashbefore reconnecting, and one discards a tab throughchrome://discards(from #42930). The latter compares against the actual url of the discards page, since Edge rewrites it toedge://discards/.Fixes #41714.