From 34644bc4823bd7f582f55a4ca371d67df4717653 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 31 Aug 2026 23:21:54 +0000 Subject: [PATCH 1/2] JIT: read single-character string constants without a GCOwnedDataScope 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. --- Source/JavaScriptCore/heap/GCOwnedDataScope.cpp | 8 ++++++++ Source/JavaScriptCore/jit/JITArithmetic.cpp | 8 ++++++-- Source/JavaScriptCore/lol/LOLJIT.cpp | 8 ++++++-- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/Source/JavaScriptCore/heap/GCOwnedDataScope.cpp b/Source/JavaScriptCore/heap/GCOwnedDataScope.cpp index c49a6ed6c5b8..ec6aaa37c64b 100644 --- a/Source/JavaScriptCore/heap/GCOwnedDataScope.cpp +++ b/Source/JavaScriptCore/heap/GCOwnedDataScope.cpp @@ -32,10 +32,16 @@ namespace JSC { #if ASSERT_ENABLED +// Heap::m_topGCOwnedDataScope tracks the scopes on the mutator's stack, and it is not synchronized. +// Heap::clearConcurrentRetainedDataIfPossible asserts it is null before it drops the strings it kept +// alive for those scopes. Compiler and GC threads have their own guards there (no ongoing compilation, +// mutator not fenced), so a scope constructed on one of them must not touch the mutator's tracker. void setTopGCOwnedDataScopeIfNeeded(const JSCell* cell, const void* scope) { if (!cell) return; + if (isCompilationThread() || Thread::mayBeGCThread()) + return; if (!cell->vm().heap.m_topGCOwnedDataScope) cell->vm().heap.m_topGCOwnedDataScope = scope; } @@ -44,6 +50,8 @@ void clearTopGCOwnedDataScopeIfNeeded(const JSCell* cell, const void* scope) { if (!cell) return; + if (isCompilationThread() || Thread::mayBeGCThread()) + return; if (cell->vm().heap.m_topGCOwnedDataScope == scope) cell->vm().heap.m_topGCOwnedDataScope = nullptr; } diff --git a/Source/JavaScriptCore/jit/JITArithmetic.cpp b/Source/JavaScriptCore/jit/JITArithmetic.cpp index 65c62783179e..ac903baa777e 100644 --- a/Source/JavaScriptCore/jit/JITArithmetic.cpp +++ b/Source/JavaScriptCore/jit/JITArithmetic.cpp @@ -234,17 +234,21 @@ ALWAYS_INLINE void JIT::emit_compareImpl(VirtualRegister op1, VirtualRegister op // - constant int immediate to int immediate // - int immediate to int immediate - constexpr bool disallowAllocation = false; auto handleConstantCharOperand = [&](VirtualRegister left, VirtualRegister right, RelationalCondition cond) { if (!isOperandConstantChar(left)) return false; + // This may run on a compiler thread, so read the StringImpl directly rather than through + // tryGetValue(), whose GCOwnedDataScope is only meant to be constructed on the mutator. + // A single character constant is never a rope. + const StringImpl* impl = asString(getConstantOperand(left))->tryGetValueImpl(); + RELEASE_ASSERT(impl); emitGetVirtualRegister(right, jsRegT10); addSlowCase(branchIfNotCell(jsRegT10)); JumpList failures; emitLoadCharacterString(jsRegT10.payloadGPR(), jsRegT10.payloadGPR(), failures); addSlowCase(failures); - emitCompare(commute(cond), jsRegT10, Imm32(asString(getConstantOperand(left))->tryGetValue(disallowAllocation).data[0])); + emitCompare(commute(cond), jsRegT10, Imm32(impl->at(0))); return true; }; diff --git a/Source/JavaScriptCore/lol/LOLJIT.cpp b/Source/JavaScriptCore/lol/LOLJIT.cpp index 8b5f990d0396..63ba4f2d6397 100644 --- a/Source/JavaScriptCore/lol/LOLJIT.cpp +++ b/Source/JavaScriptCore/lol/LOLJIT.cpp @@ -961,17 +961,21 @@ ALWAYS_INLINE void LOLJIT::emitCompareImpl(VirtualRegister op1, JSValueRegs op1R // - constant int immediate to int immediate // - int immediate to int immediate - constexpr bool disallowAllocation = false; auto handleConstantCharOperand = [&](VirtualRegister left, JSValueRegs rightRegs, RelationalCondition cond) { if (!isOperandConstantChar(left)) return false; + // This may run on a compiler thread, so read the StringImpl directly rather than through + // tryGetValue(), whose GCOwnedDataScope is only meant to be constructed on the mutator. + // A single character constant is never a rope. + const StringImpl* impl = asString(getConstantOperand(left))->tryGetValueImpl(); + RELEASE_ASSERT(impl); addSlowCase(branchIfNotCell(rightRegs)); JumpList failures; // FIXME: We could deduplicate the String's data load in emitLoadCharacterString if we had an extra scratch but we'd have to teach the register allocator about constants to do that unless we wanted to have the scratch in all cases, which doesn't seem worth it. emitLoadCharacterString(rightRegs.payloadGPR(), s_scratch, failures); addSlowCase(failures); - emitCompare(commute(cond), JSValueRegs { s_scratch }, Imm32(asString(getConstantOperand(left))->tryGetValue(disallowAllocation).data[0])); + emitCompare(commute(cond), JSValueRegs { s_scratch }, Imm32(impl->at(0))); return true; }; From 8d674e63f8a9d3e0e345adeb013cbe784e853f94 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:31:25 +0000 Subject: [PATCH 2/2] JIT: read the constant character right after loading its StringImpl 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. --- Source/JavaScriptCore/jit/JITArithmetic.cpp | 6 ++++-- Source/JavaScriptCore/lol/LOLJIT.cpp | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Source/JavaScriptCore/jit/JITArithmetic.cpp b/Source/JavaScriptCore/jit/JITArithmetic.cpp index ac903baa777e..37a19878e914 100644 --- a/Source/JavaScriptCore/jit/JITArithmetic.cpp +++ b/Source/JavaScriptCore/jit/JITArithmetic.cpp @@ -239,16 +239,18 @@ ALWAYS_INLINE void JIT::emit_compareImpl(VirtualRegister op1, VirtualRegister op return false; // This may run on a compiler thread, so read the StringImpl directly rather than through // tryGetValue(), whose GCOwnedDataScope is only meant to be constructed on the mutator. - // A single character constant is never a rope. + // Read the character at once: the mutator may atomize the constant at any time, and a GC + // during this compile then drops the old StringImpl. A single character constant is never a rope. const StringImpl* impl = asString(getConstantOperand(left))->tryGetValueImpl(); RELEASE_ASSERT(impl); + char16_t character = impl->at(0); emitGetVirtualRegister(right, jsRegT10); addSlowCase(branchIfNotCell(jsRegT10)); JumpList failures; emitLoadCharacterString(jsRegT10.payloadGPR(), jsRegT10.payloadGPR(), failures); addSlowCase(failures); - emitCompare(commute(cond), jsRegT10, Imm32(impl->at(0))); + emitCompare(commute(cond), jsRegT10, Imm32(character)); return true; }; diff --git a/Source/JavaScriptCore/lol/LOLJIT.cpp b/Source/JavaScriptCore/lol/LOLJIT.cpp index 63ba4f2d6397..c716506e505f 100644 --- a/Source/JavaScriptCore/lol/LOLJIT.cpp +++ b/Source/JavaScriptCore/lol/LOLJIT.cpp @@ -966,16 +966,18 @@ ALWAYS_INLINE void LOLJIT::emitCompareImpl(VirtualRegister op1, JSValueRegs op1R return false; // This may run on a compiler thread, so read the StringImpl directly rather than through // tryGetValue(), whose GCOwnedDataScope is only meant to be constructed on the mutator. - // A single character constant is never a rope. + // Read the character at once: the mutator may atomize the constant at any time, and a GC + // during this compile then drops the old StringImpl. A single character constant is never a rope. const StringImpl* impl = asString(getConstantOperand(left))->tryGetValueImpl(); RELEASE_ASSERT(impl); + char16_t character = impl->at(0); addSlowCase(branchIfNotCell(rightRegs)); JumpList failures; // FIXME: We could deduplicate the String's data load in emitLoadCharacterString if we had an extra scratch but we'd have to teach the register allocator about constants to do that unless we wanted to have the scratch in all cases, which doesn't seem worth it. emitLoadCharacterString(rightRegs.payloadGPR(), s_scratch, failures); addSlowCase(failures); - emitCompare(commute(cond), JSValueRegs { s_scratch }, Imm32(impl->at(0))); + emitCompare(commute(cond), JSValueRegs { s_scratch }, Imm32(character)); return true; };