Bug 116134 – fourthTier: infrequent segfault in DFGCodeBlocks::deleteUnmarkedJettisonedCodeBlocks()

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

Created attachment 201787 [details]
crash log

Encountered while running benchmarks on the branch. Seems to happen during random benchmarks. Running gbemu in a loop seems to always result in the crash eventually.

[Mark Hahnenberg]

Looks like disabling the DFG causes the crash to go away.

[Mark Hahnenberg]

These crashes also occur when the FTL and concurrent compilation are disabled.

[Mark Hahnenberg]

Created attachment 201823 [details]
Debug crash log

Attaching a crash log generated with a debug build to get a more detailed backtrace.

[Mark Hahnenberg]

Here's a backtrace I was able to get from the last call to the destructor of the corrupted JITCode object:

[0] - 0   JavaScriptCore                      0x0000000100499538 _ZN3JSC3DFG7JITCodeD2Ev + 88
[1] - 1   JavaScriptCore                      0x00000001004994c1 _ZN3JSC3DFG7JITCodeD0Ev + 17
[2] - 2   JavaScriptCore                      0x0000000100335cd7 _ZN3JSC18FunctionExecutable28jettisonOptimizedCodeForCallERNS_2VME + 87
[3] - 3   JavaScriptCore                      0x00000001002553f5 _ZN3JSC9CodeBlock23finalizeUnconditionallyEv + 3237
[4] - 4   JavaScriptCore                      0x00000001004a7fa9 _ZN3JSC11SlotVisitor31finalizeUnconditionalFinalizersEv + 57
[5] - 5   JavaScriptCore                      0x000000010033e6e4 _ZN3JSC4Heap7collectENS0_11SweepToggleE + 340
[6] - 6   JavaScriptCore                      0x0000000100266eda _ZN3JSC11CopiedSpace13allocateBlockEv + 74
[7] - 7   JavaScriptCore                      0x0000000100265e6e _ZN3JSC11CopiedSpace19tryAllocateSlowCaseEmPPv + 94
[8] - 8   JavaScriptCore                      0x00000001003d747e _ZN3JSC9Butterfly19growPropertyStorageERNS_2VMEmmbmm + 222
[9] - 9   JavaScriptCore                      0x00000001003d6063 _ZN3JSC8JSObject20growOutOfLineStorageERNS_2VMEmm + 115

[Mark Hahnenberg]

Since this seems like our ref count is getting corrupted (e.g. by a race condition) and that this crash is seemingly random and timing-related, I'd guess that this is a bug where parallel GC is corrupting the ref count of the JITCode object. I tried disabling parallel GC and haven't seen the crash yet (after running gbemu in a loop for ~5 minutes).

The fix is probably to make JITCode thread-safe ref-counted, but I'd like to track down exactly where we're racing on the ref count rather than blindly making the change.

[Mark Hahnenberg]

I set some breakpoints in RefCounted::ref and enabled them during the marking phase. We are ref-ing the JITCode object during calls to CodeBlock::getJITType because it's passing a RefPtr instead of a PassRefPtr to JITCode::jitTypeFor. This could happen on any of the marking threads (since it occurs during a call to CodeBlock::visitAggregate).

We don't necessarily have to change JITCode to ThreadSafeRefCounted. We could probably just fix this particular call by doing m_jitCode.get() and using a raw pointer in this case.

[Filip Pizlo]

(In reply to comment #6)
> I set some breakpoints in RefCounted::ref and enabled them during the marking phase. We are ref-ing the JITCode object during calls to CodeBlock::getJITType because it's passing a RefPtr instead of a PassRefPtr to JITCode::jitTypeFor. This could happen on any of the marking threads (since it occurs during a call to CodeBlock::visitAggregate).
> 
> We don't necessarily have to change JITCode to ThreadSafeRefCounted. We could probably just fix this particular call by doing m_jitCode.get() and using a raw pointer in this case.

I would vote for making JITCode and CodeBlock both be ThreadSafeRefCounted, since both will be used by both the compilation thread and the main thread.  So the fact that the GC threads are seeing concurrency issues also means that we have all the more reason to just thread safe it.

[Mark Hahnenberg]

(In reply to comment #7)
> (In reply to comment #6)
> > I set some breakpoints in RefCounted::ref and enabled them during the marking phase. We are ref-ing the JITCode object during calls to CodeBlock::getJITType because it's passing a RefPtr instead of a PassRefPtr to JITCode::jitTypeFor. This could happen on any of the marking threads (since it occurs during a call to CodeBlock::visitAggregate).
> > 
> > We don't necessarily have to change JITCode to ThreadSafeRefCounted. We could probably just fix this particular call by doing m_jitCode.get() and using a raw pointer in this case.
> 
> I would vote for making JITCode and CodeBlock both be ThreadSafeRefCounted, since both will be used by both the compilation thread and the main thread.  So the fact that the GC threads are seeing concurrency issues also means that we have all the more reason to just thread safe it.

Makes sense. Patch coming soon.

[Mark Hahnenberg]

Created attachment 201971 [details]
Patch

[Mark Hahnenberg]

Committed r150188: <http://trac.webkit.org/changeset/150188>