fix(wait): avoid 180s mutex stall when a dialog opens during an action (#2427)
Fixes #2426 ## Problem When a tool that passes no `handleDialog` (`click`, `hover`, `fill`, `type_text`, `drag`, `new_page`) runs an action that opens a JS dialog, `McpPage`'s persistent listener stores the dialog but never dismisses it, so the renderer stays paused. `WaitForHelper` then ran `waitForStableDom`, whose setup `evaluateHandle` (`WaitForHelper.ts:43`) hung until Puppeteer's default `protocolTimeout` (**180s**, none is configured). That await happens while `ToolHandler` holds the single, timeout-less tool mutex, so every queued call — including `handle_dialog`, the only remedy — was blocked ~180s, freezing the session. Verified empirically: with a dialog left open, `page.evaluateHandle()` hangs until `protocolTimeout` then throws `ProtocolError`. ## Fix - **(A) Correctness:** always register the dialog listener (not only when `handleDialog` is set) and record any observed dialog in a separate `#dialogDetected` flag, then skip `waitForStableDom` when it is set. `#dialogDetected` does **not** affect `dialogHandled`, so callers that `clearDialog()` on `dialogHandled` (`navigate_page`, the worker/page paths of `evaluate_script`) are unaffected and never clear a genuinely-open dialog. The action returns promptly and the next dialog-blocked tool fails fast with "A dialog is open", prompting `handle_dialog`. - **(B) Defense-in-depth:** bound the `waitForStableDom` setup evaluation with the abort signal and the stable-DOM timeout, so any renderer pause can no longer exceed the intended cap. ## Testing - New `tests/WaitForHelper.test.ts` with a hanging `evaluateHandle` stub (fast + deterministic, no 180s wait): - `does not hang when the DOM-stability setup never resolves` — covers (B). - `skips the DOM-stability wait when a dialog opens (no handleDialog)` — covers (A); also asserts `dialogHandled` stays `false`. - Both fail on `main` (hang until the test's bound) and pass with this change. - Re-ran the suites that exercise `waitForEventsAfterAction` (`input`, `pages`, `script`, `snapshot`) — all pass. - ESLint + Prettier clean.
This commit is contained in:
bassem chagra
+67
-51
@@ -18,7 +18,9 @@ export class WaitForHelper {
|
||||
#expectNavigationIn: number;
|
||||
#navigationTimeout: number;
|
||||
|
||||
#dialogOpened = false;
|
||||
#dialogHandled = false;
|
||||
/** Track all dialogs as they pause the renderer. */
|
||||
#dialogDetected = false;
|
||||
#initialUrl: string;
|
||||
|
||||
constructor(
|
||||
@@ -40,31 +42,41 @@ export class WaitForHelper {
|
||||
* for the DOM to be stable before returning.
|
||||
*/
|
||||
async waitForStableDom(): Promise<void> {
|
||||
const stableDomObserver = await this.#page.evaluateHandle(timeout => {
|
||||
let timeoutId: ReturnType<typeof setTimeout>;
|
||||
function callback() {
|
||||
clearTimeout(timeoutId);
|
||||
timeoutId = setTimeout(() => {
|
||||
domObserver.resolver.resolve();
|
||||
domObserver.observer.disconnect();
|
||||
}, timeout);
|
||||
}
|
||||
const domObserver = {
|
||||
resolver: Promise.withResolvers<void>(),
|
||||
observer: new MutationObserver(callback),
|
||||
};
|
||||
// It's possible that the DOM is not gonna change so we
|
||||
// need to start the timeout initially.
|
||||
callback();
|
||||
// Bound the setup evaluation against the stable-DOM timeout. Without this
|
||||
// cap a paused renderer (e.g. an open dialog) would make evaluateHandle
|
||||
// hang until protocolTimeout (default 180s) while the tool mutex is held.
|
||||
const stableDomObserver = await Promise.race([
|
||||
this.#page.evaluateHandle(timeout => {
|
||||
let timeoutId: ReturnType<typeof setTimeout>;
|
||||
function callback() {
|
||||
clearTimeout(timeoutId);
|
||||
timeoutId = setTimeout(() => {
|
||||
domObserver.resolver.resolve();
|
||||
domObserver.observer.disconnect();
|
||||
}, timeout);
|
||||
}
|
||||
const domObserver = {
|
||||
resolver: Promise.withResolvers<void>(),
|
||||
observer: new MutationObserver(callback),
|
||||
};
|
||||
// It's possible that the DOM is not gonna change so we
|
||||
// need to start the timeout initially.
|
||||
callback();
|
||||
|
||||
domObserver.observer.observe(document.body, {
|
||||
childList: true,
|
||||
subtree: true,
|
||||
attributes: true,
|
||||
});
|
||||
domObserver.observer.observe(document.body, {
|
||||
childList: true,
|
||||
subtree: true,
|
||||
attributes: true,
|
||||
});
|
||||
|
||||
return domObserver;
|
||||
}, this.#stableDomFor);
|
||||
return domObserver;
|
||||
}, this.#stableDomFor),
|
||||
this.timeout(this.#stableDomTimeout),
|
||||
]).catch(() => undefined);
|
||||
|
||||
if (!stableDomObserver) {
|
||||
return;
|
||||
}
|
||||
|
||||
this.#abortController.signal.addEventListener('abort', async () => {
|
||||
try {
|
||||
@@ -141,34 +153,38 @@ export class WaitForHelper {
|
||||
if (this.#abortController.signal.aborted) {
|
||||
throw new Error("Can't re-use a WaitForHelper");
|
||||
}
|
||||
if (options?.handleDialog) {
|
||||
const dialogHandler = (
|
||||
dialog: Pick<Dialog, 'accept' | 'dismiss' | 'type'>,
|
||||
) => {
|
||||
let actionToTake: DialogAction | undefined;
|
||||
const dialogHandler = (
|
||||
dialog: Pick<Dialog, 'accept' | 'dismiss' | 'type'>,
|
||||
) => {
|
||||
this.#dialogDetected = true;
|
||||
|
||||
if (typeof options.handleDialog === 'object') {
|
||||
actionToTake = options.handleDialog[dialog.type()];
|
||||
if (!options?.handleDialog) {
|
||||
return;
|
||||
}
|
||||
|
||||
let actionToTake: DialogAction | undefined;
|
||||
|
||||
if (typeof options.handleDialog === 'object') {
|
||||
actionToTake = options.handleDialog[dialog.type()];
|
||||
} else {
|
||||
actionToTake = options.handleDialog;
|
||||
}
|
||||
|
||||
if (actionToTake) {
|
||||
this.#dialogHandled = true;
|
||||
if (actionToTake === 'dismiss') {
|
||||
void dialog.dismiss();
|
||||
} else if (actionToTake === 'accept') {
|
||||
void dialog.accept();
|
||||
} else {
|
||||
actionToTake = options.handleDialog;
|
||||
void dialog.accept(actionToTake);
|
||||
}
|
||||
|
||||
if (actionToTake) {
|
||||
this.#dialogOpened = true;
|
||||
if (actionToTake === 'dismiss') {
|
||||
void dialog.dismiss();
|
||||
} else if (actionToTake === 'accept') {
|
||||
void dialog.accept();
|
||||
} else {
|
||||
void dialog.accept(actionToTake);
|
||||
}
|
||||
}
|
||||
};
|
||||
this.#page.on('dialog', dialogHandler);
|
||||
this.#abortController.signal.addEventListener('abort', () => {
|
||||
this.#page.off('dialog', dialogHandler);
|
||||
});
|
||||
}
|
||||
}
|
||||
};
|
||||
this.#page.on('dialog', dialogHandler);
|
||||
this.#abortController.signal.addEventListener('abort', () => {
|
||||
this.#page.off('dialog', dialogHandler);
|
||||
});
|
||||
|
||||
const navigationFinished = this.waitForNavigationStarted()
|
||||
.then(navigationStated => {
|
||||
@@ -193,7 +209,7 @@ export class WaitForHelper {
|
||||
try {
|
||||
await navigationFinished;
|
||||
|
||||
if (this.#dialogOpened) {
|
||||
if (this.#dialogDetected) {
|
||||
return this.#getResult();
|
||||
}
|
||||
|
||||
@@ -215,7 +231,7 @@ export class WaitForHelper {
|
||||
...(urlAfterAction !== this.#initialUrl
|
||||
? {navigatedToUrl: urlAfterAction}
|
||||
: {}),
|
||||
dialogHandled: this.#dialogOpened,
|
||||
dialogHandled: this.#dialogHandled,
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
/**
|
||||
* @license
|
||||
* Copyright 2026 Google LLC
|
||||
* SPDX-License-Identifier: Apache-2.0
|
||||
*/
|
||||
|
||||
import assert from 'node:assert';
|
||||
import {describe, it} from 'node:test';
|
||||
|
||||
import {html, withMcpContext} from './utils.js';
|
||||
|
||||
describe('WaitForHelper', () => {
|
||||
it('does not stall when an action opens a dialog without handleDialog', async () => {
|
||||
await withMcpContext(async (response, context) => {
|
||||
const mcpPage = context.getSelectedMcpPage();
|
||||
await mcpPage.pptrPage.setContent(html`<button id="b">go</button>`);
|
||||
|
||||
// The action opens a dialog asynchronously and passes no handleDialog.
|
||||
// The dialog leaves the renderer paused; without the fix,
|
||||
// waitForStableDom's setup evaluation would hang until protocolTimeout
|
||||
// (~180s) while the tool mutex is held, freezing the session.
|
||||
const result = await Promise.race([
|
||||
mcpPage.waitForEventsAfterAction(async () => {
|
||||
await mcpPage.pptrPage.evaluate(() => {
|
||||
setTimeout(() => confirm('blocked?'), 0);
|
||||
});
|
||||
}),
|
||||
// Comfortably above WaitForHelper.#stableDomTimeout (3s): the call
|
||||
// should return well within this once the dialog is detected.
|
||||
new Promise<'stalled'>(resolve =>
|
||||
setTimeout(() => resolve('stalled'), 5_000),
|
||||
),
|
||||
]);
|
||||
|
||||
assert(
|
||||
result !== 'stalled',
|
||||
'stalled because a dialog was shown; would time out with ProtocolError',
|
||||
);
|
||||
// The dialog was detected but not handled (no handleDialog was passed).
|
||||
assert.strictEqual(result.dialogHandled, false);
|
||||
// The dialog is still open and recorded, so the next blockedByDialog tool
|
||||
// correctly refuses to run.
|
||||
assert.throws(() => mcpPage.throwIfDialogOpen());
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user