From 23c78e74dfd6ec33303ba362d6e5f110fbecc604 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 14 Jul 2026 18:14:14 +0000 Subject: [PATCH] fix(browser): use debugCatchError in stealth acquire paths The stealth puppeteer-core patch re-implements world acquisition without Runtime.enable and used the bare debugError logger in its new FrameManager/WebWorker catch handlers. Puppeteer leaves debugError undefined when the puppeteer:error debug channel is disabled (the default), so a transient CDP failure during world re-acquire threw TypeError: debugError is not a function, escaped as an unhandledRejection, and the postmortem handler killed the whole process along with every subagent. Replace every bare debugError catch handler added by the patch with the safe debugCatchError (already imported for upstream handlers) so a disabled logger can never throw a secondary error. Fixes #5296 --- packages/coding-agent/CHANGELOG.md | 4 + ...browser-stealth-acquire-debugerror.test.ts | 117 ++++++++++++++++++ patches/puppeteer-core@25.3.0.patch | 16 +-- 3 files changed, 129 insertions(+), 8 deletions(-) create mode 100644 packages/coding-agent/test/tools/browser-stealth-acquire-debugerror.test.ts diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 10f7883ed..4a621165f 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed the browser tool crashing the whole process (parent session and every subagent) when a CDP world re-acquire failed mid-navigation: the stealth `puppeteer-core` patch called the bare `debugError` logger, which is `undefined` while the `puppeteer:error` debug channel is disabled (the default), turning a transient acquire failure into a fatal `TypeError` unhandled rejection. The patched `FrameManager`/`WebWorker` acquire paths now use `debugCatchError` ([#5296](https://github.com/can1357/oh-my-pi/issues/5296)) + ## [16.4.8] - 2026-07-12 ### Fixed diff --git a/packages/coding-agent/test/tools/browser-stealth-acquire-debugerror.test.ts b/packages/coding-agent/test/tools/browser-stealth-acquire-debugerror.test.ts new file mode 100644 index 000000000..dec0af52e --- /dev/null +++ b/packages/coding-agent/test/tools/browser-stealth-acquire-debugerror.test.ts @@ -0,0 +1,117 @@ +/** + * Regression test for issue #5296: the stealth `puppeteer-core` patch + * (`patches/puppeteer-core@25.3.0.patch`) re-implements world acquisition + * without `Runtime.enable`. Its new catch handlers in `FrameManager` called the + * bare `debugError` logger, which puppeteer leaves `undefined` when the + * `puppeteer:error` debug channel is disabled (the default). A transient CDP + * failure during world re-acquire then threw `TypeError: debugError is not a + * function` from `#doAcquireWorlds`, escaped as an `unhandledRejection`, and the + * postmortem handler killed the whole OMP process (parent session + every + * subagent). + * + * The test drives the real patched `FrameManager` with a `send()` that always + * rejects (a mid-flight CDP failure) and asserts the acquire path emits no + * unhandled `TypeError`. + * + * Real timers are deliberate here (see repo rule ts-no-test-timers): the fatal + * path is the coalescing acquirer's fire-and-forget `void this.#acquireWorlds()` + * retrigger, whose rejection escapes only to the global `unhandledRejection` + * handler — there is no promise or event the test can await, and fake timers + * serialise the two concurrent acquires so the retrigger (and thus the bug) + * never fires. Short real delays let the event loop interleave the acquires the + * way it does in production. + */ + +import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { CdpFrame } from "puppeteer-core/lib/puppeteer/cdp/Frame.js"; +import { FrameManager } from "puppeteer-core/lib/puppeteer/cdp/FrameManager.js"; +import { MAIN_WORLD, PUPPETEER_WORLD } from "puppeteer-core/lib/puppeteer/cdp/IsolatedWorlds.js"; +import { EventEmitter } from "puppeteer-core/lib/puppeteer/common/EventEmitter.js"; +import { TimeoutSettings } from "puppeteer-core/lib/puppeteer/common/TimeoutSettings.js"; +import { debugError } from "puppeteer-core/lib/puppeteer/common/util.js"; + +const ACQUIRE_TIMEOUT_MS = 40; + +// A CDP session double whose every `send` rejects, modelling a navigation that +// tears the target's execution contexts down mid-acquire. +class RejectingSession extends EventEmitter> { + constructor(readonly sessionId: string) { + super(); + } + id(): string { + return this.sessionId; + } + send(): Promise { + return Promise.reject(new Error("mid-flight CDP failure")); + } + target(): unknown { + return { _targetId: "T", type: () => "page" }; + } +} + +function makeFrameManager(session: RejectingSession): FrameManager { + const browser = { isNetworkEnabled: () => false, isIssuesEnabled: () => false, connected: true }; + const page = { browser: () => browser, isClosed: () => false, emit() {}, once() {}, off() {} }; + const timeoutSettings = new TimeoutSettings(); + timeoutSettings.setDefaultTimeout(ACQUIRE_TIMEOUT_MS); + // The patched FrameManager only touches the members exercised here; the + // puppeteer-internal `CdpCDPSession` / `CdpPage` types are far wider than the + // acquire path needs, so the doubles cross the boundary with a cast. + return new FrameManager(session as never, page as never, timeoutSettings); +} + +describe("stealth FrameManager world acquire — issue #5296", () => { + const rejections: unknown[] = []; + const onUnhandled = (reason: unknown) => rejections.push(reason); + + beforeEach(() => { + rejections.length = 0; + process.on("unhandledRejection", onUnhandled); + }); + + afterEach(() => { + process.off("unhandledRejection", onUnhandled); + }); + + it("keeps disabled debugError undefined so bare calls would crash", () => { + // The precondition that makes the bug fatal: with the puppeteer:error + // channel off, the logger the patch used is not callable. + expect(debugError).toBeUndefined(); + }); + + it("does not emit an unhandled TypeError when acquire fails mid-flight", async () => { + const session = new RejectingSession("S1"); + const frameManager = makeFrameManager(session); + const frame = new CdpFrame(frameManager, "F1", undefined, session as never); + frameManager._frameTree.addFrame(frame); + + // Navigation installs the lazy context providers and invalidates the old + // contexts; the async handler must settle before we pull a context. + session.emit("Page.frameNavigated", { + frame: { id: "F1", parentId: undefined, url: "about:blank" }, + type: "Navigation", + }); + await Bun.sleep(20); + + // Concurrent pulls on both worlds force the coalescing acquirer to + // re-run (`void this.#acquireWorlds` in its `finally`), which is the exact + // path where `#doAcquireWorlds`'s catch previously threw a bare + // `debugError(error)`. + const main = frame.worlds[MAIN_WORLD]; + const util = frame.worlds[PUPPETEER_WORLD]; + const results = await Promise.allSettled([main.evaluate(() => 1), util.evaluate(() => 1)]); + + // Let the re-triggered acquire settle and any stray rejection surface. + await Bun.sleep(ACQUIRE_TIMEOUT_MS + 40); + + const typeErrors = rejections.filter( + (reason): reason is TypeError => reason instanceof Error && reason.name === "TypeError", + ); + expect(typeErrors).toHaveLength(0); + expect(rejections).toHaveLength(0); + + // The failure is still observable as an ordinary, recoverable evaluate + // error rather than a silent process death. + expect(results.every(r => r.status === "rejected")).toBe(true); + }); +}); diff --git a/patches/puppeteer-core@25.3.0.patch b/patches/puppeteer-core@25.3.0.patch index bc93ba914..0bf86a5b2 100644 --- a/patches/puppeteer-core@25.3.0.patch +++ b/patches/puppeteer-core@25.3.0.patch @@ -352,7 +352,7 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + client.send('Page.addScriptToEvaluateOnNewDocument', { + source: `//# sourceURL=${PuppeteerURL.INTERNAL_URL}`, + worldName: UTILITY_WORLD_NAME, -+ }).catch(debugError), + }).catch(debugCatchError), ...(frame ? Array.from(this.#scriptsToEvaluateOnNewDocument.values()) : []).map(script => { @@ -445,7 +445,7 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + worldName: UTILITY_WORLD_NAME, + grantUniveralAccess: true, + }) -+ .catch(debugError); + .catch(debugCatchError); + const utilityId = iso && typeof iso.executionContextId === 'number' ? iso.executionContextId : undefined; + if (utilityId !== undefined) { + this.#onExecutionContextCreated({ @@ -486,7 +486,7 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + } + } + catch (error) { -+ debugError(error); + debugCatchError(error); + } + } + // xxx-stealth: resolve a frame's MAIN-world execution context id without @@ -511,7 +511,7 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + expression: 'globalThis', + serializationOptions: { serialization: 'idOnly' }, + }) -+ .catch(debugError); + .catch(debugCatchError); + return parse(globalThis?.result?.objectId); + } + if (utilityId === undefined) { @@ -523,21 +523,21 @@ index 2322aa136a47b446e2a7b2c4f0bc751f2fb821d0..2116367ee34bb86948bf956c2afa2f69 + contextId: utilityId, + serializationOptions: { serialization: 'idOnly' }, + }) -+ .catch(debugError); + .catch(debugCatchError); + const utilDocObjectId = utilDoc?.result?.objectId; + if (typeof utilDocObjectId !== 'string') { + return undefined; + } + const described = await session + .send('DOM.describeNode', { objectId: utilDocObjectId }) -+ .catch(debugError); + .catch(debugCatchError); + const backendNodeId = described?.node?.backendNodeId; + if (typeof backendNodeId !== 'number') { + return undefined; + } + const mainNode = await session + .send('DOM.resolveNode', { backendNodeId }) -+ .catch(debugError); + .catch(debugCatchError); + return parse(mainNode?.object?.objectId); } async #createIsolatedWorld(session, name) { @@ -605,7 +605,7 @@ index 3d68f887920ded269eb641273a5a13dee235ae1d..dcdd86c8697c0dbd2dd2162c9a739dd9 + this.#world.setContext(new ExecutionContext(client, { id }, this.#world)); + } + }) -+ .catch(debugError); + .catch(debugCatchError); this.#client.once('Inspector.workerScriptLoaded', () => { this.#workerLoaded.resolve(); });