From 19abc9bd9f6a59e5106c8502332fe3faa69024ff Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 1 Jul 2020 22:10:37 -0700 Subject: [PATCH] fix(dialogs): let click timeout, log information about dialogs (#2781) We should not stall selector actions because of dialogs and properly timeout instead. For this, we should not await the handle.dispose() call because it will never happen while dialog is shown. Also, log information about dialogs to make it easier to debug. --- src/chromium/crPage.ts | 1 + src/dialog.ts | 16 +++++++++++++++- src/firefox/ffPage.ts | 1 + src/frames.ts | 6 +++++- src/webkit/wkPage.ts | 13 +++++++------ test/click.spec.js | 8 ++++++++ test/dialog.spec.js | 29 ++++++++++++++++++++++++++++- 7 files changed, 65 insertions(+), 9 deletions(-) diff --git a/src/chromium/crPage.ts b/src/chromium/crPage.ts index 66b9a485ff..95ab316643 100644 --- a/src/chromium/crPage.ts +++ b/src/chromium/crPage.ts @@ -650,6 +650,7 @@ class FrameSession { _onDialog(event: Protocol.Page.javascriptDialogOpeningPayload) { this._page.emit(Events.Page.Dialog, new dialog.Dialog( + this._page._logger, event.type, event.message, async (accept: boolean, promptText?: string) => { diff --git a/src/dialog.ts b/src/dialog.ts index c439b987bd..5f7db6adef 100644 --- a/src/dialog.ts +++ b/src/dialog.ts @@ -16,23 +16,27 @@ */ import { assert } from './helper'; +import { Logger } from './logger'; type OnHandle = (accept: boolean, promptText?: string) => Promise; export type DialogType = 'alert' | 'beforeunload' | 'confirm' | 'prompt'; export class Dialog { + private _logger: Logger; private _type: string; private _message: string; private _onHandle: OnHandle; private _handled = false; private _defaultValue: string; - constructor(type: string, message: string, onHandle: OnHandle, defaultValue?: string) { + constructor(logger: Logger, type: string, message: string, onHandle: OnHandle, defaultValue?: string) { + this._logger = logger; this._type = type; this._message = message; this._onHandle = onHandle; this._defaultValue = defaultValue || ''; + this._logger.info(` ${this._preview()} was shown`); } type(): string { @@ -50,12 +54,22 @@ export class Dialog { async accept(promptText: string | undefined) { assert(!this._handled, 'Cannot accept dialog which is already handled!'); this._handled = true; + this._logger.info(` ${this._preview()} was accepted`); await this._onHandle(true, promptText); } async dismiss() { assert(!this._handled, 'Cannot dismiss dialog which is already handled!'); this._handled = true; + this._logger.info(` ${this._preview()} was dismissed`); await this._onHandle(false); } + + private _preview(): string { + if (!this._message) + return this._type; + if (this._message.length <= 50) + return `${this._type} "${this._message}"`; + return `${this._type} "${this._message.substring(0, 49) + '\u2026'}"`; + } } diff --git a/src/firefox/ffPage.ts b/src/firefox/ffPage.ts index 48c9a40f66..304166ebf2 100644 --- a/src/firefox/ffPage.ts +++ b/src/firefox/ffPage.ts @@ -188,6 +188,7 @@ export class FFPage implements PageDelegate { _onDialogOpened(params: Protocol.Page.dialogOpenedPayload) { this._page.emit(Events.Page.Dialog, new dialog.Dialog( + this._page._logger, params.type, params.message, async (accept: boolean, promptText?: string) => { diff --git a/src/frames.ts b/src/frames.ts index 6096f4e191..f3be8b2af4 100644 --- a/src/frames.ts +++ b/src/frames.ts @@ -726,7 +726,11 @@ export class Frame { const task = dom.waitForSelectorTask(info, 'attached'); const handle = await this._scheduleRerunnableHandleTask(progress, info.world, task); const element = handle.asElement() as dom.ElementHandle; - progress.cleanupWhenAborted(() => element.dispose()); + progress.cleanupWhenAborted(() => { + // Do not await here to avoid being blocked, either by stalled + // page (e.g. alert) or unresolved navigation in Chromium. + element.dispose(); + }); const result = await action(progress, element); element.dispose(); if (result === 'error:notconnected') { diff --git a/src/webkit/wkPage.ts b/src/webkit/wkPage.ts index e4d7438480..6f58c4279d 100644 --- a/src/webkit/wkPage.ts +++ b/src/webkit/wkPage.ts @@ -519,12 +519,13 @@ export class WKPage implements PageDelegate { _onDialog(event: Protocol.Dialog.javascriptDialogOpeningPayload) { this._page.emit(Events.Page.Dialog, new dialog.Dialog( - event.type as dialog.DialogType, - event.message, - async (accept: boolean, promptText?: string) => { - await this._pageProxySession.send('Dialog.handleJavaScriptDialog', { accept, promptText }); - }, - event.defaultPrompt)); + this._page._logger, + event.type as dialog.DialogType, + event.message, + async (accept: boolean, promptText?: string) => { + await this._pageProxySession.send('Dialog.handleJavaScriptDialog', { accept, promptText }); + }, + event.defaultPrompt)); } private async _onFileChooserOpened(event: {frameId: Protocol.Network.FrameId, element: Protocol.Runtime.RemoteObject}) { diff --git a/test/click.spec.js b/test/click.spec.js index 313d4dc8a6..248c2ee79e 100644 --- a/test/click.spec.js +++ b/test/click.spec.js @@ -835,6 +835,14 @@ describe('Page.click', function() { await page.click('button'); expect(await page.evaluate(() => result)).toBe('Clicked'); }); + it('should timeout when click opens alert', async({page, server}) => { + const dialogPromise = page.waitForEvent('dialog'); + await page.setContent(`
Click me
`); + const error = await page.click('div', { timeout: 3000 }).catch(e => e); + expect(error.message).toContain('Timeout 3000ms exceeded during page.click.'); + const dialog = await dialogPromise; + await dialog.dismiss(); + }); }); describe('Page.check', function() { diff --git a/test/dialog.spec.js b/test/dialog.spec.js index 89b71a4b79..dc50793a62 100644 --- a/test/dialog.spec.js +++ b/test/dialog.spec.js @@ -15,7 +15,7 @@ * limitations under the License. */ -const {FFOX, CHROMIUM, WEBKIT} = require('./utils').testOptions(browserType); +const {FFOX, CHROMIUM, WEBKIT, CHANNEL} = require('./utils').testOptions(browserType); describe('Page.Events.Dialog', function() { it('should fire', async({page, server}) => { @@ -58,4 +58,31 @@ describe('Page.Events.Dialog', function() { const result = await page.evaluate(() => confirm('boolean?')); expect(result).toBe(false); }); + it.fail(CHANNEL)('should log prompt actions', async({browser}) => { + const messages = []; + const context = await browser.newContext({ + logger: { + isEnabled: () => true, + log: (name, severity, message) => messages.push(message), + } + }); + const page = await context.newPage(); + const promise = page.evaluate(() => confirm('01234567890123456789012345678901234567890123456789012345678901234567890123456789')); + const dialog = await page.waitForEvent('dialog'); + expect(messages.join()).toContain('confirm "0123456789012345678901234567890123456789012345678…" was shown'); + await dialog.accept('123'); + await promise; + expect(messages.join()).toContain('confirm "0123456789012345678901234567890123456789012345678…" was accepted'); + await context.close(); + }); + it.fail(WEBKIT)('should be able to close context with open alert', async({browser}) => { + const context = await browser.newContext(); + const page = await context.newPage(); + const alertPromise = page.waitForEvent('dialog'); + await page.evaluate(() => { + setTimeout(() => alert('hello'), 0); + }); + await alertPromise; + await context.close(); + }); });