webcore: break Performance ↔ PerformanceObserver ref cycle on global teardown#29921
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Review rate limit: 0/5 reviews remaining, refill in 1 minute and 2 seconds. Comment |
Collaborator
Author
Contributor
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…l teardown Performance holds RefPtr<PerformanceObserver> in m_observers and each PerformanceObserver holds RefPtr<Performance> back. When a Worker terminates (or any global is destroyed) with an observer still registered -- i.e. the user never called .disconnect() -- neither object is released. Performance::removeAllObservers() exists to break this but was dead code. In WebKit the owning scope (WorkerGlobalScope / LocalDOMWindow) calls removeAllObservers() during removeAllEventListeners(). Bun has no such hook, so call it from ~GlobalObject() just before the ScriptExecutionContext is deref'd. Calling it from Performance::contextDestroyed() is too late: dropping the last observer ref there destroys the observer's callback, whose ~ContextDestructionObserver() then tries to unregister from the context that is already mid-destruction, tripping an assertion.
robobun
force-pushed
the
farm/dbc89287/performance-observer-cycle
branch
from
a43b363 to
adc61ca
Compare
xhjkl
pushed a commit
to xhjkl/bun
that referenced
this pull request
May 14, 2026
…teardown (oven-sh#29921) ## Problem `Performance` holds `RefPtr<PerformanceObserver>` in `m_observers` (its registered-observer list), and each `PerformanceObserver` holds `RefPtr<Performance>` in `m_performance`. When a Worker terminates with an observer still registered — i.e. the user never called `.disconnect()` — neither object is released, leaking the `Performance` instance along with everything it owns (user-timing buffer, etc). `Performance::removeAllObservers()` exists specifically to break this cycle, but nothing in Bun ever called it. ## Repro ```js // worker.js new PerformanceObserver(() => {}).observe({ entryTypes: ["mark"] }); postMessage("ready"); // never calls .disconnect() // main.js for (let i = 0; i < 5; i++) { const w = new Worker("./worker.js"); await new Promise(r => w.onmessage = r); await w.terminate(); } ``` On a release build this segfaults on the second iteration (the leaked objects outlive their VM and are accessed during the next teardown). On debug, each worker's `Performance` instance is simply never freed. ## Fix Call `m_performance->removeAllObservers()` in `~GlobalObject()` before the `ScriptExecutionContext` is deref'd. This mirrors WebKit, where `WorkerGlobalScope` / `LocalDOMWindow` call `removeAllObservers()` during `removeAllEventListeners()`; Bun has no equivalent hook, so the global destructor is the last point where the context is still fully alive. Calling it from `Performance::contextDestroyed()` instead is too late: dropping the last observer ref there cascades into `~JSPerformanceObserverCallback()` → `~ContextDestructionObserver()`, which tries to unregister from the `ScriptExecutionContext` while that context is already inside its own destructor iterating observers, tripping `ASSERT(!m_inScriptExecutionContextDestructor)`. ## Verification New test spawns 28 workers that each register an observer, store a 4 MB mark in `performance`'s user-timing buffer, and get terminated without `.disconnect()`. RSS of the second batch of 20 is compared against a warm baseline. | | RSS delta (20 workers) | `~Performance()` runs | |---|---|---| | before | ~110 MB | 0 / N | | after | ~28 MB | N / N | - `USE_SYSTEM_BUN=1 bun test` → segfault (fail) - `bun bd test` without src/ change → `RSS grew by 109.3 MB` (fail) - `bun bd test` with fix → pass (×3) Co-authored-by: robobun <robobun@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
PerformanceholdsRefPtr<PerformanceObserver>inm_observers(its registered-observer list), and eachPerformanceObserverholdsRefPtr<Performance>inm_performance. When a Worker terminates with an observer still registered — i.e. the user never called.disconnect()— neither object is released, leaking thePerformanceinstance along with everything it owns (user-timing buffer, etc).Performance::removeAllObservers()exists specifically to break this cycle, but nothing in Bun ever called it.Repro
On a release build this segfaults on the second iteration (the leaked objects outlive their VM and are accessed during the next teardown). On debug, each worker's
Performanceinstance is simply never freed.Fix
Call
m_performance->removeAllObservers()in~GlobalObject()before theScriptExecutionContextis deref'd. This mirrors WebKit, whereWorkerGlobalScope/LocalDOMWindowcallremoveAllObservers()duringremoveAllEventListeners(); Bun has no equivalent hook, so the global destructor is the last point where the context is still fully alive.Calling it from
Performance::contextDestroyed()instead is too late: dropping the last observer ref there cascades into~JSPerformanceObserverCallback()→~ContextDestructionObserver(), which tries to unregister from theScriptExecutionContextwhile that context is already inside its own destructor iterating observers, trippingASSERT(!m_inScriptExecutionContextDestructor).Verification
New test spawns 28 workers that each register an observer, store a 4 MB mark in
performance's user-timing buffer, and get terminated without.disconnect(). RSS of the second batch of 20 is compared against a warm baseline.~Performance()runsUSE_SYSTEM_BUN=1 bun test→ segfault (fail)bun bd testwithout src/ change →RSS grew by 109.3 MB(fail)bun bd testwith fix → pass (×3)