Bug 119919 - Concurrent JIT crashes in various fast/js/dfg-* tests while the main thread is setting innerHTML
Summary: Concurrent JIT crashes in various fast/js/dfg-* tests while the main thread i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-16 19:48 PDT by Filip Pizlo
Modified: 2013-08-20 12:39 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.94 KB, patch)
2013-08-17 16:20 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (11.94 KB, patch)
2013-08-20 11:57 PDT, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2013-08-16 19:48:16 PDT
In one of the crashes that I saw, the concurrent JIT crashed in QueryableExitSite::hasExitSite(), somewhere in the HashTable's iterator creation.  Super weird.
Comment 1 Filip Pizlo 2013-08-17 14:41:04 PDT
I'm seeing almost identical crashes every ~30 or so runs of v8-v6/v8-deltablue.js in the jsc shell.
Comment 2 Filip Pizlo 2013-08-17 14:53:25 PDT
Found it!!

This is because lazy write barrier adding stores pointers directly into CodeBlock::m_constantRegisters, even while it appends things to that vector, leading to resizes, leading to the pointers being borked.

This appears to be more likely to cause crashes with concurrent JIT because there is more malloc churn while the DFG corrupts memory; in sequential JIT you're less likely to crash because all of the data structures probably get allocated in one go, then they slightly corrupt each other, and then they're all freed.

The fix is to have DesiredWriteBarriers have some way of referencing a write barrier without necessarily using a direct pointer.  Probably, DesiredWriteBarrier should have a Vector*+index mode.
Comment 3 Mark Hahnenberg 2013-08-17 16:20:22 PDT
Created attachment 209014 [details]
Patch
Comment 4 WebKit Commit Bot 2013-08-17 20:07:43 PDT
Comment on attachment 209014 [details]
Patch

Clearing flags on attachment: 209014

Committed r154245: <http://trac.webkit.org/changeset/154245>
Comment 5 WebKit Commit Bot 2013-08-17 20:07:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Filip Pizlo 2013-08-19 12:15:24 PDT
I'm still seeing these crashes, for example:

http://build.webkit.org/results/Apple%20Lion%20Debug%20WK1%20(Tests)/r154283%20(10216)/fast/js/dfg-intrinsic-unused-this-method-check-crash-log.txt

Again, the concurrent JIT thread has crashed in a way that strongly implies malloc corruption.
Comment 7 Mark Hahnenberg 2013-08-20 08:35:04 PDT
Ugh, this is just embarrassing. Working on a fix.
Comment 8 Mark Hahnenberg 2013-08-20 11:57:23 PDT
Created attachment 209215 [details]
Patch
Comment 9 Mark Hahnenberg 2013-08-20 12:00:01 PDT
Comment on attachment 209215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209215&action=review

typos galore

> Source/JavaScriptCore/ChangeLog:8
> +        More fixes for WriteBarrier deferral during concurrent JIT-ing. This patch makes the use of DesiredWriteBarriers class and the 
> +        initializeLazyWriteBarrierFor* wrapper functions. 

...makes the use of the DesiredWriteBarriers class and the initializeLazyWriteBarrierFor* wrapper functions more sane.

> Source/JavaScriptCore/ChangeLog:10
> +        Refactored DesiredWriteBarrier to require an owner, a type, a CodeBlock, and an index.. The type indicates how to use CodeBlock

s/../.

s/use CodeBlock/use the CodeBlock
Comment 10 Geoffrey Garen 2013-08-20 12:03:39 PDT
Comment on attachment 209215 [details]
Patch

r=me
Comment 11 Filip Pizlo 2013-08-20 12:03:58 PDT
Comment on attachment 209215 [details]
Patch

r=me too.
Comment 12 Mark Hahnenberg 2013-08-20 12:07:08 PDT
Comment on attachment 209215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209215&action=review

Noticed an issue.

> Source/JavaScriptCore/dfg/DFGDesiredWriteBarriers.cpp:64
> +        InlineCallFrame& inlineCallFrame = m_codeBlock->inlineCallFrames()[m_index];
> +        if (!!inlineCallFrame.callee)
> +            break;

This check is redundant because we never would have added this DesiredWriteBarrier if the callee was null. Also, the check itself is the opposite of what it is meant to check :-( I'm going to replace it with an ASSERT (with the correct condition, of course).
Comment 13 Mark Hahnenberg 2013-08-20 12:39:02 PDT
Committed r154351: <http://trac.webkit.org/changeset/154351>