RESOLVED FIXED 119919
Concurrent JIT crashes in various fast/js/dfg-* tests while the main thread is setting innerHTML 
https://bugs.webkit.org/show_bug.cgi?id=119919
Summary Concurrent JIT crashes in various fast/js/dfg-* tests while the main thread i...
Filip Pizlo
Reported Saturday, August 17, 2013 3:48:16 AM UTC
In one of the crashes that I saw, the concurrent JIT crashed in QueryableExitSite::hasExitSite(), somewhere in the HashTable's iterator creation. Super weird.
Attachments
Patch (7.94 KB, patch)
2013-08-17 16:20 PDT, Mark Hahnenberg 		no flags
DetailsFormatted DiffDiff
Patch (11.94 KB, patch)
2013-08-20 11:57 PDT, Mark Hahnenberg 		ggaren: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Filip Pizlo
Comment 1 Saturday, August 17, 2013 10:41:04 PM UTC
I'm seeing almost identical crashes every ~30 or so runs of v8-v6/v8-deltablue.js in the jsc shell.
Filip Pizlo
Comment 2 Saturday, August 17, 2013 10:53:25 PM UTC
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.
Mark Hahnenberg
Comment 3 Sunday, August 18, 2013 12:20:22 AM UTC
Created attachment 209014 [details] Patch
WebKit Commit Bot
Comment 4 Sunday, August 18, 2013 4:07:43 AM UTC
Comment on attachment 209014 [details] Patch Clearing flags on attachment: 209014 Committed r154245: <http://trac.webkit.org/changeset/154245>
WebKit Commit Bot
Comment 5 Sunday, August 18, 2013 4:07:47 AM UTC
All reviewed patches have been landed. Closing bug.
Filip Pizlo
Comment 6 Monday, August 19, 2013 8:15:24 PM UTC
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.
Mark Hahnenberg
Comment 7 Tuesday, August 20, 2013 4:35:04 PM UTC
Ugh, this is just embarrassing. Working on a fix.
Mark Hahnenberg
Comment 8 Tuesday, August 20, 2013 7:57:23 PM UTC
Created attachment 209215 [details] Patch
Mark Hahnenberg
Comment 9 Tuesday, August 20, 2013 8:00:01 PM UTC
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
Geoffrey Garen
Comment 10 Tuesday, August 20, 2013 8:03:39 PM UTC
Comment on attachment 209215 [details] Patch r=me
Filip Pizlo
Comment 11 Tuesday, August 20, 2013 8:03:58 PM UTC
Comment on attachment 209215 [details] Patch r=me too.
Mark Hahnenberg
Comment 12 Tuesday, August 20, 2013 8:07:08 PM UTC
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).
Mark Hahnenberg
Comment 13 Tuesday, August 20, 2013 8:39:02 PM UTC
Committed r154351: <http://trac.webkit.org/changeset/154351>
Note You need to log in before you can comment on or make changes to this bug.