fix(chromium): do not let an unresponsive pre-existing page stall connectOverCDP

packages/playwright-core/src/server/chromium/crBrowser.ts

@@ -41,6 +41,12 @@ import type { BrowserOptions } from '../browser';
 import type { SdkObject } from '../instrumentation';
 import type * as channels from '../channels';
 
+// How long connect waits for a pre-existing page's renderer to answer before leaving it to
+// initialize in the background.
+const kUnresponsivePageTimeout = 3000;
+
+export type ContextUpdateKind = 'extraHTTPHeaders' | 'offline' | 'httpCredentials' | 'requestInterception' | 'geolocation' | 'userAgent' | 'initScripts' | 'playwrightBinding';
+
 export class CRBrowser extends Browser {
   readonly _connection: CRConnection;
   _session: CRSession;
@@ -159,7 +165,10 @@ export class CRBrowser extends Browser {
   }
 
   async _waitForAllPagesToBeInitialized() {
-    await Promise.all([...this._crPages.values()].map(crPage => crPage._page.waitForInitializedOrError()));
+    // A pre-existing tab whose renderer is frozen or asleep (Memory Saver, sleeping tabs, a hung
+    // renderer) never answers page-level CDP. Waiting on it would hold the connection forever,
+    // so wait only for pages whose renderer responds.
+    await Promise.all([...this._crPages.values()].map(crPage => crPage._waitForInitializedIfResponsive(kUnresponsivePageTimeout)));
   }
 
   _onAttachedToTarget({ targetInfo, sessionId, waitingForDebugger }: Protocol.Target.attachedToTargetPayload) {
@@ -342,6 +351,12 @@ export class CRBrowserContext extends BrowserContext<CREventsMap> {
   static CREvents = CREvents;
 
   declare readonly _browser: CRBrowser;
+  // Counts, per kind, the context-wide updates that are pushed to initialized pages only, so that a
+  // page still initializing can tell which ones it missed. See CRPage._syncContextUpdatesMissedDuringInitialization.
+  readonly _updateCounts: Record<ContextUpdateKind, number> = {
+    extraHTTPHeaders: 0, offline: 0, httpCredentials: 0, requestInterception: 0,
+    geolocation: 0, userAgent: 0, initScripts: 0, playwrightBinding: 0,
+  };
 
   constructor(browser: CRBrowser, browserContextId: string | undefined, options: types.BrowserContextOptions) {
     super(browser, options, browserContextId);
@@ -484,11 +499,13 @@ export class CRBrowserContext extends BrowserContext<CREventsMap> {
   async setGeolocation(geolocation?: types.Geolocation): Promise<void> {
     verifyGeolocation(geolocation);
     this._options.geolocation = geolocation;
+    ++this._updateCounts.geolocation;
     for (const page of this.pages())
       await (page.delegate as CRPage).updateGeolocation();
   }
 
   async doUpdateExtraHTTPHeaders(): Promise<void> {
+    ++this._updateCounts.extraHTTPHeaders;
     for (const page of this.pages())
       await (page.delegate as CRPage).updateExtraHTTPHeaders();
     for (const sw of this.serviceWorkers())
@@ -497,13 +514,15 @@ export class CRBrowserContext extends BrowserContext<CREventsMap> {
 
   async setUserAgent(userAgent: string | undefined): Promise<void> {
     this._options.userAgent = userAgent;
+    ++this._updateCounts.userAgent;
     await Promise.all([
       ...this.pages().map(page => (page.delegate as CRPage).updateUserAgent()),
       ...this.serviceWorkers().map(sw => sw.updateUserAgent()),
     ]);
   }
 
   async doUpdateOffline(): Promise<void> {
+    ++this._updateCounts.offline;
     for (const page of this.pages())
       await (page.delegate as CRPage).updateOffline();
     for (const sw of this.serviceWorkers())
@@ -512,23 +531,27 @@ export class CRBrowserContext extends BrowserContext<CREventsMap> {
 
   async doSetHTTPCredentials(httpCredentials?: HttpCredentials[]): Promise<void> {
     this._options.httpCredentials = httpCredentials;
+    ++this._updateCounts.httpCredentials;
     for (const page of this.pages())
       await (page.delegate as CRPage).updateHttpCredentials();
     for (const sw of this.serviceWorkers())
       await (sw as CRServiceWorker).updateHttpCredentials();
   }
 
   async doAddInitScript(initScript: InitScript) {
+    ++this._updateCounts.initScripts;
     for (const page of this.pages())
       await (page.delegate as CRPage).addInitScript(initScript);
   }
 
   async doRemoveInitScripts(initScripts: InitScript[]) {
+    ++this._updateCounts.initScripts;
     for (const page of this.pages())
       await (page.delegate as CRPage).removeInitScripts(initScripts);
   }
 
   async doUpdateRequestInterception(): Promise<void> {
+    ++this._updateCounts.requestInterception;
     for (const page of this.pages())
       await (page.delegate as CRPage).updateRequestInterception();
     for (const sw of this.serviceWorkers())
@@ -544,6 +567,7 @@ export class CRBrowserContext extends BrowserContext<CREventsMap> {
   }
 
   override async doExposePlaywrightBinding() {
+    ++this._updateCounts.playwrightBinding;
     for (const page of this._crPages())
       await page.exposePlaywrightBinding();
   }

packages/playwright-core/src/server/chromium/crPage.ts

@@ -16,6 +16,7 @@
  */
 
 import { assert } from '@isomorphic/assert';
+import { ManualPromise } from '@isomorphic/manualPromise';
 import { rewriteErrorMessage } from '@utils/stackTrace';
 import { eventsHelper } from '@utils/eventsHelper';
 import * as dialog from '../dialog';
@@ -38,6 +39,7 @@ import { isSessionClosedError } from '../protocolError';
 import { startAutomaticVideoRecording } from '../videoRecorder';
 import { nullProgress } from '../progress';
 
+import type { ContextUpdateKind } from './crBrowser';
 import type { CRSession } from './crConnection';
 import type { Protocol } from './protocol';
 import type { RegisteredListener } from '@utils/eventsHelper';
@@ -86,6 +88,8 @@ export class CRPage implements PageDelegate {
   // of their Page.windowOpen events is not guaranteed to match the order
   // of new popup targets.
   readonly _nextWindowOpenPopupFeatures: string[][] = [];
+  // Context-wide update counts when this page started initializing, see CRBrowserContext._updateCounts.
+  private readonly _initialUpdateCounts: Record<ContextUpdateKind, number>;
 
   static mainFrameSession(page: Page): FrameSession {
     const crPage = page.delegate as CRPage;
@@ -102,6 +106,7 @@ export class CRPage implements PageDelegate {
     this._pdf = new CRPDF(client);
     this._coverage = new CRCoverage(client);
     this._browserContext = browserContext;
+    this._initialUpdateCounts = { ...browserContext._updateCounts };
     this._page = new Page(this, browserContext);
     // Create a unique utility world for this Playwright instance, just in case there
     // are multiple instances of Playwright connected to the same browser page.
@@ -122,11 +127,48 @@ export class CRPage implements PageDelegate {
         this._page.setEmulatedSizeFromWindowOpen({ viewport: viewportSize, screen: viewportSize });
     }
 
-    this._mainFrameSession._initialize(bits.hasUIWindow).then(
+    this._mainFrameSession._initialize(bits.hasUIWindow).then(() => this._syncContextUpdatesMissedDuringInitialization()).then(
         () => this._page.reportAsNew(this._opener?._page, undefined),
         error => this._page.reportAsNew(this._opener?._page, error));
   }
 
+  // Context-wide updates only reach initialized pages, while initialization reads the context
+  // state once, up front. A page that initializes slowly - for example a pre-existing tab that
+  // was asleep when connectOverCDP attached - would otherwise miss updates made in between.
+  // Re-apply only the kinds that actually changed: some updates are not no-ops when repeated
+  // (a user agent override, clearing geolocation) and must not touch a page nobody asked about.
+  private async _syncContextUpdatesMissedDuringInitialization() {
+    const missed = (kind: ContextUpdateKind) => this._browserContext._updateCounts[kind] !== this._initialUpdateCounts[kind];
+    const updates: Record<ContextUpdateKind, () => Promise<void>> = {
+      extraHTTPHeaders: () => this.updateExtraHTTPHeaders(),
+      offline: () => this.updateOffline(),
+      httpCredentials: () => this.updateHttpCredentials(),
+      requestInterception: () => this.updateRequestInterception(),
+      geolocation: () => this.updateGeolocation(),
+      userAgent: () => this.updateUserAgent(),
+      initScripts: () => this._forAllFrameSessions(frame => frame._syncInitScripts()),
+      playwrightBinding: () => this.exposePlaywrightBinding(),
+    };
+    await Promise.all((Object.keys(updates) as ContextUpdateKind[]).filter(missed).map(kind => updates[kind]()));
+  }
+
+  // Resolves once the page is initialized, or right away if its renderer has not answered a
+  // single page-level command within `timeout`. A frozen or sleeping tab never answers, and
+  // keeps initializing in the background; it is reported through the page event if it wakes.
+  async _waitForInitializedIfResponsive(timeout: number) {
+    const initialized = this._page.waitForInitializedOrError();
+    let timer: NodeJS.Timeout | undefined;
+    const unresponsive = new Promise<boolean>(f => timer = setTimeout(() => f(false), timeout));
+    const responsive = await Promise.race([
+      initialized.then(() => true),
+      this._mainFrameSession._rendererResponded.then(() => true),
+      unresponsive,
+    ]);
+    clearTimeout(timer);
+    if (responsive)
+      await initialized;
+  }
+
   private async _forAllFrameSessions(cb: (frame: FrameSession) => Promise<any>) {
     const frameSessions = Array.from(this._sessions.values());
     await Promise.all(frameSessions.map(frameSession => {
@@ -415,6 +457,8 @@ class FrameSession {
   _metricsOverride: Protocol.Emulation.setDeviceMetricsOverrideParameters | undefined;
   private _workerSessions = new Map<string, CRSession>();
   private _initScriptIds = new Map<InitScript, string>();
+  // Page.getFrameTree is answered by the renderer, so a reply proves the renderer is responsive.
+  readonly _rendererResponded = new ManualPromise<void>();
   private _bufferedAttachedToTargetEvents: Protocol.Target.attachedToTargetPayload[] | undefined;
 
   constructor(crPage: CRPage, client: CRSession, targetId: string, parentSession: FrameSession | null) {
@@ -499,6 +543,7 @@ class FrameSession {
     const promises: Promise<any>[] = [
       this._client.send('Page.enable'),
       this._client.send('Page.getFrameTree').then(({ frameTree }) => {
+        this._rendererResponded.resolve();
         if (this._isMainFrame()) {
           this._handleFrameTree(frameTree);
           this._addRendererListeners();
@@ -1090,6 +1135,12 @@ class FrameSession {
     await Promise.all(ids.map(identifier => this._client.send('Page.removeScriptToEvaluateOnNewDocument', { identifier }).catch(() => {}))); // target can be closed
   }
 
+  async _syncInitScripts(): Promise<void> {
+    const wanted = new Set(this._crPage._page.allInitScripts());
+    await this._removeEvaluatesOnNewDocument([...this._initScriptIds.keys()].filter(script => !wanted.has(script)));
+    await Promise.all([...wanted].filter(script => !this._initScriptIds.has(script)).map(script => this._evaluateOnNewDocument(script, 'main')));
+  }
+
   async exposePlaywrightBinding() {
     await this._client.send('Runtime.addBinding', { name: PageBinding.kBindingName });
   }

tests/library/chromium/connect-over-cdp.spec.ts

@@ -781,3 +781,53 @@ test('should connect over CDP using a ConnectionTransport', async ({ browserType
     await browserServer.close();
   }
 });
+
+test('should not wait for a pre-existing page whose renderer never responds', async ({ browserType, server }, testInfo) => {
+  const port = 9339 + testInfo.workerIndex;
+  const browserServer = await browserType.launch({ args: ['--remote-debugging-port=' + port] });
+  try {
+    const first = await browserType.connectOverCDP(`http://127.0.0.1:${port}/`);
+    const responsive = await first.contexts()[0].newPage();
+    await responsive.goto(server.EMPTY_PAGE);
+    // A renderer that is busy forever stands in for a tab frozen by Memory Saver: it never answers page-level CDP.
+    const frozen = await first.contexts()[0].newPage();
+    await frozen.goto('data:text/html,<script>setTimeout(() => { for (;;) {} }, 100)</script>');
+    await new Promise(f => setTimeout(f, 500));
+
+    const second = await browserType.connectOverCDP(`http://127.0.0.1:${port}/`, { timeout: 15000 });
+    const urls = second.contexts()[0].pages().map(page => page.url());
+    expect(urls).toContain(server.EMPTY_PAGE);
+    expect(urls.filter(url => url.startsWith('data:'))).toEqual([]);
+    await second.close();
+    await first.close();
+  } finally {
+    await browserServer.close();
+  }
+});
+
+test('should report a pre-existing page once its renderer responds, with context updates made meanwhile', async ({ browserType, server }, testInfo) => {
+  const port = 9339 + testInfo.workerIndex;
+  const browserServer = await browserType.launch({ args: ['--remote-debugging-port=' + port] });
+  try {
+    const first = await browserType.connectOverCDP(`http://127.0.0.1:${port}/`);
+    const sleeping = await first.contexts()[0].newPage();
+    // Busy for well past the grace period connect gives an unresponsive page, then wakes up.
+    await sleeping.goto('data:text/html,<script>setTimeout(() => { const end = Date.now() + 6000; while (Date.now() < end) {} }, 100)</script>');
+    await new Promise(f => setTimeout(f, 500));
+
+    const second = await browserType.connectOverCDP(`http://127.0.0.1:${port}/`, { timeout: 15000 });
+    const context = second.contexts()[0];
+    expect(context.pages().filter(page => page.url().startsWith('data:'))).toEqual([]);
+    const pagePromise = context.waitForEvent('page', { timeout: 15000 });
+    // Added while the page is still unresponsive, so it only reaches the page once it wakes.
+    await context.addInitScript(() => (window as any).__addedWhileAsleep = 'yes');
+    const woken = await pagePromise;
+    expect(woken.url()).toMatch(/^data:/);
+    await woken.goto(server.EMPTY_PAGE);
+    expect(await woken.evaluate(() => (window as any).__addedWhileAsleep)).toBe('yes');
+    await second.close();
+    await first.close();
+  } finally {
+    await browserServer.close();
+  }
+});