Bug 69599

Summary:

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

Product:

WebKit

Reporter:

Dominic Cooney <dominicc>

Component:

JavaScriptCore

Assignee:

Dominic Cooney <dominicc>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, barraclough, fpizlo, ggaren, levin, levin+threading, sam, webkit.review.bot, yutak

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

67329

Attachments:

Description	Flags
Patch	none
Alternative patch	none

Dominic Cooney

Reported 2011-10-06 20:00:29 PDT

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

Attachments
Patch (1.92 KB, patch) 2011-10-07 00:52 PDT, Dominic Cooney	no flags	Details Formatted Diff Diff
Alternative patch (1.81 KB, patch) 2011-10-07 01:36 PDT, Dominic Cooney	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Dominic Cooney

Comment 1 2011-10-06 20:37:55 PDT

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

Comment 2 2011-10-06 20:47:12 PDT

Does DFG JIT have thread safety issues?

Geoffrey Garen

Comment 3 2011-10-06 20:59:35 PDT

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

Comment 4 2011-10-06 21:02:16 PDT

(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

Comment 5 2011-10-07 00:52:27 PDT

Created attachment 110101 [details] Patch

Dominic Cooney

Comment 6 2011-10-07 00:57:51 PDT

(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

Comment 7 2011-10-07 01:36:12 PDT

Created attachment 110103 [details] Alternative patch

Dominic Cooney

Comment 8 2011-10-07 01:39:29 PDT

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 9 2011-10-07 07:48:54 PDT

Comment on attachment 110101 [details] Patch Clearing flags on attachment: 110101 Committed r96936: <http://trac.webkit.org/changeset/96936>

WebKit Review Bot

Comment 10 2011-10-07 07:49:00 PDT

All reviewed patches have been landed. Closing bug.

Geoffrey Garen

Comment 11 2011-10-07 09:38:36 PDT

> 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

Comment 12 2011-10-07 12:32:29 PDT

(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

Comment 13 2011-10-07 12:36:05 PDT

> 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

Comment 14 2011-10-07 12:40:32 PDT

(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

Comment 15 2011-10-07 13:00:28 PDT

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

Comment 16 2011-10-07 14:11:33 PDT

> 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.)

Note You need to log in before you can comment on or make changes to this bug.