Skip to content
Merged
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
1 change: 1 addition & 0 deletions packages/playwright-core/src/server/browserContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export abstract class BrowserContext<EM extends EventMap = EventMap> extends Sdk
readonly requestInterceptors: network.RouteHandler[] = [];
private _isPersistentContext: boolean;
private _closedStatus: 'open' | 'closing' | 'closed' = 'open';
_skipCrashedPages = false;
readonly _closePromise: Promise<Error>;
private _closePromiseFulfill: ((error: Error) => void) | undefined;
readonly _permissions = new Map<string, string[]>();
Expand Down
25 changes: 15 additions & 10 deletions packages/playwright-core/src/server/chromium/crBrowser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,21 @@ export class CRBrowser extends Browser {
return browser;
}
browser._defaultContext = new CRBrowserContext(browser, undefined, options.persistent);
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.
// However making a dummy call afterwards fixes this.
// This can be removed after https://chromium-review.googlesource.com/c/chromium/src/+/2885888 lands in stable.
await session.send('Target.getTargetInfo');
}),
browser._defaultContext.initialize(),
]);
await browser._waitForAllPagesToBeInitialized();
browser._defaultContext._skipCrashedPages = true;
try {
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.
// However making a dummy call afterwards fixes this.
// This can be removed after https://chromium-review.googlesource.com/c/chromium/src/+/2885888 lands in stable.
await session.send('Target.getTargetInfo');
}),
browser._defaultContext.initialize(),
]);
await browser._waitForAllPagesToBeInitialized();
} finally {
browser._defaultContext._skipCrashedPages = false;
}
return browser;
}

Expand Down
2 changes: 2 additions & 0 deletions packages/playwright-core/src/server/chromium/crConnection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ export class CRSession extends SdkObject<Protocol.EventMap & ConnectionEventMap>
}
} else if (object.id && object.error?.code === -32001) {
// Message to a closed session, just ignore it.
} else if (object.id && this._crashed) {
// _markAsCrashed() has already rejected all pending calls, nothing to do here.
} else {
assert(!object.id, object?.error?.message || undefined);
Promise.resolve().then(() => {
Expand Down
8 changes: 8 additions & 0 deletions packages/playwright-core/src/server/chromium/crPage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,12 @@ class FrameSession {
}

async _initialize(hasUIWindow: boolean) {
let inspectorEnabled: Promise<any> | undefined;
if (this._isMainFrame() && this._crPage._browserContext._skipCrashedPages) {
// Get notified with Inspector.targetCrashed right away, so we can skip pages without a renderer.
inspectorEnabled = this._client._sendMayFail('Inspector.enable');
}

const browserOptions = this._crPage._browserContext._browser.options;
if (!this._page.isStorageStatePage && hasUIWindow &&
!this._crPage._browserContext._browser.isClank() &&
Expand Down Expand Up @@ -579,6 +585,8 @@ class FrameSession {
for (const initScript of this._crPage._page.allInitScripts())
promises.push(this._evaluateOnNewDocument(initScript, 'main', true /* runImmediately */));
}
if (inspectorEnabled)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we actually get a response if the tab is unloaded?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we get it from the browser process.

promises.push(inspectorEnabled);
promises.push(this._client.send('Runtime.runIfWaitingForDebugger'));
promises.push(this._firstNonInitialNavigationCommittedPromise);
await Promise.all(promises);
Expand Down
7 changes: 7 additions & 0 deletions packages/playwright-core/src/server/page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,13 @@ export class Page extends SdkObject<PageEventMap> {
// context/browser closure. Just ignore the page.
if (this.browserContext.isClosingOrClosed())
return;
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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If it is just navigating slowly, it should not be marked as 'crashed', no?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, indeed. That's the upstream issue we should fix.

this._initializedPromise.resolve(error);
return;
}
this.frameManager.createDummyMainFrameIfNeeded();
}
this._initialized = error || this;
Expand Down
81 changes: 81 additions & 0 deletions tests/library/chromium/connect-over-cdp.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,87 @@ test('should connect to an existing cdp session', async ({ browserType, mode },
}
});

test('should connect when an existing page has no renderer', {
annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/41714' },
}, async ({ browserType, server }, testInfo) => {
const port = 9339 + testInfo.workerIndex;
const browserServer = await browserType.launch({
args: ['--remote-debugging-port=' + port]
});
try {
// Prepare a page without a renderer, e.g. one discarded by the Memory Saver.
const cdpBrowser1 = await browserType.connectOverCDP({
endpointURL: `http://127.0.0.1:${port}/`,
});
const context1 = cdpBrowser1.contexts()[0];
const healthyPage1 = await context1.newPage();
await healthyPage1.goto(server.PREFIX + '/title.html');
const crashedPage1 = await context1.newPage();
await crashedPage1.goto(server.EMPTY_PAGE);
crashedPage1.goto('chrome://crash').catch(() => {});
await crashedPage1.waitForEvent('crash');
await cdpBrowser1.close();

// Connecting again should not hang on the page without a renderer,
// and should not report it at all.
const cdpBrowser2 = await browserType.connectOverCDP({
endpointURL: `http://127.0.0.1:${port}/`,
});
const pages = cdpBrowser2.contexts()[0].pages();
expect(pages.map(page => page.url())).toEqual([server.PREFIX + '/title.html']);
expect(await pages[0].title()).toBe('Woof-Woof');
await cdpBrowser2.close();
} finally {
await browserServer.close();
}
});

test('should connect when an existing page has been discarded', {
annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright/issues/41714' },
}, async ({ browserType, createUserDataDir, server }, testInfo) => {
const port = 9339 + testInfo.workerIndex;
const userDataDir = await createUserDataDir();
// chrome://discards is an internal debug page that is only available with this pref.
fs.writeFileSync(path.join(userDataDir, 'Local State'), JSON.stringify({ internal_only_uis_enabled: true }));
const context = await browserType.launchPersistentContext(userDataDir, {
headless: false,
// Emulating the viewport of a discarded tab crashes the browser in
// WebContentsImpl::SetDeviceEmulationSize, because it has no view.
viewport: null,
args: [
'--remote-debugging-port=' + port,
// Chrome refuses to discard tabs with DevTools attached, and we attach to all of them.
'--enable-features=AllowDevtoolsConnectedDiscard',
],
});
try {
const victimUrl = server.PREFIX + '/title.html';
const victim = await context.newPage();
await victim.goto(victimUrl);
const discards = await context.newPage();
await discards.goto('chrome://discards/');
// Discarding replaces the tab's WebContents, but the url in the table survives.
const row = discards.getByRole('row', { name: victimUrl });
await row.getByText('Urgent Discard').click();
await expect(row).toContainText('discarded');
// The renderer process is shut down asynchronously after the discard.
await discards.waitForTimeout(3000);
await testInfo.attach('discards rows', { body: (await discards.getByRole('row').allInnerTexts()).map(text => text.replace(/\s+/g, ' ')).join('\n') });
const targets = await (await fetch(`http://127.0.0.1:${port}/json/list`)).json();
await testInfo.attach('targets', { body: JSON.stringify(targets, null, 2), contentType: 'application/json' });

// Connecting should not hang on the discarded page, and the page should not be reported.
const cdpBrowser = await browserType.connectOverCDP({
endpointURL: `http://127.0.0.1:${port}/`,
});
const pages = cdpBrowser.contexts()[0].pages();
expect(pages.map(page => page.url()).sort()).toEqual(['about:blank', discards.url()].sort());
await cdpBrowser.close();
} finally {
await context.close();
}
});

test('should cleanup artifacts dir after connectOverCDP disconnects due to ws close', async ({ browserType, toImpl, mode }, testInfo) => {
const port = 9339 + testInfo.workerIndex;
const browserServer = await browserType.launch({
Expand Down
Loading