feat: do not wait for first page in non-persistent mode (#939)

Author: dgozman

src/chromium/crBrowser.ts

@@ -46,7 +46,6 @@ export class CRBrowser extends platform.EventEmitter implements Browser {
     const connection = new CRConnection(SlowMoTransport.wrap(transport, slowMo));
     const browser = new CRBrowser(connection);
     await connection.rootSession.send('Target.setDiscoverTargets', { discover: true });
-    await browser.waitForTarget(t => t.type() === 'page');
     return browser;
   }
 

src/firefox/ffBrowser.ts

@@ -39,7 +39,6 @@ export class FFBrowser extends platform.EventEmitter implements Browser {
     const connection = new FFConnection(SlowMoTransport.wrap(transport, slowMo));
     const browser = new FFBrowser(connection);
     await connection.send('Target.enable');
-    await browser._waitForTarget(t => t.type() === 'page');
     return browser;
   }
 

src/server/chromium.ts

@@ -22,7 +22,7 @@ import * as util from 'util';
 import { BrowserFetcher, OnProgressCallback, BrowserFetcherOptions } from '../server/browserFetcher';
 import { DeviceDescriptors } from '../deviceDescriptors';
 import * as types from '../types';
-import { assert } from '../helper';
+import { assert, helper } from '../helper';
 import { CRBrowser } from '../chromium/crBrowser';
 import * as platform from '../platform';
 import { TimeoutError } from '../errors';
@@ -65,8 +65,10 @@ export class Chromium implements BrowserType {
   }
 
   async launchPersistent(userDataDir: string, options?: LaunchOptions): Promise<BrowserContext> {
+    const { timeout = 30000 } = options || {};
     const { browserServer, transport } = await this._launchServer(options, 'persistent', userDataDir);
     const browser = await CRBrowser.connect(transport!);
+    await helper.waitWithTimeout(browser.waitForTarget(t => t.type() === 'page'), 'first page', timeout);
     // Hack: for typical launch scenario, ensure that close waits for actual process termination.
     const browserContext = browser._defaultContext;
     browserContext.close = () => browserServer.close();

src/server/firefox.ts

@@ -27,7 +27,7 @@ import * as os from 'os';
 import * as path from 'path';
 import * as util from 'util';
 import { TimeoutError } from '../errors';
-import { assert } from '../helper';
+import { assert, helper } from '../helper';
 import { LaunchOptions, BrowserArgOptions, BrowserType } from './browserType';
 import { ConnectOptions, LaunchType } from '../browser';
 import { BrowserServer } from './browserServer';
@@ -75,8 +75,10 @@ export class Firefox implements BrowserType {
   }
 
   async launchPersistent(userDataDir: string, options?: LaunchOptions): Promise<BrowserContext> {
+    const { timeout = 30000 } = options || {};
     const { browserServer, transport } = await this._launchServer(options, 'persistent', userDataDir);
     const browser = await FFBrowser.connect(transport!);
+    await helper.waitWithTimeout(browser._waitForTarget(t => t.type() === 'page'), 'first page', timeout);
     // Hack: for typical launch scenario, ensure that close waits for actual process termination.
     const browserContext = browser._defaultContext;
     browserContext.close = () => browserServer.close();

src/server/webkit.ts

@@ -28,7 +28,7 @@ import * as path from 'path';
 import * as platform from '../platform';
 import * as util from 'util';
 import * as os from 'os';
-import { assert } from '../helper';
+import { assert, helper } from '../helper';
 import { kBrowserCloseMessageId } from '../webkit/wkConnection';
 import { LaunchOptions, BrowserArgOptions, BrowserType } from './browserType';
 import { ConnectionTransport } from '../transport';
@@ -77,8 +77,10 @@ export class WebKit implements BrowserType {
   }
 
   async launchPersistent(userDataDir: string, options?: LaunchOptions): Promise<BrowserContext> {
+    const { timeout = 30000 } = options || {};
     const { browserServer, transport } = await this._launchServer(options, 'persistent', userDataDir);
     const browser = await WKBrowser.connect(transport!);
+    await helper.waitWithTimeout(browser._waitForFirstPageTarget(), 'first page', timeout);
     // Hack: for typical launch scenario, ensure that close waits for actual process termination.
     const browserContext = browser._defaultContext;
     browserContext.close = () => browserServer.close();

src/webkit/wkBrowser.ts

@@ -43,8 +43,6 @@ export class WKBrowser extends platform.EventEmitter implements Browser {
 
   static async connect(transport: ConnectionTransport, slowMo: number = 0): Promise<WKBrowser> {
     const browser = new WKBrowser(SlowMoTransport.wrap(transport, slowMo));
-    // TODO: figure out the timeout.
-    await browser._waitForFirstPageTarget(30000);
     return browser;
   }
 
@@ -93,9 +91,9 @@ export class WKBrowser extends platform.EventEmitter implements Browser {
     return createPageInNewContext(this, options);
   }
 
-  async _waitForFirstPageTarget(timeout: number): Promise<void> {
+  async _waitForFirstPageTarget(): Promise<void> {
     assert(!this._pageProxies.size);
-    await helper.waitWithTimeout(this._firstPageProxyPromise, 'firstPageProxy', timeout);
+    return this._firstPageProxyPromise;
   }
 
   _onPageProxyCreated(event: Protocol.Browser.pageProxyCreatedPayload) {

test/chromium/chromium.spec.js

@@ -153,17 +153,15 @@ module.exports.describe = function({testRunner, expect, playwright, FFOX, CHROMI
       expect(browser.pageTarget(page).opener()).toBe(null);
     });
     it('should close all belonging targets once closing context', async function({browser, newContext}) {
-      const targets = async () => (await browser.targets()).filter(t => t.type() === 'page');
-      // There is one page in a default profile and one page created by test harness.
-      expect((await targets()).length).toBe(2);
+      const targets = async (context) => (await browser.targets()).filter(t => t.type() === 'page' && t.context() === context);
 
       const context = await newContext();
       await context.newPage();
-      expect((await targets()).length).toBe(3);
+      expect((await targets(context)).length).toBe(1);
       expect((await context.pages()).length).toBe(1);
 
       await context.close();
-      expect((await targets()).length).toBe(2);
+      expect((await targets(context)).length).toBe(0);
     });
   });
 

test/chromium/launcher.spec.js

@@ -57,11 +57,12 @@ module.exports.describe = function({testRunner, expect, defaultBrowserOptions, p
   describe('Browser target events', function() {
     it('should work', async({server}) => {
       const browser = await playwright.launch(defaultBrowserOptions);
+      const context = await browser.newContext();
       const events = [];
-      browser.on('targetcreated', () => events.push('CREATED'));
-      browser.on('targetchanged', () => events.push('CHANGED'));
-      browser.on('targetdestroyed', () => events.push('DESTROYED'));
-      const page = await browser.newPage();
+      browser.on('targetcreated', target => target.context() === context && events.push('CREATED'));
+      browser.on('targetchanged', target => target.context() === context && events.push('CHANGED'));
+      browser.on('targetdestroyed', target => target.context() === context && events.push('DESTROYED'));
+      const page = await context.newPage();
       await page.goto(server.EMPTY_PAGE);
       await page.close();
       expect(events).toEqual(['CREATED', 'CHANGED', 'DESTROYED']);