From 4d069dda0f1c12a039877caed4403bf5ed753d85 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Tue, 9 Jun 2020 18:57:11 -0700 Subject: [PATCH] feat: avoid side effects after progress aborts (#2518) Actions like click, focus or polling now avoid doing any work with side-effects after their progress has been aborted. --- src/dom.ts | 37 +++++++++++++++++++++++-------------- src/progress.ts | 7 +++++++ test/click.spec.js | 7 +++++++ test/waittask.spec.js | 15 +++++++++++++++ 4 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/dom.ts b/src/dom.ts index 0e8903a756..f67c1e1def 100644 --- a/src/dom.ts +++ b/src/dom.ts @@ -262,6 +262,7 @@ export class ElementHandle extends js.JSHandle { await this._waitForDisplayedAtStablePositionAndEnabled(progress); progress.log(apiLog, ' scrolling into view if needed'); + progress.throwIfAborted(); // Avoid action that has side-effects. const scrolled = await this._scrollRectIntoViewIfNeeded(position ? { x: position.x, y: position.y, width: 0, height: 0 } : undefined); if (scrolled === 'invisible') { if (force) @@ -299,6 +300,9 @@ export class ElementHandle extends js.JSHandle { } await this._page._frameManager.waitForSignalsCreatedBy(progress, options.noWaitAfter, async () => { + if ((options as any).__testHookBeforePointerAction) + await (options as any).__testHookBeforePointerAction(); + progress.throwIfAborted(); // Avoid action that has side-effects. let restoreModifiers: input.Modifier[] | undefined; if (options && options.modifiers) restoreModifiers = await this._page.keyboard._ensureModifiers(options.modifiers); @@ -362,6 +366,7 @@ export class ElementHandle extends js.JSHandle { assert(helper.isNumber(option.index), 'Indices must be numbers. Found index "' + option.index + '" of type "' + (typeof option.index) + '"'); } return this._page._frameManager.waitForSignalsCreatedBy(progress, options.noWaitAfter, async () => { + progress.throwIfAborted(); // Avoid action that has side-effects. const injectedResult = await this._evaluateInUtility(([injected, node, selectOptions]) => injected.selectOptions(node, selectOptions), selectOptions); return handleInjectedResult(injectedResult); }); @@ -381,6 +386,7 @@ export class ElementHandle extends js.JSHandle { const pollHandler = new InjectedScriptPollHandler(progress, poll); const injectedResult = await pollHandler.finish(); const needsInput = handleInjectedResult(injectedResult); + progress.throwIfAborted(); // Avoid action that has side-effects. if (needsInput) { if (value) await this._page.keyboard.insertText(value); @@ -392,6 +398,7 @@ export class ElementHandle extends js.JSHandle { async selectText(): Promise { return runAbortableTask(async progress => { + progress.throwIfAborted(); // Avoid action that has side-effects. const injectedResult = await this._evaluateInUtility(([injected, node]) => injected.selectText(node), {}); handleInjectedResult(injectedResult); }, this._page._logger, 0); @@ -431,6 +438,7 @@ export class ElementHandle extends js.JSHandle { } } await this._page._frameManager.waitForSignalsCreatedBy(progress, options.noWaitAfter, async () => { + progress.throwIfAborted(); // Avoid action that has side-effects. await this._page._delegate.setInputFiles(this as any as ElementHandle, filePayloads); }); } @@ -440,6 +448,7 @@ export class ElementHandle extends js.JSHandle { } async _focus(progress: Progress) { + progress.throwIfAborted(); // Avoid action that has side-effects. const injectedResult = await this._evaluateInUtility(([injected, node]) => injected.focusNode(node), {}); handleInjectedResult(injectedResult); } @@ -452,6 +461,7 @@ export class ElementHandle extends js.JSHandle { progress.log(apiLog, `elementHandle.type("${text}")`); return this._page._frameManager.waitForSignalsCreatedBy(progress, options.noWaitAfter, async () => { await this._focus(progress); + progress.throwIfAborted(); // Avoid action that has side-effects. await this._page.keyboard.type(text, options); }, 'input'); } @@ -464,22 +474,17 @@ export class ElementHandle extends js.JSHandle { progress.log(apiLog, `elementHandle.press("${key}")`); return this._page._frameManager.waitForSignalsCreatedBy(progress, options.noWaitAfter, async () => { await this._focus(progress); + progress.throwIfAborted(); // Avoid action that has side-effects. await this._page.keyboard.press(key, options); }, 'input'); } async check(options: types.PointerActionWaitOptions & types.NavigatingActionWaitOptions = {}) { - return runAbortableTask(async progress => { - progress.log(apiLog, `elementHandle.check()`); - await this._setChecked(progress, true, options); - }, this._page._logger, this._page._timeoutSettings.timeout(options)); + return runAbortableTask(progress => this._setChecked(progress, true, options), this._page._logger, this._page._timeoutSettings.timeout(options)); } async uncheck(options: types.PointerActionWaitOptions & types.NavigatingActionWaitOptions = {}) { - return runAbortableTask(async progress => { - progress.log(apiLog, `elementHandle.uncheck()`); - await this._setChecked(progress, false, options); - }, this._page._logger, this._page._timeoutSettings.timeout(options)); + return runAbortableTask(progress => this._setChecked(progress, false, options), this._page._logger, this._page._timeoutSettings.timeout(options)); } async _setChecked(progress: Progress, state: boolean, options: types.PointerActionWaitOptions & types.NavigatingActionWaitOptions) { @@ -529,10 +534,10 @@ export class ElementHandle extends js.JSHandle { async _waitForDisplayedAtStablePositionAndEnabled(progress: Progress): Promise { progress.log(apiLog, ' waiting for element to be displayed, enabled and not moving'); const rafCount = this._page._delegate.rafCountForStablePosition(); - const poll = await this._evaluateHandleInUtility(([injected, node, rafCount]) => { + const poll = this._evaluateHandleInUtility(([injected, node, rafCount]) => { return injected.waitForDisplayedAtStablePositionAndEnabled(node, rafCount); }, rafCount); - const pollHandler = new InjectedScriptPollHandler(progress, poll); + const pollHandler = new InjectedScriptPollHandler(progress, await poll); const injectedResult = await pollHandler.finish(); handleInjectedResult(injectedResult); progress.log(apiLog, ' element is displayed and does not move'); @@ -565,6 +570,9 @@ export class InjectedScriptPollHandler { constructor(progress: Progress, poll: js.JSHandle>) { this._progress = progress; this._poll = poll; + // Ensure we cancel the poll before progress aborts and returns: + // - no unnecessary work in the page; + // - no possible side effects after progress promsie rejects. this._progress.cleanupWhenAborted(() => this.cancel()); this._streamLogs(poll.evaluateHandle(poll => poll.logs)); } @@ -588,7 +596,7 @@ export class InjectedScriptPollHandler { await this._finishInternal(); return result; } finally { - this.cancel(); + await this.cancel(); } } @@ -598,7 +606,7 @@ export class InjectedScriptPollHandler { await this._finishInternal(); return result; } finally { - this.cancel(); + await this.cancel(); } } @@ -611,12 +619,13 @@ export class InjectedScriptPollHandler { this._progress.log(apiLog, message); } - cancel() { + async cancel() { if (!this._poll) return; const copy = this._poll; this._poll = null; - copy.evaluate(p => p.cancel()).catch(e => {}).then(() => copy.dispose()); + await copy.evaluate(p => p.cancel()).catch(e => {}); + copy.dispose(); } } diff --git a/src/progress.ts b/src/progress.ts index caf26966e1..c9155b7191 100644 --- a/src/progress.ts +++ b/src/progress.ts @@ -26,6 +26,7 @@ export interface Progress { isRunning(): boolean; cleanupWhenAborted(cleanup: () => any): void; log(log: Log, message: string | Error): void; + throwIfAborted(): void; } export async function runAbortableTask(task: (progress: Progress) => Promise, logger: InnerLogger, timeout: number, apiName?: string): Promise { @@ -89,6 +90,10 @@ export class ProgressController { this._logger.log(log, message); } }, + throwIfAborted: () => { + if (this._state === 'aborted') + throw new AbortedError(); + }, }; this._logger.log(apiLog, `=> ${this._apiName} started`); @@ -142,3 +147,5 @@ function monotonicTime(): number { const [seconds, nanoseconds] = process.hrtime(); return seconds * 1000 + (nanoseconds / 1000000 | 0); } + +class AbortedError extends Error {} diff --git a/test/click.spec.js b/test/click.spec.js index 4569ca74cc..1e875bc89b 100644 --- a/test/click.spec.js +++ b/test/click.spec.js @@ -66,6 +66,13 @@ describe('Page.click', function() { ]).catch(e => {}); await context.close(); }); + it('should avoid side effects after timeout', async({page, server}) => { + await page.goto(server.PREFIX + '/input/button.html'); + const error = await page.click('button', { timeout: 2000, __testHookBeforePointerAction: () => new Promise(f => setTimeout(f, 2500))}).catch(e => e); + await page.waitForTimeout(5000); // Give it some time to click after the test hook is done waiting. + expect(await page.evaluate(() => result)).toBe('Was not clicked'); + expect(error.message).toContain('Timeout 2000ms exceeded during page.click.'); + }); it('should click the button after navigation ', async({page, server}) => { await page.goto(server.PREFIX + '/input/button.html'); await page.click('button'); diff --git a/test/waittask.spec.js b/test/waittask.spec.js index 5f3a4dddaa..b3e1175ad9 100644 --- a/test/waittask.spec.js +++ b/test/waittask.spec.js @@ -57,6 +57,21 @@ describe('Frame.waitForFunction', function() { }, {}, {polling}); expect(await timeDelta.jsonValue()).not.toBeLessThan(polling); }); + it('should avoid side effects after timeout', async({page, server}) => { + let counter = 0; + page.on('console', () => ++counter); + + const error = await page.waitForFunction(() => { + window.counter = (window.counter || 0) + 1; + console.log(window.counter); + }, {}, { polling: 1, timeout: 1000 }).catch(e => e); + + const savedCounter = counter; + await page.waitForTimeout(2000); // Give it some time to produce more logs. + + expect(error.message).toContain('Timeout 1000ms exceeded during page.waitForFunction'); + expect(counter).toBe(savedCounter); + }); it('should throw on polling:mutation', async({page, server}) => { const error = await page.waitForFunction(() => true, {}, {polling: 'mutation'}).catch(e => e); expect(error.message).toBe('Unknown polling option: mutation');