Free a global object's console client together with the global - #38395
Free a global object's console client together with the global#38395robobun wants to merge 1 commit into
Conversation
GlobalObject::setConsole() allocated a Bun::ConsoleObject and handed JSGlobalObject::setConsoleClient() a WeakPtr to it, so nothing ever deleted it. Every ShadowRealm global leaked its client (plus the WeakPtrImpl), as did the main/worker globals; the latter were hidden by the blanket leak:Zig__GlobalObject__create LSan suppression. The global now owns the client through a unique_ptr, so it is deleted when the global cell is destroyed. ConsoleClient is CanMakeCheckedPtr, so Bun::ConsoleObject gets WTF_OVERRIDE_DELETE_FOR_CHECKED_PTR, which is what makes deleting it legal (debug builds assert otherwise) and catches a CheckedPtr outliving it. Drop the Zig__GlobalObject__create suppression, whose only known job was hiding this leak, and add Malloc=1 LSan tests for realms created on the main thread and inside a worker.
|
Warning Review limit reached
Next review available in: 8 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 (5)
Comment |
|
Status: fix and tests pushed, waiting on CI. Reproduced on the unfixed debug build with With this branch the same runs are leak-free (with no suppressions at all for the main-thread cases), and the new cases in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs. Because it changes object lifetime in a GC-swept cell (ZigGlobalObject's destructor now frees the console client) and removes a broad leaksan.supp entry that was only spot-checked against the wider suite, a human look would still be worthwhile.
Checked: setConsole() is called exactly once per global (four creation-time call sites, none re-entrant); ~GlobalObject() is out-of-line in a TU that includes ConsoleObject.h, so the forward-declared unique_ptr deletes with a complete type; member destruction runs before ~JSGlobalObject destroys its WeakPtr, which is the order CanMakeWeakPtr is designed for; the new tests drain both pipes concurrently and gate on isASAN.
Extended reasoning...
Overview
The PR fixes a per-global-object leak of Bun::ConsoleObject: setConsole() previously did setConsoleClient(new Bun::ConsoleObject(...)), and since JSGlobalObject::setConsoleClient only holds a WeakPtr, nothing owned the allocation. The fix stores the client in a new std::unique_ptr<Bun::ConsoleObject> m_consoleObject on Zig::GlobalObject so it is freed when the global cell is swept, adds WTF_OVERRIDE_DELETE_FOR_CHECKED_PTR to ConsoleObject (required because ConsoleClient is CanMakeCheckedPtr), removes the leak:Zig__GlobalObject__create LSan suppression that was hiding this, and adds ASAN-gated LSan regression tests for both main-thread and worker ShadowRealm globals.
Security risks
None. This is internal memory ownership of a bridge object; no user input, parsing, or trust boundary is involved.
Level of scrutiny
High. Per REVIEW.md, native memory safety is the most-blocked category, and this touches destruction of a GC-managed cell in ZigGlobalObject — a header the file itself warns is included nearly everywhere. That said, the actual production change is two lines plus a macro and a member declaration, and it mirrors WebCore's WorkerConsoleClient ownership exactly (which the author cites). I verified: all four setConsole() callers are one-shot at global creation; ConsoleObject.h is included in ZigGlobalObject.cpp so ~GlobalObject() sees the complete type for the unique_ptr deleter; and destruction ordering (derived member m_consoleObject before base m_consoleClient WeakPtr) is the ordering CanMakeWeakPtr expects.
Other factors
The leaksan.supp removal is the piece with the widest blast radius — it was a blanket suppression on Zig__GlobalObject__create, and the author validated it against a sample of suites rather than the full matrix. If any other allocation under that frame still leaks, unrelated CI lanes will start failing. That is the right direction (strengthening rather than weakening sanitizer coverage), but it is worth a maintainer being aware of before merge. The new tests follow harness conventions well: describe.concurrent.skipIf(!isASAN || isWindows), await using on spawned processes, concurrent pipe draining, and a documented per-test timeout for LSan symbolization. The worker case's weaker not.toContain assertion is justified inline with a linked issue (#38164).
|
On the On the lifetime change: the only holder of the client is the global's own |
|
Updated 6:05 AM PT - Aug 14th, 2026
✅ @robobun, your commit 3a1d5204752009752641989d51f6d6d9871ee788 passed in 🧪 To try this PR locally: bunx bun-pr 38395That installs a local version of the PR into your bun-38395 --bun |
1 participant
Problem
new ShadowRealm()leaks the realm global'sBun::ConsoleObject(40 bytes) and theWeakPtrImplJSC holds on it (32 bytes). LSan (withMalloc=1) reports them asBun::ConsoleObject::operator new/Zig::GlobalObject::setConsole/Zig::deriveShadowRealmGlobalObject/JSC::ShadowRealmObject::create, on the main thread afterBUN_DESTRUCT_VM_ON_EXITteardown and after a worker's VM is torn down.GlobalObject::setConsole()(src/jsc/bindings/ZigGlobalObject.cpp:1126) doessetConsoleClient(new Bun::ConsoleObject(...)).JSGlobalObject::setConsoleClienttakes aWeakPtr, so nothing owns the object and nothing deletes it when the global is collected.Malloc=1); those reports were hidden by the blanketleak:Zig__GlobalObject__createentry intest/leaksan.supp.Fix
Zig::GlobalObjectkeeps the client in astd::unique_ptr<Bun::ConsoleObject>(m_consoleObject);setConsole()fills it and installs theWeakPtr, and the client is deleted when the global cell is destroyed. This covers every caller ofsetConsole():Zig__GlobalObject__create(main thread and workers),Zig__GlobalObject__createForTestIsolation,BakeCreateProdGlobal, andderiveShadowRealmGlobalObject.Bun::ConsoleObjectgetsWTF_OVERRIDE_DELETE_FOR_CHECKED_PTR.JSC::ConsoleClientisCanMakeCheckedPtr, and deleting such an object through a subclass without the override tripsASSERTION FAILED: m_didBeginDeletion || deleteException == CheckedPtrDeleteCheckException::Yesin debug builds (the first build of this change did). It also makes aCheckedPtrthat outlives the client a detected error instead of a use-after-free.JSGlobalObject::m_consoleClient, aWeakPtr; the Rust side ignores the pointer it is handed and resolves the console through the VM), and any code that can still call into a realm keeps that realm's global alive, so the client can never be reached after the global is destroyed. Deleting it at that point is exactly how WebCore ownsWorkerConsoleClient(std::unique_ptrinWorkerOrWorkletScriptController,WTF_OVERRIDE_DELETE_FOR_CHECKED_PTRon the client).leak:Zig__GlobalObject__createis removed fromtest/leaksan.supp: with the fix an empty script and a worker script run leak-free underMalloc=1with no suppressions at all, so the entry's only known job was hiding this leak. A sample of test files (test/cli/test/isolation.test.ts, worker, BroadcastChannel, console and jsc suites) passes under the CI LSan environment with it removed.test/js/bun/jsc/shadow.test.jscreates realms fromsetImmediateunderMalloc=1+BUN_DESTRUCT_VM_ON_EXIT=1+detect_leaks=1, on the main thread (expects a clean exit) and inside aworker_threadsworker (expects noderiveShadowRealmGlobalObject/Zig__GlobalObject__createframes in the report; the exit code cannot be asserted there until worker: free the thread's event name table when the worker thread exits #38164 frees the worker'sEventNamestable). Both fail on the unfixed build (git stash push -- src/ && bun bd test ...) and pass with it.test-shadow-realm*.jstests (including the GC stress, which sweeps realm globals while the VM is running), a 100-realm GC loop that calls a realm function after itsShadowRealmwrapper was collected,bun test --isolateunderMalloc=1LSan, and the production half oftest/bake/dev-and-prod.test.ts.Background
consoleobject forwards every call to theJSC::ConsoleClientregistered on the calling global object.Bun::ConsoleObjectis Bun's implementation; it bridges into the Rust console code. EachZig::GlobalObjectinstalls one on itself.new ShadowRealm()asks the embedder (deriveShadowRealmGlobalObject) for a fresh global object in the same VM. It is an ordinary GC cell: when the realm becomes unreachable the cell is swept and~GlobalObjectruns, long before the VM goes away.bun test --isolateretires globals the same way.WeakPtr/CanMakeWeakPtr: WTF's non-owning pointer. The target owns a small refcountedWeakPtrImplthat is cleared when the target is destroyed;setConsoleClientstores one of these, so the embedder must own the client itself.CanMakeCheckedPtr/WTF_OVERRIDE_DELETE_FOR_CHECKED_PTR: WTF's dangling-pointer check. The object counts outstandingCheckedPtrs; the macro replacesoperator deleteon the most derived class with one that marks the deletion as begun (which the base destructor asserts in debug builds) and, if anyCheckedPtris still outstanding, zeroes the object instead of freeing it.Malloc=1: routes bmalloc/libpas (which backsfastMallocand therefore this object) to the system allocator so ASAN/LSan can see those allocations.BUN_DESTRUCT_VM_ON_EXIT=1makes exit really destroy the VM, which is what frees the globals and turns the unowned clients into reported leaks.