Fix WriteBarrier owner and add cellLock for JSCommonJSExtensions::m_registeredFunctions - #30524
Fix WriteBarrier owner and add cellLock for JSCommonJSExtensions::m_registeredFunctions#30524cirospaciari wants to merge 4 commits into
Conversation
…egisteredFunctions JSCommonJSExtensions__setFunction and JSCommonJSExtensions__swapRemove pass globalObject as the owner cell to WriteBarrier::set() while writing to the extensions object's m_registeredFunctions vector. The write barrier therefore adds globalObject to the remembered set, but it is JSCommonJSExtensions::visitChildrenImpl — not the global object's visitChildren — that scans m_registeredFunctions. Under an eden collection where the extensions object is already old/black and the new value is young, the value is never re-marked and can be collected while still referenced. JSCommonJSExtensions__appendFunction at line 208 already passes the correct owner (extensions); make the other two call sites consistent. Also take cellLock() around m_registeredFunctions mutation and visit, matching the fix that was applied to JSCommonJSModule::m_children in #29995. The same WTF::Vector<WriteBarrier<>> pattern is mutated on the mutator thread (append / takeLast / removeLast / clear) without a lock while parallel mark threads iterate it in visitChildrenImpl; a Vector reallocation can free the backing buffer mid-scan.
|
Updated 7:08 PM PT - May 13th, 2026
❌ @robobun, your commit 7c5ca15 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 30524That installs a local version of the PR into your bun-30524 --bun |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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)
WalkthroughThis PR adds thread-safety protections to ChangesCommonJS Module Registration Locking
🚥 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/JSCommonJSExtensions.cpp`:
- Around line 213-216: The setter JSCommonJSExtensions__setFunction mutates the
vector-backed WriteBarrier storage m_registeredFunctions without acquiring the
same lock used by visitChildrenImpl; wrap the mutation in the
extensions->cellLock() (or equivalent lock guard) before calling
m_registeredFunctions[index].set(...) so writers synchronize with readers in
visitChildrenImpl, ensuring the lock is acquired and released properly around
the set operation.
🪄 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: e31d8141-4603-41e0-bab4-86356268d9c2
📒 Files selected for processing (1)
src/jsc/bindings/JSCommonJSExtensions.cpp
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical GC-correctness fix that mirrors the existing cellLock() pattern in JSCommonJSModule::m_children.
Extended reasoning...
Overview
This PR touches a single file, src/jsc/bindings/JSCommonJSExtensions.cpp, with two narrow changes to the three extern "C" helpers (__appendFunction / __setFunction / __swapRemove) and to visitChildrenImpl: (1) pass extensions instead of globalObject as the owner cell to WriteBarrier::set(), and (2) wrap mutation and iteration of m_registeredFunctions (a WTF::Vector<WriteBarrier<Unknown>>) in cellLock().
Security risks
None. This is internal JSC GC bookkeeping with no user-facing input handling, auth, crypto, or permission changes.
Level of scrutiny
Low. Both fixes are textbook JSC GC hygiene and copy the exact pattern already applied to JSCommonJSModule::m_children in this codebase (WTF::Locker locker { ->cellLock() } around vector append/clear and around visitor.append in visitChildrenImpl). The WriteBarrier::set owner argument must be the cell whose visitChildren scans the slot — extensions is unambiguously correct here and __appendFunction already did it that way; the other two call sites were just inconsistent.
Other factors
The bug-hunter inline note observes that these three functions currently have no Zig call sites (m_registeredFunctions is always empty), so the change has no runtime effect today. That cuts both ways: it means the PR can't be the actual fix for the reported SlotVisitor::drain segfault, but it also means there is effectively zero regression risk. Hardening latent code to match the established locking pattern is reasonable; whether to delete the dead code instead is a follow-up cleanup decision, not a blocker for this PR. No outstanding human reviewer comments and no CODEOWNERS for this path.
|
@robobun adopt |
|
✅ Adopted — ready for merge.
All review threads resolved. CI: all Linux (including Note: the three |
For consistency with __appendFunction and __swapRemove, take cellLock() in __setFunction so all mutators of m_registeredFunctions synchronize with visitChildrenImpl running on parallel mark threads.
Mirrors module-children-concurrent-gc.test.ts (#29995): churns Module._extensions handlers under BUN_JSC_collectContinuously=1 so the JSCommonJSExtensions cell is repeatedly visited on concurrent mark threads while the mutator registers/replaces/deletes handlers, and asserts the program runs to completion with correct output.
The test exercises put/defineOwnProperty/deleteProperty on the JSCommonJSExtensions cell under concurrent GC, which routes through jsc.Strong in Zig rather than the (currently caller-less) m_registeredFunctions helpers. Make the header and test name describe what is actually covered, and note explicitly that m_registeredFunctions stays empty today so the test is a guard for the live paths and for visitChildrenImpl's locking rather than a direct repro of the WriteBarrier-owner / cellLock bugs in the dead helpers.
There was a problem hiding this comment.
LGTM — 7c5ca15 reworded the test header/name as suggested, and all three mutators + visitChildrenImpl now take cellLock() with the correct WriteBarrier owner.
Extended reasoning...
Overview
This PR makes two GC-correctness fixes to src/jsc/bindings/JSCommonJSExtensions.cpp:
- Corrects the
WriteBarrier::set()owner cell fromglobalObjecttoextensionsin__setFunctionand__swapRemoveso eden collections re-scan the right object. - Adds
WTF::Locker { cellLock() }around all threem_registeredFunctionsmutators and around the iteration invisitChildrenImpl, mirroring the established fix forJSCommonJSModule::m_childrenin #29995.
It also adds test/js/node/module/module-extensions-concurrent-gc.test.ts, a concurrent-GC smoke test for Module._extensions put/defineOwnProperty/deleteProperty paths, modeled on the existing module-children-concurrent-gc test.
Security risks
None. No user-controlled input parsing, auth, crypto, or network surface is touched. The change is purely internal GC locking/write-barrier hygiene.
Level of scrutiny
Low-to-moderate. The C++ diff is ~10 lines of mechanical lock insertions and two one-token owner-cell corrections that follow an existing precedent in this codebase (#29995 / bd5149f). As noted in the resolved review threads, the three extern "C" helpers have no Zig callers since #19231 switched CustomLoader.custom to jsc.Strong, so m_registeredFunctions is always empty at runtime — the blast radius of these changes is effectively zero today, and they make the code correct should the path ever be re-enabled.
Other factors
- Bug-hunting system found no issues.
- Both prior review threads (CodeRabbit's missing lock in
__setFunction; my note that the test header/name overstated coverage) are resolved — 8b7c175 added the lock, and 7c5ca15 reworded the test to accurately describe what it exercises (including an explicit note that it cannot observe a regression in the dead helpers). - The new test follows the harness conventions (
bunEnv,bunExe,tempDir,isWindowsskip) and the same shape as the siblingmodule-children-concurrent-gctest. - A maintainer adopted the PR via robobun and confirmed local test passes; prior CI failures were unrelated flakes.
Given the small, pattern-following diff on currently-unreachable code plus an accurately-labeled smoke test, this is safe to approve without further human review. Dead-code removal of m_registeredFunctions and the three helpers remains a clean follow-up.
2 participants
What
Two GC-correctness fixes for
JSCommonJSExtensions::m_registeredFunctionsinsrc/jsc/bindings/JSCommonJSExtensions.cpp.1. Wrong
WriteBarrier::set()owner cellJSCommonJSExtensions__setFunction(line 215) andJSCommonJSExtensions__swapRemove(line 229) passglobalObjectas the owner argument toWriteBarrier::set():WriteBarrier::set(vm, owner, value)registersownerin the GC's remembered set so the write is re-scanned during an eden collection. But it isJSCommonJSExtensions::visitChildrenImpl— not the global object'svisitChildren— that scansm_registeredFunctions. WithglobalObjectas the registered owner, an eden collection whereextensionsis already old/black andvalueis young never re-marksvalue, and it can be collected while still referenced. The next timevisitChildrenImplruns,visitor.append(func)reads a dangling cell pointer.JSCommonJSExtensions__appendFunction(line 208) already passes the correct owner (extensions); these two call sites were the only ones that did not.2. Missing
cellLock()aroundVector<WriteBarrier<>>mutation/visitm_registeredFunctionsis aWTF::Vector<WriteBarrier<Unknown>>mutated on the mutator thread by__appendFunction/__setFunction/__swapRemove(append()/takeLast()/removeLast()/clear()) without any lock, whilevisitChildrenImpliterates it on parallel mark threads. AVectorreallocation can free the backing buffer while a mark thread is mid-scan, putting garbageJSCell*pointers on the mark stack.This is the same pattern that was fixed for
JSCommonJSModule::m_childrenin #29995 (bd5149f927). This change applies the samecellLock()discipline tom_registeredFunctions.Context
Found while auditing the bindings for sources of an intermittent JSC GC segfault on Linux x64 with concurrent GC enabled —
SlotVisitor::drainfaults on parallel-mark helper threads, and~InlineCacheHandler()faults duringHeap::finalizeUnconditionalFinalizers(). These two issues are correctness bugs regardless of whether they are the root cause of those particular crash reports.Reproduction hint
with a workload that registers/swaps
Module._extensionshandlers (e.g.ts-node,source-map-support) under load.