From b7d0c3233802a52aaa541c18bba9a57ef4cea637 Mon Sep 17 00:00:00 2001 From: Joel Einbinder Date: Fri, 3 Apr 2020 16:34:07 -0700 Subject: [PATCH] fix(browser): wait for the pipe to disconnect in browser.close (#1652) With WebKit, sometimes the process closes before the stdio is streams are closed. I explicitly wait for the browser disconnect event now when closing. --- src/browser.ts | 18 +++++++++++++++--- src/chromium/crBrowser.ts | 14 +------------- src/firefox/ffBrowser.ts | 14 +------------- src/server/webkit.ts | 5 +++-- src/webkit/wkBrowser.ts | 21 ++++----------------- 5 files changed, 24 insertions(+), 48 deletions(-) diff --git a/src/browser.ts b/src/browser.ts index 4622981b09..79f8511ee9 100644 --- a/src/browser.ts +++ b/src/browser.ts @@ -19,6 +19,8 @@ import { Page } from './page'; import { EventEmitter } from 'events'; import { Download } from './download'; import { debugProtocol } from './transport'; +import type { BrowserServer } from './server/browserServer'; +import { Events } from './events'; export interface Browser extends EventEmitter { newContext(options?: BrowserContextOptions): Promise; @@ -26,19 +28,18 @@ export interface Browser extends EventEmitter { newPage(options?: BrowserContextOptions): Promise; isConnected(): boolean; close(): Promise; - _disconnect(): Promise; } export abstract class BrowserBase extends EventEmitter implements Browser { _downloadsPath: string = ''; private _downloads = new Map(); _debugProtocol = debugProtocol; + _ownedServer: BrowserServer | null = null; abstract newContext(options?: BrowserContextOptions): Promise; abstract contexts(): BrowserContext[]; abstract isConnected(): boolean; - abstract close(): Promise; - abstract _disconnect(): Promise; + abstract _disconnect(): void; async newPage(options?: BrowserContextOptions): Promise { const context = await this.newContext(options); @@ -59,6 +60,17 @@ export abstract class BrowserBase extends EventEmitter implements Browser { download._reportFinished(error); this._downloads.delete(uuid); } + + async close() { + if (this._ownedServer) { + await this._ownedServer.close(); + } else { + await Promise.all(this.contexts().map(context => context.close())); + this._disconnect(); + } + if (this.isConnected()) + await new Promise(x => this.once(Events.Browser.Disconnected, x)); + } } export type LaunchType = 'local' | 'server' | 'persistent'; diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index bc208a6dca..fb16b5eed8 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -29,7 +29,6 @@ import { readProtocolStream } from './crProtocolHelper'; import { Events } from './events'; import { Protocol } from './protocol'; import { CRExecutionContext } from './crExecutionContext'; -import { BrowserServer } from '../server/browserServer'; export class CRBrowser extends BrowserBase { readonly _connection: CRConnection; @@ -46,7 +45,6 @@ export class CRBrowser extends BrowserBase { private _tracingRecording = false; private _tracingPath: string | null = ''; private _tracingClient: CRSession | undefined; - _ownedServer: BrowserServer | null = null; static async connect(transport: ConnectionTransport, isPersistent: boolean, slowMo?: number): Promise { const connection = new CRConnection(SlowMoTransport.wrap(transport, slowMo)); @@ -180,18 +178,8 @@ export class CRBrowser extends BrowserBase { await this._session.send('Target.closeTarget', { targetId: crPage._targetId }); } - async _disconnect() { - const disconnected = new Promise(f => this._connection.once(ConnectionEvents.Disconnected, f)); - await Promise.all(this.contexts().map(context => context.close())); + _disconnect() { this._connection.close(); - await disconnected; - } - - async close() { - if (this._ownedServer) - await this._ownedServer.close(); - else - await this._disconnect(); } async newBrowserCDPSession(): Promise { diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index 44fe0de183..5df38ab58c 100644 --- a/src/firefox/ffBrowser.ts +++ b/src/firefox/ffBrowser.ts @@ -27,7 +27,6 @@ import { ConnectionEvents, FFConnection } from './ffConnection'; import { headersArray } from './ffNetworkManager'; import { FFPage } from './ffPage'; import { Protocol } from './protocol'; -import { BrowserServer } from '../server/browserServer'; export class FFBrowser extends BrowserBase { _connection: FFConnection; @@ -37,7 +36,6 @@ export class FFBrowser extends BrowserBase { private _eventListeners: RegisteredListener[]; readonly _firstPagePromise: Promise; private _firstPageCallback = () => {}; - _ownedServer: BrowserServer | null = null; static async connect(transport: ConnectionTransport, attachToDefaultContext: boolean, slowMo?: number): Promise { const connection = new FFConnection(SlowMoTransport.wrap(transport, slowMo)); @@ -137,19 +135,9 @@ export class FFBrowser extends BrowserBase { }); } - async _disconnect() { - await Promise.all(this.contexts().map(context => context.close())); + _disconnect() { helper.removeEventListeners(this._eventListeners); - const disconnected = new Promise(f => this.once(Events.Browser.Disconnected, f)); this._connection.close(); - await disconnected; - } - - async close() { - if (this._ownedServer) - await this._ownedServer.close(); - else - await this._disconnect(); } } diff --git a/src/server/webkit.ts b/src/server/webkit.ts index 47e07c2985..0ec61cddb7 100644 --- a/src/server/webkit.ts +++ b/src/server/webkit.ts @@ -48,7 +48,7 @@ export class WebKit implements BrowserType { async launch(options: LaunchOptions = {}): Promise { assert(!(options as any).userDataDir, 'userDataDir option is not supported in `browserType.launch`. Use `browserType.launchPersistentContext` instead'); const { browserServer, transport, downloadsPath } = await this._launchServer(options, 'local'); - const browser = await WKBrowser.connect(transport!, options.slowMo, false, () => browserServer.close()); + const browser = await WKBrowser.connect(transport!, options.slowMo, false); browser._ownedServer = browserServer; browser._downloadsPath = downloadsPath; return browser; @@ -64,7 +64,8 @@ export class WebKit implements BrowserType { slowMo = 0, } = options; const { transport, browserServer } = await this._launchServer(options, 'persistent', userDataDir); - const browser = await WKBrowser.connect(transport!, slowMo, true, () => browserServer.close()); + const browser = await WKBrowser.connect(transport!, slowMo, true); + browser._ownedServer = browserServer; await helper.waitWithTimeout(browser._waitForFirstPageTarget(), 'first page', timeout); return browser._defaultContext; } diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 8316829c0a..cdca4cc826 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -26,7 +26,6 @@ import * as types from '../types'; import { Protocol } from './protocol'; import { kPageProxyMessageReceived, PageProxyMessageReceivedPayload, WKConnection, WKSession } from './wkConnection'; import { WKPage } from './wkPage'; -import { BrowserServer } from '../server/browserServer'; const DEFAULT_USER_AGENT = 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_2) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/13.0.4 Safari/605.1.15'; @@ -43,18 +42,16 @@ export class WKBrowser extends BrowserBase { private _firstPageCallback: () => void = () => {}; private readonly _firstPagePromise: Promise; - _ownedServer: BrowserServer | null = null; - static async connect(transport: ConnectionTransport, slowMo: number = 0, attachToDefaultContext: boolean = false, closeOverride?: () => Promise): Promise { - const browser = new WKBrowser(SlowMoTransport.wrap(transport, slowMo), attachToDefaultContext, closeOverride); + static async connect(transport: ConnectionTransport, slowMo: number = 0, attachToDefaultContext: boolean = false): Promise { + const browser = new WKBrowser(SlowMoTransport.wrap(transport, slowMo), attachToDefaultContext); return browser; } - constructor(transport: ConnectionTransport, attachToDefaultContext: boolean, closeOverride?: () => Promise) { + constructor(transport: ConnectionTransport, attachToDefaultContext: boolean) { super(); this._connection = new WKConnection(transport, this._onDisconnect.bind(this)); this._attachToDefaultContext = attachToDefaultContext; - this._closeOverride = closeOverride; this._browserSession = this._connection.browserSession; this._defaultContext = new WKBrowserContext(this, undefined, validateBrowserContextOptions({})); @@ -190,19 +187,9 @@ export class WKBrowser extends BrowserBase { return !this._connection.isClosed(); } - async _disconnect() { + _disconnect() { helper.removeEventListeners(this._eventListeners); - const disconnected = new Promise(f => this.once(Events.Browser.Disconnected, f)); - await Promise.all(this.contexts().map(context => context.close())); this._connection.close(); - await disconnected; - } - - async close() { - if (this._closeOverride) - await this._closeOverride(); - else - await this._disconnect(); } }