Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: close WebDriver BiDi session on disconnect #11470

Merged
merged 6 commits into from
Nov 30, 2023
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/api/puppeteer.browser.disconnect.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ Disconnects Puppeteer from this [browser](./puppeteer.browser.md), but leaves th

```typescript
class Browser {
abstract disconnect(): void;
abstract disconnect(): Promise<void>;
}
```

**Returns:**

void
Promise&lt;void&gt;
2 changes: 1 addition & 1 deletion docs/api/puppeteer.browser.md
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Expand Down
4 changes: 2 additions & 2 deletions packages/puppeteer-core/src/api/Browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ export interface BrowserEvents extends Record<EventType, unknown> {
* // 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});
Expand Down Expand Up @@ -414,7 +414,7 @@ export abstract class Browser extends EventEmitter<BrowserEvents> {
* Disconnects Puppeteer from this {@link Browser | browser}, but leaves the
* process running.
*/
abstract disconnect(): void;
abstract disconnect(): Promise<void>;

/**
* Whether Puppeteer is connected to this {@link Browser | browser}.
Expand Down
1 change: 1 addition & 0 deletions packages/puppeteer-core/src/bidi/BidiOverCdp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
12 changes: 9 additions & 3 deletions packages/puppeteer-core/src/bidi/Browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -323,7 +323,13 @@ export class BidiBrowser extends Browser {
return this.#browserTarget;
}

override disconnect(): void {
this;
override async disconnect(): Promise<void> {
try {
// Fail silently if the session cannot be ended.
await this.#connection.send('session.end', {});
} catch (e) {
debugError(e);
}
this.#connection.dispose();
}
}
4 changes: 4 additions & 0 deletions packages/puppeteer-core/src/bidi/Connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 3 additions & 2 deletions packages/puppeteer-core/src/cdp/Browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -410,13 +410,14 @@ export class CdpBrowser extends BrowserBase {

override async close(): Promise<void> {
await this.#closeCallback.call(null);
this.disconnect();
await this.disconnect();
}

override disconnect(): void {
override disconnect(): Promise<void> {
this.#targetManager.dispose();
this.#connection.dispose();
this._detach();
return Promise.resolve();
}

override get connected(): boolean {
Expand Down
6 changes: 0 additions & 6 deletions test/TestExpectations.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
4 changes: 2 additions & 2 deletions test/src/browser.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ describe('Browser specs', function () {
protocol: browser.protocol,
});
expect(remoteBrowser.process()).toBe(null);
remoteBrowser.disconnect();
await remoteBrowser.disconnect();
});
});

Expand All @@ -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);
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/src/browsercontext.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ describe('BrowserContext', function () {
});
const contexts = remoteBrowser.browserContexts();
expect(contexts).toHaveLength(2);
remoteBrowser.disconnect();
await remoteBrowser.disconnect();
await context.close();
});

Expand Down
4 changes: 2 additions & 2 deletions test/src/chromiumonly.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 + '/',
Expand All @@ -49,7 +49,7 @@ describe('Chromium-Specific Launcher tests', function () {
return 8 * 7;
})
).toBe(56);
browser2.disconnect();
await browser2.disconnect();
} finally {
await close();
}
Expand Down
12 changes: 6 additions & 6 deletions test/src/launcher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
[
Expand All @@ -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 {
Expand Down Expand Up @@ -635,7 +635,7 @@ describe('Launcher specs', function () {
return 7 * 8;
})
).toBe(56);
otherBrowser.disconnect();
await otherBrowser.disconnect();

const secondPage = await browser.newPage();
expect(
Expand Down Expand Up @@ -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();
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion test/src/oopif.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ describe('OOPIF', function () {
return target.url().endsWith('dynamic-oopif.html');
});
await target.page();
browser1.disconnect();
await browser1.disconnect();
sadym-chromium marked this conversation as resolved.
Show resolved Hide resolved
});

it('should support lazy OOP frames', async () => {
Expand Down