diff --git a/src/browserContext.ts b/src/browserContext.ts index eacd967304..0a41322b9b 100644 --- a/src/browserContext.ts +++ b/src/browserContext.ts @@ -99,7 +99,7 @@ export abstract class BrowserContextBase extends EventEmitter implements Browser const progressController = new ProgressController(this._apiLogger, this._timeoutSettings.timeout(options), 'browserContext.waitForEvent'); if (event !== Events.BrowserContext.Close) this._closePromise.then(error => progressController.abort(error)); - return progressController.run(progress => helper.waitForEvent(progress, this, event, options.predicate)); + return progressController.run(progress => helper.waitForEvent(progress, this, event, options.predicate).promise); } _browserClosed() { diff --git a/src/frames.ts b/src/frames.ts index 79f7865355..1103702679 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -28,6 +28,7 @@ import { selectors } from './selectors'; import * as types from './types'; import { BrowserContext } from './browserContext'; import { Progress, ProgressController } from './progress'; +import { EventEmitter } from 'events'; type ContextData = { contextPromise: Promise; @@ -51,6 +52,13 @@ type ConsoleTagHandler = () => void; export type FunctionWithSource = (source: { context: BrowserContext, page: Page, frame: Frame}, ...args: any) => any; +export const kNavigationEvent = Symbol('navigation'); +type NavigationEvent = { + documentInfo?: DocumentInfo, // Undefined for same-document navigations. + error?: Error, +}; +export const kLifecycleEvent = Symbol('lifecycle'); + export class FrameManager { private _page: Page; private _frames = new Map(); @@ -158,11 +166,13 @@ export class FrameManager { else frame._currentDocument = { documentId, request: undefined }; frame._pendingDocument = undefined; - for (const task of frame._frameTasks) - task.onNewDocument(frame._currentDocument); + const navigationEvent: NavigationEvent = { documentInfo: frame._currentDocument }; + frame._eventEmitter.emit(kNavigationEvent, navigationEvent); frame._onClearLifecycle(); - if (!initial) + if (!initial) { + this._page._logger.info(` navigated to "${url}"`); this._page.emit(Events.Page.FrameNavigated, frame); + } } frameCommittedSameDocumentNavigation(frameId: string, url: string) { @@ -170,8 +180,9 @@ export class FrameManager { if (!frame) return; frame._url = url; - for (const task of frame._frameTasks) - task.onSameDocument(); + const navigationEvent: NavigationEvent = {}; + frame._eventEmitter.emit(kNavigationEvent, navigationEvent); + this._page._logger.info(` navigated to "${url}"`); this._page.emit(Events.Page.FrameNavigated, frame); } @@ -181,10 +192,9 @@ export class FrameManager { return; if (documentId !== undefined && frame._pendingDocument.documentId !== documentId) return; - const pending = frame._pendingDocument; + const navigationEvent: NavigationEvent = { documentInfo: frame._pendingDocument, error: new Error(errorText) }; frame._pendingDocument = undefined; - for (const task of frame._frameTasks) - task.onNewDocument(pending, new Error(errorText)); + frame._eventEmitter.emit(kNavigationEvent, navigationEvent); } frameDetached(frameId: string) { @@ -288,12 +298,12 @@ export class FrameManager { } export class Frame { + readonly _eventEmitter: EventEmitter; _id: string; private _firedLifecycleEvents = new Set(); _subtreeLifecycleEvents = new Set(); _currentDocument: DocumentInfo; _pendingDocument?: DocumentInfo; - _frameTasks = new Set(); readonly _page: Page; private _parentFrame: Frame | null; _url = ''; @@ -308,6 +318,8 @@ export class Frame { private _detachedCallback = () => {}; constructor(page: Page, id: string, parentFrame: Frame | null) { + this._eventEmitter = new EventEmitter(); + this._eventEmitter.setMaxListeners(0); this._id = id; this._page = page; this._parentFrame = parentFrame; @@ -363,8 +375,9 @@ export class Frame { for (const event of events) { // Checking whether we have already notified about this event. if (!this._subtreeLifecycleEvents.has(event)) { - for (const frameTask of this._frameTasks) - frameTask.onLifecycle(event); + this._eventEmitter.emit(kLifecycleEvent, event); + if (this === mainFrame && this._url !== 'about:blank') + this._page._logger.info(` "${event}" event fired`); if (this === mainFrame && event === 'load') this._page.emit(Events.Page.Load); if (this === mainFrame && event === 'domcontentloaded') @@ -376,7 +389,8 @@ export class Frame { async goto(url: string, options: types.GotoOptions = {}): Promise { return runNavigationTask(this, options, this._apiName('goto'), async progress => { - progress.logger.info(`navigating to "${url}", waiting until "${options.waitUntil || 'load'}"`); + const waitUntil = verifyLifecycle(options.waitUntil === undefined ? 'load' : options.waitUntil); + progress.logger.info(`navigating to "${url}", waiting until "${waitUntil}"`); const headers = (this._page._state.extraHTTPHeaders || {}); let referer = headers['referer'] || headers['Referer']; if (options.referer !== undefined) { @@ -386,23 +400,33 @@ export class Frame { } url = helper.completeUserURL(url); - const frameTask = new FrameTask(this, progress); - const sameDocumentPromise = frameTask.waitForSameDocumentNavigation(); - const navigateResult = await this._page._delegate.navigateFrame(this, url, referer).catch(e => { - // Do not leave sameDocumentPromise unhandled. - sameDocumentPromise.catch(e => {}); - throw e; - }); - let request: network.Request | undefined; + const sameDocument = helper.waitForEvent(progress, this._eventEmitter, kNavigationEvent, (e: NavigationEvent) => !e.documentInfo); + const navigateResult = await this._page._delegate.navigateFrame(this, url, referer); + + let event: NavigationEvent; if (navigateResult.newDocumentId) { - // Do not leave sameDocumentPromise unhandled. - sameDocumentPromise.catch(e => {}); - request = await frameTask.waitForSpecificDocument(navigateResult.newDocumentId); + sameDocument.dispose(); + event = await helper.waitForEvent(progress, this._eventEmitter, kNavigationEvent, (event: NavigationEvent) => { + // We are interested either in this specific document, or any other document that + // did commit and replaced the expected document. + return event.documentInfo && (event.documentInfo.documentId === navigateResult.newDocumentId || !event.error); + }).promise; + + if (event.documentInfo!.documentId !== navigateResult.newDocumentId) { + // This is just a sanity check. In practice, new navigation should + // cancel the previous one and report "request cancelled"-like error. + throw new Error('Navigation interrupted by another one'); + } + if (event.error) + throw event.error; } else { - await sameDocumentPromise; + event = await sameDocument.promise; } - await frameTask.waitForLifecycle(options.waitUntil === undefined ? 'load' : options.waitUntil); - frameTask.done(); + + if (!this._subtreeLifecycleEvents.has(waitUntil)) + await helper.waitForEvent(progress, this._eventEmitter, kLifecycleEvent, (e: types.LifecycleEvent) => e === waitUntil).promise; + + const request = event.documentInfo ? event.documentInfo.request : undefined; return request ? request._finalRequest().response() : null; }); } @@ -410,15 +434,23 @@ export class Frame { async waitForNavigation(options: types.WaitForNavigationOptions = {}): Promise { return runNavigationTask(this, options, this._apiName('waitForNavigation'), async progress => { const toUrl = typeof options.url === 'string' ? ` to "${options.url}"` : ''; - progress.logger.info(`waiting for navigation${toUrl} until "${options.waitUntil || 'load'}"`); - const frameTask = new FrameTask(this, progress); - let request: network.Request | undefined; - await Promise.race([ - frameTask.waitForNewDocument(options.url).then(r => request = r), - frameTask.waitForSameDocumentNavigation(options.url), - ]); - await frameTask.waitForLifecycle(options.waitUntil === undefined ? 'load' : options.waitUntil); - frameTask.done(); + const waitUntil = verifyLifecycle(options.waitUntil === undefined ? 'load' : options.waitUntil); + progress.logger.info(`waiting for navigation${toUrl} until "${waitUntil}"`); + + const navigationEvent: NavigationEvent = await helper.waitForEvent(progress, this._eventEmitter, kNavigationEvent, (event: NavigationEvent) => { + // Any failed navigation results in a rejection. + if (event.error) + return true; + progress.logger.info(` navigated to "${this._url}"`); + return helper.urlMatches(this._url, options.url); + }).promise; + if (navigationEvent.error) + throw navigationEvent.error; + + if (!this._subtreeLifecycleEvents.has(waitUntil)) + await helper.waitForEvent(progress, this._eventEmitter, kLifecycleEvent, (e: types.LifecycleEvent) => e === waitUntil).promise; + + const request = navigationEvent.documentInfo ? navigationEvent.documentInfo.request : undefined; return request ? request._finalRequest().response() : null; }); } @@ -428,9 +460,9 @@ export class Frame { } async _waitForLoadState(progress: Progress, state: types.LifecycleEvent): Promise { - const frameTask = new FrameTask(this, progress); - await frameTask.waitForLifecycle(state); - frameTask.done(); + const waitUntil = verifyLifecycle(state); + if (!this._subtreeLifecycleEvents.has(waitUntil)) + await helper.waitForEvent(progress, this._eventEmitter, kLifecycleEvent, (e: types.LifecycleEvent) => e === waitUntil).promise; } async frameElement(): Promise { @@ -1035,15 +1067,18 @@ class SignalBarrier { async addFrameNavigation(frame: Frame) { this.retain(); - const frameTask = new FrameTask(frame, this._progress); + const waiter = helper.waitForEvent(null, frame._eventEmitter, kNavigationEvent, (e: NavigationEvent) => { + if (!e.error && this._progress) + this._progress.logger.info(` navigated to "${frame._url}"`); + return true; + }); await Promise.race([ frame._page._disconnectedPromise, frame._page._crashedPromise, frame._detachedPromise, - frameTask.waitForNewDocument(), - frameTask.waitForSameDocumentNavigation(), + waiter.promise, ]).catch(e => {}); - frameTask.done(); + waiter.dispose(); this.release(); } @@ -1058,96 +1093,6 @@ class SignalBarrier { } } -class FrameTask { - private readonly _frame: Frame; - private readonly _progress: Progress | null = null; - private _onSameDocument?: { url?: types.URLMatch, resolve: () => void }; - private _onSpecificDocument?: { expectedDocumentId: string, resolve: (request: network.Request | undefined) => void, reject: (error: Error) => void }; - private _onNewDocument?: { url?: types.URLMatch, resolve: (request: network.Request | undefined) => void, reject: (error: Error) => void }; - private _onLifecycle?: { waitUntil: types.LifecycleEvent, resolve: () => void }; - - constructor(frame: Frame, progress: Progress | null) { - this._frame = frame; - frame._frameTasks.add(this); - this._progress = progress; - if (progress) - progress.cleanupWhenAborted(() => this.done()); - } - - onSameDocument() { - if (this._progress) - this._progress.logger.info(`navigated to "${this._frame._url}"`); - if (this._onSameDocument && helper.urlMatches(this._frame.url(), this._onSameDocument.url)) - this._onSameDocument.resolve(); - } - - onNewDocument(documentInfo: DocumentInfo, error?: Error) { - if (this._progress && !error) - this._progress.logger.info(`navigated to "${this._frame._url}"`); - if (this._onSpecificDocument) { - if (documentInfo.documentId === this._onSpecificDocument.expectedDocumentId) { - if (error) - this._onSpecificDocument.reject(error); - else - this._onSpecificDocument.resolve(documentInfo.request); - } else if (!error) { - this._onSpecificDocument.reject(new Error('Navigation interrupted by another one')); - } - } - if (this._onNewDocument) { - if (error) - this._onNewDocument.reject(error); - else if (helper.urlMatches(this._frame.url(), this._onNewDocument.url)) - this._onNewDocument.resolve(documentInfo.request); - } - } - - onLifecycle(lifecycleEvent: types.LifecycleEvent) { - if (this._progress && this._frame._url !== 'about:blank') - this._progress.logger.info(`"${lifecycleEvent}" event fired`); - if (this._onLifecycle && this._onLifecycle.waitUntil === lifecycleEvent) - this._onLifecycle.resolve(); - } - - waitForSameDocumentNavigation(url?: types.URLMatch): Promise { - return new Promise(resolve => { - assert(!this._onSameDocument); - this._onSameDocument = { url, resolve }; - }); - } - - waitForSpecificDocument(expectedDocumentId: string): Promise { - return new Promise((resolve, reject) => { - assert(!this._onSpecificDocument); - this._onSpecificDocument = { expectedDocumentId, resolve, reject }; - }); - } - - waitForNewDocument(url?: types.URLMatch): Promise { - return new Promise((resolve, reject) => { - assert(!this._onNewDocument); - this._onNewDocument = { url, resolve, reject }; - }); - } - - waitForLifecycle(waitUntil: types.LifecycleEvent): Promise { - if (waitUntil as unknown === 'networkidle0') - waitUntil = 'networkidle'; - if (!types.kLifecycleEvents.has(waitUntil)) - throw new Error(`Unsupported waitUntil option ${String(waitUntil)}`); - if (this._frame._subtreeLifecycleEvents.has(waitUntil)) - return Promise.resolve(); - return new Promise(resolve => { - assert(!this._onLifecycle); - this._onLifecycle = { waitUntil, resolve }; - }); - } - - done() { - this._frame._frameTasks.delete(this); - } -} - async function runNavigationTask(frame: Frame, options: types.TimeoutOptions, apiName: string, task: (progress: Progress) => Promise): Promise { const page = frame._page; const controller = new ProgressController(page._logger, page._timeoutSettings.navigationTimeout(options), apiName); @@ -1156,3 +1101,11 @@ async function runNavigationTask(frame: Frame, options: types.TimeoutOptions, frame._detachedPromise.then(() => controller.abort(new Error('Navigating frame was detached!'))); return controller.run(task); } + +function verifyLifecycle(waitUntil: types.LifecycleEvent): types.LifecycleEvent { + if (waitUntil as unknown === 'networkidle0') + waitUntil = 'networkidle'; + if (!types.kLifecycleEvents.has(waitUntil)) + throw new Error(`Unsupported waitUntil option ${String(waitUntil)}`); + return waitUntil; +} diff --git a/src/helper.ts b/src/helper.ts index 16b7af1a53..9342342304 100644 --- a/src/helper.ts +++ b/src/helper.ts @@ -296,23 +296,25 @@ class Helper { })); } - static async waitForEvent(progress: Progress, emitter: EventEmitter, event: string, predicate?: Function): Promise { + static waitForEvent(progress: Progress | null, emitter: EventEmitter, event: string | symbol, predicate?: Function): { promise: Promise, dispose: () => void } { const listeners: RegisteredListener[] = []; const promise = new Promise((resolve, reject) => { listeners.push(helper.addEventListener(emitter, event, eventArg => { try { if (predicate && !predicate(eventArg)) return; + helper.removeEventListeners(listeners); resolve(eventArg); } catch (e) { + helper.removeEventListeners(listeners); reject(e); } })); }); - progress.cleanupWhenAborted(() => helper.removeEventListeners(listeners)); - const result = await promise; - helper.removeEventListeners(listeners); - return result; + const dispose = () => helper.removeEventListeners(listeners); + if (progress) + progress.cleanupWhenAborted(dispose); + return { promise, dispose }; } static isDebugMode(): boolean { diff --git a/src/page.ts b/src/page.ts index 99a45004f5..1c043e7ea9 100644 --- a/src/page.ts +++ b/src/page.ts @@ -356,7 +356,7 @@ export class Page extends EventEmitter { this._disconnectedPromise.then(error => progressController.abort(error)); if (event !== Events.Page.Crash) this._crashedPromise.then(error => progressController.abort(error)); - return progressController.run(progress => helper.waitForEvent(progress, this, event, options.predicate)); + return progressController.run(progress => helper.waitForEvent(progress, this, event, options.predicate).promise); } async goBack(options?: types.NavigateOptions): Promise { diff --git a/src/progress.ts b/src/progress.ts index 3602d45cad..caecace190 100644 --- a/src/progress.ts +++ b/src/progress.ts @@ -29,12 +29,6 @@ export interface Progress { throwIfAborted(): void; } -let runningTaskCount = 0; - -export function isRunningTask(): boolean { - return !!runningTaskCount; -} - export async function runAbortableTask(task: (progress: Progress) => Promise, logger: Logger, timeout: number, apiName: string): Promise { const controller = new ProgressController(logger, timeout, apiName); return controller.run(task); @@ -75,7 +69,6 @@ export class ProgressController { async run(task: (progress: Progress) => Promise): Promise { assert(this._state === 'before'); this._state = 'running'; - ++runningTaskCount; const loggerScope = this._logger.createScope(this._apiName, true); @@ -114,8 +107,6 @@ export class ProgressController { loggerScope.endScope(`failed`); await Promise.all(this._cleanups.splice(0).map(cleanup => runCleanup(cleanup))); throw e; - } finally { - --runningTaskCount; } } diff --git a/src/server/electron.ts b/src/server/electron.ts index 9267d2d316..3adedfe309 100644 --- a/src/server/electron.ts +++ b/src/server/electron.ts @@ -135,7 +135,7 @@ export class ElectronApplication extends EventEmitter { const progressController = new ProgressController(this._apiLogger, this._timeoutSettings.timeout(options), 'electron.waitForEvent'); if (event !== ElectronEvents.ElectronApplication.Close) this._browserContext._closePromise.then(error => progressController.abort(error)); - return progressController.run(progress => helper.waitForEvent(progress, this, event, options.predicate)); + return progressController.run(progress => helper.waitForEvent(progress, this, event, options.predicate).promise); } async _init() { diff --git a/test/navigation.spec.js b/test/navigation.spec.js index 16340e4a8a..bd1cbb3bce 100644 --- a/test/navigation.spec.js +++ b/test/navigation.spec.js @@ -263,6 +263,21 @@ describe('Page.goto', function() { expect(error).toBe(null); expect(loaded).toBe(true); }); + it('should fail when replaced by another navigation', async({page, server}) => { + let anotherPromise; + server.setRoute('/empty.html', (req, res) => { + anotherPromise = page.goto(server.PREFIX + '/one-style.html'); + // Hang request to empty.html. + }); + const error = await page.goto(server.PREFIX + '/empty.html').catch(e => e); + await anotherPromise; + if (CHROMIUM) + expect(error.message).toContain('net::ERR_ABORTED'); + else if (WEBKIT) + expect(error.message).toContain('cancelled'); + else + expect(error.message).toContain('NS_BINDING_ABORTED'); + }); it('should work when navigating to valid url', async({page, server}) => { const response = await page.goto(server.EMPTY_PAGE); expect(response.ok()).toBe(true); @@ -302,15 +317,23 @@ describe('Page.goto', function() { process.removeListener('warning', warningHandler); expect(warning).toBe(null); }); - it('should not leak listeners during navigation of 11 pages', async({page, context, server}) => { + it('should not leak listeners during navigation of 20 pages', async({page, context, server}) => { let warning = null; const warningHandler = w => warning = w; process.on('warning', warningHandler); - await Promise.all([...Array(20)].map(async() => { - const page = await context.newPage(); - await page.goto(server.EMPTY_PAGE); - await page.close(); - })); + const pages = await Promise.all([...Array(20)].map(() => context.newPage())); + await Promise.all(pages.map(page => page.goto(server.EMPTY_PAGE))); + await Promise.all(pages.map(page => page.close())); + process.removeListener('warning', warningHandler); + expect(warning).toBe(null); + }); + it('should not leak listeners during 20 waitForNavigation', async({page, context, server}) => { + let warning = null; + const warningHandler = w => warning = w; + process.on('warning', warningHandler); + const promises = [...Array(20)].map(() => page.waitForNavigation()); + await page.goto(server.EMPTY_PAGE); + await Promise.all(promises); process.removeListener('warning', warningHandler); expect(warning).toBe(null); }); @@ -558,7 +581,6 @@ describe.skip(CHANNEL)('Page.waitForNavigation', function() { expect(error.message).toContain('Timeout 5000ms exceeded during page.waitForNavigation.'); expect(error.message).toContain('waiting for navigation to "**/frame.html" until "load"'); expect(error.message).toContain(`navigated to "${server.EMPTY_PAGE}"`); - expect(error.message).toContain(`"load" event fired`); }); it('should work with both domcontentloaded and load', async({page, server}) => { let response = null; @@ -951,9 +973,6 @@ describe('Frame.goto', function() { const error = await page.goto(url, { timeout: 5000, waitUntil: 'networkidle' }).catch(e => e); expect(error.message).toContain('Timeout 5000ms exceeded during page.goto.'); expect(error.message).toContain(`navigating to "${url}", waiting until "networkidle"`); - expect(error.message).toContain(`navigated to "${url}"`); - expect(error.message).toContain(`navigated to "${server.PREFIX + '/frames/one-frame.html'}"`); - expect(error.message).toContain(`"domcontentloaded" event fired`); }); it('should return matching responses', async({page, server}) => { await page.goto(server.EMPTY_PAGE);