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

@@ -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<BrowserContext>;
@@ -26,19 +28,18 @@ export interface Browser extends EventEmitter {
   newPage(options?: BrowserContextOptions): Promise<Page>;
   isConnected(): boolean;
   close(): Promise<void>;
-  _disconnect(): Promise<void>;
 }
 
 export abstract class BrowserBase extends EventEmitter implements Browser {
   _downloadsPath: string = '';
   private _downloads = new Map<string, Download>();
   _debugProtocol = debugProtocol;
+  _ownedServer: BrowserServer | null = null;
 
   abstract newContext(options?: BrowserContextOptions): Promise<BrowserContext>;
   abstract contexts(): BrowserContext[];
   abstract isConnected(): boolean;
-  abstract close(): Promise<void>;
+  abstract _disconnect(): void;
-  abstract _disconnect(): Promise<void>;
 
   async newPage(options?: BrowserContextOptions): Promise<Page> {
     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';

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<CRBrowser> {
     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() {
+  _disconnect() {
-    const disconnected = new Promise(f => this._connection.once(ConnectionEvents.Disconnected, f));
-    await Promise.all(this.contexts().map(context => context.close()));
     this._connection.close();
-    await disconnected;
-  }
-
-  async close() {
-    if (this._ownedServer)
-      await this._ownedServer.close();
-    else
-      await this._disconnect();
   }
 
   async newBrowserCDPSession(): Promise<CRSession> {

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<void>;
   private _firstPageCallback = () => {};
-  _ownedServer: BrowserServer | null = null;
 
   static async connect(transport: ConnectionTransport, attachToDefaultContext: boolean, slowMo?: number): Promise<FFBrowser> {
     const connection = new FFConnection(SlowMoTransport.wrap(transport, slowMo));
@@ -137,19 +135,9 @@ export class FFBrowser extends BrowserBase {
     });
   }
 
-  async _disconnect() {
+  _disconnect() {
-    await Promise.all(this.contexts().map(context => context.close()));
     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();
   }
 }
 

src/server/webkit.ts

@@ -48,7 +48,7 @@ export class WebKit implements BrowserType<WKBrowser> {
   async launch(options: LaunchOptions = {}): Promise<WKBrowser> {
     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<WKBrowser> {
       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;
   }

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<void>;
-  _ownedServer: BrowserServer | null = null;
 
-  static async connect(transport: ConnectionTransport, slowMo: number = 0, attachToDefaultContext: boolean = false, closeOverride?: () => Promise<void>): Promise<WKBrowser> {
+  static async connect(transport: ConnectionTransport, slowMo: number = 0, attachToDefaultContext: boolean = false): Promise<WKBrowser> {
-    const browser = new WKBrowser(SlowMoTransport.wrap(transport, slowMo), attachToDefaultContext, closeOverride);
+    const browser = new WKBrowser(SlowMoTransport.wrap(transport, slowMo), attachToDefaultContext);
     return browser;
   }
 
-  constructor(transport: ConnectionTransport, attachToDefaultContext: boolean, closeOverride?: () => Promise<void>) {
+  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();
   }
 }