Repository navigation
Conversation
…ates Ops::close pruned every m_pending entry of the view, including a Target.createTarget that Chrome had already received. Its reply is the only message carrying the new tab's targetId, so handleResponse dropped it and the about:blank tab stayed open, unattached, for the life of the browser. Retag that entry instead of erasing it; when the reply arrives the transport sends Target.closeTarget for the returned targetId and releases the keep-alive ref. A createTarget still queued behind the WebSocket handshake is erased as before, so it is never sent.
|
Status: reproduced on the released build (google-chrome-stable 151, Linux) with the script in the PR description: after |
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
There was a problem hiding this comment.
Automated review ran and found no bugs. The C++ change is small and the reasoning in the PR description checks out end to end, but because it touches event-loop keep-alive refcounting and the WebSocket-transport close path, a human look from someone familiar with the WebView backend would still be worthwhile.
What was reviewed:
- The retag-in-
removeIf+ earlyTargetCreateTargetOrphanedbranch inhandleResponse: runs beforeviewFor, touches no view/slot, andupdateKeepAlive()ordering matches the existingOps::closefire-and-forget path. isQueuedUnsentgating:m_wsPendingis empty in pipe mode so retag always applies there; in WS pre-handshake the entry is dropped and thewsOnOpen/wsOnClosedrains already skip it viam_pending.contains(id).WTF::Vector::containsIfand mutating a value insideHashMap::removeIfwhile returningfalse— both have precedent in-tree.- Tests await real CDP events (no sleep-for-condition), cover both transport modes plus the keep-alive release, and the fixture's 2 s bound is a fail-open on the leak case rather than a timing assumption.
Extended reasoning...
Overview
Fixes a Chrome-tab leak in Bun.WebView: when close() runs while the first navigate's Target.createTarget is in flight, the reply carrying the new tab's targetId was dropped and the tab was never closed. The fix retags the pending entry as TargetCreateTargetOrphaned so handleResponse can send Target.closeTarget from the late reply, and adds isQueuedUnsent so a command still parked behind the WS handshake is cancelled outright instead. ~35 net C++ lines across ChromeBackend.{h,cpp} plus ~200 lines of tests and a WS-mode fixture.
Security risks
None. No user-controlled input reaches new parsing; the only new parse is jsonField(result, "targetId") on Chrome's own reply, using the existing scanner. No auth, crypto, or filesystem paths are touched.
Level of scrutiny
Medium-high. The core logic is small and well-argued, but it sits in the intersection of C++ lifecycle management, event-loop keep-alive refs (updateKeepAlive deciding when to drop the loop ref and, in connect mode, close the user's Chrome session), and WebSocket-transport ordering. Per the repo's review guidance, ref-count / lifecycle exits are the most-blocked category, so a maintainer sanity check on the keep-alive interaction is warranted even though I found no defect.
Other factors
- Test coverage is unusually thorough: pipe-mode in-flight close, post-chain close (guards the untouched branch), a subprocess exit test that pins down the
updateKeepAlive()call, and a WS-mode test that spawns its own Chrome and verifies both the pre-handshake cancel and the post-handshake close on an independent CDP connection. The PR description documents that each load-bearing clause breaks a specific test when removed and thatUSE_SYSTEM_BUN=1fails the new tests. - I checked that mutating
pair.value.methodinsideHashMap::removeIfand returningfalseis sound (value mutation, entry kept), thatcontainsIfis an existingWTF::Vectormethod used elsewhere insrc/jsc/bindings/webcore/, and that the orphaned handler'ssend(0, closeTarget)→updateKeepAlive()sequence is the same orderingOps::closealready uses whenm_targetIdis known, so the WS-close-after-send behaviour is unchanged. - The one nit I noticed (the
DevToolsActivePortread loop can briefly busy-spin if the file exists but is partial) is a sub-millisecond window and not worth blocking on. - No prior human review comments; CI status is pending per the robobun comment.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Updated 12:05 PM PT - Aug 15th, 2026
❌ @robobun, your commit 741722f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 39075That installs a local version of the PR into your bun-39075 --bun |
Problem
Bun.WebView(chrome backend): callingview.close()right after the firstview.navigate()rejects the navigate promise withWebView closedas intended, but the tab Chrome creates for that navigate is never closed. It stays inTarget.getTargetsas{ type: "page", url: "about:blank", attached: false }for as long as the browser lives (the spawned Chrome is shared by every view in the process; in connect mode it is the user's own Chrome). Each such close leaks one tab and its renderer process.src/runtime/webview/ChromeBackend.cpp:Ops::closeremoves everym_pendingentry of the view, including theTarget.createTargetthat Chrome has already received.view->m_targetIdis only filled in from that command's reply, soclose()has nothing to sendTarget.closeTargetfor, and when the reply arrivesTransport::handleResponsefinds no entry and drops it together with thetargetIdit carries. Every later step of the attach chain already hasm_targetIdwhenclose()runs, so this first step was the only gap.close()already cancels it correctly.Fix
Ops::closekeeps the in-flightTargetCreateTargetentry and retags itMethod::TargetCreateTargetOrphanedinstead of erasing it.handleResponsehandles that tag before any view lookup: it sendsTarget.closeTargetfor the returnedtargetId(fire-and-forget, same asclose()does when the id is already known), callsupdateKeepAlive(), and returns. An error reply has no result and therefore notargetId, so nothing is sent.createTargetstill parked behind the WebSocket handshake (Transport::isQueuedUnsent, new) is erased exactly as before, so it is never sent and no tab is created in the first place.attachToTarget,Page.enable,Page.navigate) are issued, so the property the prune was added for (nothing runs on a closed view) still holds.m_pendinguntil the reply, which keeps the event loop (and, in connect mode, the WebSocket to the user's Chrome) alive just long enough forcloseTargetto go out;updateKeepAlive()in the handler drops it afterwards. This is the roleupdateKeepAlive'sm_pendingclause already documents. If Chrome dies instead,rejectAllAndMarkDeadclears the entry as it does every other one.Closeis keyed by our own view id, which exists before anything is sent.close()as is, so it does not cover this.test/js/bun/webview/webview-chrome.test.ts, new tests: close() while createTarget is in flight (pipe mode) now observesTarget.targetDestroyedfor the created tab and no new page targets remain; close() after the chain completed still closes the tab; a process that does navigate+close exits on its own once the reply has been handled; connect mode (Chrome spawned with--remote-debugging-port=0, fixturewebview-chrome-ws-close-fixture.tsin a child process, tab creation counted on an independent connection) cancels the pre-handshake createTarget outright and closes the post-handshake one. The existing attach-chain test is renamed to what it actually checks; its assertions are unchanged.USE_SYSTEM_BUN=1: the pipe-mode test times out waiting fortargetDestroyedand the connect-mode test fails withlate: tab still open; both pass withbun bd test. The wholetest/js/bun/webview/directory passes with the debug build, also underBUN_JSC_validateExceptionChecks=1.isQueuedUnsentcondition makes the connect-mode test fail (three pages created instead of two); removing theupdateKeepAlive()call makes the exit test hang.Background
id, answered by a reply with the sameid; events carry amethodand, for page-scoped events, asessionId. The backend talks to one Chrome per process, over a pipe it spawned or over a WebSocket to an existing Chrome.navigate()on a view sendsTarget.createTarget(makes the tab; the reply returns itstargetId), thenTarget.attachToTarget(returns thesessionIdlater commands are scoped to),Page.enable,Page.navigate. Each reply triggers the next command;Transport::m_pendingmaps each outstanding command id to the method tag and view it belongs to, andhandleResponsedispatches on that tag.Target.closeTargetis the browser-level command that closes a tab given itstargetId;close()sends it without tracking the reply.Transport::updateKeepAliveholds a ref on the event loop while any view exists or any command is outstanding, and in connect mode closes the WebSocket when neither is true.Target.setDiscoverTargets, because browser-level events (nosessionId) are not routed to any view; Chrome then emitsTarget.targetCreated/Target.targetDestroyedon that session and they reach the view'sEventTarget.Repro against the released build
Release bun:
[ { type: "page", url: "about:blank", attached: false, ... } ], still present later. With this change:[].