Bug 23245 – REGRESSION: Use of JavaScriptCore C API without using WebKit leads to immediate crash inside JSC::Identifier::add

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Rowe (bdash)]

#include <JavaScriptCore/JavaScriptCore.h>

int main(int argc, char **argv)
{
    JSGlobalContextRef context = JSGlobalContextCreate(0);
    return 0;
}

running this against TOT crashes inside JSC::Identifier::add when calling UString::Rep::null().hash();, as the data used by UString::Rep::null() has not been initialized

[Mark Rowe (bdash)]

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x0000000c
0x004bda64 in JSC::UString::Rep::hash (this=0x0) at UString.h:96
96	            unsigned hash() const { if (_hash == 0) _hash = computeHash(data(), len); return _hash; }
(gdb) bt
#0  0x004bda64 in JSC::UString::Rep::hash (this=0x0) at UString.h:96
#1  0x0047d664 in JSC::Identifier::add (globalData=0x1009800, c=0x0) at Identifier.cpp:127
#2  0x00507f9f in JSC::Identifier::Identifier (this=0x904ad0, globalData=0x1009800, s=0x0) at Identifier.h:41
#3  0x0048388a in JSC::CommonIdentifiers::CommonIdentifiers (this=0x904ad0, globalData=0x1009800) at CommonIdentifiers.cpp:34
#4  0x00569e81 in JSC::JSGlobalData::JSGlobalData (this=0x1009800, isShared=true) at JavaScriptCore/runtime/JSGlobalData.cpp:94
#5  0x00569ff6 in JSC::JSGlobalData::sharedInstance () at JavaScriptCore/runtime/JSGlobalData.cpp:169
#6  0x00566635 in JSGlobalContextCreate (globalObjectClass=0x0) at JavaScriptCore/API/JSContextRef.cpp:72
#7  0x00001ff4 in main (argc=1, argv=0xbffff860) at test.c:5


Looks like perhaps JSGlobalContextCreate needs to call initializeThreading() before calling JSGlobalData::sharedInstance().

[Mark Rowe (bdash)]

<rdar://problem/6488045>

[David Levin]

Created attachment 26622 [details]
Fix for bug.

[Darin Adler]

Comment on attachment 26622 [details]
Fix for bug.

I think it's subtle and non-obvious that OpaqueJSString::ustring is a suitable bottleneck, yet OpaqueJSString::identifier, a function with a nearly identical purpose, doesn't need the initializeThreading call. I think it might be better to initialize in the individual JSStringCreate functions, even though there are many of them, because the subtle relationship between the external functions and the reason OpaqueJSString has initialization inside it is very likely to get broken in the future even though it's fine right now.

You missed JSGlobalContextCreateInGroup, which can take NULL for the group.

prepare-ChangeLog somehow missed JSGlobalContextCreate, because it's not listed in your change log.

I'm going to say review- because you missed JSGlobalContextCreateInGroup.

[David Levin]

Created attachment 26623 [details]
Patch for bug.

I put the init in the JSString api where it creates the JSStringRef.

About JSGlobalContextCreateInGroup, I likely would have missed it (because of its ability to take NULL), but fortunately the init call was already there.

[David Levin]

Created attachment 26624 [details]
Patch with the comments addressed.

[Darin Adler]

Comment on attachment 26624 [details]
Patch with the comments addressed.

Oh, I am so evil. I told you to move it into JSStringCreate functions, *knowing* you'd probably missing JSStringCreateWithBSTR. But did I say anything? No! So I made you take a perfectly good, working patch, and ruin it.

review-, but I'm sure it will take you like 10 seconds to fix it

[David Levin]

(In reply to comment #7)
> (From update of attachment 26624 [details] [review])
> Oh, I am so evil. I told you to move it into JSStringCreate functions,
> *knowing* you'd probably missing JSStringCreateWithBSTR. But did I say
> anything? No! So I made you take a perfectly good, working patch, and ruin it.
> 
> review-, but I'm sure it will take you like 10 seconds to fix it
> 

Actually, I did look at them. :)

  * JSStringCreateWithBSTR just calls JSStringCreateWithCharacters (which has the init function).
Is it too tricky to rely on that?

  * JSStringCopyBSTR takes a JSStringRef so it doesn't need a call to the init function.

[Darin Adler]

(In reply to comment #8)
>   * JSStringCreateWithBSTR just calls JSStringCreateWithCharacters (which has
> the init function).
> Is it too tricky to rely on that?

No. I don't know why the others don't work that way. It's better!

[Darin Adler]

Comment on attachment 26624 [details]
Patch with the comments addressed.

r=me

[David Levin]

fwiw, the bstr api worries me a little bit.  BSTR are a windows construct (ole automation), so this is a windows api.  However, initializeThreading is only threadsafe on OSX.  I guess these string apis could be called on any thread which would be trouble on Windows.

It may help to know when these apis are called.  Is it just there for Apple products that run on windows (and they'll call something on the main thread that initializes this)?  Or do I need to be more concerned about this?

[Alexey Proskuryakov]

Committed revision 39817.

It isn't all that important for a pure JS API client to have a correct main thread identifier (it's needed for WTF MainThread functionality, which is not reachable via API). There could be problems if a client first uses JSC from secondary thread, and later uses WebCore from main thread. So, there are improvements to be made, but the issue is not too horrible.