|Product:||WebKit||Reporter:||Mark Rowe (bdash) <email@example.com>|
|Priority:||P1||Keywords:||HasReduction, InRadar, Regression, ReviewedForRadar|
|Version:||528+ (Nightly build)|
|OS:||Mac OS X 10.5|
Created an attachment (id=26622) [details] Fix for bug.
(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.
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.
Created an attachment (id=26624) [details] Patch with the comments addressed.
(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
(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.
(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!
(From update of attachment 26624 [details]) r=me
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?
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.