From a56712359664c3ab13599af04c6dd95a81146e17 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 13 Feb 2020 10:51:17 -0800 Subject: [PATCH] feat: do not wait for first page in non-persistent mode (#939) --- src/chromium/crBrowser.ts | 1 - src/firefox/ffBrowser.ts | 1 - src/server/chromium.ts | 4 +++- src/server/firefox.ts | 4 +++- src/server/webkit.ts | 4 +++- src/webkit/wkBrowser.ts | 6 ++---- test/chromium/chromium.spec.js | 8 +++----- test/chromium/launcher.spec.js | 9 +++++---- 8 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 8b71ea8d01..994ace6b85 100644 --- a/src/chromium/crBrowser.ts +++ b/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; } diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index aa51f8ff5b..578aeee12f 100644 --- a/src/firefox/ffBrowser.ts +++ b/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; } diff --git a/src/server/chromium.ts b/src/server/chromium.ts index 175b2b9a4f..0a55ee56ae 100644 --- a/src/server/chromium.ts +++ b/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 { + 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(); diff --git a/src/server/firefox.ts b/src/server/firefox.ts index f33c2d2c76..a05d50e058 100644 --- a/src/server/firefox.ts +++ b/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 { + 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(); diff --git a/src/server/webkit.ts b/src/server/webkit.ts index 4a53b2dcbd..28b5fc2b9e 100644 --- a/src/server/webkit.ts +++ b/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 { + 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(); diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 1350c941e8..26b66d55cb 100644 --- a/src/webkit/wkBrowser.ts +++ b/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 { 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 { + async _waitForFirstPageTarget(): Promise { assert(!this._pageProxies.size); - await helper.waitWithTimeout(this._firstPageProxyPromise, 'firstPageProxy', timeout); + return this._firstPageProxyPromise; } _onPageProxyCreated(event: Protocol.Browser.pageProxyCreatedPayload) { diff --git a/test/chromium/chromium.spec.js b/test/chromium/chromium.spec.js index 6b169fe382..3234e8ef56 100644 --- a/test/chromium/chromium.spec.js +++ b/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); }); }); diff --git a/test/chromium/launcher.spec.js b/test/chromium/launcher.spec.js index 0c96e30090..b1bbc910d8 100644 --- a/test/chromium/launcher.spec.js +++ b/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']);