Bug 69599 – [JSC] JIT buffer refcounting causing assertions in debug WebSocket tests when using proxy PAC

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Dominic Cooney]

When I have a proxy PAC on OS X and run-webkit-tests --debug http/tests/websocket, I get a high rate (~8%) of DRT crashes with the below callstack. Removing proxy configuration PAC results in no DRT crashes. I believe JSC is used to interpret the proxy PAC file, and when it does this it reuses code buffers in different threads. We should verify that this reuse is safe, and if so, change the verifier to not squawk at this.

ASSERTION FAILED: m_verifier.isSafeToUse()
./wtf/RefCounted.h(122) : bool WTF::RefCountedBase::derefBase()
2   0x1002288a4 WTF::RefCountedBase::derefBase()
3   0x1003ac931 WTF::RefCounted<WTF::MetaAllocatorHandle>::deref()
4   0x100217303 void WTF::derefIfNotNull<WTF::MetaAllocatorHandle>(WTF::MetaAllocatorHandle*)
5   0x10021731e WTF::RefPtr<WTF::MetaAllocatorHandle>::~RefPtr()
6   0x1003ac96d JSC::MacroAssemblerCodeRef::~MacroAssemblerCodeRef()
7   0x1002175e3 JSC::JITCode::~JITCode()
8   0x100208244 JSC::CodeBlock::~CodeBlock()
9   0x1002179bf JSC::GlobalCodeBlock::~GlobalCodeBlock()
10  0x1002179f7 JSC::ProgramCodeBlock::~ProgramCodeBlock()
11  0x1002649f0 void WTF::deleteOwnedPtr<JSC::ProgramCodeBlock>(JSC::ProgramCodeBlock*)
12  0x100264a51 WTF::OwnPtr<JSC::ProgramCodeBlock>::clear()
13  0x100261770 JSC::ProgramExecutable::clearCodeVirtual()
14  0x100260262 JSC::ExecutableBase::clearCode(JSC::JSCell*)
15  0x1003bcbde JSC::Heap::FinalizerOwner::finalize(JSC::Handle<JSC::Unknown>, void*)
16  0x10026d1ec JSC::HandleHeap::finalizeWeakHandles()
17  0x1003be777 JSC::Heap::collect(JSC::Heap::SweepToggle)
18  0x1003d0042 JSC::AllocationSpace::allocateSlowCase(JSC::MarkedSpace::SizeClass&)
19  0x1001ca832 JSC::AllocationSpace::allocate(JSC::MarkedSpace::SizeClass&)
20  0x1002021b6 JSC::AllocationSpace::allocate(unsigned long)
21  0x100202223 JSC::Heap::allocate(unsigned long)
22  0x10024c692 void* JSC::allocateCell<JSC::JSFinalObject>(JSC::Heap&)
23  0x10024c6c4 JSC::JSFinalObject::create(JSC::ExecState*, JSC::Structure*)
24  0x1002ac281 JSC::constructEmptyObject(JSC::ExecState*, JSC::Structure*)
25  0x10024c72f JSC::constructEmptyObject(JSC::ExecState*, JSC::JSGlobalObject*)
26  0x1003333e1 JSC::constructEmptyObject(JSC::ExecState*)
27  0x10029bfaa cti_op_new_object
28  0x10029b301 jscGeneratedNativeCode
29  0x1002797f4 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*)
30  0x100273aaf JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
31  0x100205021 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
32  0x1002e6303 JSObjectCallAsFunction

[Dominic Cooney]

Here’s evidence of calling FindProxyForURL using the same objects.

ASSERTION FAILED: m_verifier.isSafeToUse()
./wtf/RefCounted.h(122) : bool WTF::RefCountedBase::derefBase()
2   0x1002288a4 WTF::RefCountedBase::derefBase()
3   0x1003ac931 WTF::RefCounted<WTF::MetaAllocatorHandle>::deref()
4   0x100217303 void WTF::derefIfNotNull<WTF::MetaAllocatorHandle>(WTF::MetaAllocatorHandle*)
5   0x1003acc49 WTF::RefPtr<WTF::MetaAllocatorHandle>::operator=(WTF::RefPtr<WTF::MetaAllocatorHandle> const&)
6   0x1003acc83 JSC::MacroAssemblerCodeRef::operator=(JSC::MacroAssemblerCodeRef const&)
7   0x100247b2d JSC::JITCode::operator=(JSC::JITCode const&)
8   0x100242466 JSC::DFG::JITCompiler::compileFunction(JSC::JITCode&, JSC::MacroAssemblerCodePtr&)
9   0x1001c11a9 JSC::DFG::compile(JSC::DFG::CompileMode, JSC::ExecState*, JSC::ExecState*, JSC::CodeBlock*, JSC::JITCode&, JSC::MacroAssemblerCodePtr*)
10  0x1001bc9a0 JSC::DFG::tryCompileFunction(JSC::ExecState*, JSC::ExecState*, JSC::CodeBlock*, JSC::JITCode&, JSC::MacroAssemblerCodePtr&)
11  0x100262d0e JSC::FunctionExecutable::compileForCallInternal(JSC::ExecState*, JSC::ScopeChainNode*, JSC::ExecState*, JSC::JITCode::JITType)
12  0x100263076 JSC::FunctionExecutable::compileOptimizedForCall(JSC::ExecState*, JSC::ScopeChainNode*, JSC::ExecState*)
13  0x100215c5d JSC::FunctionExecutable::compileOptimizedFor(JSC::ExecState*, JSC::ScopeChainNode*, JSC::CodeSpecializationKind)
14  0x100206766 JSC::FunctionCodeBlock::compileOptimized(JSC::ExecState*, JSC::ScopeChainNode*)
15  0x1002a2f86 cti_optimize_from_ret
16  0x10029b301 jscGeneratedNativeCode
17  0x1002797f4 JSC::JITCode::execute(JSC::RegisterFile*, JSC::ExecState*, JSC::JSGlobalData*)
18  0x100273aaf JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
19  0x100205021 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
20  0x1002e6303 JSObjectCallAsFunction
21  0x7fff8210c6c1 CallFindProxyForURL
22  0x7fff8210eb44 executionContextPerform(void*)
23  0x7fff83d22401 __CFRunLoopDoSources0
24  0x7fff83d205f9 __CFRunLoopRun
25  0x7fff83d1fdbf CFRunLoopRunSpecific
26  0x7fff8a090c64 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
27  0x100013788 runTest(std::string const&)
28  0x100013c9f runTestingServerLoop()
29  0x1000140b4 dumpRenderTree(int, char const**)
30  0x1000142d6 main
31  0x1000023a8 start

This is with a small PAC file:

function FindProxyForURL(url, host) {
  return 'DIRECT';
}

[Alexey Proskuryakov]

Does DFG JIT have thread safety issues?

[Geoffrey Garen]

FYI, what's interesting about a PAC (Proxy Auto-Config) file is that the networking thread will run the PAC .js code on a secondary thread while JS runs on the main thread.

A cursory glance shows that MetaAllocator uses locking in a seemingly appropriate way. I believe the bug here is that the RefCountedBase debug verifier is in SingleThreadVerificationMode, but the JSC API allows an object to be used on more than one thread, as long as it's not at the same time. 

NoVerificationMode seems to be the only mode in ThreadRestrictionVerifier that's compatible with our API.

[David Levin]

(In reply to comment #3)
> FYI, what's interesting about a PAC (Proxy Auto-Config) file is that the networking thread will run the PAC .js code on a secondary thread while JS runs on the main thread.
> 
> A cursory glance shows that MetaAllocator uses locking in a seemingly appropriate way. I believe the bug here is that the RefCountedBase debug verifier is in SingleThreadVerificationMode, but the JSC API allows an object to be used on more than one thread, as long as it's not at the same time. 
> 
> NoVerificationMode seems to be the only mode in ThreadRestrictionVerifier that's compatible with our API.

I haven't yet added anything for JSC, so this object will need to call the deprecated api that I put in for the other JSC objects.  If anyone would like to add to ThreadRestrictionVerifier to make a mode compatible with the JSC API, that would be delightful! (because I must admit that I don't understand it completely so it will take me a while when I get to it).

[Dominic Cooney]

Created attachment 110101 [details]
Patch

[Dominic Cooney]

(In reply to comment #3)
> A cursory glance shows that MetaAllocator uses locking in a seemingly appropriate way. I believe the bug here is that the RefCountedBase debug verifier is in SingleThreadVerificationMode, but the JSC API allows an object to be used on more than one thread, as long as it's not at the same time. 

It looks like a JITCode buffer touched by one thread is deref'ed during GC on another thread. Is the GC under a mutex? If not, should MetaAllocatorHandles be thread-shared refcounted?

> NoVerificationMode seems to be the only mode in ThreadRestrictionVerifier that's compatible with our API.

I have uploaded a patch that sets this mode with a FIXME to work out a more detailed verification scheme.

[Dominic Cooney]

Created attachment 110103 [details]
Alternative patch

[Dominic Cooney]

I am having a hard time convincing myself that this isn’t racy, given that callstacks include both JITting and GC; it isn’t immediately obvious what is stopping these being used concurrently. So I have uploaded an alternative patch that makes MetaAllocatorHandle : ThreadSafeRefCounted<…>.

[WebKit Review Bot]

Comment on attachment 110101 [details]
Patch

Clearing flags on attachment: 110101

Committed r96936: <http://trac.webkit.org/changeset/96936>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

> I am having a hard time convincing myself that this isn’t racy, given that callstacks include both JITting and GC; it isn’t immediately obvious what is stopping these being used concurrently.

The design here is that clients should either use separate virtual machines ("JSGlobalData") on separate threads, or use locking external to JSC to ensure that the same virtual machine is not used concurrently on two different threads. For PAC files, I believe you should end up with separate heaps on separate threads. 

The only shared resource here is the MetaAllocator. Access to the MetaAllocator should be locked and, as far as I can tell, is.

[Filip Pizlo]

(In reply to comment #11)
> > I am having a hard time convincing myself that this isn’t racy, given that callstacks include both JITting and GC; it isn’t immediately obvious what is stopping these being used concurrently.
> 
> The design here is that clients should either use separate virtual machines ("JSGlobalData") on separate threads, or use locking external to JSC to ensure that the same virtual machine is not used concurrently on two different threads. For PAC files, I believe you should end up with separate heaps on separate threads. 
> 
> The only shared resource here is the MetaAllocator. Access to the MetaAllocator should be locked and, as far as I can tell, is.

Indeed.

The code was definitely not racy, so this patch does more synchronization than necessary.  In particular, since:

- Each instance of JSC is itself not thread safe (i.e. if you call into one instance of JSC from multiple threads, you die), and

- An instance of JSC will never leak its meta allocator handles (since these are references to executable code, and executable code generated by one JSC instance is invalid for use in another),

Then if you do encounter a case where two threads are simultaneously trying to fiddle with the same meta allocator handle's reference count, it means that you're already dead for a multitude of other reasons.

And yes, it is true that there is one MetaAllocator instance globally, but that instance only hands out handles and frees them; there is no way for it to take one JSC instance's handle and somehow transfer it to someone else.  And MetaAllocator itself is thread safe by brute force: all entrypoints into it are locked.

But on the other hand, this patch doesn't appear to have regressed our performance.  So, I guess we can revisit this if we ever find that the synchronization on MetaAllocator handle starts to get hot.

[Filip Pizlo]

> NoVerificationMode seems to be the only mode in ThreadRestrictionVerifier that's compatible with our API.

Exactly!

The JavaScript language is not designed for concurrency.  That does not preclude multiple threads for running JS code within the same VM so long as the code that calls into JS ensures that there is no way for multiple threads to simultaneously run JS code in the same VM.  JavaScriptCore offloads this work onto the user of the JavaScriptCore APIs as a matter of policy.

So I don't see how anything but NoVerificationMode is appropriate for reference counted objects in JSC.

[David Levin]

(In reply to comment #13)
> > NoVerificationMode seems to be the only mode in ThreadRestrictionVerifier that's compatible with our API.
> 
> Exactly!
> 
> The JavaScript language is not designed for concurrency.  That does not preclude multiple threads for running JS code within the same VM so long as the code that calls into JS ensures that there is no way for multiple threads to simultaneously run JS code in the same VM.  JavaScriptCore offloads this work onto the user of the JavaScriptCore APIs as a matter of policy.
> 
> So I don't see how anything but NoVerificationMode is appropriate for reference counted objects in JSC.

To my memory I heard something about JSLock that may not do anything but would be useful for setting a flag.  Anyway, this is on my list for later. I have a bug about adding a mode for jsc.

Sorry for this disturbance. I "fixed" as many of the jsc objects as I found by using this deprecated api call.

[Alexey Proskuryakov]

JSLock is indeed something that automatically prevents concurrent JavaScript execution. It's an obsolete feature, and is only used for Mac OS X applications linked against old versions of WebKit.

Comments in JSContextRef.h have more information about JSC threading.

[Geoffrey Garen]

> But on the other hand, this patch doesn't appear to have regressed our performance.  So, I guess we can revisit this if we ever find that the synchronization on MetaAllocator handle starts to get hot.

Note that the fix that landed just disabled verification. The patch that changed to using ThreadSafeRefCounted did not land. (And should not, for the reasons you've explained.)