From 00c1f18927026fbfc1183a6586dc62b61a56c021 Mon Sep 17 00:00:00 2001 From: NianJiuZst <180004567+NianJiuZst@users.noreply.github.com> Date: Sun, 27 Sep 2026 02:01:08 +0800 Subject: [PATCH 1/3] fix(download): bound trigger lifetime and retire cancelled input --- .github/workflows/ci.yml | 6 + .../download-deadline.browser.test.ts | 175 ++++++++++ .../tools/__tests__/download-deadline.test.ts | 314 ++++++++++++++++++ .../tools/__tests__/download-trigger.test.ts | 70 ++++ apps/extension/src/tools/download-capture.ts | 69 +++- apps/extension/src/tools/download-trigger.ts | 88 +++++ apps/extension/src/tools/download.ts | 8 +- 7 files changed, 718 insertions(+), 12 deletions(-) create mode 100644 apps/extension/src/tools/__tests__/download-deadline.browser.test.ts create mode 100644 apps/extension/src/tools/__tests__/download-deadline.test.ts create mode 100644 apps/extension/src/tools/__tests__/download-trigger.test.ts create mode 100644 apps/extension/src/tools/download-trigger.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index af909851..3a62c1c2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -203,6 +203,12 @@ jobs: - name: Prepare extension types run: pnpm --filter @browser-skill/extension exec wxt prepare + - name: Run download cancellation browser regression + working-directory: apps/extension + env: + BSK_CLICK_CHROME: google-chrome + run: pnpm exec vitest run src/tools/__tests__/download-deadline.browser.test.ts + - name: Run browser input and debugging regressions working-directory: apps/extension env: diff --git a/apps/extension/src/tools/__tests__/download-deadline.browser.test.ts b/apps/extension/src/tools/__tests__/download-deadline.browser.test.ts new file mode 100644 index 00000000..dfa0af9a --- /dev/null +++ b/apps/extension/src/tools/__tests__/download-deadline.browser.test.ts @@ -0,0 +1,175 @@ +// @vitest-environment node +// Opt in with BSK_CLICK_CHROME; the harness owns an isolated browser/profile. +import { describe, expect, it } from "vitest"; +import { type CdpDebuggerApi, ChromiumCdp } from "@/browser-driver/chromium-cdp"; +import { SessionManager } from "@/session-manager/manager"; +import { handleDownload } from "../download"; +import type { CdpRunner } from "../shared"; + +type Send = >( + method: string, + params?: object, + sessionId?: string, +) => Promise; + +async function browser( + run: (send: Send, cdp: ChromiumCdp, sessionId: () => string) => Promise, +) { + const { withChrome } = await import( + new URL( + "../../../../../evals/browser/cases/regression/snapshot-coordinates/chrome.mjs", + import.meta.url, + ).href + ); + await withChrome( + { + executable: process.env.BSK_CLICK_CHROME, + deviceScale: 1, + zoom: 1, + startupTimeout: 30_000, + }, + async (send: Send) => { + const { targetId } = await send<{ targetId: string }>("Target.createTarget", { + url: "about:blank", + }); + let activeSession = ""; + const api: CdpDebuggerApi = { + attach: async () => { + const reply = await send<{ sessionId: string }>("Target.attachToTarget", { + targetId, + flatten: true, + }); + activeSession = reply.sessionId; + }, + detach: async () => { + await send("Target.detachFromTarget", { sessionId: activeSession }); + activeSession = ""; + }, + sendCommand: async (_target, method, params) => send(method, params, activeSession), + onEvent: { addListener() {}, removeListener() {} } as unknown as CdpDebuggerApi["onEvent"], + onDetach: { + addListener() {}, + removeListener() {}, + } as unknown as CdpDebuggerApi["onDetach"], + }; + const cdp = new ChromiumCdp(api); + try { + await run(send, cdp, () => activeSession); + } finally { + await cdp.detach(7); + } + }, + ); +} + +function manager() { + return new SessionManager({ + agentWindow: { + create: async () => ({ windowId: 100, initialTabIds: [7] }), + remove: async () => {}, + ensureActiveTab: async () => 7, + }, + }); +} +const tab = { id: 7, windowId: 100, active: true, url: "about:blank" } as chrome.tabs.Tab; +const tabsApi = { get: async () => tab, query: async () => [tab] }; + +describe.skipIf(!process.env.BSK_CLICK_CHROME)("download trigger retirement", () => { + it.each([ + "mouseMoved", + "mousePressed", + ])("never resumes native input after cancellation at %s", async (heldType) => { + await browser(async (_send, cdp) => { + const sessions = manager(); + const ctx = await sessions.start("download-retire"); + await cdp.send(7, "Runtime.evaluate", { + expression: ` + document.body.innerHTML = ''; + window.startedExports = 0; + document.querySelector('#export').onclick = () => { window.startedExports++; }; + `, + }); + const abort = new AbortController(); + let release!: (reply: unknown) => void; + let reached!: () => void; + const ready = new Promise((resolve) => { + reached = resolve; + }); + const reply = new Promise((resolve) => { + release = resolve; + }); + const inputs: string[] = []; + const wrapped: CdpRunner = { + send: (async (tabId: number, method: string, params?: object) => { + const value = await cdp.send(tabId, method, params); + if (method === "Input.dispatchMouseEvent") { + const type = (params as { type: string }).type; + inputs.push(type); + if (type === heldType) { + reached(); + await reply; + } + } + return value; + }) as CdpRunner["send"], + detach: cdp.detach.bind(cdp), + getAttachmentId: cdp.getAttachmentId.bind(cdp), + onEvent: () => ({ dispose() {} }), + }; + const event = () => ({ addListener() {}, removeListener() {} }); + const downloads = { + onCreated: event(), + onChanged: event(), + onDeterminingFilename: event(), + search: async () => [], + cancel: async () => {}, + removeFile: async () => {}, + }; + const pending = handleDownload( + sessions, + { + session_id: ctx.sessionId, + tab_id: 7, + selector: "#export", + browser_relative_dir: "BrowserSkill/retire", + timeout_ms: 2_000, + }, + { + cdp: wrapped, + tabsApi, + downloads, + signal: abort.signal, + navigationTargets: { onCreatedNavigationTarget: event() }, + }, + ); + try { + await ready; + abort.abort(); + const result = await pending; + expect(result).toHaveProperty("code"); + const count = async () => + ( + await cdp.send<{ result: { value: number } }>(7, "Runtime.evaluate", { + expression: "startedExports", + returnByValue: true, + }) + ).result.value; + const expected = heldType === "mousePressed" ? 1 : 0; + expect(await count()).toBe(expected); + const before = [...inputs]; + release({}); + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(await count()).toBe(expected); + expect(inputs).toEqual(before); + expect(inputs).toEqual( + heldType === "mousePressed" + ? ["mouseMoved", "mousePressed", "mouseReleased"] + : ["mouseMoved"], + ); + } finally { + release({}); + await pending; + } + }); + }, 40_000); +}); diff --git a/apps/extension/src/tools/__tests__/download-deadline.test.ts b/apps/extension/src/tools/__tests__/download-deadline.test.ts new file mode 100644 index 00000000..1aaf48e7 --- /dev/null +++ b/apps/extension/src/tools/__tests__/download-deadline.test.ts @@ -0,0 +1,314 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { SessionManager } from "@/session-manager/manager"; +import type { ClickResult } from "@/transport/types"; +import { handleDownload } from "../download"; +import { captureBrowserDownload, type DownloadsApi } from "../download-capture"; +import type { CdpRunner } from "../shared"; + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (error: Error) => void; + const promise = new Promise((yes, no) => { + resolve = yes; + reject = no; + }); + return { promise, resolve, reject }; +} + +function event unknown>() { + const listeners = new Set(); + return { + addListener: vi.fn((listener: T) => { + listeners.add(listener); + }), + removeListener: vi.fn((listener: T) => { + listeners.delete(listener); + }), + emit: (...args: Parameters) => { + for (const listener of listeners) listener(...args); + }, + }; +} + +function fixture() { + const onCreated = event<(item: chrome.downloads.DownloadItem) => void>(); + const onChanged = event<(delta: chrome.downloads.DownloadDelta) => void>(); + const onDeterminingFilename = + event< + ( + item: chrome.downloads.DownloadItem, + suggest: (suggestion?: chrome.downloads.DownloadFilenameSuggestion) => void, + ) => void | true + >(); + const downloads: DownloadsApi = { + onCreated, + onChanged, + onDeterminingFilename, + search: vi.fn(async () => []), + cancel: vi.fn(async () => {}), + removeFile: vi.fn(async () => {}), + }; + let listener!: Parameters>[0]; + const dispose = vi.fn(); + const cdp: CdpRunner = { + send: vi.fn(async () => ({})) as CdpRunner["send"], + onEvent: (callback) => { + listener = callback; + return { dispose }; + }, + }; + const item = { + id: 9, + url: "https://example.test/report", + finalUrl: "https://example.test/report", + filename: "report.csv", + state: "in_progress", + fileSize: -1, + totalBytes: 4, + } as chrome.downloads.DownloadItem; + const offer = () => { + listener({ tabId: 7 }, "Page.downloadWillBegin", { + url: item.url, + suggestedFilename: item.filename, + }); + onDeterminingFilename.emit(item, vi.fn()); + }; + return { downloads, cdp, dispose, item, offer, onCreated, onChanged, onDeterminingFilename }; +} + +afterEach(() => vi.useRealTimers()); + +describe("download deadlines", () => { + it.each([ + "timeout", + "abort", + ])("settles a pending trigger on %s and ignores its late rejection", async (boundary) => { + vi.useFakeTimers(); + const f = fixture(); + const trigger = deferred(); + const abort = new AbortController(); + let signal!: AbortSignal; + let settled = false; + const pending = captureBrowserDownload({ + ...f, + target: { tabId: 7 }, + browserRelativeDir: "BrowserSkill/deadline", + timeoutMs: 100, + signal: abort.signal, + trigger: async (mark, operationSignal) => { + signal = operationSignal; + mark(); + return trigger.promise; + }, + }).then((result) => { + settled = true; + return result; + }); + if (boundary === "abort") abort.abort(); + await vi.advanceTimersByTimeAsync(100); + expect(settled).toBe(true); + expect(signal.aborted).toBe(true); + expect(f.dispose).toHaveBeenCalledOnce(); + expect(f.onChanged.removeListener).toHaveBeenCalledOnce(); + expect(await pending).toMatchObject({ data: { effect_state: "unknown" } }); + trigger.reject(new Error("late debugger failure")); + await vi.advanceTimersByTimeAsync(0); + // A callback already queued before dispose must not arm another download. + f.offer(); + expect(f.downloads.cancel).not.toHaveBeenCalled(); + expect(vi.getTimerCount()).toBe(0); + }); + + it("does not invoke a trigger for an already cancelled request", async () => { + const f = fixture(); + const controller = new AbortController(); + controller.abort(); + const trigger = vi.fn(); + const result = await captureBrowserDownload({ + ...f, + target: { tabId: 7 }, + browserRelativeDir: "BrowserSkill/deadline", + timeoutMs: 100, + signal: controller.signal, + trigger, + }); + expect(trigger).not.toHaveBeenCalled(); + expect(result).toMatchObject({ data: { effect_state: "none" } }); + }); + + it.each([ + "search", + "cancel", + "removeFile", + ] as const)("bounds a stalled %s during claimed-download cleanup", async (method) => { + vi.useFakeTimers(); + const f = fixture(); + const item = { + ...f.item, + state: method === "removeFile" ? "complete" : "in_progress", + } as chrome.downloads.DownloadItem; + vi.mocked(f.downloads.search).mockResolvedValue([item]); + const stalled = deferred(); + vi.mocked(f.downloads[method]).mockImplementation(() => stalled.promise); + let settled = false; + const pending = captureBrowserDownload({ + ...f, + target: { tabId: 7 }, + browserRelativeDir: "BrowserSkill/deadline", + timeoutMs: 100, + trigger: async (mark) => { + mark(); + f.offer(); + return { tab_id: 7, x: 10, y: 10 }; + }, + }).then((result) => { + settled = true; + return result; + }); + await vi.advanceTimersByTimeAsync(100); + expect(settled).toBe(false); + await vi.advanceTimersByTimeAsync(1_000); + expect(settled).toBe(true); + expect(await pending).toMatchObject({ + data: { effect_state: "committed", cleanup_state: "failed" }, + }); + stalled.reject(new Error("late cleanup failure")); + await vi.advanceTimersByTimeAsync(0); + expect(vi.getTimerCount()).toBe(0); + }); + + it("still bounds the trigger after the file has already completed", async () => { + vi.useFakeTimers(); + const f = fixture(); + const stalled = deferred(); + const completed = { + ...f.item, + state: "complete", + fileSize: 4, + } as chrome.downloads.DownloadItem; + vi.mocked(f.downloads.search).mockResolvedValue([completed]); + let settled = false; + const pending = captureBrowserDownload({ + ...f, + target: { tabId: 7 }, + browserRelativeDir: "BrowserSkill/deadline", + timeoutMs: 100, + trigger: async (mark) => { + mark(); + f.offer(); + f.onCreated.emit(completed); + return stalled.promise; + }, + }).then((result) => { + settled = true; + return result; + }); + await vi.advanceTimersByTimeAsync(100); + expect(settled).toBe(true); + expect(await pending).toMatchObject({ data: { effect_state: "committed" } }); + expect(f.downloads.removeFile).toHaveBeenCalledWith(9); + stalled.resolve({ tab_id: 7, x: 10, y: 10 }); + }); + + it("bounds trigger cleanup itself", async () => { + vi.useFakeTimers(); + const f = fixture(); + let settled = false; + const pending = captureBrowserDownload({ + ...f, + target: { tabId: 7 }, + browserRelativeDir: "BrowserSkill/deadline", + timeoutMs: 100, + trigger: async () => ({ code: "cancelled", message: "cancelled" }), + cleanupTrigger: async () => new Promise(() => {}), + }).then((result) => { + settled = true; + return result; + }); + await vi.advanceTimersByTimeAsync(1_000); + expect(settled).toBe(true); + expect(await pending).toMatchObject({ data: { cleanup_state: "failed" } }); + }); + + it.each([ + ["timeout", "mouseMoved"], + ["abort", "mouseMoved"], + ["timeout", "mousePressed"], + ["abort", "mousePressed"], + ])("releases the global gate after %s at %s without resuming late input", async (boundary, heldType) => { + vi.useFakeTimers(); + const f = fixture(); + const sessions = new SessionManager({ + agentWindow: { + create: async () => ({ windowId: 100, initialTabIds: [7] }), + remove: async () => {}, + ensureActiveTab: async () => 7, + }, + }); + const first = await sessions.start("first"); + const second = await sessions.start("second"); + first.refStore.set("e1", 123, { tabId: 7 }); + const input = deferred(); + const inputs: string[] = []; + f.cdp.send = (async (_tabId, method, params) => { + if (method === "Runtime.evaluate") return { result: { value: "absent" } }; + if (method === "Page.getLayoutMetrics") + return { cssLayoutViewport: { clientWidth: 1280, clientHeight: 720 } }; + if (method === "DOM.getContentQuads") return { quads: [[0, 0, 20, 0, 20, 20, 0, 20]] }; + if (method === "Input.dispatchMouseEvent") { + const type = (params as { type: string }).type; + inputs.push(type); + if (type === heldType) return input.promise; + } + return {}; + }) as CdpRunner["send"]; + const tab = { id: 7, windowId: 100, active: true } as chrome.tabs.Tab; + const tabsApi = { get: async () => tab, query: async () => [tab] }; + const abort = new AbortController(); + let settled = false; + const pending = handleDownload( + sessions, + { + session_id: first.sessionId, + ref: "e1", + browser_relative_dir: "BrowserSkill/first", + timeout_ms: 100, + }, + { + ...f, + tabsApi, + signal: abort.signal, + navigationTargets: { onCreatedNavigationTarget: event() }, + }, + ).then((result) => { + settled = true; + return result; + }); + try { + for (let n = 0; n < 100 && !inputs.includes(heldType); n++) await Promise.resolve(); + expect(inputs).toContain(heldType); + if (boundary === "abort") abort.abort(); + await vi.advanceTimersByTimeAsync(1_100); + expect(settled).toBe(true); + const next = await handleDownload( + sessions, + { session_id: second.sessionId }, + { ...f, tabsApi }, + ); + expect(next).toMatchObject({ message: "download requires a daemon capability directory" }); + const beforeLateReply = [...inputs]; + expect(beforeLateReply).toEqual( + heldType === "mousePressed" + ? ["mouseMoved", "mousePressed", "mouseReleased"] + : ["mouseMoved"], + ); + input.resolve({}); + await vi.advanceTimersByTimeAsync(0); + expect(inputs).toEqual(beforeLateReply); + expect(f.onChanged.removeListener).toHaveBeenCalledOnce(); + } finally { + input.resolve({}); + await pending; + } + }); +}); diff --git a/apps/extension/src/tools/__tests__/download-trigger.test.ts b/apps/extension/src/tools/__tests__/download-trigger.test.ts new file mode 100644 index 00000000..12cdbbaf --- /dev/null +++ b/apps/extension/src/tools/__tests__/download-trigger.test.ts @@ -0,0 +1,70 @@ +import { afterEach, expect, it, vi } from "vitest"; +import { downloadTriggerDeps } from "../download-trigger"; +import type { CdpRunner } from "../shared"; + +afterEach(() => vi.useRealTimers()); + +it("does not send cleanup input to a replacement attachment", async () => { + const abort = new AbortController(); + let attachment = "first"; + const cdp: CdpRunner = { + send: vi.fn(async () => ({})) as CdpRunner["send"], + getAttachmentId: () => attachment, + }; + const scope = downloadTriggerDeps( + { cdp, tabsApi: { get: vi.fn(), query: vi.fn() } }, + abort.signal, + ); + await scope.deps.cdp.send(7, "Input.dispatchMouseEvent", { + type: "mousePressed", + button: "left", + }); + abort.abort(); + attachment = "replacement"; + await scope.cleanup(Date.now() + 1_000); + expect(cdp.send).toHaveBeenCalledTimes(1); + await expect( + scope.deps.cdp.send(7, "Input.dispatchMouseEvent", { type: "mouseReleased" }), + ).rejects.toThrow("aborted"); +}); + +it("bounds a lost button-release reply and detaches without replaying input", async () => { + vi.useFakeTimers(); + const abort = new AbortController(); + const cdp: CdpRunner = { + send: vi.fn(async () => ({})) as CdpRunner["send"], + detach: vi.fn(async () => {}), + }; + const scope = downloadTriggerDeps( + { cdp, tabsApi: { get: vi.fn(), query: vi.fn() } }, + abort.signal, + ); + await scope.deps.cdp.send(7, "Input.dispatchMouseEvent", { type: "mousePressed" }); + vi.mocked(cdp.send).mockImplementation(async () => new Promise(() => {})); + abort.abort(); + const cleanup = scope.cleanup(Date.now() + 1_000); + const failed = expect(cleanup).rejects.toThrow("timed out"); + await vi.advanceTimersByTimeAsync(1_000); + await failed; + expect(cdp.detach).toHaveBeenCalledOnce(); + expect(cdp.send).toHaveBeenCalledTimes(2); +}); + +it("fences an already pending release rather than sending another one", async () => { + const abort = new AbortController(); + const cdp: CdpRunner = { + send: vi.fn(async () => ({})) as CdpRunner["send"], + detach: vi.fn(async () => {}), + }; + const scope = downloadTriggerDeps( + { cdp, tabsApi: { get: vi.fn(), query: vi.fn() } }, + abort.signal, + ); + await scope.deps.cdp.send(7, "Input.dispatchMouseEvent", { type: "mousePressed" }); + vi.mocked(cdp.send).mockImplementation(async () => new Promise(() => {})); + void scope.deps.cdp.send(7, "Input.dispatchMouseEvent", { type: "mouseReleased" }); + abort.abort(); + await scope.cleanup(Date.now() + 1_000); + expect(cdp.detach).toHaveBeenCalledWith(7); + expect(cdp.send).toHaveBeenCalledTimes(2); +}); diff --git a/apps/extension/src/tools/download-capture.ts b/apps/extension/src/tools/download-capture.ts index f6d32bd1..e2dbbed3 100644 --- a/apps/extension/src/tools/download-capture.ts +++ b/apps/extension/src/tools/download-capture.ts @@ -9,10 +9,12 @@ import type { CdpTarget } from "@/browser-driver/frame-graph"; import type { ClickResult, RpcError, TransferEffectState } from "@/transport/types"; import { transferError } from "./errors"; import { type CdpRunner, isRpcError } from "./shared"; +import { waitBounded } from "./transfer-transaction"; const CORRELATION_GRACE_MS = 750; const UNIQUE_SETTLE_MS = 50; const SIZE_POLL_MS = 250; +const CLEANUP_TIMEOUT_MS = 1_000; type DeterminingFilenameListener = ( item: chrome.downloads.DownloadItem, @@ -71,7 +73,9 @@ export interface DownloadCaptureOptions { timeoutMs: number; signal?: AbortSignal; /** `markDispatched` must be called immediately before the mouse press is sent. */ - trigger(markDispatched: () => void): Promise; + trigger(markDispatched: () => void, signal: AbortSignal): Promise; + /** Retire the trigger and release any held input within this cleanup deadline. */ + cleanupTrigger?(deadline: number): Promise; } export interface DownloadCaptureResult { @@ -158,6 +162,8 @@ async function cleanupClaimedDownload(downloads: DownloadsApi, downloadId: numbe export async function captureBrowserDownload( options: DownloadCaptureOptions, ): Promise { + const deadline = Date.now() + options.timeoutMs; + const triggerController = new AbortController(); let click: ClickResult | undefined; let intent: DownloadIntent | undefined; let dispatched = false; @@ -178,9 +184,12 @@ export async function captureBrowserDownload( resolveCompletion = resolve; rejectCompletion = reject; }); + // The trigger may still be pending when timeout, abort or attribution fails. + void completion.catch(() => undefined); const fail = (error: Error) => { if (settled) return; settled = true; + triggerController.abort(); rejectCompletion(error); }; const complete = (item: chrome.downloads.DownloadItem) => { @@ -254,6 +263,10 @@ export async function captureBrowserDownload( }; const determiningListener: DeterminingFilenameListener = (item, suggest) => { + if (settled) { + suggest(); + return; + } const candidate: DownloadCandidate = { item, suggest, @@ -272,6 +285,7 @@ export async function captureBrowserDownload( return true; }; const createdListener = (item: chrome.downloads.DownloadItem) => { + if (settled) return; createdItems.set(item.id, item); if (capturedId !== item.id) return; if (item.state === "interrupted") { @@ -300,12 +314,13 @@ export async function captureBrowserDownload( const navigationTargetListener = ( details: chrome.webNavigation.WebNavigationSourceCallbackDetails, ) => { - if (!dispatched || details.sourceTabId !== options.target.tabId) return; + if (settled || !dispatched || details.sourceTabId !== options.target.tabId) return; popupUrls.add(details.url); reconcile(); }; const cdpSubscription = options.cdp.onEvent?.((source, method, raw) => { - if (method !== "Page.downloadWillBegin" || !sameTarget(source, options.target)) return; + if (settled || method !== "Page.downloadWillBegin" || !sameTarget(source, options.target)) + return; const event = raw as { url?: unknown; suggestedFilename?: unknown; frameId?: unknown }; if (typeof event.url !== "string" || typeof event.suggestedFilename !== "string") return; if (options.expectedFrameId && event.frameId !== options.expectedFrameId) { @@ -350,9 +365,22 @@ export async function captureBrowserDownload( }, SIZE_POLL_MS); try { - const triggered = await options.trigger(() => { - dispatched = true; - }); + if (options.signal?.aborted) throw new DOMException("aborted", "AbortError"); + // A failed completion can stop a pending trigger, while successful file + // completion still needs a click acknowledgement within the same deadline. + const captureFailure = completion.then(() => new Promise(() => {})); + const triggered = await waitBounded( + Promise.race([ + options.trigger(() => { + if (triggerController.signal.aborted) throw new DOMException("aborted", "AbortError"); + dispatched = true; + }, triggerController.signal), + captureFailure, + ]), + deadline, + options.signal, + "download trigger timed out", + ); if (isRpcError(triggered)) { void completion.catch(() => undefined); const effect: TransferEffectState = @@ -369,7 +397,11 @@ export async function captureBrowserDownload( return { click, item }; } catch (err) { const effect: TransferEffectState = - capturedId !== undefined ? "committed" : click ? "unknown" : "none"; + capturedId !== undefined + ? "committed" + : dispatched || click || intent || popupUrls.size > 0 + ? "unknown" + : "none"; failureResult = captureError( err instanceof Error ? err.message : String(err), effect, @@ -378,6 +410,7 @@ export async function captureBrowserDownload( return failureResult; } finally { settled = true; + triggerController.abort(); if (operationTimer) clearTimeout(operationTimer); if (uniquenessTimer) clearTimeout(uniquenessTimer); if (sizePoll) clearInterval(sizePoll); @@ -391,12 +424,26 @@ export async function captureBrowserDownload( clearTimeout(candidate.graceTimer); if (candidate.item.id !== capturedId) suggestDefault(candidate); } + const cleanupDeadline = Date.now() + CLEANUP_TIMEOUT_MS; + const cleanups: Promise[] = []; + if (options.cleanupTrigger) cleanups.push(options.cleanupTrigger(cleanupDeadline)); if (!succeeded && capturedId !== undefined) { - try { - await cleanupClaimedDownload(options.downloads, capturedId); - } catch { - if (failureResult?.data) failureResult.data.cleanup_state = "failed"; + cleanups.push(cleanupClaimedDownload(options.downloads, capturedId)); + } + try { + // One failed cleanup must not skip or outlive the other. Start both + // while this capture still owns the gate and share one bounded budget. + const results = await waitBounded( + Promise.allSettled(cleanups), + cleanupDeadline, + undefined, + "download cleanup timed out", + ); + if (results.some((result) => result.status === "rejected") && failureResult?.data) { + failureResult.data.cleanup_state = "failed"; } + } catch { + if (failureResult?.data) failureResult.data.cleanup_state = "failed"; } } } diff --git a/apps/extension/src/tools/download-trigger.ts b/apps/extension/src/tools/download-trigger.ts new file mode 100644 index 00000000..b43b5222 --- /dev/null +++ b/apps/extension/src/tools/download-trigger.ts @@ -0,0 +1,88 @@ +import type { CdpTarget } from "@/browser-driver/frame-graph"; +import type { InteractionDeps } from "./interaction"; +import { type CdpRunner, sendToCdpTarget } from "./shared"; +import { waitBounded } from "./transfer-transaction"; + +/** A retired download click must never resume input when a CDP reply arrives late. */ +export function downloadTriggerDeps(deps: InteractionDeps, signal: AbortSignal) { + let retired = false; + let release: { tabId: number; params: object; attachmentId?: string } | undefined; + let releasing = false; + const check = () => { + if (retired || signal.aborted) throw new DOMException("download trigger aborted", "AbortError"); + }; + const send = async (target: CdpTarget, method: string, params?: object): Promise => { + check(); + const mouse = params as { type?: string } | undefined; + if (method === "Input.dispatchMouseEvent") { + if (mouse?.type === "mousePressed") { + release = { + tabId: target.tabId, + params: { ...params, type: "mouseReleased" }, + attachmentId: deps.cdp.getAttachmentId?.(target.tabId), + }; + } else if (mouse?.type === "mouseReleased") { + releasing = true; + } + } + const result = await sendToCdpTarget(deps.cdp, target, method, params); + if (method === "Input.dispatchMouseEvent" && mouse?.type === "mouseReleased") { + release = undefined; + releasing = false; + } + return result; + }; + const cdp: CdpRunner = { + send: (tabId, method, params) => send({ tabId }, method, params), + sendToTarget: send, + trackSessionTab: deps.cdp.trackSessionTab?.bind(deps.cdp), + dialogCursor: deps.cdp.dialogCursor?.bind(deps.cdp), + dialogsSince: deps.cdp.dialogsSince?.bind(deps.cdp), + getAttachmentId: deps.cdp.getAttachmentId?.bind(deps.cdp), + getFrameGraph: deps.cdp.getFrameGraph + ? (tabId) => { + check(); + return deps.cdp.getFrameGraph!(tabId); + } + : undefined, + }; + return { + deps: { + ...deps, + cdp, + signal, + onInputSent: deps.onInputSent + ? (tabId: number) => { + check(); + deps.onInputSent!(tabId); + } + : undefined, + }, + async cleanup(deadline: number): Promise { + retired = true; + if (!release) return; + const pending = release; + release = undefined; + if ( + pending.attachmentId !== undefined && + deps.cdp.getAttachmentId?.(pending.tabId) !== pending.attachmentId + ) + return; + try { + // A release already in flight must not be replayed. Detaching fences + // that debugger connection; otherwise release the button once while + // this download still owns the global gate. + const cleanup = releasing + ? deps.cdp.detach?.(pending.tabId) + : deps.cdp.send(pending.tabId, "Input.dispatchMouseEvent", pending.params); + if (!cleanup) throw new Error("download input cleanup unavailable"); + await waitBounded(cleanup, deadline, undefined, "download input cleanup timed out"); + } catch (error) { + // ChromiumCdp invalidates attachment identity synchronously and fences + // a pending detach. Do not wait beyond the shared cleanup budget. + void deps.cdp.detach?.(pending.tabId).catch(() => {}); + throw error; + } + }, + }; +} diff --git a/apps/extension/src/tools/download.ts b/apps/extension/src/tools/download.ts index 546ae8dc..ae9d83b9 100644 --- a/apps/extension/src/tools/download.ts +++ b/apps/extension/src/tools/download.ts @@ -10,6 +10,7 @@ import { type DownloadsApi, type NavigationTargetsApi, } from "./download-capture"; +import { downloadTriggerDeps } from "./download-trigger"; import { clickResolvedTarget, type InteractionDeps, resolveActionTarget } from "./interaction"; import { enforceAgentWindow, isRpcError, lookupSession, resolveTargetTab } from "./shared"; @@ -42,6 +43,7 @@ export async function handleDownload( const address = await resolveActionTarget(deps.cdp, ctx, target, params, "download"); if (isRpcError(address)) return address; + let trigger: ReturnType | undefined; const capture = await captureBrowserDownload({ cdp: deps.cdp, target: address.cdpTarget, @@ -52,7 +54,11 @@ export async function handleDownload( timeoutMs: params.timeout_ms ?? 120_000, signal: deps.signal, expectedFrameId: address.frameId, - trigger: (markDispatched) => clickResolvedTarget(ctx, address, {}, deps, markDispatched), + trigger: (markDispatched, signal) => { + trigger = downloadTriggerDeps(deps, signal); + return clickResolvedTarget(ctx, address, {}, trigger.deps, markDispatched); + }, + cleanupTrigger: (deadline) => trigger?.cleanup(deadline) ?? Promise.resolve(), }); if (isRpcError(capture)) return capture; const { click, item } = capture; From 78bd77e8e984497db19183dfa8416055d530a51f Mon Sep 17 00:00:00 2001 From: NianJiuZst <180004567+NianJiuZst@users.noreply.github.com> Date: Sun, 27 Sep 2026 02:02:36 +0800 Subject: [PATCH 2/3] test(download): run cancellation regression in its own CI job --- .github/workflows/ci.yml | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3a62c1c2..ad4da7bf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -183,6 +183,27 @@ jobs: New-Item -ItemType Directory -Force -Path $env:TEMP | Out-Null node scripts/check-dsh-package.mjs + browser-download: + name: Browser download cancellation regression + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v6 + - uses: pnpm/action-setup@v6 + - uses: actions/setup-node@v6 + with: + node-version: 22 + cache: pnpm + - name: Install dependencies + run: pnpm install --frozen-lockfile + - name: Prepare extension types + run: pnpm --filter @browser-skill/extension exec wxt prepare + - name: Run download cancellation browser regression + working-directory: apps/extension + env: + BSK_CLICK_CHROME: google-chrome + run: pnpm exec vitest run src/tools/__tests__/download-deadline.browser.test.ts + browser-input: name: Browser input readiness regression runs-on: ubuntu-latest @@ -203,12 +224,6 @@ jobs: - name: Prepare extension types run: pnpm --filter @browser-skill/extension exec wxt prepare - - name: Run download cancellation browser regression - working-directory: apps/extension - env: - BSK_CLICK_CHROME: google-chrome - run: pnpm exec vitest run src/tools/__tests__/download-deadline.browser.test.ts - - name: Run browser input and debugging regressions working-directory: apps/extension env: From b967b27c03b3218ee37e4520d0e4258e254e20f8 Mon Sep 17 00:00:00 2001 From: NianJiuZst <180004567+NianJiuZst@users.noreply.github.com> Date: Sun, 27 Sep 2026 21:45:08 +0800 Subject: [PATCH 3/3] fix(download): fence dispatch and retire owned overlay resources --- .../src/browser-driver/chromium-cdp.ts | 42 ++- .../download-deadline.browser.test.ts | 147 +++++++- .../__tests__/download-lifecycle.test.ts | 348 ++++++++++++++++++ apps/extension/src/tools/click-overlay.ts | 58 ++- apps/extension/src/tools/download-trigger.ts | 112 ++++-- apps/extension/src/tools/shared.ts | 11 +- 6 files changed, 646 insertions(+), 72 deletions(-) create mode 100644 apps/extension/src/tools/__tests__/download-lifecycle.test.ts diff --git a/apps/extension/src/browser-driver/chromium-cdp.ts b/apps/extension/src/browser-driver/chromium-cdp.ts index 2687e934..17e4cbb9 100644 --- a/apps/extension/src/browser-driver/chromium-cdp.ts +++ b/apps/extension/src/browser-driver/chromium-cdp.ts @@ -44,6 +44,15 @@ import { export type CdpDebuggee = chrome.debugger.Debuggee & { sessionId?: string }; +/** Guards checked at native dispatch, after any asynchronous connection setup. */ +export interface CdpDispatchGuard { + signal?: AbortSignal; + /** Only use this live attachment; never reconnect a cleanup command. */ + attachmentId?: string; + /** Called immediately before native send, never for a cancelled or stale command. */ + onDispatch?(): void; +} + /** * Minimal slice of `chrome.debugger` the rest of the extension * depends on. Stays as an explicit interface so vitest can inject a @@ -350,6 +359,29 @@ export class ChromiumCdp { } } + async sendGuarded( + target: CdpTarget, + method: string, + params: object | undefined, + guard: CdpDispatchGuard, + ): Promise { + guard.signal?.throwIfAborted(); + if (guard.attachmentId === undefined) await this.ensureAttached(target.tabId); + try { + return (await this.command(target, method, params ?? {}, undefined, () => { + guard.signal?.throwIfAborted(); + if ( + guard.attachmentId !== undefined && + this.getAttachmentId(target.tabId) !== guard.attachmentId + ) + throw new Error("Debugger attachment changed before dispatch"); + guard.onDispatch?.(); + })) as T; + } catch (err) { + throw normalizeError(err); + } + } + async getFrameGraph(tabId: number): Promise { await this.ensureAttached(tabId); // Ordinary configuration failures leave the graph to the root tree. A @@ -531,7 +563,9 @@ export class ChromiumCdp { } /** Detach if attached; never throws. */ - async detach(tabId: number): Promise { + async detach(tabId: number, expectedAttachmentId?: string): Promise { + if (expectedAttachmentId !== undefined && this.getAttachmentId(tabId) !== expectedAttachmentId) + return; this.attachmentVersions.set(tabId, (this.attachmentVersions.get(tabId) ?? 0) + 1); const existing = this.detachInFlight.get(tabId); if (existing) { @@ -671,11 +705,15 @@ export class ChromiumCdp { method: string, params: object, readTimeoutMs?: number, + beforeDispatch?: () => void, ): Promise { return this.readGate.run( target, method, - () => this.api.sendCommand(target, method, params), + () => { + beforeDispatch?.(); + return this.api.sendCommand(target, method, params); + }, readTimeoutMs, ); } diff --git a/apps/extension/src/tools/__tests__/download-deadline.browser.test.ts b/apps/extension/src/tools/__tests__/download-deadline.browser.test.ts index dfa0af9a..5f564908 100644 --- a/apps/extension/src/tools/__tests__/download-deadline.browser.test.ts +++ b/apps/extension/src/tools/__tests__/download-deadline.browser.test.ts @@ -13,7 +13,12 @@ type Send = >( ) => Promise; async function browser( - run: (send: Send, cdp: ChromiumCdp, sessionId: () => string) => Promise, + run: ( + send: Send, + cdp: ChromiumCdp, + sessionId: () => string, + api: CdpDebuggerApi, + ) => Promise, ) { const { withChrome } = await import( new URL( @@ -54,7 +59,7 @@ async function browser( }; const cdp = new ChromiumCdp(api); try { - await run(send, cdp, () => activeSession); + await run(send, cdp, () => activeSession, api); } finally { await cdp.detach(7); } @@ -99,19 +104,26 @@ describe.skipIf(!process.env.BSK_CLICK_CHROME)("download trigger retirement", () release = resolve; }); const inputs: string[] = []; - const wrapped: CdpRunner = { - send: (async (tabId: number, method: string, params?: object) => { - const value = await cdp.send(tabId, method, params); - if (method === "Input.dispatchMouseEvent") { - const type = (params as { type: string }).type; - inputs.push(type); - if (type === heldType) { - reached(); - await reply; - } + const holdReply = async ( + method: string, + params: object | undefined, + value: T, + ): Promise => { + if (method === "Input.dispatchMouseEvent") { + const type = (params as { type: string }).type; + inputs.push(type); + if (type === heldType) { + reached(); + await reply; } - return value; - }) as CdpRunner["send"], + } + return value; + }; + const wrapped: CdpRunner = { + send: async (tabId, method, params) => + holdReply(method, params, await cdp.send(tabId, method, params)), + sendGuarded: async (target, method, params, guard) => + holdReply(method, params, await cdp.sendGuarded(target, method, params, guard)), detach: cdp.detach.bind(cdp), getAttachmentId: cdp.getAttachmentId.bind(cdp), onEvent: () => ({ dispose() {} }), @@ -172,4 +184,111 @@ describe.skipIf(!process.env.BSK_CLICK_CHROME)("download trigger retirement", () } }); }, 40_000); + + it("does not deliver a queued mouse move after cancellation during reattachment", async () => { + await browser(async (_send, cdp, _sessionId, api) => { + const sessions = manager(); + const ctx = await sessions.start("download-reattach"); + await cdp.send(7, "Runtime.evaluate", { + expression: ` + document.body.innerHTML = ''; + window.moves = 0; window.startedExports = 0; + document.addEventListener('mousemove', () => { window.moves++; }); + document.querySelector('#export').onclick = () => { window.startedExports++; }; + `, + }); + const abort = new AbortController(); + let release!: () => void; + let reached!: () => void; + let settled!: () => void; + const held = new Promise((resolve) => { + release = resolve; + }); + const ready = new Promise((resolve) => { + reached = resolve; + }); + const inputSettled = new Promise((resolve) => { + settled = resolve; + }); + const attach = api.attach; + const sendCommand = api.sendCommand; + const inputs: string[] = []; + api.sendCommand = (target, method, params) => { + if (method === "Input.dispatchMouseEvent") inputs.push((params as { type: string }).type); + return sendCommand(target, method, params); + }; + let interceptMove = true; + const wrapped: CdpRunner = { + send: cdp.send.bind(cdp), + sendGuarded: async (target, method, params, guard) => { + if (method === "Input.dispatchMouseEvent" && interceptMove) { + interceptMove = false; + await cdp.detach(7); + api.attach = async (target, version) => { + await attach(target, version); + reached(); + await held; + }; + try { + return await cdp.sendGuarded(target, method, params, guard); + } finally { + settled(); + } + } + return cdp.sendGuarded(target, method, params, guard); + }, + detach: cdp.detach.bind(cdp), + getAttachmentId: cdp.getAttachmentId.bind(cdp), + onEvent: () => ({ dispose() {} }), + }; + const event = () => ({ addListener() {}, removeListener() {} }); + const pending = handleDownload( + sessions, + { + session_id: ctx.sessionId, + tab_id: 7, + selector: "#export", + browser_relative_dir: "BrowserSkill/reattach", + timeout_ms: 2_000, + }, + { + cdp: wrapped, + tabsApi, + signal: abort.signal, + navigationTargets: { onCreatedNavigationTarget: event() }, + downloads: { + onCreated: event(), + onChanged: event(), + onDeterminingFilename: event(), + search: async () => [], + cancel: async () => {}, + removeFile: async () => {}, + }, + }, + ); + try { + await ready; + abort.abort(); + expect(await pending).toHaveProperty("code"); + expect(inputs).toEqual([]); + release(); + await inputSettled; + expect(inputs).toEqual([]); + const { result } = await cdp.send<{ result: { value: number[] } }>(7, "Runtime.evaluate", { + expression: "[moves, startedExports]", + returnByValue: true, + }); + expect(result.value).toEqual([0, 0]); + console.log( + "DOWNLOAD_REATTACH_PROOF", + JSON.stringify({ inputs, moves: result.value[0], startedExports: result.value[1] }), + ); + } finally { + abort.abort(); + release(); + await pending; + api.attach = attach; + } + }); + }, 40_000); }); diff --git a/apps/extension/src/tools/__tests__/download-lifecycle.test.ts b/apps/extension/src/tools/__tests__/download-lifecycle.test.ts new file mode 100644 index 00000000..cbd389c3 --- /dev/null +++ b/apps/extension/src/tools/__tests__/download-lifecycle.test.ts @@ -0,0 +1,348 @@ +import { afterEach, expect, it, vi } from "vitest"; +import { type CdpDebuggee, type CdpDebuggerApi, ChromiumCdp } from "@/browser-driver/chromium-cdp"; +import type { InputPassthroughMessage } from "@/lib/input-passthrough-bridge"; +import { SessionManager } from "@/session-manager/manager"; +import { handleDownload } from "../download"; +import { downloadTriggerDeps } from "../download-trigger"; + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((done) => { + resolve = done; + }); + return { promise, resolve }; +} + +function event() { + const listeners = new Set<(...args: T) => void>(); + return { + listeners, + addListener: (listener: (...args: T) => void) => listeners.add(listener), + removeListener: (listener: (...args: T) => void) => listeners.delete(listener), + fire: (...args: T) => { + for (const listener of listeners) listener(...args); + }, + }; +} + +async function fixture() { + const onEvent = event<[CdpDebuggee, string, unknown]>(); + const onDetach = event<[chrome.debugger.Debuggee, string]>(); + const inputs: string[] = []; + const api: CdpDebuggerApi = { + attach: vi.fn(async () => {}), + detach: vi.fn(async () => {}), + sendCommand: vi.fn(async (_target, method, params) => { + if (method === "Runtime.evaluate") return { result: { value: "absent" } }; + if (method === "Page.getFrameTree") return { frameTree: { frame: { id: "root" } } }; + if (method === "Page.getLayoutMetrics") + return { cssLayoutViewport: { clientWidth: 1280, clientHeight: 720 } }; + if (method === "DOM.getContentQuads") return { quads: [[0, 0, 20, 0, 20, 20, 0, 20]] }; + if (method === "Input.dispatchMouseEvent") inputs.push((params as { type: string }).type); + return {}; + }), + onEvent: onEvent as unknown as CdpDebuggerApi["onEvent"], + onDetach: onDetach as unknown as CdpDebuggerApi["onDetach"], + }; + const cdp = new ChromiumCdp(api); + const sessions = new SessionManager({ + agentWindow: { + create: async () => ({ windowId: 100, initialTabIds: [7] }), + remove: async () => {}, + ensureActiveTab: async () => 7, + }, + }); + const ctx = await sessions.start("download-lifecycle"); + ctx.refStore.set("e1", 123, { tabId: 7 }); + const tab = { id: 7, windowId: 100, active: true } as chrome.tabs.Tab; + const downloads = { + onCreated: event<[chrome.downloads.DownloadItem]>(), + onChanged: event<[chrome.downloads.DownloadDelta]>(), + onDeterminingFilename: + event< + [ + chrome.downloads.DownloadItem, + (suggestion?: chrome.downloads.DownloadFilenameSuggestion) => void, + ] + >(), + search: async () => [], + cancel: async () => {}, + removeFile: async () => {}, + }; + return { + api, + cdp, + onDetach, + inputs, + sessions, + params: { + session_id: ctx.sessionId, + ref: "e1", + browser_relative_dir: "BrowserSkill/lifecycle", + timeout_ms: 10_000, + }, + deps: { + cdp, + downloads, + tabsApi: { get: async () => tab, query: async () => [tab] }, + navigationTargets: { onCreatedNavigationTarget: event<[]>() }, + }, + }; +} + +afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); +}); + +it("does not detach the replacement attachment when an old cleanup release times out", async () => { + vi.useFakeTimers(); + const f = await fixture(); + const abort = new AbortController(); + const scope = downloadTriggerDeps(f.deps, abort.signal); + const release = deferred(); + const releasing = deferred(); + const send = vi.mocked(f.api.sendCommand).getMockImplementation()!; + vi.spyOn(f.api, "sendCommand").mockImplementation(async (target, method, params) => { + const reply = await send(target, method, params); + if ( + method === "Input.dispatchMouseEvent" && + (params as { type: string }).type === "mouseReleased" + ) { + releasing.resolve(); + return release.promise; + } + return reply; + }); + try { + await scope.deps.cdp.send(7, "Input.dispatchMouseEvent", { type: "mousePressed" }); + const original = f.cdp.getAttachmentId(7); + abort.abort(); + const cleanup = scope.cleanup(Date.now() + 1_000); + const failed = expect(cleanup).rejects.toThrow("timed out"); + await releasing.promise; + await f.cdp.detach(7); + await f.cdp.ensureAttached(7); + const replacement = f.cdp.getAttachmentId(7); + expect(replacement).toBeDefined(); + expect(replacement).not.toBe(original); + await vi.advanceTimersByTimeAsync(1_000); + await failed; + expect(f.cdp.getAttachmentId(7)).toBe(replacement); + expect(f.api.detach).toHaveBeenCalledTimes(1); + release.resolve({}); + await vi.advanceTimersByTimeAsync(0); + expect(f.cdp.getAttachmentId(7)).toBe(replacement); + expect(f.inputs).toEqual(["mousePressed", "mouseReleased"]); + } finally { + release.resolve({}); + f.cdp.dispose(); + } +}); + +it.each([ + "mouseMoved", + "mousePressed", +])("does not dispatch %s after cancellation during reattachment", async (type) => { + vi.useFakeTimers(); + const f = await fixture(); + const abort = new AbortController(); + const attach = deferred(); + const attaching = deferred(); + let probes = 0; + const send = vi.mocked(f.api.sendCommand).getMockImplementation()!; + vi.spyOn(f.api, "sendCommand").mockImplementation(async (target, method, params) => { + const reply = await send(target, method, params); + if (method === "Runtime.evaluate" && ++probes === (type === "mouseMoved" ? 1 : 2)) { + f.onDetach.fire({ tabId: 7 }, "connection_lost"); + vi.mocked(f.api.attach).mockImplementationOnce(() => { + attaching.resolve(); + return attach.promise; + }); + } + return reply; + }); + const pending = handleDownload(f.sessions, f.params, { ...f.deps, signal: abort.signal }); + try { + await attaching.promise; + abort.abort(); + await vi.advanceTimersByTimeAsync(1_000); + expect(await pending).toMatchObject({ + code: "cdp_failed", + message: expect.stringMatching(/abort/), + }); + expect(f.deps.downloads.onChanged.listeners.size).toBe(0); + const before = [...f.inputs]; + expect(before).toEqual(type === "mouseMoved" ? [] : ["mouseMoved"]); + attach.resolve(); + await vi.advanceTimersByTimeAsync(0); + expect(f.inputs).toEqual(before); + expect(vi.getTimerCount()).toBe(0); + } finally { + abort.abort(); + attach.resolve(); + await pending; + f.cdp.dispose(); + } +}); + +it.each([ + undefined, + "child-session", +])("checks cancellation at native dispatch for target %s", async (sessionId) => { + const f = await fixture(); + const abort = new AbortController(); + const attach = deferred(); + const attaching = deferred(); + vi.mocked(f.api.attach).mockImplementationOnce(() => { + attaching.resolve(); + return attach.promise; + }); + const scope = downloadTriggerDeps(f.deps, abort.signal); + const pending = scope.deps.cdp.sendToTarget!( + { tabId: 7, sessionId }, + "Input.dispatchMouseEvent", + { type: "mousePressed" }, + ); + const failed = expect(pending).rejects.toThrow(/abort/i); + try { + await attaching.promise; + abort.abort(); + await scope.cleanup(Date.now() + 1_000); + expect(f.inputs).toEqual([]); + attach.resolve(); + await failed; + expect(f.inputs).toEqual([]); + expect(f.api.detach).not.toHaveBeenCalled(); + } finally { + attach.resolve(); + await failed; + f.cdp.dispose(); + } +}); + +it("refuses guarded cleanup sends and detaches for a replaced attachment without reconnecting", async () => { + const f = await fixture(); + try { + await f.cdp.ensureAttached(7); + const attachmentId = f.cdp.getAttachmentId(7)!; + await f.cdp.detach(7); + const cleanup = () => + f.cdp.sendGuarded( + { tabId: 7 }, + "Input.dispatchMouseEvent", + { type: "mouseReleased" }, + { attachmentId }, + ); + await expect(cleanup()).rejects.toThrow("attachment changed"); + expect(f.api.attach).toHaveBeenCalledTimes(1); + await f.cdp.ensureAttached(7); + const replacement = f.cdp.getAttachmentId(7); + await expect(cleanup()).rejects.toThrow("attachment changed"); + await f.cdp.detach(7, attachmentId); + expect(f.cdp.getAttachmentId(7)).toBe(replacement); + expect(f.api.detach).toHaveBeenCalledTimes(1); + expect(f.inputs).toEqual([]); + } finally { + f.cdp.dispose(); + } +}); + +it.each([ + "released", + "pending-end", + "rejected-end", +])("releases overlay resources before a held press reply and preserves the next operation's ownership: %s", async (mode) => { + vi.useFakeTimers(); + const f = await fixture(); + const abort = new AbortController(); + const nextAbort = new AbortController(); + const press = [deferred(), deferred()]; + const pressed = [deferred(), deferred()]; + let pressIndex = 0; + let bypassCount = 1; // An unrelated retained hover owns this reference. + const leases = new Set(); + const endReply = deferred(); + let endCount = 0; + const bypassOverlay = vi.fn(async (_tabId: number, enabled: boolean) => { + bypassCount += enabled ? 1 : -1; + }); + const sendInputPassthrough = vi.fn(async (_tabId: number, message: InputPassthroughMessage) => { + if (message.phase === "begin") leases.add(message.id); + else { + leases.delete(message.id); + if (++endCount === 1) { + if (mode === "pending-end") await endReply.promise; + if (mode === "rejected-end") throw new Error("passthrough end failed"); + } + } + }); + const send = vi.mocked(f.api.sendCommand).getMockImplementation()!; + vi.spyOn(f.api, "sendCommand").mockImplementation(async (target, method, params) => { + if (method === "Runtime.evaluate") + return { result: { value: leases.size ? "clear" : "covered" } }; + const reply = await send(target, method, params); + if ( + method === "Input.dispatchMouseEvent" && + (params as { type: string }).type === "mousePressed" + ) { + const index = pressIndex++; + pressed[index].resolve(); + return press[index].promise; + } + return reply; + }); + const deps = { ...f.deps, bypassOverlay, sendInputPassthrough }; + const pending = handleDownload(f.sessions, f.params, { ...deps, signal: abort.signal }); + let next: ReturnType | undefined; + try { + await pressed[0].promise; + expect(bypassCount).toBe(2); + expect(leases.size).toBe(1); + abort.abort(); + await vi.advanceTimersByTimeAsync(1_000); + if (mode !== "released") expect(await pending).toHaveProperty("data.cleanup_state", "failed"); + expect(await pending).toMatchObject({ + code: "cdp_failed", + message: expect.stringMatching(/abort/), + }); + expect(f.deps.downloads.onChanged.listeners.size).toBe(0); + expect(bypassCount).toBe(1); + expect(leases.size).toBe(0); + + next = handleDownload(f.sessions, f.params, { ...deps, signal: nextAbort.signal }); + await pressed[1].promise; + const nextLease = [...leases]; + expect(nextLease).toHaveLength(1); + const before = [...f.inputs]; + press[0].resolve({}); + await vi.advanceTimersByTimeAsync(0); + expect(bypassCount).toBe(2); + expect([...leases]).toEqual(nextLease); + expect(f.inputs).toEqual(before); + endReply.resolve(); + await vi.advanceTimersByTimeAsync(0); + expect(bypassCount).toBe(2); + expect([...leases]).toEqual(nextLease); + + nextAbort.abort(); + await vi.advanceTimersByTimeAsync(0); + expect(await next).toMatchObject({ + code: "cdp_failed", + message: expect.stringMatching(/abort/), + }); + expect(bypassCount).toBe(1); + expect(leases.size).toBe(0); + press[1].resolve({}); + await vi.advanceTimersByTimeAsync(0); + expect(bypassOverlay.mock.calls.map((call) => call[1])).toEqual([true, false, true, false]); + } finally { + abort.abort(); + nextAbort.abort(); + endReply.resolve(); + for (const held of press) held.resolve({}); + await vi.advanceTimersByTimeAsync(1_000); + await Promise.all([pending, next]); + f.cdp.dispose(); + } +}); diff --git a/apps/extension/src/tools/click-overlay.ts b/apps/extension/src/tools/click-overlay.ts index 7fa7d0be..8edac05e 100644 --- a/apps/extension/src/tools/click-overlay.ts +++ b/apps/extension/src/tools/click-overlay.ts @@ -15,6 +15,8 @@ export interface ClickOverlayDeps { /** Acquire/release only this operation's automation bypass reference. */ bypassOverlay?: (tabId: number, enabled: boolean) => Promise; sendInputPassthrough?: InputPassthroughSendToTab; + /** Let a bounded operation release these resources without waiting for input replies. */ + registerClickCleanup?: (cleanup: () => Promise) => () => void; } type OverlayHit = "absent" | "clear" | "covered" | "unknown"; @@ -29,7 +31,29 @@ export async function withClickOverlay( ): Promise { const send = deps.sendInputPassthrough ?? sendInputPassthrough; let bypass = false; + let bypassAcquisition: Promise | undefined; let passthroughId: string | undefined; + let retired = false; + let cleanupPromise: Promise | undefined; + const cleanup = () => { + retired = true; + // Outer retirement and a late inner finally share one release of each resource. + cleanupPromise ??= (async () => { + const results = await Promise.allSettled([ + (async () => { + if (passthroughId) + await send(tabId, { type: INPUT_PASSTHROUGH, phase: "end", id: passthroughId }); + })(), + (async () => { + await bypassAcquisition; + if (bypass) await deps.bypassOverlay!(tabId, false); + })(), + ]); + for (const result of results) if (result.status === "rejected") throw result.reason; + })(); + return cleanupPromise; + }; + const unregisterCleanup = deps.registerClickCleanup?.(cleanup); let passthroughExpiresAt = Infinity; const expired = () => Date.now() >= passthroughExpiresAt; // Reserve one second for input delivery; completed clicks still use the full lease. @@ -73,14 +97,16 @@ export async function withClickOverlay( return "unknown"; }; try { - if (deps.signal?.aborted) return cancelled(); + if (retired || deps.signal?.aborted) return cancelled(); let hit = await probe(); let covered = hit === "covered"; - if (deps.signal?.aborted) return cancelled(); + if (retired || deps.signal?.aborted) return cancelled(); if ((alwaysBypass || covered) && deps.bypassOverlay) { try { - await deps.bypassOverlay(tabId, true); - bypass = true; + bypassAcquisition = deps.bypassOverlay(tabId, true).then(() => { + bypass = true; + }); + await bypassAcquisition; } catch (error) { console.debug("[bsk click] overlay bypass enable failed", error); } @@ -88,7 +114,7 @@ export async function withClickOverlay( hit = await probe(); covered ||= hit === "covered"; } - if (deps.signal?.aborted) return cancelled(); + if (retired || deps.signal?.aborted) return cancelled(); if (hit === "covered") { passthroughId = crypto.randomUUID(); // Start before sending, so our deadline cannot outlive the content-side lease. @@ -100,7 +126,7 @@ export async function withClickOverlay( } hit = await probe(); } - if (deps.signal?.aborted) return cancelled(); + if (retired || deps.signal?.aborted) return cancelled(); if (pressDeadlineReached() || hit === "covered" || (covered && hit === "unknown")) return notReady(); const result = await click(async () => { @@ -126,20 +152,12 @@ export async function withClickOverlay( } return result; } finally { - // Release our lease even if begin's acknowledgement was lost. - if (passthroughId) { - try { - await send(tabId, { type: INPUT_PASSTHROUGH, phase: "end", id: passthroughId }); - } catch (error) { - console.debug("[bsk click] overlay passthrough restore failed", error); - } - } - if (bypass) { - try { - await deps.bypassOverlay!(tabId, false); - } catch (error) { - console.debug("[bsk click] overlay bypass restore failed", error); - } + try { + await cleanup(); + } catch (error) { + console.debug("[bsk click] overlay restore failed", error); + } finally { + unregisterCleanup?.(); } } } diff --git a/apps/extension/src/tools/download-trigger.ts b/apps/extension/src/tools/download-trigger.ts index b43b5222..036b05a5 100644 --- a/apps/extension/src/tools/download-trigger.ts +++ b/apps/extension/src/tools/download-trigger.ts @@ -8,24 +8,40 @@ export function downloadTriggerDeps(deps: InteractionDeps, signal: AbortSignal) let retired = false; let release: { tabId: number; params: object; attachmentId?: string } | undefined; let releasing = false; + const clickCleanups = new Set<() => Promise>(); const check = () => { if (retired || signal.aborted) throw new DOMException("download trigger aborted", "AbortError"); }; const send = async (target: CdpTarget, method: string, params?: object): Promise => { check(); const mouse = params as { type?: string } | undefined; - if (method === "Input.dispatchMouseEvent") { - if (mouse?.type === "mousePressed") { - release = { - tabId: target.tabId, - params: { ...params, type: "mouseReleased" }, - attachmentId: deps.cdp.getAttachmentId?.(target.tabId), - }; - } else if (mouse?.type === "mouseReleased") { - releasing = true; + const onDispatch = () => { + check(); + if (method === "Input.dispatchMouseEvent") { + if (mouse?.type === "mousePressed") { + release = { + tabId: target.tabId, + params: { ...params, type: "mouseReleased" }, + attachmentId: deps.cdp.getAttachmentId?.(target.tabId), + }; + } else if (mouse?.type === "mouseReleased") { + releasing = true; + } } - } - const result = await sendToCdpTarget(deps.cdp, target, method, params); + }; + const result = deps.cdp.sendGuarded + ? await deps.cdp.sendGuarded(target, method, params, { + signal, + attachmentId: + method === "Input.dispatchMouseEvent" && mouse?.type === "mouseReleased" + ? release?.attachmentId + : undefined, + onDispatch, + }) + : await (() => { + onDispatch(); + return sendToCdpTarget(deps.cdp, target, method, params); + })(); if (method === "Input.dispatchMouseEvent" && mouse?.type === "mouseReleased") { release = undefined; releasing = false; @@ -46,11 +62,54 @@ export function downloadTriggerDeps(deps: InteractionDeps, signal: AbortSignal) } : undefined, }; + const cleanupInput = async (deadline: number): Promise => { + if (!release) return; + const pending = release; + release = undefined; + const ownsAttachment = () => + pending.attachmentId === undefined || + deps.cdp.getAttachmentId?.(pending.tabId) === pending.attachmentId; + const detach = () => { + if (!ownsAttachment()) return Promise.resolve(); + return pending.attachmentId === undefined + ? deps.cdp.detach?.(pending.tabId) + : deps.cdp.detach?.(pending.tabId, pending.attachmentId); + }; + if (!ownsAttachment()) return; + try { + // Never replay an in-flight release or send cleanup on a new attachment. + const cleanup = releasing + ? detach() + : deps.cdp.sendGuarded + ? deps.cdp.sendGuarded( + { tabId: pending.tabId }, + "Input.dispatchMouseEvent", + pending.params, + { + attachmentId: pending.attachmentId, + }, + ) + : deps.cdp.send(pending.tabId, "Input.dispatchMouseEvent", pending.params); + if (!cleanup) throw new Error("download input cleanup unavailable"); + await waitBounded(cleanup, deadline, undefined, "download input cleanup timed out"); + } catch (error) { + // Replacement may happen while release is pending. Recheck ownership, + // and have the driver enforce it again when executing the fallback. + void detach()?.catch(() => {}); + throw error; + } + }; return { deps: { ...deps, cdp, signal, + registerClickCleanup: (cleanup: () => Promise) => { + clickCleanups.add(cleanup); + return () => { + clickCleanups.delete(cleanup); + }; + }, onInputSent: deps.onInputSent ? (tabId: number) => { check(); @@ -60,29 +119,14 @@ export function downloadTriggerDeps(deps: InteractionDeps, signal: AbortSignal) }, async cleanup(deadline: number): Promise { retired = true; - if (!release) return; - const pending = release; - release = undefined; - if ( - pending.attachmentId !== undefined && - deps.cdp.getAttachmentId?.(pending.tabId) !== pending.attachmentId - ) - return; - try { - // A release already in flight must not be replayed. Detaching fences - // that debugger connection; otherwise release the button once while - // this download still owns the global gate. - const cleanup = releasing - ? deps.cdp.detach?.(pending.tabId) - : deps.cdp.send(pending.tabId, "Input.dispatchMouseEvent", pending.params); - if (!cleanup) throw new Error("download input cleanup unavailable"); - await waitBounded(cleanup, deadline, undefined, "download input cleanup timed out"); - } catch (error) { - // ChromiumCdp invalidates attachment identity synchronously and fences - // a pending detach. Do not wait beyond the shared cleanup budget. - void deps.cdp.detach?.(pending.tabId).catch(() => {}); - throw error; - } + const cleanups = [cleanupInput(deadline), ...[...clickCleanups].map((cleanup) => cleanup())]; + const results = await waitBounded( + Promise.allSettled(cleanups), + deadline, + undefined, + "download trigger cleanup timed out", + ); + for (const result of results) if (result.status === "rejected") throw result.reason; }, }; } diff --git a/apps/extension/src/tools/shared.ts b/apps/extension/src/tools/shared.ts index da6b2472..8e2f819c 100644 --- a/apps/extension/src/tools/shared.ts +++ b/apps/extension/src/tools/shared.ts @@ -6,7 +6,7 @@ // exactly the same sandbox + visibility rules as the M6 observation // handlers (review parity). -import type { CdpDebuggee, DialogCursor } from "@/browser-driver/chromium-cdp"; +import type { CdpDebuggee, CdpDispatchGuard, DialogCursor } from "@/browser-driver/chromium-cdp"; import type { CdpFrameGraph, CdpTarget } from "@/browser-driver/frame-graph"; import { isAgentControlledTab, @@ -58,7 +58,14 @@ export type { DialogCursor }; export interface CdpRunner { send(tabId: number, method: string, params?: object): Promise; sendToTarget?(target: CdpTarget, method: string, params?: object): Promise; - detach?(tabId: number): Promise; + /** Production dispatch boundary; optional for lightweight test runners. */ + sendGuarded?( + target: CdpTarget, + method: string, + params: object | undefined, + guard: CdpDispatchGuard, + ): Promise; + detach?(tabId: number, expectedAttachmentId?: string): Promise; getFrameGraph?(tabId: number): Promise; getAttachmentId?(tabId: number): string | undefined; ensureAttachedToUrl?(tabId: number, expectedUrl: string | undefined): Promise;