From ae8d97cdf9e78334ef2cacbfa848f9690560e6e2 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Sun, 10 May 2020 15:23:53 -0700 Subject: [PATCH] feat(persistent context): ensure initial about:blank (#2161) We declare only the initial about:blank to be a supported usecase, so that we can support options for the default context in the future. --- src/browserContext.ts | 11 +++++++++++ src/chromium/crBrowser.ts | 4 ---- src/firefox/ffBrowser.ts | 4 ---- src/server/chromium.ts | 21 +++++++++++---------- src/server/firefox.ts | 25 ++++++++++--------------- src/server/webkit.ts | 12 +++++++++--- src/timeoutSettings.ts | 7 +++++-- src/webkit/wkBrowser.ts | 13 +------------ test/launcher.spec.js | 28 +++++++++++++++++++--------- 9 files changed, 66 insertions(+), 59 deletions(-) diff --git a/src/browserContext.ts b/src/browserContext.ts index e1b86a87df..479a85bada 100644 --- a/src/browserContext.ts +++ b/src/browserContext.ts @@ -164,6 +164,17 @@ export abstract class BrowserContextBase extends ExtendedEventEmitter implements _log(log: Log, message: string | Error, ...args: any[]) { return this._logger._log(log, message, ...args); } + + async _loadDefaultContext() { + if (!this.pages().length) + await this.waitForEvent('page'); + const pages = this.pages(); + await pages[0].waitForLoadState(); + if (pages.length !== 1 || pages[0].url() !== 'about:blank') { + await this.close().catch(e => null); + throw new Error('Arguments can not specify page to be opened'); + } + } } export function assertBrowserContextIsNotOwned(context: BrowserContextBase) { diff --git a/src/chromium/crBrowser.ts b/src/chromium/crBrowser.ts index 9f72da597b..14ed5b313c 100644 --- a/src/chromium/crBrowser.ts +++ b/src/chromium/crBrowser.ts @@ -40,8 +40,6 @@ export class CRBrowser extends BrowserBase { _crPages = new Map(); _backgroundPages = new Map(); _serviceWorkers = new Map(); - readonly _firstPagePromise: Promise; - private _firstPageCallback = () => {}; private _tracingRecording = false; private _tracingPath: string | null = ''; @@ -100,7 +98,6 @@ export class CRBrowser extends BrowserBase { }); this._session.on('Target.attachedToTarget', this._onAttachedToTarget.bind(this)); this._session.on('Target.detachedFromTarget', this._onDetachedFromTarget.bind(this)); - this._firstPagePromise = new Promise(f => this._firstPageCallback = f); } async newContext(options: BrowserContextOptions = {}): Promise { @@ -160,7 +157,6 @@ export class CRBrowser extends BrowserBase { signalBarrier.addPopup(crPage.pageOrError()); } crPage.pageOrError().then(() => { - this._firstPageCallback(); context!.emit(CommonEvents.BrowserContext.Page, crPage._page); if (opener) { opener.pageOrError().then(openerPage => { diff --git a/src/firefox/ffBrowser.ts b/src/firefox/ffBrowser.ts index 5f50b3522e..1d4532e768 100644 --- a/src/firefox/ffBrowser.ts +++ b/src/firefox/ffBrowser.ts @@ -35,8 +35,6 @@ export class FFBrowser extends BrowserBase { readonly _defaultContext: FFBrowserContext | null = null; readonly _contexts: Map; private _eventListeners: RegisteredListener[]; - readonly _firstPagePromise: Promise; - private _firstPageCallback = () => {}; static async connect(transport: ConnectionTransport, logger: InnerLogger, attachToDefaultContext: boolean, slowMo?: number): Promise { const connection = new FFConnection(SlowMoTransport.wrap(transport, slowMo), logger); @@ -64,7 +62,6 @@ export class FFBrowser extends BrowserBase { helper.addEventListener(this._connection, 'Browser.downloadCreated', this._onDownloadCreated.bind(this)), helper.addEventListener(this._connection, 'Browser.downloadFinished', this._onDownloadFinished.bind(this)), ]; - this._firstPagePromise = new Promise(f => this._firstPageCallback = f); } isConnected(): boolean { @@ -137,7 +134,6 @@ export class FFBrowser extends BrowserBase { signalBarrier.addPopup(ffPage.pageOrError()); } ffPage.pageOrError().then(async () => { - this._firstPageCallback(); const page = ffPage._page; context.emit(Events.BrowserContext.Page, page); if (!opener) diff --git a/src/server/chromium.ts b/src/server/chromium.ts index e7d6b6f727..eb8c28fbd5 100644 --- a/src/server/chromium.ts +++ b/src/server/chromium.ts @@ -33,6 +33,7 @@ import { ConnectionTransport, ProtocolRequest, WebSocketTransport } from '../tra import { BrowserContext } from '../browserContext'; import { InnerLogger, logError, RootLogger } from '../logger'; import { BrowserDescriptor } from '../install/browserPaths'; +import { TimeoutSettings } from '../timeoutSettings'; export class Chromium extends AbstractBrowserType { constructor(packagePath: string, browser: BrowserDescriptor) { @@ -53,11 +54,15 @@ export class Chromium extends AbstractBrowserType { } async launchPersistentContext(userDataDir: string, options: LaunchOptions = {}): Promise { + const { timeout = 30000 } = options; + const deadline = TimeoutSettings.computeDeadline(timeout); const { transport, browserServer, logger } = await this._launchServer(options, 'persistent', userDataDir); const browser = await CRBrowser.connect(transport!, true, logger, options); browser._ownedServer = browserServer; - await helper.waitWithTimeout(browser._firstPagePromise, 'first page', options.timeout || 30000); - return browser._defaultContext!; + const context = browser._defaultContext!; + if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs)) + await helper.waitWithTimeout(context._loadDefaultContext(), 'first page', helper.timeUntilDeadline(deadline)); + return context; } private async _launchServer(options: LaunchServerOptions, launchType: LaunchType, userDataDir?: string): Promise<{ browserServer: BrowserServer, transport?: ConnectionTransport, downloadsPath: string, logger: InnerLogger }> { @@ -141,9 +146,8 @@ export class Chromium extends AbstractBrowserType { throw new Error('Pass userDataDir parameter instead of specifying --user-data-dir argument'); if (args.find(arg => arg.startsWith('--remote-debugging-pipe'))) throw new Error('Playwright manages remote debugging connection itself.'); - if (launchType !== 'persistent' && args.find(arg => !arg.startsWith('-'))) + if (args.find(arg => !arg.startsWith('-'))) throw new Error('Arguments can not specify page to be opened'); - const chromeArguments = [...DEFAULT_ARGS]; chromeArguments.push(`--user-data-dir=${userDataDir}`); chromeArguments.push('--remote-debugging-pipe'); @@ -157,13 +161,10 @@ export class Chromium extends AbstractBrowserType { ); } chromeArguments.push(...args); - if (launchType === 'persistent') { - if (args.every(arg => arg.startsWith('-'))) - chromeArguments.push('about:blank'); - } else { + if (launchType === 'persistent') + chromeArguments.push('about:blank'); + else chromeArguments.push('--no-startup-window'); - } - return chromeArguments; } } diff --git a/src/server/firefox.ts b/src/server/firefox.ts index 81d8021f67..c8a1ff6107 100644 --- a/src/server/firefox.ts +++ b/src/server/firefox.ts @@ -33,6 +33,7 @@ import { launchProcess, waitForLine } from './processLauncher'; import { ConnectionTransport, SequenceNumberMixer, WebSocketTransport } from '../transport'; import { RootLogger, InnerLogger, logError } from '../logger'; import { BrowserDescriptor } from '../install/browserPaths'; +import { TimeoutSettings } from '../timeoutSettings'; const mkdtempAsync = util.promisify(fs.mkdtemp); @@ -61,15 +62,17 @@ export class Firefox extends AbstractBrowserType { timeout = 30000, slowMo = 0, } = options; + const deadline = TimeoutSettings.computeDeadline(timeout); const { browserServer, downloadsPath, logger } = await this._launchServer(options, 'persistent', userDataDir); const browser = await WebSocketTransport.connect(browserServer.wsEndpoint()!, transport => { return FFBrowser.connect(transport, logger, true, slowMo); }); browser._ownedServer = browserServer; browser._downloadsPath = downloadsPath; - await helper.waitWithTimeout(browser._firstPagePromise, 'first page', timeout); - const browserContext = browser._defaultContext!; - return browserContext; + const context = browser._defaultContext!; + if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs)) + await helper.waitWithTimeout(context._loadDefaultContext(), 'first page', helper.timeUntilDeadline(deadline)); + return context; } private async _launchServer(options: LaunchServerOptions, launchType: LaunchType, userDataDir?: string): Promise<{ browserServer: BrowserServer, downloadsPath: string, logger: InnerLogger }> { @@ -87,14 +90,13 @@ export class Firefox extends AbstractBrowserType { assert(!port || launchType === 'server', 'Cannot specify a port without launching as a server.'); const logger = new RootLogger(options.logger); - const firefoxArguments = []; - let temporaryProfileDir = null; if (!userDataDir) { userDataDir = await mkdtempAsync(path.join(os.tmpdir(), 'playwright_dev_firefox_profile-')); temporaryProfileDir = userDataDir; } + const firefoxArguments = []; if (!ignoreDefaultArgs) firefoxArguments.push(...this._defaultArgs(options, launchType, userDataDir, 0)); else if (Array.isArray(ignoreDefaultArgs)) @@ -165,8 +167,6 @@ export class Firefox extends AbstractBrowserType { throw new Error('Pass userDataDir parameter instead of specifying -profile argument'); if (args.find(arg => arg.startsWith('-juggler'))) throw new Error('Use the port parameter instead of -juggler argument'); - if (launchType !== 'persistent' && args.find(arg => !arg.startsWith('-'))) - throw new Error('Arguments can not specify page to be opened'); const firefoxArguments = ['-no-remote']; if (headless) { @@ -175,18 +175,13 @@ export class Firefox extends AbstractBrowserType { firefoxArguments.push('-wait-for-browser'); firefoxArguments.push('-foreground'); } - firefoxArguments.push(`-profile`, userDataDir); firefoxArguments.push('-juggler', String(port)); firefoxArguments.push(...args); - - if (launchType === 'persistent') { - if (args.every(arg => arg.startsWith('-'))) - firefoxArguments.push('about:blank'); - } else { + if (launchType === 'persistent') + firefoxArguments.push('about:blank'); + else firefoxArguments.push('-silent'); - } - return firefoxArguments; } } diff --git a/src/server/webkit.ts b/src/server/webkit.ts index 84d92e5255..fd504fedfa 100644 --- a/src/server/webkit.ts +++ b/src/server/webkit.ts @@ -33,6 +33,7 @@ import { Events } from '../events'; import { BrowserContext } from '../browserContext'; import { InnerLogger, logError, RootLogger } from '../logger'; import { BrowserDescriptor } from '../install/browserPaths'; +import { TimeoutSettings } from '../timeoutSettings'; export class WebKit extends AbstractBrowserType { constructor(packagePath: string, browser: BrowserDescriptor) { @@ -57,11 +58,14 @@ export class WebKit extends AbstractBrowserType { timeout = 30000, slowMo = 0, } = options; + const deadline = TimeoutSettings.computeDeadline(timeout); const { transport, browserServer, logger } = await this._launchServer(options, 'persistent', userDataDir); const browser = await WKBrowser.connect(transport!, logger, slowMo, true); browser._ownedServer = browserServer; - await helper.waitWithTimeout(browser._waitForFirstPageTarget(), 'first page', timeout); - return browser._defaultContext!; + const context = browser._defaultContext!; + if (!options.ignoreDefaultArgs || Array.isArray(options.ignoreDefaultArgs)) + await helper.waitWithTimeout(context._loadDefaultContext(), 'first page', helper.timeUntilDeadline(deadline)); + return context; } private async _launchServer(options: LaunchServerOptions, launchType: LaunchType, userDataDir?: string): Promise<{ browserServer: BrowserServer, transport?: ConnectionTransport, downloadsPath: string, logger: InnerLogger }> { @@ -145,7 +149,7 @@ export class WebKit extends AbstractBrowserType { const userDataDirArg = args.find(arg => arg.startsWith('--user-data-dir=')); if (userDataDirArg) throw new Error('Pass userDataDir parameter instead of specifying --user-data-dir argument'); - if (launchType !== 'persistent' && args.find(arg => !arg.startsWith('-'))) + if (args.find(arg => !arg.startsWith('-'))) throw new Error('Arguments can not specify page to be opened'); const webkitArguments = ['--inspector-pipe']; if (headless) @@ -155,6 +159,8 @@ export class WebKit extends AbstractBrowserType { else webkitArguments.push(`--no-startup-window`); webkitArguments.push(...args); + if (launchType === 'persistent') + webkitArguments.push('about:blank'); return webkitArguments; } } diff --git a/src/timeoutSettings.ts b/src/timeoutSettings.ts index f3ae69c8c3..04840f5b72 100644 --- a/src/timeoutSettings.ts +++ b/src/timeoutSettings.ts @@ -57,10 +57,13 @@ export class TimeoutSettings { computeDeadline(options?: TimeoutOptions) { const { timeout } = options || {}; + return TimeoutSettings.computeDeadline(typeof timeout === 'number' ? timeout : this._timeout()); + } + + static computeDeadline(timeout: number): number { if (timeout === 0) return Number.MAX_SAFE_INTEGER; - else if (typeof timeout === 'number') + else return helper.monotonicTime() + timeout; - return helper.monotonicTime() + this._timeout(); } } diff --git a/src/webkit/wkBrowser.ts b/src/webkit/wkBrowser.ts index 6cc34c9e0f..81d2e96502 100644 --- a/src/webkit/wkBrowser.ts +++ b/src/webkit/wkBrowser.ts @@ -18,7 +18,7 @@ import { BrowserBase } from '../browser'; import { assertBrowserContextIsNotOwned, BrowserContext, BrowserContextBase, BrowserContextOptions, validateBrowserContextOptions, verifyGeolocation } from '../browserContext'; import { Events } from '../events'; -import { assert, helper, RegisteredListener } from '../helper'; +import { helper, RegisteredListener } from '../helper'; import * as network from '../network'; import { Page, PageBinding } from '../page'; import { ConnectionTransport, SlowMoTransport } from '../transport'; @@ -38,9 +38,6 @@ export class WKBrowser extends BrowserBase { readonly _wkPages = new Map(); private readonly _eventListeners: RegisteredListener[]; - private _firstPageCallback: () => void = () => {}; - private readonly _firstPagePromise: Promise; - static async connect(transport: ConnectionTransport, logger: InnerLogger, slowMo: number = 0, attachToDefaultContext: boolean = false): Promise { const browser = new WKBrowser(SlowMoTransport.wrap(transport, slowMo), logger, attachToDefaultContext); return browser; @@ -62,8 +59,6 @@ export class WKBrowser extends BrowserBase { helper.addEventListener(this._browserSession, 'Playwright.downloadFinished', this._onDownloadFinished.bind(this)), helper.addEventListener(this._browserSession, kPageProxyMessageReceived, this._onPageProxyMessageReceived.bind(this)), ]; - - this._firstPagePromise = new Promise(resolve => this._firstPageCallback = resolve); } _onDisconnect() { @@ -90,11 +85,6 @@ export class WKBrowser extends BrowserBase { return Array.from(this._contexts.values()); } - async _waitForFirstPageTarget(): Promise { - assert(!this._wkPages.size); - return this._firstPagePromise; - } - _onDownloadCreated(payload: Protocol.Playwright.downloadCreatedPayload) { const page = this._wkPages.get(payload.pageProxyId); if (!page) @@ -152,7 +142,6 @@ export class WKBrowser extends BrowserBase { signalBarrier.addPopup(wkPage.pageOrError()); } wkPage.pageOrError().then(async () => { - this._firstPageCallback(); const page = wkPage._page; context!.emit(Events.BrowserContext.Page, page); if (!opener) diff --git a/test/launcher.spec.js b/test/launcher.spec.js index 0e03f4cdbf..46203f8fce 100644 --- a/test/launcher.spec.js +++ b/test/launcher.spec.js @@ -39,7 +39,7 @@ describe('Playwright', function() { await browserType.launch(options).catch(e => waitError = e); expect(waitError.message).toContain('launchPersistentContext'); }); - it('should throw if page argument is passed', async({browserType, defaultBrowserOptions}) => { + it.skip(FFOX)('should throw if page argument is passed', async({browserType, defaultBrowserOptions}) => { let waitError = null; const options = Object.assign({}, defaultBrowserOptions, { args: ['http://example.com'] }); await browserType.launch(options).catch(e => waitError = e); @@ -62,18 +62,28 @@ describe('Playwright', function() { await browserContext.close(); await removeUserDataDir(userDataDir); }); - it('should have custom URL when launching browser', async ({browserType, defaultBrowserOptions, server}) => { + it.skip(FFOX)('should throw if page argument is passed', async ({browserType, defaultBrowserOptions, server}) => { const userDataDir = await makeUserDataDir(); const options = Object.assign({}, defaultBrowserOptions); options.args = [server.EMPTY_PAGE].concat(options.args || []); + const error = await browserType.launchPersistentContext(userDataDir, options).catch(e => e); + expect(error.message).toContain('can not specify page'); + await removeUserDataDir(userDataDir); + }); + it('should have passed URL when launching with ignoreDefaultArgs: true', async ({browserType, defaultBrowserOptions, server}) => { + const userDataDir = await makeUserDataDir(); + const args = browserType._defaultArgs(defaultBrowserOptions, 'persistent', userDataDir, 0).filter(a => a !== 'about:blank'); + const options = { + ...defaultBrowserOptions, + args: [...args, server.EMPTY_PAGE], + ignoreDefaultArgs: true, + }; const browserContext = await browserType.launchPersistentContext(userDataDir, options); - const pages = browserContext.pages(); - expect(pages.length).toBe(1); - const page = pages[0]; - if (page.url() !== server.EMPTY_PAGE) { - await page.waitForNavigation(); - } - expect(page.url()).toBe(server.EMPTY_PAGE); + if (!browserContext.pages().length) + await browserContext.waitForEvent('page'); + await browserContext.pages()[0].waitForLoadState(); + const gotUrls = browserContext.pages().map(page => page.url()); + expect(gotUrls).toEqual([server.EMPTY_PAGE]); await browserContext.close(); await removeUserDataDir(userDataDir); });