diff --git a/packages/playwright-core/src/server/agent/actionRunner.ts b/packages/playwright-core/src/server/agent/actionRunner.ts index 1244fae2d7a54..fe889d9555004 100644 --- a/packages/playwright-core/src/server/agent/actionRunner.ts +++ b/packages/playwright-core/src/server/agent/actionRunner.ts @@ -18,7 +18,6 @@ import { serializeExpectedTextValues } from '../utils/expectUtils'; import { monotonicTime } from '../../utils/isomorphic/time'; import { createGuid } from '../utils/crypto'; import { parseAriaSnapshotUnsafe } from '../../utils/isomorphic/ariaSnapshot'; -import { renderTitleForCall } from '../../utils/isomorphic/protocolFormatter'; import { ProgressController } from '../progress'; import { yaml } from '../../utilsBundle'; import { serializeError } from '../errors'; @@ -43,7 +42,7 @@ export async function runAction(parentProgress: Progress, mode: 'generate' | 'ru await frame.instrumentation.onBeforeCall(frame, callMetadata, parentProgress.metadata.id); let error: Error | undefined; - const result = await innerRunAction(progress, page, action, secrets).catch(e => error = e); + const result = await innerRunAction(progress, mode, page, action, secrets).catch(e => error = e); callMetadata.endTime = monotonicTime(); callMetadata.error = error ? serializeError(error) : undefined; callMetadata.result = error ? undefined : result; @@ -54,8 +53,9 @@ export async function runAction(parentProgress: Progress, mode: 'generate' | 'ru }, minDeadline - mt); } -async function innerRunAction(progress: Progress, page: Page, action: actions.Action, secrets: NameValue[]) { +async function innerRunAction(progress: Progress, mode: 'generate' | 'run', page: Page, action: actions.Action, secrets: NameValue[]) { const frame = page.mainFrame(); + const commonOptions = { strict: true, noAutoWaiting: mode === 'generate' }; switch (action.method) { case 'navigate': await frame.goto(progress, action.url); @@ -65,43 +65,43 @@ async function innerRunAction(progress: Progress, page: Page, action: actions.Ac button: action.button, clickCount: action.clickCount, modifiers: action.modifiers, - ...strictTrue + ...commonOptions }); break; case 'drag': - await frame.dragAndDrop(progress, action.sourceSelector, action.targetSelector, { ...strictTrue }); + await frame.dragAndDrop(progress, action.sourceSelector, action.targetSelector, { ...commonOptions }); break; case 'hover': await frame.hover(progress, action.selector, { modifiers: action.modifiers, - ...strictTrue + ...commonOptions }); break; case 'selectOption': - await frame.selectOption(progress, action.selector, [], action.labels.map(a => ({ label: a })), { ...strictTrue }); + await frame.selectOption(progress, action.selector, [], action.labels.map(a => ({ label: a })), { ...commonOptions }); break; case 'pressKey': await page.keyboard.press(progress, action.key); break; case 'pressSequentially': { const secret = secrets?.find(s => s.name === action.text)?.value ?? action.text; - await frame.type(progress, action.selector, secret, { ...strictTrue }); + await frame.type(progress, action.selector, secret, { ...commonOptions }); if (action.submit) await page.keyboard.press(progress, 'Enter'); break; } case 'fill': { const secret = secrets?.find(s => s.name === action.text)?.value ?? action.text; - await frame.fill(progress, action.selector, secret, { ...strictTrue }); + await frame.fill(progress, action.selector, secret, { ...commonOptions }); if (action.submit) await page.keyboard.press(progress, 'Enter'); break; } case 'setChecked': if (action.checked) - await frame.check(progress, action.selector, { ...strictTrue }); + await frame.check(progress, action.selector, { ...commonOptions }); else - await frame.uncheck(progress, action.selector, { ...strictTrue }); + await frame.uncheck(progress, action.selector, { ...commonOptions }); break; case 'expectVisible': { const result = await frame.expect(progress, action.selector, { expression: 'to.be.visible', isNot: !!action.isNot }); @@ -307,9 +307,6 @@ export function traceParamsForAction(action: actions.Action): { title?: string, } function callMetadataForAction(frame: Frame, action: actions.Action): CallMetadata { - const traceParams = traceParamsForAction(action); - const title = renderTitleForCall(traceParams); - const callMetadata: CallMetadata = { id: `call@${createGuid()}`, objectId: frame.guid, @@ -317,14 +314,10 @@ function callMetadataForAction(frame: Frame, action: actions.Action): CallMetada frameId: frame.guid, startTime: monotonicTime(), endTime: 0, - type: 'Frame', - method: traceParams.method, - params: traceParams.params, - title, log: [], + ...traceParamsForAction(action), }; return callMetadata; } const kDefaultTimeout = 5000; -const strictTrue = { strict: true }; diff --git a/packages/playwright-core/src/server/browserType.ts b/packages/playwright-core/src/server/browserType.ts index 8bb1fb4bb23a2..3182f671b9c5e 100644 --- a/packages/playwright-core/src/server/browserType.ts +++ b/packages/playwright-core/src/server/browserType.ts @@ -264,6 +264,8 @@ export abstract class BrowserType extends SdkObject { this.waitForReadyState(options, browserLogsCollector), exitPromise.then(() => ({ wsEndpoint: undefined })), ]); + if (exitPromise.isDone()) + throw new Error(`Failed to launch the browser process.`); if (options.cdpPort !== undefined || !this.supportsPipeTransport()) { transport = await WebSocketTransport.connect(progress, wsEndpoint!); } else { diff --git a/packages/playwright-core/src/server/dom.ts b/packages/playwright-core/src/server/dom.ts index d5a06c5c9a08e..de84b4165272e 100644 --- a/packages/playwright-core/src/server/dom.ts +++ b/packages/playwright-core/src/server/dom.ts @@ -294,10 +294,11 @@ export class ElementHandle extends js.JSHandle { }; } - async _retryAction(progress: Progress, actionName: string, action: (retry: number) => Promise, options: { trial?: boolean, force?: boolean, skipActionPreChecks?: boolean }): Promise<'error:notconnected' | 'done'> { + async _retryAction(progress: Progress, actionName: string, action: (retry: number) => Promise, options: { trial?: boolean, force?: boolean, skipActionPreChecks?: boolean, noAutoWaiting?: boolean }): Promise<'error:notconnected' | 'done'> { let retry = 0; // We progressively wait longer between retries, up to 500ms. const waitTime = [0, 20, 100, 100, 500]; + const noAutoWaiting = (options as any).__testHookNoAutoWaiting ?? options.noAutoWaiting; while (true) { if (retry) { @@ -312,35 +313,43 @@ export class ElementHandle extends js.JSHandle { } else { progress.log(`attempting ${actionName} action${options.trial ? ' (trial run)' : ''}`); } - if (!options.skipActionPreChecks && !options.force) + if (!options.skipActionPreChecks && !options.force && !noAutoWaiting) await this._frame._page.performActionPreChecks(progress); const result = await action(retry); ++retry; if (result === 'error:notvisible') { - if (options.force) + if (options.force || noAutoWaiting) throw new NonRecoverableDOMError('Element is not visible'); progress.log(' element is not visible'); continue; } if (result === 'error:notinviewport') { - if (options.force) + if (options.force || noAutoWaiting) throw new NonRecoverableDOMError('Element is outside of the viewport'); progress.log(' element is outside of the viewport'); continue; } if (result === 'error:optionsnotfound') { + if (noAutoWaiting) + throw new NonRecoverableDOMError('Did not find some options'); progress.log(' did not find some options'); continue; } if (result === 'error:optionnotenabled') { + if (noAutoWaiting) + throw new NonRecoverableDOMError('Option being selected is not enabled'); progress.log(' option being selected is not enabled'); continue; } if (typeof result === 'object' && 'hitTargetDescription' in result) { + if (noAutoWaiting) + throw new NonRecoverableDOMError(`${result.hitTargetDescription} intercepts pointer events`); progress.log(` ${result.hitTargetDescription} intercepts pointer events`); continue; } if (typeof result === 'object' && 'missingState' in result) { + if (noAutoWaiting) + throw new NonRecoverableDOMError(`Element is not ${result.missingState}`); progress.log(` element is not ${result.missingState}`); continue; } diff --git a/packages/playwright-core/src/server/frames.ts b/packages/playwright-core/src/server/frames.ts index d6c2285c4100a..619d4973f2460 100644 --- a/packages/playwright-core/src/server/frames.ts +++ b/packages/playwright-core/src/server/frames.ts @@ -1100,17 +1100,21 @@ export class Frame extends SdkObject { private async _retryWithProgressIfNotConnected( progress: Progress, selector: string, - strict: boolean | undefined, - performActionPreChecks: boolean, + options: { strict?: boolean, noAutoWaiting?: boolean, force?: boolean, performActionPreChecks?: boolean }, action: (handle: dom.ElementHandle) => Promise): Promise { progress.log(`waiting for ${this._asLocator(selector)}`); + const noAutoWaiting = (options as any).__testHookNoAutoWaiting ?? options.noAutoWaiting; + const performActionPreChecks = (options.performActionPreChecks ?? !options.force) && !noAutoWaiting; return this.retryWithProgressAndTimeouts(progress, [0, 20, 50, 100, 100, 500], async continuePolling => { if (performActionPreChecks) await this._page.performActionPreChecks(progress); - const resolved = await progress.race(this.selectors.resolveInjectedForSelector(selector, { strict })); - if (!resolved) + const resolved = await progress.race(this.selectors.resolveInjectedForSelector(selector, { strict: options.strict })); + if (!resolved) { + if (noAutoWaiting) + throw new dom.NonRecoverableDOMError('Element(s) not found'); return continuePolling; + } const result = await progress.race(resolved.injected.evaluateHandle((injected, { info, callId }) => { const elements = injected.querySelectorAll(info.parsed, document); if (callId) @@ -1131,6 +1135,8 @@ export class Frame extends SdkObject { if (log) progress.log(log); if (!success) { + if (noAutoWaiting) + throw new dom.NonRecoverableDOMError('Element(s) not found'); result.dispose(); return continuePolling; } @@ -1139,6 +1145,8 @@ export class Frame extends SdkObject { try { const result = await action(element); if (result === 'error:notconnected') { + if (noAutoWaiting) + throw new dom.NonRecoverableDOMError('Element is not attached to the DOM'); progress.log('element was detached from the DOM, retrying'); return continuePolling; } @@ -1150,22 +1158,22 @@ 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 /* performActionPreChecks */, async handle => { + return await this._retryWithProgressIfNotConnected(progress, selector, { strict: true, performActionPreChecks: true }, async handle => { await handle._frame.rafrafTimeout(progress, timeout); return await this._page.screenshotter.screenshotElement(progress, handle, options); }); } async click(progress: Progress, selector: string, options: { noWaitAfter?: boolean } & types.MouseClickOptions & types.PointerActionWaitOptions) { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._click(progress, { ...options, waitAfter: !options.noWaitAfter }))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._click(progress, { ...options, waitAfter: !options.noWaitAfter }))); } async dblclick(progress: Progress, selector: string, options: types.MouseMultiClickOptions & types.PointerActionWaitOptions) { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._dblclick(progress, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._dblclick(progress, options))); } async dragAndDrop(progress: Progress, source: string, target: string, options: types.DragActionOptions & types.PointerActionWaitOptions) { - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, source, options.strict, !options.force /* performActionPreChecks */, async handle => { + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, source, options, async handle => { return handle._retryPointerAction(progress, 'move and down', false, async point => { await this._page.mouse.move(progress, point.x, point.y); await this._page.mouse.down(progress); @@ -1176,7 +1184,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 /* performActionPreChecks */, async handle => { + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, target, { ...options, performActionPreChecks: false }, async handle => { return handle._retryPointerAction(progress, 'move and up', false, async point => { await this._page.mouse.move(progress, point.x, point.y, { steps: options.steps }); await this._page.mouse.up(progress); @@ -1191,19 +1199,19 @@ export class Frame extends SdkObject { async tap(progress: Progress, selector: string, options: types.PointerActionWaitOptions) { if (!this._page.browserContext._options.hasTouch) throw new Error('The page does not support tap. Use hasTouch context option to enable touch support.'); - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._tap(progress, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._tap(progress, options))); } - async fill(progress: Progress, selector: string, value: string, options: types.StrictOptions & { force?: boolean }) { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._fill(progress, value, options))); + async fill(progress: Progress, selector: string, value: string, options: types.CommonActionOptions) { + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._fill(progress, value, options))); } - async focus(progress: Progress, selector: string, options: types.StrictOptions) { - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._focus(progress))); + async focus(progress: Progress, selector: string, options: types.StrictOptions & { noAutoWaiting?: boolean }) { + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._focus(progress))); } - async blur(progress: Progress, selector: string, options: types.StrictOptions) { - dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._blur(progress))); + async blur(progress: Progress, selector: string, options: types.StrictOptions & { noAutoWaiting?: boolean }) { + dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._blur(progress))); } async resolveSelector(progress: Progress, selector: string, options: { mainWorld?: boolean } = {}): Promise<{ resolvedSelector: string }> { @@ -1336,32 +1344,32 @@ export class Frame extends SdkObject { } async hover(progress: Progress, selector: string, options: types.PointerActionOptions & types.PointerActionWaitOptions) { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._hover(progress, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._hover(progress, options))); } async selectOption(progress: Progress, selector: string, elements: dom.ElementHandle[], values: types.SelectOption[], options: types.CommonActionOptions): Promise { - return await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._selectOption(progress, elements, values, options)); + return await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._selectOption(progress, elements, values, options)); } - async setInputFiles(progress: Progress, selector: string, params: Omit): Promise { + async setInputFiles(progress: Progress, selector: string, params: Omit & { noAutoWaiting?: boolean }): Promise { const inputFileItems = await prepareFilesForUpload(this, params); - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, params.strict, true /* performActionPreChecks */, handle => handle._setInputFiles(progress, inputFileItems))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, params, handle => handle._setInputFiles(progress, inputFileItems))); } - async type(progress: Progress, selector: string, text: string, options: { delay?: number } & types.StrictOptions) { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._type(progress, text, options))); + async type(progress: Progress, selector: string, text: string, options: { delay?: number, noAutoWaiting?: boolean } & types.StrictOptions) { + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._type(progress, text, options))); } - async press(progress: Progress, selector: string, key: string, options: { delay?: number, noWaitAfter?: boolean } & types.StrictOptions) { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, true /* performActionPreChecks */, handle => handle._press(progress, key, options))); + async press(progress: Progress, selector: string, key: string, options: { delay?: number, noWaitAfter?: boolean, noAutoWaiting?: boolean } & types.StrictOptions) { + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._press(progress, key, options))); } async check(progress: Progress, selector: string, options: types.PointerActionWaitOptions) { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._setChecked(progress, true, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._setChecked(progress, true, options))); } async uncheck(progress: Progress, selector: string, options: types.PointerActionWaitOptions) { - return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options.strict, !options.force /* performActionPreChecks */, handle => handle._setChecked(progress, false, options))); + return dom.assertDone(await this._retryWithProgressIfNotConnected(progress, selector, options, handle => handle._setChecked(progress, false, options))); } async waitForTimeout(progress: Progress, timeout: number) { @@ -1369,7 +1377,7 @@ export class Frame extends SdkObject { } async ariaSnapshot(progress: Progress, selector: string): Promise { - return await this._retryWithProgressIfNotConnected(progress, selector, true /* strict */, true /* performActionPreChecks */, handle => progress.race(handle.ariaSnapshot())); + return await this._retryWithProgressIfNotConnected(progress, selector, { strict: true, performActionPreChecks: true }, handle => progress.race(handle.ariaSnapshot())); } async expect(progress: Progress, selector: string | undefined, options: FrameExpectParams, timeout?: number): Promise { diff --git a/packages/playwright-core/src/server/types.ts b/packages/playwright-core/src/server/types.ts index 9f5ef8c7ead16..a400152d7f8fb 100644 --- a/packages/playwright-core/src/server/types.ts +++ b/packages/playwright-core/src/server/types.ts @@ -36,6 +36,7 @@ export type NavigateOptions = { export type CommonActionOptions = StrictOptions & { force?: boolean, + noAutoWaiting?: boolean, }; export type PointerActionWaitOptions = CommonActionOptions & { diff --git a/packages/playwright/src/mcp/browser/browserContextFactory.ts b/packages/playwright/src/mcp/browser/browserContextFactory.ts index b064f49785e8a..0158b4e8414d3 100644 --- a/packages/playwright/src/mcp/browser/browserContextFactory.ts +++ b/packages/playwright/src/mcp/browser/browserContextFactory.ts @@ -224,7 +224,9 @@ class PersistentContextFactory implements BrowserContextFactory { } catch (error: any) { if (error.message.includes('Executable doesn\'t exist')) throw new Error(`Browser specified in your config is not installed. Either install it (likely) or change the config.`); - if (error.message.includes('ProcessSingleton') || error.message.includes('Invalid URL')) { + if (error.message.includes('ProcessSingleton') || + // On Windows the process exits silently with code 21 when the profile is in use. + error.message.includes('exitCode=21')) { // User data directory is already in use, try again. await new Promise(resolve => setTimeout(resolve, 1000)); continue; diff --git a/tests/mcp/launch.spec.ts b/tests/mcp/launch.spec.ts index f37103535159d..c008c72e86a92 100644 --- a/tests/mcp/launch.spec.ts +++ b/tests/mcp/launch.spec.ts @@ -175,3 +175,37 @@ test('isolated context with storage state', async ({ startClient, server }, test pageState: expect.stringContaining(`Storage: session-value`), }); }); + +test('proper launch error message for broken browser and persistent context', { + annotation: { type: 'issue', description: 'https://github.com/microsoft/playwright-mcp/issues/1305' } +}, async ({ startClient, server, mcpBrowser }, testInfo) => { + test.skip(process.platform === 'win32', 'Skipping on Windows because we need /bin/sh.'); + const scriptPath = testInfo.outputPath('launcher.sh'); + const scriptContent = `#!/bin/sh +echo "Bogus browser script" +exit 1 +`; + await fs.promises.writeFile(scriptPath, scriptContent, { mode: 0o755 }); + + const { client } = await startClient({ + args: [`--executable-path=${scriptPath}`], + }); + const result = await client.callTool({ + name: 'browser_navigate', + arguments: { url: server.PREFIX }, + }); + expect.soft(result).toHaveResponse({ + isError: true, + result: expect.stringContaining(`Bogus browser script`), + }); + // Chromium waits for the CDP endpoint, so we know if the process failed to launch + // before connecting. + if (mcpBrowser === 'chromium') { + expect.soft(result).toHaveResponse({ + result: expect.stringContaining(`Failed to launch the browser process.`), + }); + } + expect.soft(result).toHaveResponse({ + result: expect.not.stringContaining(`Browser is already in use`), + }); +}); diff --git a/tests/page/page-click.spec.ts b/tests/page/page-click.spec.ts index 05b1c3068b947..5221be5b95a91 100644 --- a/tests/page/page-click.spec.ts +++ b/tests/page/page-click.spec.ts @@ -1321,3 +1321,30 @@ it('should click with tweened mouse movement', async ({ page, browserName, isAnd [200, 300] ]); }); + +it('should not wait with noAutoWaiting', async ({ page }) => { + await page.setContent(``); + const error = await page.locator('#target').click({ __testHookNoAutoWaiting: true } as any).catch(e => e); + expect(error.message).toContain('locator.click: Element(s) not found'); +}); + +it('should not wait with noAutoWaiting 2', async ({ page }) => { + await page.setContent(` + +
+ +
+ `); + const error = await page.locator('button').click({ __testHookNoAutoWaiting: true } as any).catch(e => e); + expect(error.message).toContain('locator.click:
…
intercepts pointer events'); +}); + +it('should not wait with noAutoWaiting 3', async ({ page }) => { + await page.setContent(``); + const error = await page.locator('button').click({ __testHookNoAutoWaiting: true } as any).catch(e => e); + expect(error.message).toContain('locator.click: Element is not enabled'); +});