JIT: read single-character string constants without a GCOwnedDataScope on the compiler thread - #544
JIT: read single-character string constants without a GCOwnedDataScope on the compiler thread#544robobun wants to merge 2 commits into
Conversation
…e on the compiler thread JIT::emit_compareImpl and LOLJIT::emitCompareImpl read a one-character string constant through JSString::tryGetValue(), which constructs a GCOwnedDataScope. In debug builds that writes Heap::m_topGCOwnedDataScope, an unsynchronized field that tracks the scopes on the mutator's stack. When the mutator's IncrementalSweeper fires while the JIT thread is inside that scope, Heap::clearConcurrentRetainedDataIfPossible hits ASSERTION FAILED: !m_topGCOwnedDataScope (Heap.cpp:1303). Read the constant through tryGetValueImpl() instead, like the other compiler-thread readers of string constants do. Also make setTopGCOwnedDataScopeIfNeeded and clearTopGCOwnedDataScopeIfNeeded ignore compilation and GC threads, so the tracker only ever reflects the mutator's stack.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it touches JIT codegen and GC-thread interaction in a CODEOWNER-covered path (/Source/JavaScriptCore), a jsc-reviewer look is still worthwhile.
What was reviewed:
tryGetValueImpl()+impl->at(0)is semantically equivalent to the oldtryGetValue(false).data[0]path (both yieldchar16_t), and matches the existing compiler-thread pattern inDFGAbstractInterpreterInlines.handJITOpcodes.cpp.isOperandConstantCharguaranteeslength() == 1, andtryGetValueImpl()returns null only for ropes, soRELEASE_ASSERT(impl)preserves the prior rope invariant without weakening it.- The
GCOwnedDataScope.cppearly-returns areASSERT_ENABLED-only and only skip writes to the unsynchronizedm_topGCOwnedDataScope;clearConcurrentRetainedDataIfPossiblestill gates onentryScope/ongoing-compilation for those threads.
Extended reasoning...
Overview
This PR touches three files: heap/GCOwnedDataScope.cpp (debug-only mutator-scope tracker), jit/JITArithmetic.cpp (Baseline JIT compare emission), and lol/LOLJIT.cpp (LOL JIT compare emission). The root cause being fixed is that the concurrent Baseline/LOL JIT called JSString::tryGetValue() on a compiler thread, which constructs a GCOwnedDataScope whose debug-build constructor/destructor write to Heap::m_topGCOwnedDataScope — an unsynchronized field intended only for the mutator. When IncrementalSweeper fired on the mutator while a JIT thread was inside such a scope, the ASSERT(!m_topGCOwnedDataScope) in clearConcurrentRetainedDataIfPossible could fail. The fix is two-layered: the JIT paths now read the constant via tryGetValueImpl() (raw StringImpl*, no scope), and the tracker functions early-return on compilation/GC threads as defense in depth.
Security risks
None identified. Release builds are unaffected: m_topGCOwnedDataScope and its assertion exist only under ASSERT_ENABLED, and the underlying StringImpl lifetime was already covered by the totalOngoingCompilations() guard in clearConcurrentRetainedDataIfPossible. The JIT change swaps one accessor for another that returns identical data (char16_t at index 0) without allocation or side effects; the RELEASE_ASSERT(impl) cannot fire because isOperandConstantChar requires a length-1 JSString, which is never a rope (small-string cache). No new untrusted input paths, no memory-safety change to generated code.
Level of scrutiny
Moderate-to-high. The diff is small (~20 net lines) and mechanically follows an established pattern already used on compiler threads elsewhere (DFGAbstractInterpreterInlines.h:1648/2322, JITOpcodes.cpp:730/749). However, it sits at the intersection of concurrent JIT compilation and GC bookkeeping — an area where subtle threading assumptions matter — and /Source/JavaScriptCore is CODEOWNER-covered by WebKit/jsc-reviewers. Per the approval guidelines, CODEOWNER coverage on a critical path means a human reviewer should sign off even when the automated review is clean.
Other factors
The PR description is unusually thorough (repro methodology, log-line verification of 32000→0 scope constructions, JSTests/stress compare tests passing), and both JIT tiers were updated symmetrically as required by the JSC CLAUDE.md convention. The disallowAllocation local is fully removed with no remaining references. Exit reason was dry_streak, so the bug hunt ran to completion with nothing found. Deferring rather than approving solely due to CODEOWNER coverage and the JIT/heap subject matter.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe changes prevent non-mutator threads from updating the GC-owned scope tracker. JIT and LOLJIT constant-character comparisons now read characters directly from ChangesCompiler and runtime safety
Merge Risk: ⚪ Minimal · up to This localized change preserves emitted behavior while removing compiler-thread debug-scope tracking; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed, on topic, and covers the problem, fix, background, testing, scope, and known limitations. It does not include the required Bugzilla link, review-status line, or template-style changed-file list, but the core information is substantially complete. Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
Preview Builds
|
The mutator can atomize a string constant while the baseline compile runs, and the whole baseline compile sits inside a Safepoint, so a GC during the compile can drop the old StringImpl from Heap::m_possiblyAccessedStringsFromConcurrentThreadsOrGCOwnedDataScope. Read the character as soon as the impl is loaded, which keeps the window the old tryGetValue(false).data[0] expression had, instead of holding the raw pointer across the emission calls.
2 participants
Problem
ASSERTION FAILED: !m_topGCOwnedDataScopeinHeap::clearConcurrentRetainedDataIfPossible(Heap.cpp:1303), fromIncrementalSweeper::doSweep. Seen once in a Bun ASan CI lane. It is a rare timing race.Heap::m_topGCOwnedDataScopeis an unsynchronizedASSERT_ENABLEDfield that tracks theGCOwnedDataScopeobjects on the mutator's stack. The concurrent baseline JIT writes it:JIT::emit_compareImpl(JITArithmetic.cpp:247) reads a one-character string constant withJSString::tryGetValue(), which constructs aGCOwnedDataScope, on theJITWorkerthread.LOLJIT::emitCompareImpl(LOLJIT.cpp:974) has the same code.Fix
tryGetValueImpl()andStringImpl::at(0), at once. That is how the other compiler-thread readers of string constants work (the DFG, the atom string fast paths in JITOpcodes.cpp). The emitted code is unchanged. The ropeRELEASE_ASSERTstays, as a null check.setTopGCOwnedDataScopeIfNeededandclearTopGCOwnedDataScopeIfNeededreturn early on a compilation thread or a GC thread. Those threads have their own guards inclearConcurrentRetainedDataIfPossible. Same thread predicate thatJSString::swapToAtomStringasserts. Debug-only code.s < "a"style compare sites.JSTests/stress/string-compare.jsand 40 other compare tests pass with the baseline JIT, the LOL JIT, and--useJIT=false.Background
GCOwnedDataScope<T>wraps data that aJSCellowns, here theStringinside aJSString. Its destructor callsensureStillAliveHere(owner). In debug builds it also maintainsHeap::m_topGCOwnedDataScope.JSStringto its atom, the oldStringImplgoes intoHeap::m_possiblyAccessedStringsFromConcurrentThreadsOrGCOwnedDataScope.clearConcurrentRetainedDataIfPossibledrops that list when no JS runs, no compile is in flight, and the mutator is not fenced. The assertion covers the one reader those checks miss: a scope on the mutator's stack.tryGetValueImpl()returns the rawStringImpl*, or null for a rope.Upstream
Nothing here is Bun-specific. Upstream
mainhas the same code. TheGCOwnedDataScope.cpphunk applies there as is. Upstream has since edited the two compare lambdas (LOLJIT's takes aGPRReg), so the two JIT hunks need a small re-resolution at the next upgrade merge.Notes
ASSERT_ENABLED, and the JIT hunks emit the same code.JITWorkerthread right aftersetTopGCOwnedDataScopeIfNeededran for its scope, let the main thread'sIncrementalSweepertimer fire, and the exact assertion fires with the stack seen in CI (clearConcurrentRetainedDataIfPossible <- IncrementalSweeper::doSweep <- JSRunLoopTimer::timerDidFire). An unaided stress loop (baseline compiles plusgc()every 350 ms) did not hit it in 6 runs of 2 to 3 minutes.setTopGCOwnedDataScopeIfNeeded, active whenisCompilationThread(), with--thresholdForJITAfterWarmUp=10 --thresholdForJITSoon=10. The probe script givestotal=14800under every tier configuration.--useConcurrentJIT=falsethe log line also fired 32000 times:JITPlan::compileInThreadenters aCompilationScopeon the mutator too. That case has no race, and the early return only skips tracking inside the synchronous compile, where the sweeper cannot run.tryGetValue()from compile code. The other callers run on the mutator (bytecode generator, runtime functions, inspector).BaselineJITPlan::compileInThreadImplruns the whole compile inside aSafepoint, andHeap::runCollectionEpilogue(Heap.cpp:2406) prunes the string list without a check for ongoing compilations. So a baseline compile that reads a non-atom constant's impl while the mutator atomizes it, and a GC runs before the read completes, reads a freed impl. The window is the few instructions between the fiber load and the read, the same as before this PR. The atom string fast paths in JITOpcodes.cpp have the same window onisAtom(). In this fork the bytecode cache can create such non-atom one-character constants (DecoderStringTable::jsStringFor, code units above 0xFF). A fix belongs in the prune: skip entries, or the whole prune, whiletotalOngoingCompilations()is non-zero.Number.prototype.toLocaleStringfails in it, so the threemissing-exception-check-in-string-*compare.jstests fail at their first line in that build, with and without the JIT.