From 71892b4aaa104dbb1484ba7a9c6b6ae1e9e81ce2 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Thu, 13 Feb 2020 13:44:13 -0800 Subject: [PATCH] Revert "feat: do not wait for first page in non-persistent mode (#939)" This reverts commit a56712359664c3ab13599af04c6dd95a81146e17. Reason for revert: WK-Win fails to start if we start talking over the pipe too early. --- 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, 18 insertions(+), 19 deletions(-) diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 994ace6b85..8b71ea8d01 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -46,6 +46,7 @@ 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 578aeee12f..aa51f8ff5b 100644 --- a/src/firefox/ffBrowser.ts +++ b/src/firefox/ffBrowser.ts @@ -39,6 +39,7 @@ 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 0a55ee56ae..175b2b9a4f 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, helper } from '../helper'; +import { assert } from '../helper'; import { CRBrowser } from '../chromium/crBrowser'; import * as platform from '../platform'; import { TimeoutError } from '../errors'; @@ -65,10 +65,8 @@ 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 a05d50e058..f33c2d2c76 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, helper } from '../helper'; +import { assert } from '../helper'; import { LaunchOptions, BrowserArgOptions, BrowserType } from './browserType'; import { ConnectOptions, LaunchType } from '../browser'; import { BrowserServer } from './browserServer'; @@ -75,10 +75,8 @@ 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 28b5fc2b9e..4a53b2dcbd 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, helper } from '../helper'; +import { assert } from '../helper'; import { kBrowserCloseMessageId } from '../webkit/wkConnection'; import { LaunchOptions, BrowserArgOptions, BrowserType } from './browserType'; import { ConnectionTransport } from '../transport'; @@ -77,10 +77,8 @@ 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 3b93574441..821d793ba1 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -43,6 +43,8 @@ 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 +95,9 @@ export class WKBrowser extends platform.EventEmitter implements Browser { return createPageInNewContext(this, options); } - async _waitForFirstPageTarget(): Promise { + async _waitForFirstPageTarget(timeout: number): Promise { assert(!this._pageProxies.size); - return this._firstPageProxyPromise; + await helper.waitWithTimeout(this._firstPageProxyPromise, 'firstPageProxy', timeout); } _onPageProxyCreated(event: Protocol.Browser.pageProxyCreatedPayload) { diff --git a/test/chromium/chromium.spec.js b/test/chromium/chromium.spec.js index 3234e8ef56..6b169fe382 100644 --- a/test/chromium/chromium.spec.js +++ b/test/chromium/chromium.spec.js @@ -153,15 +153,17 @@ 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 (context) => (await browser.targets()).filter(t => t.type() === 'page' && t.context() === context); + 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 context = await newContext(); await context.newPage(); - expect((await targets(context)).length).toBe(1); + expect((await targets()).length).toBe(3); expect((await context.pages()).length).toBe(1); await context.close(); - expect((await targets(context)).length).toBe(0); + expect((await targets()).length).toBe(2); }); }); diff --git a/test/chromium/launcher.spec.js b/test/chromium/launcher.spec.js index b1bbc910d8..0c96e30090 100644 --- a/test/chromium/launcher.spec.js +++ b/test/chromium/launcher.spec.js @@ -57,12 +57,11 @@ 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', 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(); + browser.on('targetcreated', () => events.push('CREATED')); + browser.on('targetchanged', () => events.push('CHANGED')); + browser.on('targetdestroyed', () => events.push('DESTROYED')); + const page = await browser.newPage(); await page.goto(server.EMPTY_PAGE); await page.close(); expect(events).toEqual(['CREATED', 'CHANGED', 'DESTROYED']);