fitcored crashes at JavaScriptCore: JSC::Heap::releaseAccessSlow

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

We are getting crash reports from fitcored like:

 JavaScriptCore: WTFCrashWithInfo(int, char const*, char const*, int)
 JavaScriptCore: JSC::Heap::releaseAccessSlow() <==
 JavaScriptCore: JSC::JITWorklist::waitUntilAllPlansForVMAreReady(JSC::VM&)
 JavaScriptCore: JSC::JITWorklist::cancelAllPlansForVM(JSC::VM&)
 JavaScriptCore: JSC::VM::~VM()
 JavaScriptCore: JSC::JSLock::willReleaseLock()
 JavaScriptCore: JSC::JSLock::unlock()
 JavaScriptCore: JSC::JSRunLoopTimer::timerDidFire()
 JavaScriptCore: JSC::JSRunLoopTimer::Manager::timerDidFire()
 JavaScriptCore: WTF::RunLoop::TimerBase::start(WTF::Seconds, bool)::$_1::__invoke(__CFRunLoopTimer*, void*)
 CoreFoundation: __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
 CoreFoundation: __CFRunLoopDoTimer
 CoreFoundation: __CFRunLoopDoTimers
 CoreFoundation: __CFRunLoopRun
 CoreFoundation: CFRunLoopRunSpecific
 Foundation: -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
 Foundation: -[NSRunLoop(NSRunLoop) run]
 SeymourServices: static DaemonProtocol.main()
 fitcored: main
 dyld: 

Although I am unable to reproduce the crash, the theory is that the heap access lock is being dropped in JSLock::willReleaseLock() so when we try unlocking it again in JITWorklist::waitUntilAllPlansForVMAreReady() via ReleaseHeapAccessScope, we fault.  This path can happen when the main thread becomes the sole reference to the VM during processing a request like the timer in this stack trace.  I suspect it is a race with other threads that are done using the VM.

[Michael Saboff]

<rdar://78758218>

[Radar WebKit Bug Importer]

<rdar://problem/80286291>

[Michael Saboff]

Created attachment 433066 [details]
Patch

[Geoffrey Garen]

Comment on attachment 433066 [details]
Patch

r=me

Is it ever correct to have heap access when we run the VM destructor? I think it is not. So, I think the VM destructor should RELEASE_ASSERT !hasAccess().

(I'm asking because that RELEASE_ASSERT would remove all the speculation in your reasoning.)

[Michael Saboff]

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 433066 [details]
> Patch
> 
> r=me
> 
> Is it ever correct to have heap access when we run the VM destructor? I
> think it is not. So, I think the VM destructor should RELEASE_ASSERT
> !hasAccess().

You can have heap access when you destruct the VM, so such a RELEASE_ASSERT is not valid.
 
> (I'm asking because that RELEASE_ASSERT would remove all the speculation in
> your reasoning.)

[Michael Saboff]

Committed r279677 (239482@main): <https://commits.webkit.org/239482@main>