diff --git a/src/runtime/webview/ChromeBackend.cpp b/src/runtime/webview/ChromeBackend.cpp index 2bce2e3c961e..26773de43fcb 100644 --- a/src/runtime/webview/ChromeBackend.cpp +++ b/src/runtime/webview/ChromeBackend.cpp @@ -705,6 +705,19 @@ void Transport::handleResponse(uint32_t id, std::span result, std::s auto entry = WTF::move(it->value); m_pending.remove(it); + if (entry.method == Method::TargetCreateTargetOrphaned) { + // The view is gone; all that's left is closing the tab Chrome just + // made. An error reply has no result and so no targetId: nothing + // was made. Fire-and-forget like Ops::close. This entry may have + // been the last thing holding the keep-alive ref (and, in WebSocket + // mode, the user's Chrome session), so re-evaluate now it's gone. + auto tid = jsonString(jsonField(result, { "targetId", 8 })); + if (!tid.empty()) + send(0, Command(nextId(), "Target.closeTarget"_s).str("targetId"_s, WTF::String::fromUTF8(tid))); + updateKeepAlive(); + return; + } + auto* g = m_global; auto& vm = g->vm(); JSWebView* view = viewFor(entry.viewId); @@ -777,10 +790,13 @@ void Transport::handleResponse(uint32_t id, std::span result, std::s } case Method::RuntimeEnable: case Method::TargetCloseTarget: + case Method::TargetCreateTargetOrphaned: // Untracked fire-and-forget — close() sends TargetCloseTarget // without adding to m_pending (the view is going away). Chrome's // reply finds no entry, handleResponse's find()==end() drops it. - // This case arm is unreachable; present for switch completeness. + // TargetCreateTargetOrphaned is handled before the view lookup + // above. These arms are unreachable; present for switch + // completeness. return; case Method::PageNavigate: { @@ -1718,8 +1734,21 @@ void close(JSWebView* view) // PageEnable sends Page.navigate, the tab navigates after dispose. // removeIf breaks the chain at the next reply — handleResponse's // find(id)==end() early-return drops it. - t.m_pending.removeIf([vid = view->m_viewId](auto& pair) { - return pair.value.viewId == vid; + // + // Exception: a Target.createTarget Chrome has already received. The + // tab is being created whether or not we listen, and m_targetId is + // only learned from its reply, so dropping the entry would leak the + // tab for the life of the browser. Retag it and let handleResponse + // close the tab; until then the entry also keeps the keep-alive ref. + // One still parked behind the WebSocket handshake is dropped like the + // rest: the drain skips it and no tab is made. + t.m_pending.removeIf([&t, vid = view->m_viewId](auto& pair) { + if (pair.value.viewId != vid) return false; + if (pair.value.method == Method::TargetCreateTarget && !t.isQueuedUnsent(pair.key)) { + pair.value.method = Method::TargetCreateTargetOrphaned; + return false; + } + return true; }); // Target.closeTarget — fire-and-forget. targetId is stashed at // TargetCreateTarget's reply (before sessionId) so it's populated diff --git a/src/runtime/webview/ChromeBackend.h b/src/runtime/webview/ChromeBackend.h index 9f13970fb939..fd4cc10324b1 100644 --- a/src/runtime/webview/ChromeBackend.h +++ b/src/runtime/webview/ChromeBackend.h @@ -235,6 +235,11 @@ class Command { enum class Method : uint8_t { // Internal attach chain — responses chain into the next command. TargetCreateTarget, + // A TargetCreateTarget whose view was close()d before Chrome replied + // (Ops::close retags it). Its reply is the only place the new tab's + // targetId appears, so the handler closes that tab and nothing else: + // there is no view and no slot behind it anymore. + TargetCreateTargetOrphaned, TargetAttachToTarget, PageEnable, RuntimeEnable, @@ -489,6 +494,16 @@ class Transport { return it->value.get(); } + // True while the command is still parked behind the WebSocket + // handshake: Chrome hasn't seen it, so erasing its m_pending entry + // cancels it (the wsOnOpen drain and the wsOnClose replay skip it). + // Anything else (pipe mode, or WebSocket mode once open) is on the + // wire and will be answered. + bool isQueuedUnsent(uint32_t cdpId) const + { + return m_wsPending.containsIf([cdpId](auto& cmd) { return cmd.id == cdpId; }); + } + // Register a fresh view. Returns its viewId and stores one Weak. // Called from JSWebView::createChrome after the transport is up. uint32_t registerView(JSWebView*); diff --git a/test/js/bun/webview/webview-chrome-ws-close-fixture.ts b/test/js/bun/webview/webview-chrome-ws-close-fixture.ts new file mode 100644 index 000000000000..8320add0d9fc --- /dev/null +++ b/test/js/bun/webview/webview-chrome-ws-close-fixture.ts @@ -0,0 +1,63 @@ +// Child of "close() over the WebSocket transport" in webview-chrome.test.ts. +// Runs in its own process because the CDP transport is a per-process +// singleton locked to pipe mode by the rest of that file. argv[2] is the +// ws:// browser endpoint of a Chrome the parent spawned; the parent watches +// tab creation on its own connection and checks this process's stdout. +const backend = { type: "chrome", url: process.argv[2] } as const; + +// close() before the handshake completes: Target.createTarget is still +// queued inside the transport and must be cancelled, never sent. The +// parent sees that as "no tab created" for this step. Closing the only +// view also drops the connection; the next view reconnects. +const early = new Bun.WebView({ backend, width: 100, height: 100 }); +const earlyNav = early.navigate("data:text/html,early"); +early.close(); +console.log( + "early:", + await earlyNav.then( + () => "resolved", + (e: Error) => e.message, + ), +); + +const probe = new Bun.WebView({ backend, width: 100, height: 100 }); +await probe.navigate("data:text/html,probe"); +await probe.cdp("Target.setDiscoverTargets", { discover: true }); +const page = Promise.withResolvers(); +const destroyed = new Map>(); +const destroyedEntry = (targetId: string) => { + let entry = destroyed.get(targetId); + if (!entry) destroyed.set(targetId, (entry = Promise.withResolvers())); + return entry; +}; +probe.addEventListener<{ targetInfo: { targetId: string; type: string } }>("Target.targetCreated", e => { + if (e.data.targetInfo.type === "page") page.resolve(e.data.targetInfo.targetId); +}); +probe.addEventListener<{ targetId: string }>("Target.targetDestroyed", e => destroyedEntry(e.data.targetId).resolve()); + +// close() after the handshake, while Target.createTarget is in flight: the +// tab gets created and the late reply has to be turned into +// Target.closeTarget. +const late = new Bun.WebView({ backend, width: 100, height: 100 }); +const lateNav = late.navigate("data:text/html,late"); +late.close(); +console.log( + "late:", + await lateNav.then( + () => "resolved", + (e: Error) => e.message, + ), +); +const tab = await page.promise; +console.log("late: tab created"); +// A leaked tab produces no event at all, so a bounded wait is the only way +// to report it and exit instead of hanging here (and leaving the parent to +// time out with this process and its Chrome still running). +const leakReport = setTimeout(() => { + console.log("late: tab still open"); + process.exit(1); +}, 2000); +await destroyedEntry(tab).promise; +clearTimeout(leakReport); +console.log("late: tab closed"); +probe.close(); diff --git a/test/js/bun/webview/webview-chrome.test.ts b/test/js/bun/webview/webview-chrome.test.ts index 2ae750fa29f4..5fdb3cd5de0a 100644 --- a/test/js/bun/webview/webview-chrome.test.ts +++ b/test/js/bun/webview/webview-chrome.test.ts @@ -1,5 +1,5 @@ import { expect, test } from "bun:test"; -import { bunEnv, bunExe, isCI, isMacOS, isMacOSVersionAtLeast } from "harness"; +import { bunEnv, bunExe, isCI, isMacOS, isMacOSVersionAtLeast, tempDir } from "harness"; // Chrome backend works on any platform with Chrome/Chromium installed. // Mark tests todo if no Chrome found (CI may not have it). Mirrors @@ -7,7 +7,7 @@ import { bunEnv, bunExe, isCI, isMacOS, isMacOSVersionAtLeast } from "harness"; // paths, then Playwright cache — so the test detects Chrome whenever the // runtime would. import { dlopen, FFIType, ptr } from "bun:ffi"; -import { accessSync, constants as fsConstants, readdirSync, rmSync } from "node:fs"; +import { accessSync, constants as fsConstants, readdirSync, readFileSync, rmSync } from "node:fs"; import { homedir } from "node:os"; import { join } from "node:path"; @@ -789,14 +789,15 @@ it("chrome: sequential navigates work", async () => { expect(await view.evaluate("document.body.textContent")).toBe("C"); }); -it("chrome: close() during attach chain doesn't leak the tab", async () => { +it("chrome: close() during attach chain doesn't continue the navigation", async () => { // close() settles all slots and prunes m_pending entries for the view. // If the attach chain (createTarget → attach → Page.enable → navigate) - // is in-flight, the next chain reply drops on m_pending.find()==end(). - // Without the prune, the chain would continue: m_sessions.add would - // re-register a closed view, PageEnable would send Page.navigate, the - // tab would navigate and fire Page.frameNavigated → onNavigated on a - // disposed view. + // is in-flight, the next chain reply is consumed only to close the tab + // (createTarget, see the tests below) or dropped on + // m_pending.find()==end(). Without the prune, the chain would continue: + // m_sessions.add would re-register a closed view, PageEnable would send + // Page.navigate, the tab would navigate and fire Page.frameNavigated → + // onNavigated on a disposed view. const navigated: string[] = []; await using view = new Bun.WebView({ backend: chrome, width: 100, height: 100 }); view.onNavigated = (u: string) => navigated.push(u); @@ -811,6 +812,206 @@ it("chrome: close() during attach chain doesn't leak the tab", async () => { expect(navigated).toEqual([]); }); +// Browser-level Target.* events carry no sessionId and the transport routes +// those nowhere, so tabs coming and going are observed through a second +// view's page session: Target.setDiscoverTargets there makes Chrome deliver +// targetCreated/targetDestroyed on that session, which reach the view's +// EventTarget. Chrome replays the pre-existing targets before it replies to +// setDiscoverTargets, so listeners attached after the await only see tabs +// created from then on. +async function watchPageTargets(probe: Bun.WebView) { + await probe.cdp("Target.setDiscoverTargets", { discover: true }); + const firstPage = Promise.withResolvers(); + const destroyed = new Map>(); + const destroyedEntry = (targetId: string) => { + let entry = destroyed.get(targetId); + if (!entry) destroyed.set(targetId, (entry = Promise.withResolvers())); + return entry; + }; + probe.addEventListener<{ targetInfo: { targetId: string; type: string } }>("Target.targetCreated", e => { + const { targetId, type } = e.data.targetInfo; + if (type === "page") firstPage.resolve(targetId); + }); + probe.addEventListener<{ targetId: string }>("Target.targetDestroyed", e => + destroyedEntry(e.data.targetId).resolve(), + ); + return { + /** targetId of the first page target created after watching started. */ + page: firstPage.promise, + destroyed: (targetId: string) => destroyedEntry(targetId).promise, + }; +} + +async function pageTargetIds(view: Bun.WebView): Promise { + const { targetInfos } = await view.cdp<{ targetInfos: { targetId: string; type: string }[] }>("Target.getTargets"); + return targetInfos.filter(t => t.type === "page").map(t => t.targetId); +} + +// Serial on purpose: "the first page created after watching started" only +// identifies this test's tab if no other test is creating tabs meanwhile. +it("chrome: close() while Target.createTarget is in flight closes the tab it creates", async () => { + await using probe = new Bun.WebView({ backend: chrome, width: 100, height: 100 }); + await probe.navigate(html("probe")); + const targets = await watchPageTargets(probe); + const pagesBefore = await pageTargetIds(probe); + + const view = new Bun.WebView({ backend: chrome, width: 100, height: 100 }); + const navP = view.navigate(html("orphan")); + // Target.createTarget has been written to Chrome; its reply, the only + // message that ever carries the new tab's targetId, has not come back. + view.close(); + await expect(navP).rejects.toThrow("WebView closed"); + + // Chrome creates the tab either way. close() has to feed the late + // reply's targetId into Target.closeTarget; when it dropped the reply + // instead, the tab outlived every view in the process and this second + // await never settled. + const tab = await targets.page; + await targets.destroyed(tab); + const pagesAfter = await pageTargetIds(probe); + expect(pagesAfter.filter(id => !pagesBefore.includes(id))).toEqual([]); +}); + +it("chrome: close() after the attach chain completed closes the tab", async () => { + // The other side of the same branch in close(): the createTarget reply + // already arrived, so m_targetId is known and Target.closeTarget goes + // out from close() itself. + await using probe = new Bun.WebView({ backend: chrome, width: 100, height: 100 }); + await probe.navigate(html("probe")); + const targets = await watchPageTargets(probe); + + const view = new Bun.WebView({ backend: chrome, width: 100, height: 100 }); + await view.navigate(html("tab")); + const tab = await targets.page; + view.close(); + await targets.destroyed(tab); + expect(await pageTargetIds(probe)).not.toContain(tab); +}); + +it("chrome: process exits on its own after close() with Target.createTarget in flight", async () => { + // The entry close() leaves behind for the in-flight createTarget holds + // the event loop open until Chrome replies (Target.closeTarget has to go + // out), and has to let go once it has. Subprocess-isolated so "the loop + // drains" is observable as the process exiting. The view is unreachable + // by the time the reply arrives (Bun.gc lets it be collected first); the + // cleanup must not depend on it. + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + function closeDuringCreateTarget() { + const view = new Bun.WebView({ backend: { type: "chrome", url: false }, width: 100, height: 100 }); + view.navigate("data:text/html,orphan").catch(e => console.log(e.message)); + view.close(); + } + closeDuringCreateTarget(); + Bun.gc(true); + `, + ], + env: bunEnv, + stderr: "pipe", + }); + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, exitCode }).toEqual({ stdout: "WebView closed\n", exitCode: 0 }); +}); + +it("chrome: close() over the WebSocket transport cancels a queued createTarget and closes an in-flight one", async () => { + // Connect mode differs from pipe mode in one way that matters to close(): + // until the handshake completes, commands sit in a queue inside the + // transport, so a createTarget close()d at that point can still be + // cancelled outright, while one close()d after the handshake is on the + // wire like in pipe mode. Spawn a Chrome with a DevTools port for the + // fixture to connect to (this process's transport is locked to pipe + // mode) and count tab creations on a separate connection: exactly two + // pages, the fixture's probe and the in-flight one, and the latter gets + // destroyed again. + using profile = tempDir("webview-ws-close", {}); + await using chromeProc = Bun.spawn({ + cmd: [ + chromePath!, + `--user-data-dir=${profile}`, + "--remote-debugging-port=0", + "--headless", + "--no-first-run", + "--no-default-browser-check", + "--disable-gpu", + "--no-startup-window", + ], + stdin: "ignore", + stdout: "ignore", + stderr: "ignore", + killSignal: "SIGKILL", + }); + // Chrome writes "\n" once the server is up. + let wsUrl: string | undefined; + while (!wsUrl) { + if (chromeProc.exitCode !== null || chromeProc.signalCode !== null) + throw new Error(`Chrome exited (${chromeProc.exitCode ?? chromeProc.signalCode}) before listening`); + try { + const [port, path] = readFileSync(join(String(profile), "DevToolsActivePort"), "utf8") + .trim() + .split("\n"); + if (port && path) wsUrl = `ws://127.0.0.1:${port}${path}`; + } catch { + await Bun.sleep(10); + } + } + + const ws = new WebSocket(wsUrl); + try { + const opened = Promise.withResolvers(); + const lost = Promise.withResolvers(); + ws.onopen = () => opened.resolve(); + ws.onerror = () => lost.reject(new Error("connection to Chrome failed")); + ws.onclose = () => lost.reject(new Error("connection to Chrome closed")); + await Promise.race([opened.promise, lost.promise]); + + const createdPages: string[] = []; + const destroyed: string[] = []; + const replies = new Map void>(); + let watching = false; + ws.onmessage = ({ data }) => { + const { id, method, params } = JSON.parse(String(data)); + if (id) replies.get(id)?.(); + // Events before setDiscoverTargets replies describe what already existed. + if (!watching) return; + if (method === "Target.targetCreated" && params.targetInfo.type === "page") + createdPages.push(params.targetInfo.targetId); + if (method === "Target.targetDestroyed") destroyed.push(params.targetId); + }; + const roundTrip = (id: number, method: string, params = {}) => { + const reply = Promise.withResolvers(); + replies.set(id, reply.resolve); + ws.send(JSON.stringify({ id, method, params })); + return Promise.race([reply.promise, lost.promise]); + }; + await roundTrip(1, "Target.setDiscoverTargets", { discover: true }); + watching = true; + + await using proc = Bun.spawn({ + cmd: [bunExe(), join(import.meta.dir, "webview-chrome-ws-close-fixture.ts"), wsUrl], + env: bunEnv, + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: ["early: WebView closed", "late: WebView closed", "late: tab created", "late: tab closed", ""].join("\n"), + stderr: "", + exitCode: 0, + }); + // The fixture saw its tab destroyed before exiting, so Chrome has + // notified this connection too; one round trip drains anything still + // in flight before reading the counters. + await roundTrip(2, "Target.getTargets"); + expect(createdPages).toHaveLength(2); + expect(destroyed).toContain(createdPages[1]); + } finally { + ws.onerror = ws.onclose = null; + ws.close(); + } +}); + it("chrome: url/title getters populated after navigate", async () => { await using view = new Bun.WebView({ backend: chrome, width: 200, height: 200 }); await view.navigate(html("Page Titlehi"));