From 74052575fb983a2ecfdca2fe9687822504052973 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Thu, 27 Aug 2026 11:57:36 -0700 Subject: [PATCH] fix(browser): close the created tab when opened-tab discovery is aborted (#131125) * fix(browser): close the created tab when opened-tab discovery is aborted * fix(browser): complete cancelled tab creation cleanup Close newly created CDP targets on their creating socket when cancellation prevents returning the target ID. Preserve the original error even when close fails. Capture cleanup policy and timeout before asynchronous work so runtime shutdown cannot fence compensation or mask the open error. Extend the existing cancellation tests through the real profile lifecycle. --- extensions/browser/src/browser/cdp.test.ts | 90 +++-- extensions/browser/src/browser/cdp.ts | 25 +- ...r-context.tab-discovery-poll-abort.test.ts | 95 +++-- .../src/browser/server-context.tab-ops.ts | 372 +++++++++--------- .../server-context.tab-ownership.test.ts | 138 +++---- ...server-context.tab-selection-state.test.ts | 5 +- 6 files changed, 395 insertions(+), 330 deletions(-) diff --git a/extensions/browser/src/browser/cdp.test.ts b/extensions/browser/src/browser/cdp.test.ts index 71314f5d608c..e98d7e926510 100644 --- a/extensions/browser/src/browser/cdp.test.ts +++ b/extensions/browser/src/browser/cdp.test.ts @@ -318,41 +318,65 @@ describe("cdp", () => { expect(methods).not.toContain("Runtime.evaluate"); }); - it("preserves caller cancellation after target creation while navigation is settling", async () => { - const controller = new AbortController(); - const reason = new Error("cancel after target creation"); - const wsPort = await startWsServerWithMessages((msg, socket) => { - if (msg.method === "Target.createTarget") { - socket.send(JSON.stringify({ id: msg.id, result: { targetId: "TARGET_CANCEL" } })); - return; - } - if (msg.method === "Page.getFrameTree") { - controller.abort(reason); - socket.send( - JSON.stringify({ - id: msg.id, - result: { - frameTree: { - frame: { loaderId: "LOADER_CANCEL", url: "https://example.com" }, + it.each([ + { abortAt: "creation", closeFails: false }, + { abortAt: "navigation", closeFails: false }, + { abortAt: "navigation", closeFails: true }, + ])( + "closes an unreturned target after $abortAt abort (close fails: $closeFails)", + async ({ abortAt, closeFails }) => { + const controller = new AbortController(); + const reason = new Error("cancel after target creation"); + const closedTargets: unknown[] = []; + const wsPort = await startWsServerWithMessages((msg, socket) => { + if (msg.method === "Target.createTarget") { + if (abortAt === "creation") { + controller.abort(reason); + } + socket.send(JSON.stringify({ id: msg.id, result: { targetId: "TARGET_CANCEL" } })); + return; + } + if (msg.method === "Target.closeTarget") { + closedTargets.push(msg.params?.targetId); + socket.send( + JSON.stringify({ + id: msg.id, + ...(closeFails + ? { error: { message: "close failed" } } + : { result: { success: true } }), + }), + ); + return; + } + if (msg.method === "Page.getFrameTree") { + controller.abort(reason); + socket.send( + JSON.stringify({ + id: msg.id, + result: { + frameTree: { + frame: { loaderId: "LOADER_CANCEL", url: "https://example.com" }, + }, }, - }, - }), - ); - } - }); - const httpPort = await startVersionHttpServer({ - webSocketDebuggerUrl: `ws://127.0.0.1:${wsPort}/devtools/browser/TEST`, - }); + }), + ); + } + }); + const httpPort = await startVersionHttpServer({ + webSocketDebuggerUrl: `ws://127.0.0.1:${wsPort}/devtools/browser/TEST`, + }); - await expect( - createTargetViaCdp({ - cdpUrl: `http://127.0.0.1:${httpPort}`, - url: "https://example.com", - signal: controller.signal, - waitForNavigationResult: true, - }), - ).rejects.toBe(reason); - }); + await expect( + createTargetViaCdp({ + cdpUrl: `http://127.0.0.1:${httpPort}`, + url: "https://example.com", + signal: controller.signal, + waitForNavigationResult: true, + }), + ).rejects.toBe(reason); + expect(closedTargets).toEqual(["TARGET_CANCEL"]); + }, + ); it("cancels hanging endpoint discovery without creating a target", async () => { const controller = new AbortController(); diff --git a/extensions/browser/src/browser/cdp.ts b/extensions/browser/src/browser/cdp.ts index 331d1e08badf..f1db064980c4 100644 --- a/extensions/browser/src/browser/cdp.ts +++ b/extensions/browser/src/browser/cdp.ts @@ -300,15 +300,22 @@ export async function createTargetViaCdp(opts: { if (!targetId) { throw new Error("CDP Target.createTarget returned no targetId"); } - opts.signal?.throwIfAborted(); - const finalUrl = await prepareCdpTargetSession( - send, - targetId, - opts.waitForNavigationResult ? opts.url : undefined, - opts.signal, - ); - opts.signal?.throwIfAborted(); - return finalUrl ? { targetId, finalUrl } : { targetId }; + try { + opts.signal?.throwIfAborted(); + const finalUrl = await prepareCdpTargetSession( + send, + targetId, + opts.waitForNavigationResult ? opts.url : undefined, + opts.signal, + ); + opts.signal?.throwIfAborted(); + return finalUrl ? { targetId, finalUrl } : { targetId }; + } catch (error) { + // The caller cannot compensate until it receives this id. Keep cleanup + // on the creating socket, independent of cancellation, before releasing it. + await send("Target.closeTarget", { targetId }).catch(() => {}); + throw error; + } }, { commandTimeoutMs: opts.timeouts?.httpTimeoutMs ?? 5000, diff --git a/extensions/browser/src/browser/server-context.tab-discovery-poll-abort.test.ts b/extensions/browser/src/browser/server-context.tab-discovery-poll-abort.test.ts index e967d0575966..25d0a1a450ef 100644 --- a/extensions/browser/src/browser/server-context.tab-discovery-poll-abort.test.ts +++ b/extensions/browser/src/browser/server-context.tab-discovery-poll-abort.test.ts @@ -4,11 +4,9 @@ import "../test-support/browser-security.mock.js"; import "./server-context.chrome-test-harness.js"; import * as cdpModule from "./cdp.js"; import { OPEN_TAB_DISCOVERY_POLL_MS } from "./server-context.constants.js"; -import { - createTestBrowserRouteContext, - makeState, - originalFetch, -} from "./server-context.remote-tab-ops.harness.js"; +import { createBrowserRouteContext } from "./server-context.js"; +import { beginProfileTransition, markBrowserRuntimeStopping } from "./server-context.lifecycle.js"; +import { makeState, originalFetch } from "./server-context.remote-tab-ops.harness.js"; import { createProfileSelectionOps } from "./server-context.selection.js"; import type { ProfileRuntimeState } from "./server-context.types.js"; @@ -116,35 +114,64 @@ describe("browser tab discovery poll abort", () => { expect(vi.getTimerCount()).toBe(0); }); - it("cancels the opened-target discovery timer", async () => { - vi.useFakeTimers(); - const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout"); - vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ - targetId: "PENDING", - finalUrl: "about:blank", - }); - const fetchMock = vi.fn(async (url: unknown) => { - if (!String(url).includes("/json/list")) { - throw new Error(`unexpected fetch: ${String(url)}`); + it.each([ + { closeFails: false, shutdown: false }, + { closeFails: true, shutdown: false }, + { closeFails: false, shutdown: true }, + ])( + "cancels opened-target discovery and closes the target (close fails: $closeFails, shutdown: $shutdown)", + async ({ closeFails, shutdown }) => { + vi.useFakeTimers(); + const setTimeoutSpy = vi.spyOn(globalThis, "setTimeout"); + vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ + targetId: "PENDING", + finalUrl: "about:blank", + }); + const closeRequests: string[] = []; + const closeSignalsAborted: Array = []; + const fetchMock = vi.fn(async (url: unknown, init?: RequestInit) => { + if (String(url).includes("/json/close/")) { + closeRequests.push(String(url)); + closeSignalsAborted.push(init?.signal?.aborted); + if (closeFails) { + throw new Error("close request failed"); + } + return { ok: true } as Response; + } + if (!String(url).includes("/json/list")) { + throw new Error(`unexpected fetch: ${String(url)}`); + } + return { ok: true, json: async () => [] } as unknown as Response; + }); + globalThis.fetch = withBrowserFetchPreconnect(fetchMock); + const state = makeState("openclaw"); + const openclaw = createBrowserRouteContext({ getState: () => state }).forProfile("openclaw"); + const controller = new AbortController(); + const openPromise = openclaw.openTab("about:blank", { signal: controller.signal }); + + await vi.advanceTimersByTimeAsync(0); + expect(setTimeoutSpy.mock.calls.some((call) => call[1] === OPEN_TAB_DISCOVERY_POLL_MS)).toBe( + true, + ); + expect(vi.getTimerCount()).toBe(1); + let stopping: Promise | undefined; + if (shutdown) { + markBrowserRuntimeStopping(state); + stopping = beginProfileTransition({ + state, + runtime: state.profiles.get("openclaw")!, + reason: "runtime shutdown", + closeSharedAdapters: false, + }); + } else { + controller.abort(); } - return { ok: true, json: async () => [] } as unknown as Response; - }); - globalThis.fetch = withBrowserFetchPreconnect(fetchMock); - const state = makeState("openclaw"); - const openclaw = createTestBrowserRouteContext({ getState: () => state }).forProfile( - "openclaw", - ); - const controller = new AbortController(); - const openPromise = openclaw.openTab("about:blank", { signal: controller.signal }); - await vi.advanceTimersByTimeAsync(0); - expect(setTimeoutSpy.mock.calls.some((call) => call[1] === OPEN_TAB_DISCOVERY_POLL_MS)).toBe( - true, - ); - expect(vi.getTimerCount()).toBe(1); - controller.abort(); - - await expect(openPromise).rejects.toThrow(/aborted/i); - expect(vi.getTimerCount()).toBe(0); - }); + await expect(openPromise).rejects.toMatchObject({ name: "AbortError", message: "aborted" }); + expect(closeRequests).toEqual(["http://127.0.0.1:18800/json/close/PENDING"]); + expect(closeSignalsAborted).toEqual([false]); + await stopping; + expect(vi.getTimerCount()).toBe(0); + }, + ); }); diff --git a/extensions/browser/src/browser/server-context.tab-ops.ts b/extensions/browser/src/browser/server-context.tab-ops.ts index 04eb02f5693f..f6f77b9dad19 100644 --- a/extensions/browser/src/browser/server-context.tab-ops.ts +++ b/extensions/browser/src/browser/server-context.tab-ops.ts @@ -252,39 +252,16 @@ export function createProfileTabOps({ profile, state, runtime }: TabOpsDeps): Pr options?: BrowserOperationOptions, ): Promise => { const cdpTimeouts = getRemoteCdpActionTimeouts(); - let ownership: BrowserOpenResult["ownership"]; - try { - ownership = await resolveCdpTabOwnership({ + return { + ...tab, + ownership: await resolveCdpTabOwnership({ profileName: profile.name, cdpUrl: profile.cdpUrl, nativeTargetId: tab.targetId, signal: options?.signal, timeoutMs: cdpTimeouts?.httpTimeoutMs, ssrfPolicy: getCdpControlPolicy(), - }); - } catch (ownershipError) { - try { - // Ownership probing happens after target creation. Cleanup must not - // inherit a caller abort that would strand the new untracked page. - await fetchOk( - appendCdpPath(cdpHttpBase, `/json/close/${encodeURIComponent(tab.targetId)}`), - state().resolved.remoteCdpTimeoutMs, - undefined, - getCdpControlPolicy(), - ); - } catch (closeError) { - throw Object.assign( - new Error("Failed to resolve browser tab ownership and close the new target", { - cause: ownershipError, - }), - { errors: [ownershipError, closeError] }, - ); - } - throw ownershipError; - } - return { - ...tab, - ownership, + }), }; }; @@ -295,10 +272,12 @@ export function createProfileTabOps({ profile, state, runtime }: TabOpsDeps): Pr opts?.signal?.throwIfAborted(); const normalizedLabel = opts?.label === undefined ? undefined : normalizeTabLabel(opts.label); const ssrfPolicyOpts = getNavigationPolicy(); + const cdpPolicy = getCdpControlPolicy(); + // Runtime shutdown fences state() before draining this operation's cleanup. + const cleanupTimeoutMs = state().resolved.remoteCdpTimeoutMs; if (capabilities.usesChromeMcp) { await assertBrowserNavigationAllowed({ url, ...ssrfPolicyOpts }); - const cdpPolicy = getCdpControlPolicy(); assertChromeMcpCdpTransportAllowed(profile, cdpPolicy); const { openChromeMcpTab } = await getChromeMcpModule(); const cdpTimeouts = getRemoteCdpActionTimeouts(); @@ -312,185 +291,204 @@ export function createProfileTabOps({ profile, state, runtime }: TabOpsDeps): Pr return adoptValidatedTab(page, { ...opts, label: normalizedLabel }); } - if (capabilities.usesPersistentPlaywright) { - const mod = await getPwAiModule({ mode: "strict" }); - const createPageViaPlaywright = (mod as Partial | null)?.createPageViaPlaywright; - if (typeof createPageViaPlaywright === "function") { - const page = await createPageViaPlaywright({ - cdpUrl: profile.cdpUrl, - url, - cdpPolicy: getCdpControlPolicy(), - ...ssrfPolicyOpts, - }); - return adoptValidatedTab( - await withTabOwnership( - { - targetId: page.targetId, - title: page.title, - url: page.url, - type: page.type, - }, - opts, - ), - { ...opts, label: normalizedLabel }, + let createdTargetId: string | undefined; + try { + if (capabilities.usesPersistentPlaywright) { + const mod = await getPwAiModule({ mode: "strict" }); + const createPageViaPlaywright = (mod as Partial | null) + ?.createPageViaPlaywright; + if (typeof createPageViaPlaywright === "function") { + const page = await createPageViaPlaywright({ + cdpUrl: profile.cdpUrl, + url, + cdpPolicy, + ...ssrfPolicyOpts, + }); + createdTargetId = page.targetId; + return adoptValidatedTab( + await withTabOwnership( + { + targetId: page.targetId, + title: page.title, + url: page.url, + type: page.type, + }, + opts, + ), + { ...opts, label: normalizedLabel }, + ); + } + } + + if (requiresInspectableBrowserNavigationRedirectsForUrl(url, state().resolved.ssrfPolicy)) { + throw new InvalidBrowserNavigationUrlError( + "Navigation blocked: strict browser SSRF policy requires Playwright-backed redirect-hop inspection", ); } - } - if (requiresInspectableBrowserNavigationRedirectsForUrl(url, state().resolved.ssrfPolicy)) { - throw new InvalidBrowserNavigationUrlError( - "Navigation blocked: strict browser SSRF policy requires Playwright-backed redirect-hop inspection", - ); - } + await assertBrowserNavigationAllowed({ url, ...ssrfPolicyOpts }); + const cdpActionTimeouts = getRemoteCdpActionTimeouts(); + const createTargetOpts: Parameters[0] = { + cdpUrl: profile.cdpUrl, + url, + ssrfPolicy: cdpPolicy, + waitForNavigationResult: true, + }; + if (cdpActionTimeouts) { + createTargetOpts.timeouts = cdpActionTimeouts; + } + if (opts?.signal) { + createTargetOpts.signal = opts.signal; + } + const createdViaCdp = await createTargetViaCdp(createTargetOpts).catch(() => null); + createdTargetId = createdViaCdp?.targetId; + opts?.signal?.throwIfAborted(); - await assertBrowserNavigationAllowed({ url, ...ssrfPolicyOpts }); - const cdpActionTimeouts = getRemoteCdpActionTimeouts(); - const createTargetOpts: Parameters[0] = { - cdpUrl: profile.cdpUrl, - url, - ssrfPolicy: getCdpControlPolicy(), - waitForNavigationResult: true, - }; - if (cdpActionTimeouts) { - createTargetOpts.timeouts = cdpActionTimeouts; - } - if (opts?.signal) { - createTargetOpts.signal = opts.signal; - } - const createdViaCdp = await createTargetViaCdp(createTargetOpts).catch(() => null); - opts?.signal?.throwIfAborted(); - - if (createdViaCdp) { - if (!createdViaCdp.finalUrl) { - // The target exists, but its committed document is not authoritative. - // Preserve the explicit result without sticky, alias, or cleanup adoption. + if (createdViaCdp) { + if (!createdViaCdp.finalUrl) { + // The target exists, but its committed document is not authoritative. + // Preserve the explicit result without sticky, alias, or cleanup adoption. + return await withTabOwnership( + { + targetId: createdViaCdp.targetId, + title: "", + url, + type: "page", + }, + opts, + ); + } + await assertBrowserNavigationResultAllowed({ + url: createdViaCdp.finalUrl, + ...ssrfPolicyOpts, + }); + const deadline = Date.now() + OPEN_TAB_DISCOVERY_WINDOW_MS; + while (Date.now() < deadline) { + opts?.signal?.throwIfAborted(); + const tabs = await readTabs(opts).catch(() => [] as BrowserTab[]); + const found = tabs.find((t) => t.targetId === createdViaCdp.targetId); + if (found) { + await assertBrowserNavigationResultAllowed({ url: found.url, ...ssrfPolicyOpts }); + // The attached target owns the committed URL; /json/list supplies the + // remaining metadata and may briefly lag that exact document snapshot. + return adoptValidatedTab( + await withTabOwnership({ ...found, url: createdViaCdp.finalUrl }, opts), + { ...opts, label: normalizedLabel }, + ); + } + await sleepWithAbort(OPEN_TAB_DISCOVERY_POLL_MS, opts?.signal); + } + opts?.signal?.throwIfAborted(); + // Preserve the explicit target-id result for callers, but do not adopt an + // undiscovered target into sticky, alias, or managed-cleanup state. return await withTabOwnership( { targetId: createdViaCdp.targetId, title: "", - url, + url: createdViaCdp.finalUrl, type: "page", }, opts, ); } - await assertBrowserNavigationResultAllowed({ - url: createdViaCdp.finalUrl, - ...ssrfPolicyOpts, - }); - const deadline = Date.now() + OPEN_TAB_DISCOVERY_WINDOW_MS; - while (Date.now() < deadline) { - opts?.signal?.throwIfAborted(); - const tabs = await readTabs(opts).catch(() => [] as BrowserTab[]); - const found = tabs.find((t) => t.targetId === createdViaCdp.targetId); - if (found) { - await assertBrowserNavigationResultAllowed({ url: found.url, ...ssrfPolicyOpts }); - // The attached target owns the committed URL; /json/list supplies the - // remaining metadata and may briefly lag that exact document snapshot. - return adoptValidatedTab( - await withTabOwnership({ ...found, url: createdViaCdp.finalUrl }, opts), - { ...opts, label: normalizedLabel }, + + const encoded = encodeURIComponent(url); + const endpointUrl = new URL(appendCdpPath(cdpHttpBase, "/json/new")); + const endpoint = endpointUrl.search + ? (() => { + endpointUrl.searchParams.set("url", url); + return endpointUrl.toString(); + })() + : `${endpointUrl.toString()}?${encoded}`; + opts?.signal?.throwIfAborted(); + const created = await fetchJson( + endpoint, + cdpActionTimeouts?.httpTimeoutMs ?? CDP_JSON_NEW_TIMEOUT_MS, + { + method: "PUT", + }, + getCdpControlPolicy(), + ).catch(async (err: unknown) => { + if (String(err).includes("HTTP 405")) { + return await fetchJson( + endpoint, + cdpActionTimeouts?.httpTimeoutMs ?? CDP_JSON_NEW_TIMEOUT_MS, + undefined, + getCdpControlPolicy(), ); } - await sleepWithAbort(OPEN_TAB_DISCOVERY_POLL_MS, opts?.signal); - } - opts?.signal?.throwIfAborted(); - // Preserve the explicit target-id result for callers, but do not adopt an - // undiscovered target into sticky, alias, or managed-cleanup state. - return await withTabOwnership( - { - targetId: createdViaCdp.targetId, - title: "", - url: createdViaCdp.finalUrl, - type: "page", - }, - opts, - ); - } + throw err; + }); - const encoded = encodeURIComponent(url); - const endpointUrl = new URL(appendCdpPath(cdpHttpBase, "/json/new")); - const endpoint = endpointUrl.search - ? (() => { - endpointUrl.searchParams.set("url", url); - return endpointUrl.toString(); - })() - : `${endpointUrl.toString()}?${encoded}`; - opts?.signal?.throwIfAborted(); - const created = await fetchJson( - endpoint, - cdpActionTimeouts?.httpTimeoutMs ?? CDP_JSON_NEW_TIMEOUT_MS, - { - method: "PUT", - }, - getCdpControlPolicy(), - ).catch(async (err: unknown) => { - if (String(err).includes("HTTP 405")) { - return await fetchJson( - endpoint, - cdpActionTimeouts?.httpTimeoutMs ?? CDP_JSON_NEW_TIMEOUT_MS, - undefined, - getCdpControlPolicy(), + createdTargetId = created.id; + opts?.signal?.throwIfAborted(); + if (!created.id) { + throw new Error("Failed to open tab (missing id)"); + } + const resolvedUrl = created.url ?? url; + if (!isSelectableCdpBrowserTarget({ url: resolvedUrl, type: created.type })) { + throw new Error("Failed to open tab (non-selectable target)"); + } + await assertBrowserNavigationResultAllowed({ url: resolvedUrl, ...ssrfPolicyOpts }); + const wsUrl = normalizeWsUrl(created.webSocketDebuggerUrl, profile.cdpUrl); + const wsPin = wsUrl + ? await assertCdpEndpointAllowed(wsUrl, getCdpControlPolicy(), { + source: "discovered", + configuredUrl: profile.cdpUrl, + }) + : undefined; + const committedUrl = wsUrl + ? await waitForCdpCommittedNavigationUrl({ + wsUrl, + configuredCdpUrl: profile.cdpUrl, + cdpPolicy: getCdpControlPolicy(), + requestedUrl: url, + signal: opts?.signal, + timeouts: cdpActionTimeouts, + }) + : undefined; + opts?.signal?.throwIfAborted(); + if (!committedUrl) { + return await withTabOwnership( + { + targetId: created.id, + title: created.title ?? "", + url: resolvedUrl, + wsUrl, + ...(wsPin?.lookup ? { wsLookup: wsPin.lookup } : {}), + type: created.type, + }, + opts, ); } - throw err; - }); - - opts?.signal?.throwIfAborted(); - if (!created.id) { - throw new Error("Failed to open tab (missing id)"); - } - const resolvedUrl = created.url ?? url; - if (!isSelectableCdpBrowserTarget({ url: resolvedUrl, type: created.type })) { - throw new Error("Failed to open tab (non-selectable target)"); - } - await assertBrowserNavigationResultAllowed({ url: resolvedUrl, ...ssrfPolicyOpts }); - const wsUrl = normalizeWsUrl(created.webSocketDebuggerUrl, profile.cdpUrl); - const wsPin = wsUrl - ? await assertCdpEndpointAllowed(wsUrl, getCdpControlPolicy(), { - source: "discovered", - configuredUrl: profile.cdpUrl, - }) - : undefined; - const committedUrl = wsUrl - ? await waitForCdpCommittedNavigationUrl({ - wsUrl, - configuredCdpUrl: profile.cdpUrl, - cdpPolicy: getCdpControlPolicy(), - requestedUrl: url, - signal: opts?.signal, - timeouts: cdpActionTimeouts, - }) - : undefined; - opts?.signal?.throwIfAborted(); - if (!committedUrl) { - return await withTabOwnership( - { - targetId: created.id, - title: created.title ?? "", - url: resolvedUrl, - wsUrl, - ...(wsPin?.lookup ? { wsLookup: wsPin.lookup } : {}), - type: created.type, - }, - opts, + await assertBrowserNavigationResultAllowed({ url: committedUrl, ...ssrfPolicyOpts }); + return adoptValidatedTab( + await withTabOwnership( + { + targetId: created.id, + title: created.title ?? "", + url: committedUrl, + wsUrl, + ...(wsPin?.lookup ? { wsLookup: wsPin.lookup } : {}), + type: created.type, + }, + opts, + ), + { ...opts, label: normalizedLabel }, ); + } catch (openError) { + if (createdTargetId) { + // Creation owns the target until a successful handoff. Cleanup must not + // inherit the caller's abort or replace the original open failure. + await fetchOk( + appendCdpPath(cdpHttpBase, `/json/close/${encodeURIComponent(createdTargetId)}`), + cleanupTimeoutMs, + undefined, + cdpPolicy, + ).catch(() => {}); + } + throw openError; } - await assertBrowserNavigationResultAllowed({ url: committedUrl, ...ssrfPolicyOpts }); - return adoptValidatedTab( - await withTabOwnership( - { - targetId: created.id, - title: created.title ?? "", - url: committedUrl, - wsUrl, - ...(wsPin?.lookup ? { wsLookup: wsPin.lookup } : {}), - type: created.type, - }, - opts, - ), - { ...opts, label: normalizedLabel }, - ); }; const labelTab = async ( diff --git a/extensions/browser/src/browser/server-context.tab-ownership.test.ts b/extensions/browser/src/browser/server-context.tab-ownership.test.ts index 42fa8694a59c..d134ecac3e34 100644 --- a/extensions/browser/src/browser/server-context.tab-ownership.test.ts +++ b/extensions/browser/src/browser/server-context.tab-ownership.test.ts @@ -15,72 +15,78 @@ afterEach(() => { }); describe("browser tab ownership probes", () => { - it("propagates caller abort through the managed ownership version probe", async () => { - vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ - targetId: "CREATED", - finalUrl: "http://127.0.0.1:8080", - }); - let versionSignal: AbortSignal | undefined; - let closedCreatedTarget = false; - let markProbeStarted!: () => void; - const probeStarted = new Promise((resolve) => { - markProbeStarted = resolve; - }); - const fetchMock = vi.fn(async (url: unknown, init?: RequestInit) => { - const value = String(url); - if (value.includes("/json/list")) { - return { - ok: true, - json: async () => [ - { - id: "CREATED", - title: "New Tab", - url: "http://127.0.0.1:8080", - webSocketDebuggerUrl: "ws://127.0.0.1/devtools/page/CREATED", - type: "page", - }, - ], - } as unknown as Response; - } - if (value.includes("/json/version")) { - versionSignal = init?.signal ?? undefined; - markProbeStarted(); - return await new Promise((_resolve, reject) => { - init?.signal?.addEventListener( - "abort", - () => - reject( - init.signal?.reason instanceof Error - ? init.signal.reason - : new Error("managed ownership probe aborted"), - ), - { once: true }, - ); - }); - } - if (value.includes("/json/close/CREATED")) { - closedCreatedTarget = true; - return { ok: true } as Response; - } - throw new Error(`unexpected fetch: ${value}`); - }); - global.fetch = withBrowserFetchPreconnect(fetchMock); - const state = makeState("openclaw"); - const openclaw = createTestBrowserRouteContext({ getState: () => state }).forProfile( - "openclaw", - ); - const controller = new AbortController(); - const abortError = new Error("caller aborted managed ownership probe"); + it.each([false, true])( + "propagates caller abort through the managed ownership version probe (close fails: %s)", + async (closeFails) => { + vi.spyOn(cdpModule, "createTargetViaCdp").mockResolvedValue({ + targetId: "CREATED", + finalUrl: "http://127.0.0.1:8080", + }); + let versionSignal: AbortSignal | undefined; + const closeRequests: string[] = []; + let markProbeStarted!: () => void; + const probeStarted = new Promise((resolve) => { + markProbeStarted = resolve; + }); + const fetchMock = vi.fn(async (url: unknown, init?: RequestInit) => { + const value = String(url); + if (value.includes("/json/list")) { + return { + ok: true, + json: async () => [ + { + id: "CREATED", + title: "New Tab", + url: "http://127.0.0.1:8080", + webSocketDebuggerUrl: "ws://127.0.0.1/devtools/page/CREATED", + type: "page", + }, + ], + } as unknown as Response; + } + if (value.includes("/json/version")) { + versionSignal = init?.signal ?? undefined; + markProbeStarted(); + return await new Promise((_resolve, reject) => { + init?.signal?.addEventListener( + "abort", + () => + reject( + init.signal?.reason instanceof Error + ? init.signal.reason + : new Error("managed ownership probe aborted"), + ), + { once: true }, + ); + }); + } + if (value.includes("/json/close/CREATED")) { + closeRequests.push(value); + if (closeFails) { + throw new Error("close request failed"); + } + return { ok: true } as Response; + } + throw new Error(`unexpected fetch: ${value}`); + }); + global.fetch = withBrowserFetchPreconnect(fetchMock); + const state = makeState("openclaw"); + const openclaw = createTestBrowserRouteContext({ getState: () => state }).forProfile( + "openclaw", + ); + const controller = new AbortController(); + const abortError = new Error("caller aborted managed ownership probe"); - const opening = openclaw.openTab("http://127.0.0.1:8080", { - signal: controller.signal, - }); - await probeStarted; - controller.abort(abortError); - const propagatedImmediately = versionSignal?.aborted; + const opening = openclaw.openTab("http://127.0.0.1:8080", { + signal: controller.signal, + }); + await probeStarted; + controller.abort(abortError); + const propagatedImmediately = versionSignal?.aborted; - await expect(opening).rejects.toBe(abortError); - expect(propagatedImmediately).toBe(true); - expect(closedCreatedTarget).toBe(true); - }); + await expect(opening).rejects.toBe(abortError); + expect(propagatedImmediately).toBe(true); + expect(closeRequests).toEqual(["http://127.0.0.1:18800/json/close/CREATED"]); + }, + ); }); diff --git a/extensions/browser/src/browser/server-context.tab-selection-state.test.ts b/extensions/browser/src/browser/server-context.tab-selection-state.test.ts index 8608034829f3..795cc60e98be 100644 --- a/extensions/browser/src/browser/server-context.tab-selection-state.test.ts +++ b/extensions/browser/src/browser/server-context.tab-selection-state.test.ts @@ -148,6 +148,7 @@ describe("browser server-context tab selection state", () => { browserInstanceFingerprint: expect.stringMatching(/^sha256:/), }); expect(state.profiles.get("openclaw")?.lastTargetId).toBe("CREATED"); + expect(fetchCallUrls(fetchMock).some((url) => url.includes("/json/close/"))).toBe(false); expect(createTargetViaCdp).toHaveBeenCalledWith({ cdpUrl: "http://127.0.0.1:18800", url: "http://127.0.0.1:8080", @@ -221,7 +222,9 @@ describe("browser server-context tab selection state", () => { expect(profileState?.lastTargetId).not.toBe("BLOCKED"); expect(profileState?.tabAliases).toEqual(aliasesBefore); expect(profileState?.tabAliases?.byTargetId.BLOCKED).toBeUndefined(); - expect(fetchCallUrls(fetchMock).some((url) => url.includes("/json/close/BLOCKED"))).toBe(false); + expect(fetchCallUrls(fetchMock).filter((url) => url.includes("/json/close/"))).toEqual([ + "http://127.0.0.1:18800/json/close/BLOCKED", + ]); await expect(openclaw.ensureTabAvailable()).resolves.toEqual( expect.objectContaining({ targetId: "GOOD" }),