From 807dc1f3248571f5dcb13731c14b349e47c6e868 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 25 Jun 2020 09:53:56 -0700 Subject: [PATCH] fix(crash): improve documentation for crash, reject waitForEvent (#2694) --- docs/api.md | 26 +++++++++++++++++++++++++- src/frames.ts | 4 +++- src/page.ts | 9 ++++++++- test/launcher.spec.js | 4 ++-- test/page.spec.js | 26 ++++++++++++++++++++++++-- 5 files changed, 62 insertions(+), 7 deletions(-) diff --git a/docs/api.md b/docs/api.md index 5067a33eba..9a38688a9e 100644 --- a/docs/api.md +++ b/docs/api.md @@ -783,7 +783,31 @@ page.evaluate(() => console.log('hello', 5, {foo: 'bar'})); #### event: 'crash' -Emitted when the page crashes. Browser pages might crash if they try to allocate too much memory. +Emitted when the page crashes. Browser pages might crash if they try to allocate too much memory. When the page crashes, ongoing and subsequent operations will throw. + +The most common way to deal with crashes is to catch an exception: +```js +try { + // Crash might happen during a click. + await page.click('button'); + // Or while waiting for an event. + await page.waitForEvent('popup'); +} catch (e) { + // When the page crashes, exception message contains 'crash'. +} +``` + +However, when manually listening to events, it might be useful to avoid stalling when the page crashes. In this case, handling `crash` event helps: + +```js +await new Promise((resolve, reject) => { + page.on('requestfinished', async request => { + if (await someProcessing(request)) + resolve(request); + }); + page.on('crash', error => reject(error)); +}); +``` #### event: 'dialog' - <[Dialog]> diff --git a/src/frames.ts b/src/frames.ts index 4234f8c2ce..1d211c3a9b 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -983,6 +983,7 @@ class SignalBarrier { const frameTask = new FrameTask(frame, this._progress); await Promise.race([ frame._page._disconnectedPromise, + frame._page._crashedPromise, frame._detachedPromise, frameTask.waitForNewDocument(), frameTask.waitForSameDocumentNavigation(), @@ -1114,6 +1115,7 @@ class FrameTask { } function abortProgressOnFrameDetach(controller: ProgressController, frame: Frame) { - frame._page._disconnectedPromise.then(() => controller.abort(new Error('Navigation failed because browser has disconnected!'))); + frame._page._disconnectedPromise.then(() => controller.abort(new Error('Navigation failed because page was closed!'))); + frame._page._crashedPromise.then(() => controller.abort(new Error('Navigation failed because page crashed!'))); frame._detachedPromise.then(() => controller.abort(new Error('Navigating frame was detached!'))); } diff --git a/src/page.ts b/src/page.ts index 4cc1e25d74..31ba7e5370 100644 --- a/src/page.ts +++ b/src/page.ts @@ -94,6 +94,8 @@ export class Page extends EventEmitter { private _disconnected = false; private _disconnectedCallback: (e: Error) => void; readonly _disconnectedPromise: Promise; + private _crashedCallback: (e: Error) => void; + readonly _crashedPromise: Promise; readonly _browserContext: BrowserContextBase; readonly keyboard: input.Keyboard; readonly mouse: input.Mouse; @@ -121,6 +123,8 @@ export class Page extends EventEmitter { this._closedPromise = new Promise(f => this._closedCallback = f); this._disconnectedCallback = () => {}; this._disconnectedPromise = new Promise(f => this._disconnectedCallback = f); + this._crashedCallback = () => {}; + this._crashedPromise = new Promise(f => this._crashedCallback = f); this._browserContext = browserContext; this._state = { viewportSize: browserContext._options.viewport ? { ...browserContext._options.viewport } : null, @@ -153,12 +157,13 @@ export class Page extends EventEmitter { _didCrash() { this.emit(Events.Page.Crash); + this._crashedCallback(new Error('Page crashed')); } _didDisconnect() { assert(!this._disconnected, 'Page disconnected twice'); this._disconnected = true; - this._disconnectedCallback(new Error('Target closed')); + this._disconnectedCallback(new Error('Page closed')); } async _onFileChooserOpened(handle: dom.ElementHandle) { @@ -335,6 +340,8 @@ export class Page extends EventEmitter { const options = typeof optionsOrPredicate === 'function' ? { predicate: optionsOrPredicate } : optionsOrPredicate; const progressController = new ProgressController(this._logger, this._timeoutSettings.timeout(options), 'page.waitForEvent'); 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)); } diff --git a/test/launcher.spec.js b/test/launcher.spec.js index 08f4a8ed4a..f3c431a7a2 100644 --- a/test/launcher.spec.js +++ b/test/launcher.spec.js @@ -154,7 +154,7 @@ describe('Browser.disconnect', function() { await server.waitForRequest('/one-style.css'); await remote.close(); const error = await navigationPromise; - expect(error.message).toContain('Navigation failed because browser has disconnected!'); + expect(error.message).toContain('Navigation failed because page was closed!'); await browserServer._checkLeaks(); await browserServer.close(); }); @@ -211,7 +211,7 @@ describe('Browser.close', function() { ]); for (let i = 0; i < 2; i++) { const message = results[i].message; - expect(message).toContain('Target closed'); + expect(message).toContain('Page closed'); expect(message).not.toContain('Timeout'); } }); diff --git a/test/page.spec.js b/test/page.spec.js index fc99c65cf2..c53ca361bc 100644 --- a/test/page.spec.js +++ b/test/page.spec.js @@ -78,7 +78,7 @@ describe('Page.close', function() { ]); for (let i = 0; i < 2; i++) { const message = results[i].message; - expect(message).toContain('Target closed'); + expect(message).toContain('Page closed'); expect(message).not.toContain('Timeout'); } }); @@ -130,6 +130,28 @@ describe.fail(FFOX && WIN)('Page.Events.Crash', function() { expect(err).toBeTruthy(); expect(err.message).toContain('crash'); }); + it('should cancel waitForEvent when page crashes', async({page}) => { + await page.setContent(`
This page should crash
`); + const promise = page.waitForEvent('response').catch(e => e); + crash(page); + const error = await promise; + expect(error.message).toContain('Page crashed'); + }); + it('should cancel navigation when page crashes', async({page, server}) => { + await page.setContent(`
This page should crash
`); + server.setRoute('/one-style.css', () => {}); + const promise = page.goto(server.PREFIX + '/one-style.html').catch(e => e); + await page.waitForNavigation({ waitUntil: 'domcontentloaded' }); + crash(page); + const error = await promise; + expect(error.message).toContain('Navigation failed because page crashed'); + }); + it('should be able to close context when page crashes', async({page}) => { + await page.setContent(`
This page should crash
`); + crash(page); + await page.waitForEvent('crash'); + await page.context().close(); + }); }); describe('Page.opener', function() { @@ -353,7 +375,7 @@ describe('Page.waitForEvent', function() { const waitForPromise = page.waitForEvent('download').catch(e => error = e); await page.close(); await waitForPromise; - expect(error.message).toContain('Target closed'); + expect(error.message).toContain('Page closed'); }); });