Bug 116134 - fourthTier: infrequent segfault in DFGCodeBlocks::deleteUnmarkedJettisonedCodeBlocks()
Summary: fourthTier: infrequent segfault in DFGCodeBlocks::deleteUnmarkedJettisonedCod...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-14 21:04 PDT by Mark Hahnenberg
Modified: 2013-05-16 11:11 PDT (History)
1 user (show)

See Also:


Attachments
crash log (19.48 KB, application/octet-stream)
2013-05-14 21:04 PDT, Mark Hahnenberg
no flags Details
Debug crash log (22.95 KB, application/octet-stream)
2013-05-15 06:53 PDT, Mark Hahnenberg
no flags Details
Patch (2.73 KB, patch)
2013-05-16 10:57 PDT, Mark Hahnenberg
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2013-05-14 21:04:37 PDT
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.
Comment 1 Mark Hahnenberg 2013-05-14 21:08:53 PDT
Looks like disabling the DFG causes the crash to go away.
Comment 2 Mark Hahnenberg 2013-05-14 21:09:31 PDT
These crashes also occur when the FTL and concurrent compilation are disabled.
Comment 3 Mark Hahnenberg 2013-05-15 06:53:03 PDT
Created attachment 201823 [details]
Debug crash log

Attaching a crash log generated with a debug build to get a more detailed backtrace.
Comment 4 Mark Hahnenberg 2013-05-15 20:52:26 PDT
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
Comment 5 Mark Hahnenberg 2013-05-16 08:47:58 PDT
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.
Comment 6 Mark Hahnenberg 2013-05-16 09:57:29 PDT
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.
Comment 7 Filip Pizlo 2013-05-16 09:59:00 PDT
(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.
Comment 8 Mark Hahnenberg 2013-05-16 10:00:12 PDT
(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.
Comment 9 Mark Hahnenberg 2013-05-16 10:57:53 PDT
Created attachment 201971 [details]
Patch
Comment 10 Mark Hahnenberg 2013-05-16 11:11:57 PDT
Committed r150188: <http://trac.webkit.org/changeset/150188>