Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
5 changes: 4 additions & 1 deletion packages/playwright-core/src/server/chromium/crBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export class CRBrowser extends Browser {
private _tracingRecording = false;
private _tracingClient: CRSession | undefined;
private _userAgent: string = '';
private _attachingPreExistingTargets = false;

static async connect(parent: SdkObject, transport: ConnectionTransport, options: BrowserOptions, devtools?: CRDevTools): Promise<CRBrowser> {
// Make a copy in case we need to update `headful` property below.
Expand Down Expand Up @@ -84,6 +85,7 @@ export class CRBrowser extends Browser {
return browser;
}
browser._defaultContext = new CRBrowserContext(browser, undefined, options.persistent);
browser._attachingPreExistingTargets = true;
await Promise.all([
session.send('Target.setAutoAttach', { autoAttach: true, waitForDebuggerOnStart: true, flatten: true }).then(async () => {
// Target.setAutoAttach has a bug where it does not wait for new Targets being attached.
Expand All @@ -93,6 +95,7 @@ export class CRBrowser extends Browser {
}),
browser._defaultContext.initialize(),
]);
browser._attachingPreExistingTargets = false;
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.

Perhaps we should not wait for all pages to be initialized instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think we can, it will lead to unpredictable page list for the context right after connect.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

trying to not wait for the pages which are on the initial page and we are in connectOverCDP mode

await browser._waitForAllPagesToBeInitialized();
return browser;
}
Expand Down Expand Up @@ -190,7 +193,7 @@ export class CRBrowser extends Browser {

if (targetInfo.type === 'page' || treatOtherAsPage) {
const opener = targetInfo.openerId ? this._crPages.get(targetInfo.openerId) || null : null;
const crPage = new CRPage(session, targetInfo.targetId, context, opener, { hasUIWindow: targetInfo.type === 'page' });
const crPage = new CRPage(session, targetInfo.targetId, context, opener, { hasUIWindow: targetInfo.type === 'page', isPreExistingTarget: this._attachingPreExistingTargets });
this._crPages.set(targetInfo.targetId, crPage);
return;
}
Expand Down
13 changes: 9 additions & 4 deletions packages/playwright-core/src/server/chromium/crPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export class CRPage implements PageDelegate {
return crPage._mainFrameSession;
}

constructor(client: CRSession, targetId: string, browserContext: CRBrowserContext, opener: CRPage | null, bits: { hasUIWindow: boolean }) {
constructor(client: CRSession, targetId: string, browserContext: CRBrowserContext, opener: CRPage | null, bits: { hasUIWindow: boolean, isPreExistingTarget: boolean }) {
this._targetId = targetId;
this._opener = opener;
const dragManager = new DragManager(this);
Expand Down Expand Up @@ -106,7 +106,7 @@ export class CRPage implements PageDelegate {
this._page.setEmulatedSizeFromWindowOpen({ viewport: viewportSize, screen: viewportSize });
}

this._mainFrameSession._initialize(bits.hasUIWindow).then(
this._mainFrameSession._initialize(bits.hasUIWindow, bits.isPreExistingTarget).then(
() => this._page.reportAsNew(this._opener?._page, undefined),
error => this._page.reportAsNew(this._opener?._page, error));
}
Expand Down Expand Up @@ -437,7 +437,7 @@ class FrameSession {
]);
}

async _initialize(hasUIWindow: boolean) {
async _initialize(hasUIWindow: boolean, isPreExistingTarget: boolean) {
if (!this._page.isStorageStatePage && hasUIWindow &&
!this._crPage._browserContext._browser.isClank() &&
!this._crPage._browserContext._options.noDefaultViewport) {
Expand Down Expand Up @@ -496,6 +496,11 @@ class FrameSession {
lifecycleEventsEnabled.catch(e => {}).then(() => {
this._eventListeners.push(eventsHelper.addEventListener(this._client, 'Page.lifecycleEvent', event => this._onLifecycleEvent(event)));
});
// A pre-existing page may sit on the initial empty document forever (e.g. a tab whose
// navigation never commits), so report it instead of blocking the connection.
// See https://github.com/microsoft/playwright/issues/41093.
if (isPreExistingTarget)
this._firstNonInitialNavigationCommittedFulfill();
} else {
this._firstNonInitialNavigationCommittedFulfill();
this._eventListeners.push(eventsHelper.addEventListener(this._client, 'Page.lifecycleEvent', event => this._onLifecycleEvent(event)));
Expand Down Expand Up @@ -719,7 +724,7 @@ class FrameSession {
}
const frameSession = new FrameSession(this._crPage, session, targetId, this);
this._crPage._sessions.set(targetId, frameSession);
frameSession._initialize(false).catch(e => e);
frameSession._initialize(false, false).catch(e => e);
return;
}

Expand Down
50 changes: 50 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,56 @@ 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 pages = cdpBrowser.contexts()[0].pages();
expect(pages.length).toBe(2);
// The stuck tab is reported while still on the initial empty document.
const stuckPage = pages.find(page => page.url() === ':');
expect(stuckPage).toBeTruthy();
// Releasing the response lets the previously-stuck page finish navigating to /hang.
releaseHang();
await stuckPage!.waitForURL(server.PREFIX + '/hang');
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