fix(chromium): do not let an unresponsive pre-existing page stall connectOverCDP - #42883
Closed
danbao (danbao) wants to merge 2 commits into
Closed
danbao (danbao) wants to merge 2 commits into
danbao (danbao) wants to merge 2 commits into
Conversation
…nectOverCDP connectOverCDP waited for every pre-existing page to finish initializing. A page whose renderer is frozen or asleep (Memory Saver, sleeping tabs, a hung renderer) never answers page-level CDP, so a single such tab held the connection until the overall timeout. noDefaults did not help. Connect now waits only for pages whose renderer responds, judged by a reply to Page.getFrameTree. A page that stays silent for 3 seconds keeps initializing in the background and is reported through the regular page event if it ever responds. Context-wide updates reach initialized pages only, while initialization reads context state once, up front, so such a page would miss updates made while it was asleep. Pages now record per-kind update counts when they start initializing and re-apply just the kinds that changed before being reported. Only changed kinds are re-applied because some updates are not no-ops when repeated: a user agent override, or clearing geolocation, must not touch a page nobody asked about. This also closes the same, much narrower, race for any page that initializes while the context is being updated. References microsoft#41093, microsoft#42730, microsoft#41714.
Author
|
@microsoft-github-policy-service agree |
Collaborator
|
Closing in favor of #42936. |
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
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.
Fixes #42882: a single pre-existing tab that no longer answers page-level CDP holds
connectOverCDPuntil the overall timeout. The same hang was reported earlier in #41093 and #42730.Cause
CRBrowser.connectawaits_waitForAllPagesToBeInitialized(), aPromise.alloverwaitForInitializedOrError()for every pre-existing page.FrameSession._initializeneeds replies toPage.enable,Page.getFrameTree,Runtime.enableand others, which the renderer answers. A renderer that is frozen or asleep — Memory Saver, Edge sleeping tabs, a hung renderer — answers none of them, so connect never finishes.noDefaultsskips the default overrides but still attaches and awaits every existing page, so it does not help.This is the normal state of a daily-driver browser, where
noDefaultsis meant to be used. Attaching to a long-running Chrome 153 profile on macOS, 11 of 62 auto-attached targets — later 6 of 41 after closing tabs, all ordinaryhttp(s)pages — left their whole init batch unanswered, and connect never resolved at 45, 60, or 120 s.Change
Wait only for responsive pages. A reply to
Page.getFrameTreeproves the renderer is alive; connect still waits for such a page to finish initializing, however slowly. A page that answers nothing for 3 seconds is left to initialize in the background. The existing machinery already covers the rest:context.pages()lists initialized pages only, and a page that finishes later is announced through the regularpageevent.Re-apply context updates a late page missed. Context-wide updates (
addInitScript,setExtraHTTPHeaders,setOffline,setHTTPCredentials, routing,setGeolocation,setUserAgent, bindings) are pushed topages()only, while initialization reads the context state once, up front. A page that stays asleep across such an update would otherwise wake up without it — likely the risk that kept #41128 from landing.CRBrowserContextnow counts updates per kind; eachCRPagesnapshots the counts when it starts initializing and, before being reported, re-applies only the kinds that changed. Init scripts are reconciled against the frame session's installed set, so nothing is installed twice. Only changed kinds are re-applied because some updates are not no-ops when repeated: re-sending a user agent override or clearing geolocation would alter a page nobody asked about, which matters most exactly in thenoDefaultscase.This also closes the same race, previously only a few milliseconds wide, for any page that initializes while the context is being updated.
Tests
Two new tests in
connect-over-cdp.spec.tsuse a renderer busy in a loop as a portable stand-in for a frozen tab, so no process suspension is needed:should not wait for a pre-existing page whose renderer never responds— connect resolves and the frozen page is absent frompages().should report a pre-existing page once its renderer responds, with context updates made meanwhile— the page is busy past the grace period and then wakes; an init script added while it was asleep is in effect after it is reported.Both fail without the change (
connectOverCDP: Timeout 15000ms exceeded) and pass with it.Run locally against Chromium 153 (
CRPATH), macOS arm64:npm run tsc, andeslinton the changed files: clean.connect-over-cdp.spec.ts: 31 passed. The 3 failures reproduce identically on unmodifiedmainin this environment — two env-proxy tests fail only whileNO_PROXYlists localhost and pass without it;should skip default overrides with noDefaultsfails when repeated because it downloads into the real Downloads folder.browsercontext-add-init-script,page-add-init-script,popup,browsercontext-user-agent,geolocationplus the two new tests: 55 passed.I have not run the full
ctestsuite.Open question
The 3-second grace period is a judgment call: a live renderer answers
Page.getFrameTreein milliseconds, including on loaded CI machines. Happy to make it configurable or change the value.References #41093, #41128, #42730, #41714.