Bug 23245

Summary: REGRESSION: Use of JavaScriptCore C API without using WebKit leads to immediate crash inside JSC::Identifier::add
Product: WebKit Reporter: Mark Rowe (bdash) <mrowe@apple.com>
Component: JavaScriptCoreAssignee: David Levin <levin@chromium.org>
Status: RESOLVED FIXED    
Severity: Major CC: ap@webkit.org, levin@chromium.org
Priority: P1 Keywords: HasReduction, InRadar, Regression, ReviewedForRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: Mac OS X 10.5   
Attachments:
Description Flags
Fix for bug.
darin: review-
Patch for bug.
none
Patch with the comments addressed. darin: review+

Description From 2009-01-11 14:44:04 PST
#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
------- Comment #1 From 2009-01-11 14:51:20 PST -------
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().
------- Comment #2 From 2009-01-11 14:57:07 PST -------
<rdar://problem/6488045>
------- Comment #3 From 2009-01-11 20:38:08 PST -------
Created an attachment (id=26622) [details]
Fix for bug.
------- Comment #4 From 2009-01-11 21:10:39 PST -------
(From update of attachment 26622 [details])
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.
------- Comment #5 From 2009-01-11 21:34:27 PST -------
Created an attachment (id=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. 
------- Comment #6 From 2009-01-11 21:38:20 PST -------
Created an attachment (id=26624) [details]
Patch with the comments addressed.
------- Comment #7 From 2009-01-11 21:41:11 PST -------
(From update of attachment 26624 [details])
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
------- Comment #8 From 2009-01-11 21:51:42 PST -------
(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.
------- Comment #9 From 2009-01-11 22:00:14 PST -------
(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!
------- Comment #10 From 2009-01-11 22:00:24 PST -------
(From update of attachment 26624 [details])
r=me
------- Comment #11 From 2009-01-11 22:02:22 PST -------
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?
------- Comment #12 From 2009-01-11 23:38:49 PST -------
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.