Bug 186827

Summary: Crash in sanitizeStackForVMImpl sometimes when switching threads with same VM
Product: WebKit Reporter: Michael Saboff <msaboff>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, keith_miller, mark.lam, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Michael Saboff 2018-06-19 16:52:35 PDT 
Crash something like:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000116f5af53 sanitizeStackForVMImpl + 15
1   com.apple.JavaScriptCore      	0x000000011754889c JSC::Heap::collectInMutatorThread() + 44
2   com.apple.JavaScriptCore      	0x0000000117548794 JSC::Heap::stopIfNecessarySlow() + 68
3   com.apple.JavaScriptCore      	0x0000000117548a0f JSC::Heap::acquireAccessSlow() + 303
4   com.apple.JavaScriptCore      	0x000000011784ae6f JSC::JSLock::didAcquireLock() + 111
5   com.apple.JavaScriptCore      	0x0000000117868f9e JSC::JSRunLoopTimer::timerDidFireCallback(__CFRunLoopTimer*, void*) + 30
6   com.apple.CoreFoundation      	0x000000010e782124 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
7   com.apple.CoreFoundation      	0x000000010e781cc9 __CFRunLoopDoTimer + 877
8   com.apple.CoreFoundation      	0x000000010e78189e __CFRunLoopDoTimers + 333
9   com.apple.CoreFoundation      	0x000000010e77a023 __CFRunLoopRun + 2193
...

This can occur if the VM was used on another thread and we need to take the Heap::acquireAccessSlow() path.  This happens because we switch VM::lastStackTop after calling Heap::acquireAccess().  The slow path of acquireAccess() ends up calling Heap::collectInMutatorThread() which sanitizes the stack, and if we haven't switched lastStackTop, we'll sanitize the current stack using the prior limits of another stack.  Then we'll walk all over memory and/or crash on an unmapped page.
Comment 1 Michael Saboff 2018-06-19 16:52:46 PDT 
<rdar://problem/40624786>
Comment 2 Michael Saboff 2018-06-19 16:57:24 PDT 
Created attachment 343121 [details]
Patch
Comment 3 Saam Barati 2018-06-19 16:58:27 PDT 
Comment on attachment 343121 [details]
Patch

Can we add a test for this?
Comment 4 Michael Saboff 2018-06-19 17:06:47 PDT 
(In reply to Saam Barati from comment #3)
> Comment on attachment 343121 [details]
> Patch
> 
> Can we add a test for this?

I tried pretty hard in testapi.  The problem is how do you force Heap::acquireAccess() to take the slow path.  As a consolation, I did move the ASSERT() as well as the setting.
Comment 5 WebKit Commit Bot 2018-06-19 17:45:14 PDT 
Comment on attachment 343121 [details]
Patch

Clearing flags on attachment: 343121

Committed r232998: <https://trac.webkit.org/changeset/232998>
Comment 6 WebKit Commit Bot 2018-06-19 17:45:15 PDT 
All reviewed patches have been landed.  Closing bug.