Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/playwright-core/src/server/chromium/chromium.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ export class Chromium extends BrowserType {
noDefaults: options.noDefaults,
};
validateBrowserContextOptions(persistent, browserOptions);
const browser = await progress.race(CRBrowser.connect(this.attribution.playwright, transport, browserOptions));
const browser = await progress.race(CRBrowser.connect(this.attribution.playwright, transport, browserOptions, undefined, true /* connectToExistingBrowser */));
if (!options.isLocal)
browser._isBrowserCollocatedWithServer = false;
browser.on(Browser.Events.Disconnected, doCleanup);
Expand Down
15 changes: 11 additions & 4 deletions packages/playwright-core/src/server/chromium/crBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export class CRBrowser extends Browser {
private _tracingClient: CRSession | undefined;
private _userAgent: string = '';

static async connect(parent: SdkObject, transport: ConnectionTransport, options: BrowserOptions, devtools?: CRDevTools): Promise<CRBrowser> {
static async connect(parent: SdkObject, transport: ConnectionTransport, options: BrowserOptions, devtools?: CRDevTools, connectToExistingBrowser?: boolean): Promise<CRBrowser> {
// Make a copy in case we need to update `headful` property below.
options = { ...options };
const connection = new CRConnection(parent, transport, options.protocolLogger, options.browserLogsCollector);
Expand Down Expand Up @@ -93,7 +93,7 @@ export class CRBrowser extends Browser {
}),
browser._defaultContext.initialize(),
]);
await browser._waitForAllPagesToBeInitialized();
await browser._waitForAllPagesToBeInitialized(connectToExistingBrowser);
return browser;
}

Expand Down Expand Up @@ -157,8 +157,15 @@ export class CRBrowser extends Browser {
return this.options.name === 'clank';
}

async _waitForAllPagesToBeInitialized() {
await Promise.all([...this._crPages.values()].map(crPage => crPage._page.waitForInitializedOrError()));
async _waitForAllPagesToBeInitialized(connectToExistingBrowser?: boolean) {
await Promise.all([...this._crPages.values()].map(crPage => {
// When connecting to an existing browser, do not block on a page that is stuck on the initial
// empty document - its first navigation may never commit. Such a page is reported later, once
// it navigates. See https://github.com/microsoft/playwright/issues/41093.
if (connectToExistingBrowser)
return Promise.race([crPage._page.waitForInitializedOrError(), crPage._initialEmptyPagePromise]);
return crPage._page.waitForInitializedOrError();
}));
}

_onAttachedToTarget({ targetInfo, sessionId, waitingForDebugger }: Protocol.Target.attachedToTargetPayload) {
Expand Down
4 changes: 4 additions & 0 deletions packages/playwright-core/src/server/chromium/crPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

import { assert } from '@isomorphic/assert';
import { ManualPromise } from '@isomorphic/manualPromise';
import { rewriteErrorMessage } from '@isomorphic/stackTrace';
import { eventsHelper } from '@utils/eventsHelper';
import * as dialog from '../dialog';
Expand Down Expand Up @@ -63,6 +64,7 @@ export class CRPage implements PageDelegate {
private readonly _pdf: CRPDF;
private readonly _coverage: CRCoverage;
readonly _browserContext: CRBrowserContext;
readonly _initialEmptyPagePromise = new ManualPromise<void>();

// Holds window features for the next popup being opened via window.open,
// until the popup target arrives. This could be racy if two oopifs
Expand Down Expand Up @@ -496,6 +498,8 @@ class FrameSession {
lifecycleEventsEnabled.catch(e => {}).then(() => {
this._eventListeners.push(eventsHelper.addEventListener(this._client, 'Page.lifecycleEvent', event => this._onLifecycleEvent(event)));
});
if (!this._crPage._initialEmptyPagePromise.isDone())
this._crPage._initialEmptyPagePromise.resolve();
} else {
this._firstNonInitialNavigationCommittedFulfill();
this._eventListeners.push(eventsHelper.addEventListener(this._client, 'Page.lifecycleEvent', event => this._onLifecycleEvent(event)));
Expand Down
47 changes: 47 additions & 0 deletions tests/library/chromium/connect-over-cdp.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,53 @@ test('should connect to existing page with iframe and navigate', async ({ browse
}
});

test('should connect to a browser with a page stuck loading its first navigation', {
annotation: {
type: 'issue',
description: 'https://github.com/microsoft/playwright/issues/41093'
}
}, async ({ browserType, server, childProcess, waitForPort, mode }, testInfo) => {
test.skip(mode !== 'default', 'Spawns the browser executable directly');

// Hold the response so the tab's first navigation never commits; release it later.
let releaseHang = () => {};
server.setRoute('/hang', (req, res) => {
releaseHang = () => {
res.writeHead(200, { 'content-type': 'text/html' });
res.end('<html></html>');
};
});

const port = 9339 + testInfo.workerIndex;
// Launch the browser directly (not via Playwright) so that we are the only CDP client.
childProcess({
command: [browserType.executablePath(),
`--remote-debugging-port=${port}`,
`--user-data-dir=${testInfo.outputPath('cdp-user-data-dir')}`,
'--headless=new',
'--no-first-run',
'--no-default-browser-check',
'about:blank',
],
});
await waitForPort(port);

// Open a tab whose very first navigation never commits.
Comment thread
yury-s marked this conversation as resolved.
await Promise.all([
server.waitForRequest('/hang'),
fetch(`http://127.0.0.1:${port}/json/new?${server.PREFIX}/hang`, { method: 'PUT' }),
]);

// connectOverCDP must not hang waiting for the stuck page to commit a navigation.
const cdpBrowser = await browserType.connectOverCDP(`http://127.0.0.1:${port}/`);
const context = cdpBrowser.contexts()[0];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expect there is just a single about:blank page?

// The stuck tab has not committed a navigation yet, so it is not reported. Releasing the
// response lets it finish navigating to /hang and show up in the context.
releaseHang();
await expect.poll(() => context.pages().map(page => page.url())).toContain(server.PREFIX + '/hang');
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can just const page = await context.waitForEvent('page').

await cdpBrowser.close();
});

test('should connect to existing service workers', async ({ browserType, mode, server }, testInfo) => {
const port = 9339 + testInfo.workerIndex;
const browserServer = await browserType.launch({
Expand Down
Loading