webview: add backend.detached option#29953
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis PR adds a new optional ChangesWebView backend detached option
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/webview/webview-chrome.test.ts`:
- Around line 987-1041: The test should record and assert the session id (sid)
as well as pgid: update the fake-chrome fixture (used by spawnWith) to capture
sid (e.g., run ps -o sid= -p $$ and write "$$,$pgid,$sid" atomically), then
update spawnWith to parse three values (pid, pgid, sid) and return sid; finally
change the detached-case assertion to expect(d.sid).toBe(d.pid) (keep the
existing pgid checks and non-detached behavior). This touches the fake-chrome
script, the spawnWith function, and the detached-case expect lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7197b245-6cd8-4407-af40-6b1c3ad6a060
📒 Files selected for processing (11)
packages/bun-types/bun.d.tssrc/bun.js/webview/ChromeBackend.cppsrc/bun.js/webview/ChromeBackend.hsrc/bun.js/webview/ChromeProcess.zigsrc/bun.js/webview/HostProcess.zigsrc/bun.js/webview/JSWebView.cppsrc/bun.js/webview/JSWebView.hsrc/bun.js/webview/JSWebViewConstructor.cppsrc/bun.js/webview/WebKitBackend.cppsrc/bun.js/webview/WebKitBackend.htest/js/bun/webview/webview-chrome.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/bun/webview/webview-chrome.test.ts`:
- Around line 1033-1038: The test currently asserts exitCode before
validating/parsing stdout which hides stdout errors; update the assertions in
the block using proc.stdout.text(), proc.stderr.text(), and proc.exited so that
you first validate stderr is empty, then parse and assert stdout (variables
stdout and stderr and the parsed pid/pgid matching /^\d+$/), and only after
those stdout-related checks move expect(exitCode).toBe(0) to the end to improve
diagnostics.
- Around line 1016-1023: The polling loop that waits for OUT_FILE can hang
forever; update the snippet around the while (!fs.existsSync(out)) await
Bun.sleep(5); (the code using out/outFile, fs.existsSync, Bun.sleep and
Bun.WebView) to implement a maximum timeout (e.g. start = Date.now(); const
TIMEOUT_MS = 30_000) and break/throw/log a clear error if Date.now() - start
exceeds TIMEOUT_MS instead of spinning; ensure you return a nonzero exit or
throw after logging so the test fails fast and include the timeout duration and
file path in the error message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 51b31136-6b83-459e-93d1-7ee3aef45e18
📒 Files selected for processing (1)
test/js/bun/webview/webview-chrome.test.ts
Status: ready for mergeThe Verification
CI on build #60652 (head ae04d6c) — failures are not from this PR
Needs a maintainer to fix/pin the bunx Angular test on main (or retry once the registry situation settles) and merge; nothing further to change in this PR. |
There was a problem hiding this comment.
LGTM — straightforward plumbing of a boolean through to the existing SpawnOptions.detached; the comment-staleness nit is non-blocking.
Extended reasoning...
Overview
Threads a new backend.detached: boolean option from the Bun.WebView constructor down to bun.spawn.SpawnOptions.detached (which already exists and sets POSIX_SPAWN_SETSID). Touches 11 files across the type defs, JS constructor option parsing, C++ createChrome/createAndSend/Transport::ensureSpawned/ensureConnected/HostClient::ensureSpawned signatures, the C-ABI Bun__Chrome__ensure/Bun__WebViewHost__ensure externs, and the Zig spawn paths. Every change is appending one bool parameter alongside the existing stdoutInherit/stderrInherit pair and forwarding it; default is false so existing behaviour is unchanged. Also stashes m_fallbackDetached for the auto-detect → WebSocket → spawn fallback, mirroring the three sibling fallback fields.
Security risks
None. The option only toggles POSIX_SPAWN_SETSID on a subprocess Bun already spawns; no new strings reach argv/env, no auth/permissions/crypto surface, and the value is strictly type-checked as a JS boolean before use.
Level of scrutiny
Low. This is mechanical parameter plumbing that exactly follows the pattern of the adjacent stdoutInherit/stderrInherit parameters added previously. The only new logic is a 6-line option-parse block in JSWebViewConstructor.cpp (boolean check + ERR_INVALID_ARG_TYPE) and a one-line .detached = detached assignment in each Zig SpawnOptions literal.
Other factors
- My earlier nit (unnecessary
skipIf(isWindows)on the validation test) and CodeRabbit'sexitCode-ordering nit were both addressed in 9788acb; CodeRabbit's other suggestions were withdrawn after the author's well-reasoned responses. All inline threads are resolved. - The new behavioural test is solid: it points
backend.pathat a fake-chrome shell script, reads pgid via/proc/$$/stat(Linux/BusyBox-safe) orps -o pgid=(macOS), and asserts both thedetached: true(pgid == pid) anddetached: false(pgid != pid) cases in isolated subprocesses. - The one remaining bug-hunter finding is a stale enumeration in a comment ("userDataDir + stdio are the only carry-over") — purely cosmetic, filed as a nit, not blocking.
- CI build status on 6437447 shows red, but that is a merge-gate concern; 9788acb only touched the test file and the code itself is internally consistent.
There was a problem hiding this comment.
LGTM — mechanical plumbing of a boolean through to the existing SpawnOptions.detached; both earlier nits addressed.
Extended reasoning...
Overview
Adds backend.detached?: boolean to Bun.WebView constructor options for both the Chrome and WebKit backends. The change threads a single bool from JSWebViewConstructor.cpp through createChrome/createAndSend → Transport::ensureSpawned/HostClient::ensureSpawned → Bun__Chrome__ensure/Bun__WebViewHost__ensure → bun.spawn.SpawnOptions.detached (which already exists and sets POSIX_SPAWN_SETSID). The auto-detect → WebSocket → spawn fallback path stashes the value in the new m_fallbackDetached member. Type definitions and a behavioral + validation test are included.
Security risks
None identified. The option only toggles POSIX_SPAWN_SETSID on a subprocess the caller is already spawning with a caller-controlled binary path; it grants no new capability and defaults to false (no behavior change). No untrusted input flows into shell/argv construction.
Level of scrutiny
Low-to-moderate. Despite touching 11 files, every hunk is the same mechanical pattern: append a bool detached parameter and forward it. The only "logic" is the option-parsing block in JSWebViewConstructor.cpp, which mirrors the adjacent stdout/stderr parsing (with RETURN_IF_EXCEPTION after the get) and throws ERR_INVALID_ARG_TYPE for non-boolean/non-undefined. The new public API surface is narrow, opt-in, and named consistently with Bun.spawn({ detached }).
Other factors
- Both inline nits I left earlier (drop
skipIf(isWindows)on the validation test; update them_fallback*comment to mentiondetached) were applied in 9788acb and 9fe2c57. - All CodeRabbit threads are resolved or withdrawn; the
exitCode-after-stdout reorder was applied. - The behavioral test uses a fake-chrome shell script (no real Chrome dependency), reads pgid via
/proc/$$/staton Linux for BusyBox compatibility andps -o pgid=on macOS, and runs each case in a fresh subprocess to avoid the process-wide transport singleton — well-constructed and portable. - CI: per the author's note, the only remaining failure on build #49262 is a pre-existing
fetch-http2-client.test.tsASAN flake unrelated to WebView. - No CODEOWNERS entry for
src/bun.js/webview/.
Jarred-Sumner
force-pushed
the
farm/203d59dd/webview-detached
branch
from
9fe2c57 to
e5c8544
Compare
There was a problem hiding this comment.
Implementation looks clean and all review nits are addressed — deferring only because this adds new public API surface (backend.detached) to Bun.WebView, which is worth a quick human sign-off on the option name/default.
Extended reasoning...
Overview
This PR threads a new backend.detached: boolean option through the Bun.WebView constructor down to bun.spawn.SpawnOptions.detached, which already implements POSIX_SPAWN_SETSID. It touches 11 files (TypeScript types, C++ constructor/backends, Zig spawn helpers, and tests), but every non-test hunk is the same mechanical change: append a bool detached parameter to a function signature and forward it to the next layer, exactly mirroring the existing stdoutInherit/stderrInherit plumbing. Default is false, so existing behavior is unchanged.
Security risks
None identified. setsid() in a child subprocess is a standard, benign operation; the option is opt-in and only affects how the browser subprocess is spawned. No auth, crypto, permissions, or user-input parsing beyond a strict isBoolean() check that throws ERR_INVALID_ARG_TYPE otherwise.
Level of scrutiny
Low-to-moderate. The implementation itself is purely mechanical plumbing with no new logic, memory management, or control-flow changes — it would be approvable on its own. The reason for deferral is that it introduces a new documented public API field on Bun.WebView's constructor options, and API surface decisions (naming, default, whether this should just always be on) are the kind of thing a maintainer typically wants to glance at before it ships.
Other factors
- All five inline review threads (CodeRabbit and my own prior nits) are resolved; the fixes are reflected in the current diff (validation test ungated on Windows,
exitCodeasserted after stdout,m_fallback*comment updated). - CI: the new
backend.detachedtest passes on all platforms per the build report; the three remaining failures (s3-storage-class,bun-create,fetch-http2-client) are pre-existing flakes unrelated to WebView. - Test coverage is solid: a subprocess-isolated behavioral test with a fake-chrome shell script verifying
pgid == pidfordetached: trueandpgid != pidforfalse, plus a type-validation test.
…ession Some endpoint-protection / antivirus hooks that intercept Chrome's exec write their rejection banner directly to /dev/tty, bypassing stdio redirection and cluttering the parent CLI's terminal. Setting backend.detached: true spawns the Chrome (or WebKit host) subprocess with POSIX_SPAWN_SETSID so it runs in a new session with no controlling TTY, keeping that output off the parent's terminal. Threaded through both backends (JSWebViewConstructor -> createChrome / createAndSend -> Transport::ensureSpawned / HostClient::ensureSpawned -> Bun__Chrome__ensure / Bun__WebViewHost__ensure -> SpawnOptions.detached); defaults to false. Only the first Bun.WebView in a process (the one that actually spawns) observes the option. The auto-detect WebSocket fallback path carries it via m_fallbackDetached. Rebased onto the Rust port: the spawn-side changes now live in ChromeProcess.rs / HostProcess.rs (the .zig siblings are non-compiled reference files).
robobun
force-pushed
the
farm/203d59dd/webview-detached
branch
from
e5c8544 to
ae04d6c
Compare
1 participant
What
Adds
backend.detached: booleantoBun.WebViewconstructor options. Whentrue, the browser subprocess (Chrome or the WebKit host) is spawned withPOSIX_SPAWN_SETSID, putting it in its own session with no controlling terminal.Why
Some endpoint-protection / antivirus products hook
execand, when they decide to block the Chrome binary, print a rejection banner directly to/dev/tty— bypassing thestdout: "ignore"/stderr: "ignore"redirection thatBun.WebViewalready does. That output clobbers the parent CLI's terminal.With
detached: truethe child runs in a fresh session (setsid()), soopen("/dev/tty")fails in the child and the banner can't reach the parent's terminal.How
bun_spawn::SpawnOptionsalready has adetachedfield that setsPOSIX_SPAWN_SETSID; this threads a new option throughJSWebViewConstructor→createChrome/createAndSend→Transport::ensureSpawned/HostClient::ensureSpawned→Bun__Chrome__ensure/Bun__WebViewHost__ensure→SpawnOptions.detached. Also carried through the auto-detect WebSocket-fallback path (m_fallbackDetached).Defaults to
false. Like the other spawn-time options (path,argv,stdout,stderr), only the firstBun.WebViewin a process observes it — subsequent views share the already-spawned subprocess.Rebase note (Zig → Rust port)
This PR predates the
src/restructure and Rust port. After rebasing onto main:ChromeProcess.zig/HostProcess.zig(now non-compiled reference files, left untouched) into the live Rust implementationssrc/runtime/webview/ChromeProcess.rsandHostProcess.rs.src/runtime/webview/paths; one trivial conflict inWebKitBackend.cpp(a comment main had reworded) was resolved by keeping main's wording plus the new parameter.Verified after the rebase:
cargo check(all 10 targets viarust:check-all), and the new tests pass with the fix and fail without it.Verification
The test points
backend.pathat a shell script that reports its ownpidandpgid:detached: true→setsid()makes the child its own process-group leader →pgid == piddetached: false→ child inherits the spawner's group →pgid != pidDoesn't need a real Chrome install, so it runs on all POSIX CI. Skipped on Windows (Chrome backend isn't implemented there and
setsidhas no direct equivalent).