|Product:||WebKit||Reporter:||Mark Rowe (bdash) <mrowe>|
|Priority:||P1||Keywords:||HasReduction, InRadar, Regression, ReviewedForRadar|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
Description Mark Rowe (bdash) 2009-01-11 14:44:04 PST
Comment 1 Mark Rowe (bdash) 2009-01-11 14:51:20 PST
Comment 2 Mark Rowe (bdash) 2009-01-11 14:57:07 PST
Comment 4 Darin Adler 2009-01-11 21:10:39 PST
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.
Comment 5 David Levin 2009-01-11 21:34:27 PST
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.
Comment 6 David Levin 2009-01-11 21:38:20 PST
Created attachment 26624 [details] Patch with the comments addressed.
Comment 7 Darin Adler 2009-01-11 21:41:11 PST
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
Comment 8 David Levin 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 Darin Adler 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 Darin Adler 2009-01-11 22:00:24 PST
Comment on attachment 26624 [details] Patch with the comments addressed. r=me
Comment 11 David Levin 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 Alexey Proskuryakov 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.