Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Seg fault when accessing Context from multiple threads #5577

Closed
crdumoul opened this issue Aug 7, 2018 · 10 comments
Closed

Seg fault when accessing Context from multiple threads #5577

crdumoul opened this issue Aug 7, 2018 · 10 comments
Labels

Comments

@crdumoul
Copy link

crdumoul commented Aug 7, 2018

I've got a test program that does the following in two separate threads. I understand that the runtime and context can only be accessed by one thread at a time.

In the main thread, create a runtime and a context, and execute some JavaScript code to add a function called "square" to the global object. The JavaScript code being run is very simple:
function square(number) { return number * number; }

The API calls in the main thread are the following, in this order:
JsCreateRuntime
JsCreateContext
JsAddRef: passing in the JsContextRef so the Context won't be garbage collected.
JsSetCurrentContext
JsCreateExternalArrayBuffer: create a buffer that points to the JavaScript code to be run.
JsCreateString: create a string for the URL parameter to JsRun
JsRun: run the JavaScript code to create the "square" function.
JsSetCurrentContext(JS_INVALID_REFERENCE)

After that a second thread is spawned. This second thread will be calling the "square" function. It makes the following API calls, in this order:
JsSetCurrentContext
JsGetGlobalObject
JsCreatePropertyId

At this point the second thread occasionally seg faults (SIGSEGV) in the call to JsCreatePropertyId.

The back trace looks like:
#0 0x000055e69854223e in Memory::RecyclerWriteBarrierManager::WriteBarrier(void*) ()
#1 0x000055e69857d6e7 in ThreadContext::GetOrAddPropertyId(char16_t const*, int, Js::PropertyRecord const**) ()
#2 0x000055e6985698ae in Js::ScriptContext::GetOrAddPropertyRecord(char16_t const*, int, Js::PropertyRecord const**) ()
#3 0x000055e6984b252b in JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1} ContextAPINoScriptWrapper_Core<_JsErrorCode ContextAPINoScriptWrapper_NoRecord<JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1}>(JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1}, bool, bool)::{lambda(Js::ScriptContext*)#1}>(_JsErrorCode, bool, bool) ()
#4 0x000055e6984af6cb in JsCreatePropertyId ()

This test program is written in Rust, which is why I'm not just including the source code, but instead trying to describe the API calls I'm making.

I'd appreciate any help in figuring out what I'm doing wrong here.

@crdumoul
Copy link
Author

crdumoul commented Aug 7, 2018

I'm wondering if what I'm doing wrong here is calling JsAddRef (to prevent the Context from being garbage collected) without any active context.

@kfarnung
Copy link
Contributor

kfarnung commented Aug 7, 2018

Hey @crdumoul, calling JsAddRef/JsRelease without an active context should be fine as far as I can see, it takes a dependency on the Runtime only:

https://github.com/Microsoft/ChakraCore/blob/6543dfb6a478d6a5f8aece6285ba12566c634797/lib/Jsrt/Jsrt.cpp#L591-L662

@digitalinfinity might see something, but this is my current understanding of how the rental threading model for ChakraCore works. When you call JsSetCurrentContext(JS_INVALID_REFERENCE) it detaches from the current thread until a new context is set.

@crdumoul
Copy link
Author

crdumoul commented Aug 7, 2018

To add a little more information to the backtrace, I built a debug version of libChakraCoreStatic.a. This is the updated backtrace:
#0 0x000055bb40b6bf43 in Memory::RecyclerWriteBarrierManager::WriteBarrier (address=0x7f39f4dfdc90) at /home/vagrant/work/ChakraCore/lib/Common/Memory/RecyclerWriteBarrierManager.cpp:331
#1 0x000055bb40be62c9 in Memory::WriteBarrierPtr<char16_t const>::WriteBarrierSet (this=0x7f39f4dfdc90, ptr=0x7f39f440f000 u"square") at /home/vagrant/work/ChakraCore/lib/Common/Memory/RecyclerPointers.h:407
#2 0x000055bb40c2ecdd in Memory::WriteBarrierPtr<char16_t const>::WriteBarrierPtr (this=0x7f39f4dfdc90, ptr=0x7f39f440f000 u"square") at /home/vagrant/work/ChakraCore/lib/Common/Memory/RecyclerPointers.h:354
#3 0x000055bb40c72252 in JsUtil::CharacterBuffer<char16_t>::CharacterBuffer (this=0x7f39f4dfdc90, string=0x7f39f440f000 u"square", len=6) at /home/vagrant/work/ChakraCore/lib/Common/DataStructures/CharacterBuffer.h:17
#4 0x000055bb40c5e7a2 in ThreadContext::GetOrAddPropertyId (this=0x7f39f7c2f058, propertyName=0x7f39f440f000 u"square", propertyNameLength=6, propertyRecord=0x7f39f4dfe0b0) at /home/vagrant/work/ChakraCore/lib/Runtime/Base/ThreadContext.cpp:1185
#5 0x000055bb40c015f9 in Js::ScriptContext::GetOrAddPropertyRecord (this=0x7f39f7c30858, propertyName=0x7f39f440f000 u"square", propertyNameLength=6, propertyRecord=0x7f39f4dfe0b0) at /home/vagrant/work/ChakraCore/lib/Runtime/Base/ScriptContext.cpp:927
#6 0x000055bb409d0ce5 in JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1}::operator()(Js::ScriptContext*) const (this=0x7f39f4dfdee0, scriptContext=0x7f39f7c30858)
at /home/vagrant/work/ChakraCore/lib/Jsrt/Jsrt.cpp:3325
#7 0x000055bb409d0bc0 in _JsErrorCode ContextAPINoScriptWrapper_NoRecord<JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1}>(JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1}, bool, bool)::{lambda(Js::ScriptContext*)#1}::operator()(Js::ScriptContext*) const (this=0x7f39f4dfdea0, scriptContext=0x7f39f7c30858) at /home/vagrant/work/ChakraCore/lib/Jsrt/JsrtInternal.h:331
#8 0x000055bb409d04d1 in ContextAPINoScriptWrapper_Core<_JsErrorCode ContextAPINoScriptWrapper_NoRecord<JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1}>(JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1}, bool, bool)::{lambda(Js::ScriptContext*)#1}>(_JsErrorCode, bool, bool) (fn=..., allowInObjectBeforeCollectCallback=false, scriptExceptionAllowed=false) at /home/vagrant/work/ChakraCore/lib/Jsrt/JsrtInternal.h:277
#9 0x000055bb409d043b in ContextAPINoScriptWrapper_NoRecord<JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1}>(JsGetPropertyIdFromNameInternal(char16_t const*, unsigned long, void**)::{lambda(Js::ScriptContext*)#1}, bool, bool) (fn=..., allowInObjectBeforeCollectCallback=false, scriptExceptionAllowed=false) at /home/vagrant/work/ChakraCore/lib/Jsrt/JsrtInternal.h:329
#10 0x000055bb409c8af4 in JsGetPropertyIdFromNameInternal (name=0x7f39f440f000 u"square", cPropertyNameLength=6, propertyId=0x7f39f4dfe0b0) at /home/vagrant/work/ChakraCore/lib/Jsrt/Jsrt.cpp:3318
#11 0x000055bb409842e5 in JsCreatePropertyId (
name=0x55bb421a88ec "squarethreadchakra::teststests::create_runtimetests::create_contexttests::activate_contexttests::create_stringtests::call_functiontests::shared_contexttests::multiple_contextstests::dropped_runtimetes"..., length=6, propertyId=0x7f39f4dfe0b0)
at /home/vagrant/work/ChakraCore/lib/Jsrt/Jsrt.cpp:5029

So, the seg fault is happening at lib/Common/Memory/RecyclerWriteBarrierManager.cpp:331
331 cardTable[index] |= DIRTYBIT;

Also, I should mention that I'm using the v1.10.1 tag in git.

@MSLaguana
Copy link
Contributor

This looks to me like it's an issue with JsUtil::CharacterBuffer; at https://github.com/Microsoft/ChakraCore/blob/master/lib/Runtime/Base/ThreadContext.cpp#L1186 one is stack allocated, but according to https://github.com/Microsoft/ChakraCore/blob/cc115671134153e9996b2c9f7fffb2c2f861e125/lib/Common/DataStructures/CharacterBuffer.h#L76-L77 it has Fields which try to set write barrier bits. Since it is stack allocated and not recycler allocated, that leads to the segfault you observe since the index bit it tries to set hasn't been prepared in the card table.

@digitalinfinity do you think this is another case where the field annotations should be removed / marked as FieldNoBarrier? After a quick look through the code I don't notice any places that do recycler allocate one...

@MSLaguana
Copy link
Contributor

I'm surprised that this hasn't come up before, since this appears to have been there for years.

I do see that the Field annotations were explicitly added in ae27ef3

Perhaps instead we should have the write barriers check if the index is in bounds? Although that will definitely add some overhead.

@MSLaguana
Copy link
Contributor

This might be what I was missing: https://github.com/Microsoft/ChakraCore/blob/master/lib/Common/Memory/RecyclerWriteBarrierManager.cpp#L52-L55

There is some per-thread handling of the card table, and it may be that was somehow missed on the second thread, so its stack isn't accounted for.

@MSLaguana
Copy link
Contributor

I've just put out a PR which I think will fix the issue for you. Are you able to pull my branch and try it out yourself @crdumoul ?

@crdumoul
Copy link
Author

crdumoul commented Aug 7, 2018

@MSLaguana, thanks. I should be able to try that branch tomorrow, and I'll let you know how it goes.

@crdumoul
Copy link
Author

crdumoul commented Aug 8, 2018

@MSLaguana those changes seem to have fixed the problem.

@MSLaguana
Copy link
Contributor

Great, thanks for confirming that!

chakrabot pushed a commit that referenced this issue Aug 9, 2018
… thread that interacts with the JSRT

Merge pull request #5580 from MSLaguana:updateStackCardTable

The card table has code to handle stack allocations,
but this code was only run on the main thread and on,
our own background threads. Threads trying to use the
JSRT could end up getting segfaults from write barriers
from Fields on stack variables that weren't known to
the write barrier.

Fixes #5577
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants