From 84b4fd4e4064d2551976d8e3f5094be44509e0cd Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 4 Oct 2024 07:25:18 -0700 Subject: [PATCH] feat: wait for pending navigation to resolve before many actions (#32899) This includes all actions that perform locator handler check. Note this makes it impossible to interact with the page while a main frame navigation is ongoing. This was already the case for Chromium, but now WebKit and Firefox align with it. Setting `PLAYWRIGHT_SKIP_NAVIGATION_CHECK` environment variable disables this behavior. --- packages/playwright-core/src/server/dom.ts | 10 +-- packages/playwright-core/src/server/frames.ts | 48 ++++++------ packages/playwright-core/src/server/page.ts | 34 +++++++- .../library/browsercontext-page-event.spec.ts | 16 ---- tests/page/page-autowaiting-no-hang.spec.ts | 77 ++++++++++++++++++- 5 files changed, 136 insertions(+), 49 deletions(-) diff --git a/packages/playwright-core/src/server/dom.ts b/packages/playwright-core/src/server/dom.ts index c2ed979fbe..76708245d4 100644 --- a/packages/playwright-core/src/server/dom.ts +++ b/packages/playwright-core/src/server/dom.ts @@ -292,7 +292,7 @@ export class ElementHandle extends js.JSHandle { }; } - async _retryAction(progress: Progress, actionName: string, action: (retry: number) => Promise, options: { trial?: boolean, force?: boolean, skipLocatorHandlersCheckpoint?: boolean }): Promise<'error:notconnected' | 'done'> { + async _retryAction(progress: Progress, actionName: string, action: (retry: number) => Promise, options: { trial?: boolean, force?: boolean, skipActionPreChecks?: boolean }): Promise<'error:notconnected' | 'done'> { let retry = 0; // We progressively wait longer between retries, up to 500ms. const waitTime = [0, 20, 100, 100, 500]; @@ -310,8 +310,8 @@ export class ElementHandle extends js.JSHandle { } else { progress.log(`attempting ${actionName} action${options.trial ? ' (trial run)' : ''}`); } - if (!options.skipLocatorHandlersCheckpoint && !options.force) - await this._frame._page.performLocatorHandlersCheckpoint(progress); + if (!options.skipActionPreChecks && !options.force) + await this._frame._page.performActionPreChecks(progress); const result = await action(retry); ++retry; if (result === 'error:notvisible') { @@ -346,7 +346,7 @@ export class ElementHandle extends js.JSHandle { async _retryPointerAction(progress: Progress, actionName: ActionName, waitForEnabled: boolean, action: (point: types.Point) => Promise, options: { waitAfter: boolean | 'disabled' } & types.PointerActionOptions & types.PointerActionWaitOptions): Promise<'error:notconnected' | 'done'> { // Note: do not perform locator handlers checkpoint to avoid moving the mouse in the middle of a drag operation. - const skipLocatorHandlersCheckpoint = actionName === 'move and up'; + const skipActionPreChecks = actionName === 'move and up'; return await this._retryAction(progress, actionName, async retry => { // By default, we scroll with protocol method to reveal the action point. // However, that might not work to scroll from under position:sticky elements @@ -360,7 +360,7 @@ export class ElementHandle extends js.JSHandle { ]; const forceScrollOptions = scrollOptions[retry % scrollOptions.length]; return await this._performPointerAction(progress, actionName, waitForEnabled, action, forceScrollOptions, options); - }, { ...options, skipLocatorHandlersCheckpoint }); + }, { ...options, skipActionPreChecks }); } async _performPointerAction( diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index d107a7f981..dd24193f21 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -791,11 +791,11 @@ export class Frame extends SdkObject { }, this._page._timeoutSettings.timeout(options)); } - async waitForSelectorInternal(progress: Progress, selector: string, performLocatorHandlersCheckpoint: boolean, options: types.WaitForElementOptions, scope?: dom.ElementHandle): Promise | null> { + async waitForSelectorInternal(progress: Progress, selector: string, performActionPreChecks: boolean, options: types.WaitForElementOptions, scope?: dom.ElementHandle): Promise | null> { const { state = 'visible' } = options; const promise = this.retryWithProgressAndTimeouts(progress, [0, 20, 50, 100, 100, 500], async continuePolling => { - if (performLocatorHandlersCheckpoint) - await this._page.performLocatorHandlersCheckpoint(progress); + if (performActionPreChecks) + await this._page.performActionPreChecks(progress); const resolved = await this.selectors.resolveInjectedForSelector(selector, options, scope); progress.throwIfAborted(); @@ -1118,12 +1118,12 @@ export class Frame extends SdkObject { progress: Progress, selector: string, strict: boolean | undefined, - performLocatorHandlersCheckpoint: boolean, + performActionPreChecks: boolean, action: (handle: dom.ElementHandle) => Promise): Promise { progress.log(`waiting for ${this._asLocator(selector)}`); return this.retryWithProgressAndTimeouts(progress, [0, 20, 50, 100, 100, 500], async continuePolling => { - if (performLocatorHandlersCheckpoint) - await this._page.performLocatorHandlersCheckpoint(progress); + if (performActionPreChecks) + await this._page.performActionPreChecks(progress); const resolved = await this.selectors.resolveInjectedForSelector(selector, { strict }); progress.throwIfAborted(); @@ -1167,7 +1167,7 @@ export class Frame extends SdkObject { } async rafrafTimeoutScreenshotElementWithProgress(progress: Progress, selector: string, timeout: number, options: ScreenshotOptions): Promise { - return await this._retryWithProgressIfNotConnected(progress, selector, true /* strict */, true /* performLocatorHandlersCheckpoint */, async handle => { + return await this._retryWithProgressIfNotConnected(progress, selector, true /* strict */, true /* performActionPreChecks */, async handle => { await handle._frame.rafrafTimeout(timeout); return await this._page._screenshotter.screenshotElement(progress, handle, options); }); @@ -1176,21 +1176,21 @@ export class Frame extends SdkObject { async click(metadata: CallMetadata, selector: string, options: { noWaitAfter?: boolean } & types.MouseClickOptions & types.PointerActionWaitOptions) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._click(progress, { ...options, waitAfter: !options.noWaitAfter }))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._click(progress, { ...options, waitAfter: !options.noWaitAfter }))); }, this._page._timeoutSettings.timeout(options)); } async dblclick(metadata: CallMetadata, selector: string, options: types.MouseMultiClickOptions & types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._dblclick(progress, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._dblclick(progress, options))); }, this._page._timeoutSettings.timeout(options)); } async dragAndDrop(metadata: CallMetadata, source: string, target: string, options: types.DragActionOptions & types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); await controller.run(async progress => { - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, source, options.strict, !options.force /* performLocatorHandlersCheckpoint */, async handle => { + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, source, options.strict, !options.force /* performActionPreChecks */, async handle => { return handle._retryPointerAction(progress, 'move and down', false, async point => { await this._page.mouse.move(point.x, point.y); await this._page.mouse.down(); @@ -1202,7 +1202,7 @@ export class Frame extends SdkObject { }); })); // Note: do not perform locator handlers checkpoint to avoid moving the mouse in the middle of a drag operation. - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, target, options.strict, false /* performLocatorHandlersCheckpoint */, async handle => { + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, target, options.strict, false /* performActionPreChecks */, async handle => { return handle._retryPointerAction(progress, 'move and up', false, async point => { await this._page.mouse.move(point.x, point.y); await this._page.mouse.up(); @@ -1221,28 +1221,28 @@ export class Frame extends SdkObject { throw new Error('The page does not support tap. Use hasTouch context option to enable touch support.'); const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._tap(progress, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._tap(progress, options))); }, this._page._timeoutSettings.timeout(options)); } async fill(metadata: CallMetadata, selector: string, value: string, options: types.TimeoutOptions & types.StrictOptions & { force?: boolean }) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._fill(progress, value, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._fill(progress, value, options))); }, this._page._timeoutSettings.timeout(options)); } async focus(metadata: CallMetadata, selector: string, options: types.TimeoutOptions & types.StrictOptions = {}) { const controller = new ProgressController(metadata, this); await controller.run(async progress => { - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._focus(progress))); + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._focus(progress))); }, this._page._timeoutSettings.timeout(options)); } async blur(metadata: CallMetadata, selector: string, options: types.TimeoutOptions & types.StrictOptions = {}) { const controller = new ProgressController(metadata, this); await controller.run(async progress => { - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._blur(progress))); + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._blur(progress))); }, this._page._timeoutSettings.timeout(options)); } @@ -1349,14 +1349,14 @@ export class Frame extends SdkObject { async hover(metadata: CallMetadata, selector: string, options: types.PointerActionOptions & types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._hover(progress, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._hover(progress, options))); }, this._page._timeoutSettings.timeout(options)); } async selectOption(metadata: CallMetadata, selector: string, elements: dom.ElementHandle[], values: types.SelectOption[], options: types.CommonActionOptions = {}): Promise { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._selectOption(progress, elements, values, options)); + return await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._selectOption(progress, elements, values, options)); }, this._page._timeoutSettings.timeout(options)); } @@ -1364,35 +1364,35 @@ export class Frame extends SdkObject { const inputFileItems = await prepareFilesForUpload(this, params); const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, params.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._setInputFiles(progress, inputFileItems))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, params.strict, true /* performActionPreChecks */, handle => handle._setInputFiles(progress, inputFileItems))); }, this._page._timeoutSettings.timeout(params)); } async type(metadata: CallMetadata, selector: string, text: string, options: { delay?: number } & types.TimeoutOptions & types.StrictOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._type(progress, text, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._type(progress, text, options))); }, this._page._timeoutSettings.timeout(options)); } async press(metadata: CallMetadata, selector: string, key: string, options: { delay?: number, noWaitAfter?: boolean } & types.TimeoutOptions & types.StrictOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performLocatorHandlersCheckpoint */, handle => handle._press(progress, key, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._press(progress, key, options))); }, this._page._timeoutSettings.timeout(options)); } async check(metadata: CallMetadata, selector: string, options: types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._setChecked(progress, true, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._setChecked(progress, true, options))); }, this._page._timeoutSettings.timeout(options)); } async uncheck(metadata: CallMetadata, selector: string, options: types.PointerActionWaitOptions = {}) { const controller = new ProgressController(metadata, this); return controller.run(async progress => { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performLocatorHandlersCheckpoint */, handle => handle._setChecked(progress, false, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._setChecked(progress, false, options))); }, this._page._timeoutSettings.timeout(options)); } @@ -1421,7 +1421,7 @@ export class Frame extends SdkObject { await (new ProgressController(metadata, this)).run(async progress => { progress.log(`${metadata.apiName}${timeout ? ` with timeout ${timeout}ms` : ''}`); progress.log(`waiting for ${this._asLocator(selector)}`); - await this._page.performLocatorHandlersCheckpoint(progress); + await this._page.performActionPreChecks(progress); }, timeout); // Step 2: perform one-shot expect check without a timeout. @@ -1448,7 +1448,7 @@ export class Frame extends SdkObject { // Step 3: auto-retry expect with increasing timeouts. Bounded by the total remaining time. return await (new ProgressController(metadata, this)).run(async progress => { return await this.retryWithProgressAndTimeouts(progress, [100, 250, 500, 1000], async continuePolling => { - await this._page.performLocatorHandlersCheckpoint(progress); + await this._page.performActionPreChecks(progress); const { matches, received } = await this._expectInternal(progress, selector, options, lastIntermediateResult); if (matches === options.isNot) { // Keep waiting in these cases: diff --git a/packages/playwright-core/src/server/page.ts b/packages/playwright-core/src/server/page.ts index 3fb72bb0c3..f7f1ef67e8 100644 --- a/packages/playwright-core/src/server/page.ts +++ b/packages/playwright-core/src/server/page.ts @@ -31,7 +31,7 @@ import * as accessibility from './accessibility'; import { FileChooser } from './fileChooser'; import type { Progress } from './progress'; import { ProgressController } from './progress'; -import { LongStandingScope, assert, createGuid } from '../utils'; +import { LongStandingScope, assert, createGuid, trimStringWithEllipsis } from '../utils'; import { ManualPromise } from '../utils/manualPromise'; import { debugLogger } from '../utils/debugLogger'; import type { ImageComparatorOptions } from '../utils/comparators'; @@ -45,6 +45,7 @@ import { parseEvaluationResultValue, source } from './isomorphic/utilityScriptSe import type { SerializedValue } from './isomorphic/utilityScriptSerializers'; import { TargetClosedError } from './errors'; import { asLocator } from '../utils'; +import { helper } from './helper'; export interface PageDelegate { readonly rawMouse: input.RawMouse; @@ -455,7 +456,34 @@ export class Page extends SdkObject { this._locatorHandlers.delete(uid); } - async performLocatorHandlersCheckpoint(progress: Progress) { + async performActionPreChecks(progress: Progress) { + await this._performWaitForNavigationCheck(progress); + progress.throwIfAborted(); + await this._performLocatorHandlersCheckpoint(progress); + progress.throwIfAborted(); + // Wait once again, just in case a locator handler caused a navigation. + await this._performWaitForNavigationCheck(progress); + } + + private async _performWaitForNavigationCheck(progress: Progress) { + if (process.env.PLAYWRIGHT_SKIP_NAVIGATION_CHECK) + return; + const mainFrame = this._frameManager.mainFrame(); + if (!mainFrame || !mainFrame.pendingDocument()) + return; + const url = mainFrame.pendingDocument()?.request?.url(); + const toUrl = url ? `" ${trimStringWithEllipsis(url, 200)}"` : ''; + progress.log(` waiting for${toUrl} navigation to finish...`); + await helper.waitForEvent(progress, mainFrame, frames.Frame.Events.InternalNavigation, (e: frames.NavigationEvent) => { + if (!e.isPublic) + return false; + if (!e.error) + progress.log(` navigated to "${trimStringWithEllipsis(mainFrame.url(), 200)}"`); + return true; + }).promise; + } + + private async _performLocatorHandlersCheckpoint(progress: Progress) { // Do not run locator handlers from inside locator handler callbacks to avoid deadlocks. if (this._locatorHandlerRunningCounter) return; @@ -559,7 +587,7 @@ export class Page extends SdkObject { const rafrafScreenshot = locator ? async (progress: Progress, timeout: number) => { return await locator.frame.rafrafTimeoutScreenshotElementWithProgress(progress, locator.selector, timeout, options || {}); } : async (progress: Progress, timeout: number) => { - await this.performLocatorHandlersCheckpoint(progress); + await this.performActionPreChecks(progress); await this.mainFrame().rafrafTimeout(timeout); return await this._screenshotter.screenshotPage(progress, options || {}); }; diff --git a/tests/library/browsercontext-page-event.spec.ts b/tests/library/browsercontext-page-event.spec.ts index b851dd0ce9..30efb63fcf 100644 --- a/tests/library/browsercontext-page-event.spec.ts +++ b/tests/library/browsercontext-page-event.spec.ts @@ -184,19 +184,3 @@ it('should work with Ctrl-clicking', async ({ browser, server, browserName }) => expect(await popup.opener()).toBe(null); await context.close(); }); - -it('should not hang on ctrl-click during provisional load', async ({ context, page, server, isWindows, browserName, isLinux }) => { - it.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/11595' }); - it.skip(browserName === 'chromium', 'Chromium does not dispatch renderer messages while navigation is provisional.'); - it.fixme(browserName === 'webkit' && isWindows, 'Timesout while trying to click'); - it.fixme(browserName === 'webkit' && isLinux, 'Timesout while trying to click'); - await page.goto(server.EMPTY_PAGE); - await page.setContent('yo'); - server.setRoute('/slow.html', () => {}); - const [popup] = await Promise.all([ - context.waitForEvent('page'), - server.waitForRequest('/slow.html').then(() => page.click('a', { modifiers: ['ControlOrMeta'] })), - page.evaluate(url => setTimeout(() => location.href = url, 0), server.CROSS_PROCESS_PREFIX + '/slow.html'), - ]); - expect(popup).toBeTruthy(); -}); diff --git a/tests/page/page-autowaiting-no-hang.spec.ts b/tests/page/page-autowaiting-no-hang.spec.ts index f580b8629a..6fc7a088af 100644 --- a/tests/page/page-autowaiting-no-hang.spec.ts +++ b/tests/page/page-autowaiting-no-hang.spec.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { test as it } from './pageTest'; +import { test as it, expect } from './pageTest'; it('clicking on links which do not commit navigation', async ({ page, server, httpsServer }) => { await page.goto(server.EMPTY_PAGE); @@ -65,3 +65,78 @@ it('opening a popup', async function({ page, server }) { page.evaluate(() => window.open(window.location.href) && 1), ]); }); + +it('clicking in the middle of navigation that aborts', async ({ page, server }) => { + let abortCallback; + const abortPromise = new Promise(f => abortCallback = f); + + let stallCallback; + const stallPromise = new Promise(f => stallCallback = f); + + server.setRoute('/stall.html', async (req, res) => { + stallCallback(); + await abortPromise; + req.socket.destroy(); + }); + + await page.goto(server.PREFIX + '/one-style.html'); + page.goto(server.PREFIX + '/stall.html').catch(() => {}); + await stallPromise; + + const clickPromise = page.click('body'); + await page.waitForTimeout(1000); + abortCallback(); + + await clickPromise; +}); + +it('clicking in the middle of navigation that commits', async ({ page, server }) => { + let commitCallback; + const abortPromise = new Promise(f => commitCallback = f); + + let stallCallback; + const stallPromise = new Promise(f => stallCallback = f); + + server.setRoute('/stall.html', async (req, res) => { + stallCallback(); + await abortPromise; + res.writeHead(200, { 'Content-Type': 'text/html' }); + res.end('hello world'); + }); + + await page.goto(server.PREFIX + '/one-style.html'); + page.goto(server.PREFIX + '/stall.html').catch(() => {}); + await stallPromise; + + const clickPromise = page.click('body'); + await page.waitForTimeout(1000); + commitCallback(); + + await clickPromise; + await expect(page.locator('body')).toContainText('hello world'); +}); + +it('goBack in the middle of navigation that commits', async ({ page, server }) => { + let commitCallback; + const abortPromise = new Promise(f => commitCallback = f); + + let stallCallback; + const stallPromise = new Promise(f => stallCallback = f); + + server.setRoute('/stall.html', async (req, res) => { + stallCallback(); + await abortPromise; + res.writeHead(200, { 'Content-Type': 'text/html' }); + res.end('hello world'); + }); + + await page.goto(server.PREFIX + '/one-style.html'); + page.goto(server.PREFIX + '/stall.html').catch(() => {}); + await stallPromise; + + const goBackPromise = page.goBack().catch(() => {}); + await page.waitForTimeout(1000); + commitCallback(); + + await goBackPromise; +});