From a66d0296077a82179a2182281a5040fd96d3843c Mon Sep 17 00:00:00 2001 From: Maksim Sadym <69349599+sadym-chromium@users.noreply.github.com> Date: Thu, 30 Nov 2023 15:27:49 +0100 Subject: [PATCH] fix: end WebDriver BiDi session on disconnect (#11470) Co-authored-by: Maksim Sadym --- docs/api/puppeteer.browser.disconnect.md | 4 ++-- docs/api/puppeteer.browser.md | 2 +- packages/puppeteer-core/src/api/Browser.ts | 4 ++-- packages/puppeteer-core/src/bidi/BidiOverCdp.ts | 1 + packages/puppeteer-core/src/bidi/Browser.ts | 12 +++++++++--- packages/puppeteer-core/src/bidi/Connection.ts | 4 ++++ packages/puppeteer-core/src/cdp/Browser.ts | 5 +++-- test/TestExpectations.json | 6 ------ test/src/browser.spec.ts | 4 ++-- test/src/browsercontext.spec.ts | 2 +- test/src/chromiumonly.spec.ts | 4 ++-- test/src/launcher.spec.ts | 12 ++++++------ test/src/oopif.spec.ts | 2 +- 13 files changed, 34 insertions(+), 28 deletions(-) diff --git a/docs/api/puppeteer.browser.disconnect.md b/docs/api/puppeteer.browser.disconnect.md index 5307952640c..c75ad5b1cd5 100644 --- a/docs/api/puppeteer.browser.disconnect.md +++ b/docs/api/puppeteer.browser.disconnect.md @@ -10,10 +10,10 @@ Disconnects Puppeteer from this [browser](./puppeteer.browser.md), but leaves th ```typescript class Browser { - abstract disconnect(): void; + abstract disconnect(): Promise; } ``` **Returns:** -void +Promise<void> diff --git a/docs/api/puppeteer.browser.md b/docs/api/puppeteer.browser.md index 289e1f67c63..126c32469d3 100644 --- a/docs/api/puppeteer.browser.md +++ b/docs/api/puppeteer.browser.md @@ -46,7 +46,7 @@ const browser = await puppeteer.launch(); // Store the endpoint to be able to reconnect to the browser. const browserWSEndpoint = browser.wsEndpoint(); // Disconnect puppeteer from the browser. -browser.disconnect(); +await browser.disconnect(); // Use the endpoint to reestablish a connection const browser2 = await puppeteer.connect({browserWSEndpoint}); diff --git a/packages/puppeteer-core/src/api/Browser.ts b/packages/puppeteer-core/src/api/Browser.ts index a664a410406..f0aa06680ce 100644 --- a/packages/puppeteer-core/src/api/Browser.ts +++ b/packages/puppeteer-core/src/api/Browser.ts @@ -228,7 +228,7 @@ export interface BrowserEvents extends Record { * // Store the endpoint to be able to reconnect to the browser. * const browserWSEndpoint = browser.wsEndpoint(); * // Disconnect puppeteer from the browser. - * browser.disconnect(); + * await browser.disconnect(); * * // Use the endpoint to reestablish a connection * const browser2 = await puppeteer.connect({browserWSEndpoint}); @@ -414,7 +414,7 @@ export abstract class Browser extends EventEmitter { * Disconnects Puppeteer from this {@link Browser | browser}, but leaves the * process running. */ - abstract disconnect(): void; + abstract disconnect(): Promise; /** * Whether Puppeteer is connected to this {@link Browser | browser}. diff --git a/packages/puppeteer-core/src/bidi/BidiOverCdp.ts b/packages/puppeteer-core/src/bidi/BidiOverCdp.ts index 4070884284d..6b8f4446c56 100644 --- a/packages/puppeteer-core/src/bidi/BidiOverCdp.ts +++ b/packages/puppeteer-core/src/bidi/BidiOverCdp.ts @@ -49,6 +49,7 @@ export async function connectBidiOverCdp( close(): void { bidiServer.close(); cdpConnectionAdapter.close(); + cdp.dispose(); }, onmessage(_message: string): void { // The method is overridden by the Connection. diff --git a/packages/puppeteer-core/src/bidi/Browser.ts b/packages/puppeteer-core/src/bidi/Browser.ts index 3ad7ac1031d..a4c852ff2e3 100644 --- a/packages/puppeteer-core/src/bidi/Browser.ts +++ b/packages/puppeteer-core/src/bidi/Browser.ts @@ -254,8 +254,8 @@ export class BidiBrowser extends Browser { return; } await this.#connection.send('browser.close', {}); - this.#connection.dispose(); await this.#closeCallback?.call(null); + this.#connection.dispose(); } override get connected(): boolean { @@ -323,7 +323,13 @@ export class BidiBrowser extends Browser { return this.#browserTarget; } - override disconnect(): void { - this; + override async disconnect(): Promise { + try { + // Fail silently if the session cannot be ended. + await this.#connection.send('session.end', {}); + } catch (e) { + debugError(e); + } + this.#connection.dispose(); } } diff --git a/packages/puppeteer-core/src/bidi/Connection.ts b/packages/puppeteer-core/src/bidi/Connection.ts index 2e4b41b2328..10cc0672ddb 100644 --- a/packages/puppeteer-core/src/bidi/Connection.ts +++ b/packages/puppeteer-core/src/bidi/Connection.ts @@ -111,6 +111,10 @@ export interface Commands { returnType: Bidi.EmptyResult; }; + 'session.end': { + params: Bidi.EmptyParams; + returnType: Bidi.EmptyResult; + }; 'session.new': { params: Bidi.Session.NewParameters; returnType: Bidi.Session.NewResult; diff --git a/packages/puppeteer-core/src/cdp/Browser.ts b/packages/puppeteer-core/src/cdp/Browser.ts index c61bd766e90..601ab4925a4 100644 --- a/packages/puppeteer-core/src/cdp/Browser.ts +++ b/packages/puppeteer-core/src/cdp/Browser.ts @@ -410,13 +410,14 @@ export class CdpBrowser extends BrowserBase { override async close(): Promise { await this.#closeCallback.call(null); - this.disconnect(); + await this.disconnect(); } - override disconnect(): void { + override disconnect(): Promise { this.#targetManager.dispose(); this.#connection.dispose(); this._detach(); + return Promise.resolve(); } override get connected(): boolean { diff --git a/test/TestExpectations.json b/test/TestExpectations.json index 7cb1c8f2559..2b81ff30477 100644 --- a/test/TestExpectations.json +++ b/test/TestExpectations.json @@ -383,12 +383,6 @@ "parameters": ["chrome", "headless"], "expectations": ["FAIL"] }, - { - "testIdPattern": "[browser.spec] Browser specs Browser.isConnected should set the browser connected state", - "platforms": ["darwin", "linux", "win32"], - "parameters": ["webDriverBiDi"], - "expectations": ["FAIL"] - }, { "testIdPattern": "[browser.spec] Browser specs Browser.process should return child_process instance", "platforms": ["darwin", "linux", "win32"], diff --git a/test/src/browser.spec.ts b/test/src/browser.spec.ts index 842b794b469..5d53e9d5cb8 100644 --- a/test/src/browser.spec.ts +++ b/test/src/browser.spec.ts @@ -70,7 +70,7 @@ describe('Browser specs', function () { protocol: browser.protocol, }); expect(remoteBrowser.process()).toBe(null); - remoteBrowser.disconnect(); + await remoteBrowser.disconnect(); }); }); @@ -84,7 +84,7 @@ describe('Browser specs', function () { protocol: browser.protocol, }); expect(newBrowser.isConnected()).toBe(true); - newBrowser.disconnect(); + await newBrowser.disconnect(); expect(newBrowser.isConnected()).toBe(false); }); }); diff --git a/test/src/browsercontext.spec.ts b/test/src/browsercontext.spec.ts index c3080e29a61..4da8adb8a2f 100644 --- a/test/src/browsercontext.spec.ts +++ b/test/src/browsercontext.spec.ts @@ -230,7 +230,7 @@ describe('BrowserContext', function () { }); const contexts = remoteBrowser.browserContexts(); expect(contexts).toHaveLength(2); - remoteBrowser.disconnect(); + await remoteBrowser.disconnect(); await context.close(); }); diff --git a/test/src/chromiumonly.spec.ts b/test/src/chromiumonly.spec.ts index d1a94c3c484..38c1139dcbc 100644 --- a/test/src/chromiumonly.spec.ts +++ b/test/src/chromiumonly.spec.ts @@ -38,7 +38,7 @@ describe('Chromium-Specific Launcher tests', function () { return 7 * 8; }) ).toBe(56); - browser1.disconnect(); + await browser1.disconnect(); const browser2 = await puppeteer.connect({ browserURL: browserURL + '/', @@ -49,7 +49,7 @@ describe('Chromium-Specific Launcher tests', function () { return 8 * 7; }) ).toBe(56); - browser2.disconnect(); + await browser2.disconnect(); } finally { await close(); } diff --git a/test/src/launcher.spec.ts b/test/src/launcher.spec.ts index 1947f8ac741..d26697cc916 100644 --- a/test/src/launcher.spec.ts +++ b/test/src/launcher.spec.ts @@ -52,7 +52,7 @@ describe('Launcher specs', function () { return error_; }); await server.waitForRequest('/one-style.css'); - remote.disconnect(); + await remote.disconnect(); const error = await navigationPromise; expect( [ @@ -78,7 +78,7 @@ describe('Launcher specs', function () { .catch(error_ => { return error_; }); - remote.disconnect(); + await remote.disconnect(); const error = await watchdog; expect(error.message).toContain('Session closed.'); } finally { @@ -635,7 +635,7 @@ describe('Launcher specs', function () { return 7 * 8; }) ).toBe(56); - otherBrowser.disconnect(); + await otherBrowser.disconnect(); const secondPage = await browser.newPage(); expect( @@ -776,7 +776,7 @@ describe('Launcher specs', function () { await page2.close(); await page1.close(); - remoteBrowser.disconnect(); + await remoteBrowser.disconnect(); await browser.close(); } finally { await close(); @@ -788,7 +788,7 @@ describe('Launcher specs', function () { const browserWSEndpoint = browser.wsEndpoint(); const page = await browser.newPage(); await page.goto(server.PREFIX + '/frames/nested-frames.html'); - browser.disconnect(); + await browser.disconnect(); const remoteBrowser = await puppeteer.connect({ browserWSEndpoint, @@ -858,7 +858,7 @@ describe('Launcher specs', function () { const browserWSEndpoint = browserOne.wsEndpoint(); const pageOne = await browserOne.newPage(); await pageOne.goto(server.EMPTY_PAGE); - browserOne.disconnect(); + await browserOne.disconnect(); const browserTwo = await puppeteer.connect({ browserWSEndpoint, diff --git a/test/src/oopif.spec.ts b/test/src/oopif.spec.ts index 5d65b89d19c..f783edbd06c 100644 --- a/test/src/oopif.spec.ts +++ b/test/src/oopif.spec.ts @@ -418,7 +418,7 @@ describe('OOPIF', function () { return target.url().endsWith('dynamic-oopif.html'); }); await target.page(); - browser1.disconnect(); + await browser1.disconnect(); }); it('should support lazy OOP frames', async () => {