Remove JSCallbackDataStrong, unify into single Weak JSCallbackData#28539
Remove JSCallbackDataStrong, unify into single Weak JSCallbackData#28539Jarred-Sumner merged 3 commits intomainfrom
Conversation
Follow-up to #28494. Backports WebKit 276563 which removed the Strong/Weak split in favor of a single Weak-only JSCallbackData class, making it structurally impossible to introduce Strong callback handles (a recurring source of GC reference cycles). The prerequisite AbortSignal::m_algorithms restructuring that #28494 deferred this work for was already completed in #28491, so this is now a pure rename + dead code removal with no behavior change. Also deletes ErrorCallback/JSErrorCallback (FileSystemEntry API callback, never used in Bun).
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
PR #28496 replaced the single component_idx with an active BitSet, but the Windows NtQueryDirectoryFile filter call from PR #28489 still referenced the old variable name. computeNtFilter operates on a single pattern component, so apply the kernel filter only when exactly one index is active. For multi-index states (e.g. after **), skip the filter to avoid hiding entries needed by other indices. The filter is a pre-filter optimization; matchPatternImpl still runs for correctness.
|
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)
WalkthroughDeleted the Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look semantically correct -- both affected classes were already Weak, so this is a pure rename plus dead-code removal -- but the GC callback ownership infrastructure warrants a human sign-off before merging.
Extended reasoning...
Overview
This PR merges JSCallbackDataWeak into the base JSCallbackData class, deletes JSCallbackDataStrong, and removes the unimplemented ErrorCallback/JSErrorCallback files (the only remaining Strong users). The JSAbortAlgorithm and JSPerformanceObserverCallback changes are type renames only -- both were already using JSCallbackDataWeak, so runtime semantics are unchanged.
Security risks
No security-sensitive paths (auth, crypto, permissions) are affected. The change eliminates JSC::Strong handles in callback wrappers, which reduces the risk of GC reference cycles -- a net security/stability improvement.
Level of scrutiny
Despite being described as a rename, this touches JSC's GC weak-handle infrastructure: WeakHandleOwner, isReachableFromOpaqueRoots, and the opaque-root chain that keeps callbacks alive during GC. Correctness depends on the opaque-root mechanism being properly wired (verified by GC stress tests in the PR description), but someone familiar with Bun's JSC integration should confirm the visitAbortAlgorithms() path from #28491 is sufficient to cover all reachability cases.
Other factors
The two reported bugs are both pre-existing: the toJS() null-guard pattern and the unused VM& constructor parameter both existed identically in JSCallbackDataWeak before this PR, so the risk profile is unchanged. The PR includes thorough verification (GC stress tests, abort/stream/performance-observer test suites, symbol-table check). The deletions (ErrorCallback, JSErrorCallback) appear safe given the documented zero call sites. Overall this is a well-justified and well-tested structural cleanup, but GC memory management code should have a human reviewer confirm.
| JSCallbackData(JSC::VM&, JSC::JSObject* callback, void* owner) | ||
| : m_callback(callback, &m_weakOwner, owner) | ||
| { | ||
| } |
There was a problem hiding this comment.
🟡 The JSCallbackData constructor accepts a JSC::VM& parameter that is never used — JSC::WeakJSC::JSObject construction takes (JSObject*, WeakHandleOwner*, void*) and needs no VM. This is a pre-existing pattern inherited from JSCallbackDataWeak, but now that this PR explicitly merges/renames that class and deletes JSCallbackDataStrong entirely, removing the dead parameter would improve API clarity.
Extended reasoning...
Analysis
The JSCallbackData constructor at src/bun.js/bindings/webcore/JSCallbackData.h:52-55 has the following signature:
JSCallbackData(JSC::VM&, JSC::JSObject* callback, void* owner)
: m_callback(callback, &m_weakOwner, owner)
{}
The JSC::VM& parameter is unnamed and unused. The only member initialized is m_callback, a JSC::Weak<JSC::JSObject>. The JSC::Weak constructor takes (JSObject*, WeakHandleOwner*, void*) — no VM reference. The VM parameter is never forwarded anywhere.
Code Path
This constructor is called at two sites:
JSAbortAlgorithm.cpp:34:new JSCallbackData(vm, callback, this)JSPerformanceObserverCallback.cpp:37:new JSCallbackData(globalObject->vm(), callback, this)
In both cases, vm is fetched and passed in, but silently discarded.
Why Existing Code Does Not Prevent It
This pattern was inherited from the old JSCallbackDataWeak class, which also had an unnamed VM& parameter that was never used. The unnamed parameter is standard C++ idiom for intentionally unused parameters, and the refutation correctly notes it could serve API-compatibility purposes.
Addressing the Refutation
The refutation argues the unnamed parameter is intentional for API compatibility — preserving call sites in case Strong/Weak semantics are ever switched. This argument was defensible when both JSCallbackDataStrong and JSCallbackDataWeak coexisted. However, this PR deletes JSCallbackDataStrong entirely. There is now no Strong variant to switch back to; JSCallbackData is permanently Weak-only. The API-compatibility justification evaporates the moment JSCallbackDataStrong is removed, making the parameter purely vestigial dead weight.
Impact
No runtime impact. The cost is mild API confusion: callers are led to believe VM participates in construction when it does not. The two call sites unnecessarily fetch and pass vm, which is misleading to readers.
How to Fix
Remove the VM& parameter from the constructor and update the two call sites:
// JSCallbackData.h
JSCallbackData(JSC::JSObject* callback, void* owner)
: m_callback(callback, &m_weakOwner, owner)
{}
// JSAbortAlgorithm.cpp
m_data(new JSCallbackData(callback, this))
// JSPerformanceObserverCallback.cpp
m_data(new JSCallbackData(callback, this))
Pre-existing Nature
The identical pattern existed in JSCallbackDataWeak before this PR. This PR did not introduce it but inherited it. Given the PR explicitly removes JSCallbackDataStrong — eliminating the only coherent justification for keeping a VM parameter around for future Strong/Weak switching — this is the natural cleanup moment. Filed as nit since there is no runtime impact.
| , m_data(new JSCallbackData(vm, callback, this)) | ||
| { | ||
| } | ||
|
|
There was a problem hiding this comment.
🟣 Pre-existing issue: toJS() in JSAbortAlgorithm.cpp and JSPerformanceObserverCallback.cpp guards callbackData() (the raw heap pointer, almost never null) but not callback() (the JSC::Weak handle, which returns nullptr when GC clears it). Returning a null JSObject* as a JSValue does not produce jsNull() -- it encodes a null cell pointer as JSValue(0), the integer 0 in JSC NaN-boxing. This identical behavior existed in JSCallbackDataWeak before this PR; the PR only renames the type in the constructor.
Extended reasoning...
What the bug is: In both JSAbortAlgorithm.cpp:34-37 and JSPerformanceObserverCallback.cpp, the toJS() function contains a guard that returns jsNull() if callbackData() is null, but callbackData() returns the raw m_data heap pointer, which is set in the constructor and almost never null during the object's lifetime. The actual GC-nullable value is callback(), which returns m_callback.get() from a JSC::Weak<JSC::JSObject> handle. When the GC clears a weak handle, m_callback.get() returns nullptr, and that null JSObject* is then implicitly returned as a JSValue.
Why the implicit conversion is wrong: In JSC's NaN-boxing scheme, converting a null cell pointer (JSObject* = nullptr) to JSValue does not produce jsNull() (which encodes a specific NaN-boxed null tag). Instead, it produces JSValue(0), encoding the integer 0. This is an invalid JSValue in the context of object identity and would corrupt any code that tries to operate on it as a JS object reference.
The specific code path:
JSC::JSValue toJS(AbortAlgorithm& impl)
{
if (!static_cast<JSAbortAlgorithm&>(impl).callbackData()) // guards m_data raw ptr, almost never null
return jsNull();
return static_cast<JSAbortAlgorithm&>(impl).callbackData()->callback(); // JSC::Weak::get(), can be null
}The guard pattern reads as if it handles the null case, but the null path it fails to check is the one that can actually occur at runtime.
Addressing the refutation: The refutation correctly notes that the WeakOwner::isReachableFromOpaqueRoots() mechanism keeps the callback alive via the opaque-root chain (AbortSignal -> JSAbortAlgorithm -> m_callback), so toJS() is only called with a live algorithm reference, making the GC-cleared scenario implausible in practice. This is a valid point. However, it does not change the fact that the guard is semantically wrong and misleading.
Why this is pre-existing: The identical behavior existed in JSCallbackDataWeak before this PR. Both JSAbortAlgorithm and JSPerformanceObserverCallback were already using JSCallbackDataWeak (which internally used JSC::Weak<JSC::JSObject>) and the same toJS() functions existed unchanged. This PR is a pure type rename (JSCallbackDataWeak* -> JSCallbackData*) in the constructor; the toJS() bodies and their null guard patterns were not modified. The risk profile is identical to before the PR.
Step-by-step proof of the issue: (1) GC runs and the weak handle m_callback gets cleared (hypothetically, if the opaque-root mechanism fails or is absent for a future callback type). (2) callbackData() returns non-null m_data pointer, so the guard passes. (3) callback() returns nullptr from m_callback.get(). (4) The null JSObject* is implicitly converted to JSValue, producing JSValue(0) (integer 0) rather than jsNull(). (5) Any downstream code receiving this JSValue and treating it as a JS object would get undefined behavior or corruption.
The fix (for completeness): Both toJS() functions should additionally check callback() and return jsNull() when it is null, making the guard honest about the actual nullable path.
| JSCallbackData(JSC::VM&, JSC::JSObject* callback, void* owner) | ||
| : m_callback(callback, &m_weakOwner, owner) | ||
| { | ||
| } |
There was a problem hiding this comment.
🟡 The class-level comment above JSCallbackData still references "unprotecting a JSObject" which is Strong-handle terminology, but this PR deletes JSCallbackDataStrong entirely leaving only a JSC::Weak handle. The threading requirement still applies to Weak handle teardown, but the stated reason is now factually wrong and will mislead future readers about the class semantics. Update the comment to explain that JSC::Weak handle teardown must happen on the JS thread.
Extended reasoning...
What the bug is: The comment at JSCallbackData.h:40-42 reads: "We have to clean up this data on the context thread because unprotecting a JSObject on the wrong thread without synchronization would corrupt the heap (and synchronization would be slow)." The term unprotecting is explicit JSC::Strong terminology. A Strong handle increments the GC refcount (protecting the object) and must be decremented (unprotected) on the JS thread to avoid unsynchronized mutation of the GC root set. This comment was accurate when JSCallbackDataStrong coexisted alongside JSCallbackDataWeak in the same file.
How this PR introduces the inaccuracy: This PR deletes JSCallbackDataStrong entirely and merges JSCallbackDataWeak into the base JSCallbackData class. The resulting unified class now holds only a JSC::Weak handle. Weak handles do not protect objects - they hold entries in JSC's weak table which the GC clears when the referent is collected. There is no protect/unprotect concept, no refcount to decrement, and no GC root set to corrupt from the wrong thread.
The threading requirement itself is still valid: JSC::Weak handle teardown and weak-table entry finalization must still happen on the JS thread - evidenced by the ASSERT in the destructor that checks Thread::currentSingleton(). The threading constraint is real; only the explanation given for it is wrong.
Step-by-step proof: (1) Pre-PR, JSCallbackData is a base class with JSCallbackDataStrong (uses JSC::Strong) and JSCallbackDataWeak (uses JSC::Weak); the comment accurately describes the Strong variant threading requirement. (2) This PR deletes JSCallbackDataStrong entirely, renames JSCallbackDataWeak to JSCallbackData, and replaces Strong.h/StrongInlines.h includes with Weak.h/WeakInlines.h. (3) The class-level comment is not updated. (4) A future reader sees the comment, reads that unprotecting a JSObject would corrupt the heap, and forms an incorrect mental model - potentially leading to wrong reasoning about GC safety or threading when working with or extending this class.
How to fix: Replace the comment with one accurate for Weak handles, for example: "We have to clean up this data on the context thread because JSC::Weak handle teardown (clearing weak-table entries) must happen on the JS thread."
Severity: Nit - no runtime impact, purely a documentation accuracy issue, but directly caused by this PR removing JSCallbackDataStrong which was the only code the original comment accurately described.
| : PerformanceObserverCallback(globalObject->scriptExecutionContext()) | ||
| , m_data(new JSCallbackDataWeak(globalObject->vm(), callback, this)) | ||
| , m_data(new JSCallbackData(globalObject->vm(), callback, this)) | ||
| { | ||
| } | ||
|
|
There was a problem hiding this comment.
🟣 Pre-existing issue: JSPerformanceObserverCallback::handleEvent dereferences m_data->callback()->globalObject() at lines 63 and 78 without a null guard, while JSAbortAlgorithm::handleEvent has a correct explicit check (if (!callback) return UnableToExecute). The hasCallback() override in JSPerformanceObserverCallback.h explicitly checks m_data->callback() for null, confirming the authors know it can be null, but handleEvent does not apply the same guard.
Extended reasoning...
What the bug is:
JSPerformanceObserverCallback::handleEvent (JSPerformanceObserverCallback.cpp:63) directly dereferences m_data->callback()->globalObject() without checking whether m_data->callback() is null. JSCallbackData::callback() returns m_callback.get() from a JSC::Weak handle, which returns nullptr when the GC has cleared the handle. A second unchecked dereference occurs at line 78 inside the exception-reporting branch: reportException(m_data->callback()->globalObject(), returnedException).
Contrast with JSAbortAlgorithm:
JSAbortAlgorithm::handleEvent has an explicit null guard that was pre-existing before this PR (the diff only changes the constructor type from JSCallbackDataWeak* to JSCallbackData*):
auto* callback = m_data->callback();
if (!callback)
return CallbackResultType::UnableToExecute;
JSPerformanceObserverCallback::handleEvent has no equivalent guard, creating an inconsistency between two structurally identical callback classes.
Why existing code does not prevent it:
canInvokeCallback() (ActiveDOMCallback.cpp) only checks activeDOMObjectsAreSuspended() / activeDOMObjectsAreStopped() on the script execution context — it does NOT call hasCallback(). So the early return at line 58 does not protect against a null weak handle. The hasCallback() override in JSPerformanceObserverCallback.h is 'return m_data && m_data->callback()' and explicitly documents the callback can be null, but the caller (PerformanceObserver::deliver()) never calls hasCallback() before calling handleEvent().
Addressing the refutations:
The refutations correctly identify that the opaque-root reachability chain provides strong practical protection: while an observer is registered, JSPerformanceObserverOwner::isReachableFromOpaqueRoots() returns true, the JSPerformanceObserver cell is kept alive, visitAdditionalChildrenInGCThread calls visitJSFunction, visitor.append(m_callback) strongly marks the JS function as reachable, so callback() cannot be null. After disconnect(), m_entriesToDeliver is cleared first, so deliver() returns early at the isEmpty() check before reaching handleEvent. The PR's GC stress test (50 observers + 30x Bun.gc(true)) confirms this chain holds in practice.
However, the protection is an implicit design invariant, not an explicit guard. A post-disassociate() scenario (which sets m_registered=false without clearing entries) followed by a GC cycle before the entries are processed could create a window where canInvokeCallback() returns true but the weak handle has been cleared.
Step-by-step proof:
- Observer calls disassociate() — sets m_registered=false, leaves entries in queue.
- GC runs; isReachableFromOpaqueRoots now returns false (observer not registered).
- m_callback weak handle is cleared by GC.
- Before the entry queue is drained, deliver() is called — the isEmpty() check passes because entries remain.
- handleEvent() is called; canInvokeCallback() returns true (context still alive).
- m_data->callback() returns nullptr.
- nullptr->globalObject() causes a null pointer dereference crash.
How to fix:
Apply the same null guard used in JSAbortAlgorithm::handleEvent:
auto* callback = m_data->callback();
if (!callback)
return CallbackResultType::UnableToExecute;
auto& globalObject = *jsCast<JSDOMGlobalObject*>(callback->globalObject());
And update the exception-reporting site to reuse the already-captured callback variable.
Pre-existing nature:
Neither handleEvent body was modified by this PR. The PR performs a pure type rename (JSCallbackDataWeak* to JSCallbackData*) in the constructor. The behavioral gap predates this PR and both classes were already using the Weak variant before the rename. However, the PR touches this file, and the addition of hasCallback() makes the missing guard more conspicuous — hasCallback() is effectively dead code since no caller checks it before invoking handleEvent().
3 participants
What
Follow-up to #28494. Backports WebKit 276563 ("Make all callbacks Weak handles"), which removed
JSCallbackDataStrong/JSCallbackDataWeakin favor of a single Weak-onlyJSCallbackDataclass.Why
JSC::Stronghandles in callback wrappers have been a recurring source of GC reference cycles (see #28491 for thepipeTo + signalleak). Upstream WebKit solved this structurally in 2024-07 by deleting the Strong variant entirely, making such leaks impossible to introduce by construction.#28494 deferred this backport because it "requires restructuring
AbortSignal::m_algorithmsto allow GC visitation." That restructuring was already done in #28491 (m_abortAlgorithms+visitAbortAlgorithms()+ Lock), so the prerequisite is satisfied and this is now a pure rename + dead code removal.Changes
JSCallbackData.h/.cppJSCallbackDataWeakinto the baseJSCallbackDataclassJSCallbackDataStrongStrong.h/StrongInlines.hincludes withWeak.h/WeakInlines.hinvokeCallback(VM&, ...)signature andvisitJSFunctionmethod name (not upstream'svisitJSFunctionInGCThread) to avoid touching base class overrides — this is a structural sync, not a full API syncJSAbortAlgorithm,JSPerformanceObserverCallbackJSCallbackDataWeak*→JSCallbackData*Deleted:
ErrorCallback.{h,cpp,idl},JSErrorCallback.{h,cpp}webkitGetAsEntry()etc.), never implemented in Buncreate()call sites, zero#includereferences outside itselfJSCallbackDataStronguserErrorCallbackunderModules/entriesapi/, but with[GenerateIsReachable]which generates a Weak callback — our copy predated that and was a stale Strong versionVerification
nm build/debug/bun-debug | grep -cE "JSCallbackData(Strong|Weak)"→0test/js/web/abort/— passtest/js/web/streams/pipeTo-signal-leak.test.ts— pass (regression check for Fix AbortSignal leak in ReadableStream.pipeTo with signal option #28491)test/js/web/timers/performance-entries.test.ts— passBUN_JSC_validateExceptionChecks=1— passBun.gc(true)→ all 100 fire (opaque-root reachability preserved)Bun.gc(true)→ all 50 fire