Created attachment 266662 [details] Patch

Comment on attachment 266662 [details] Patch Attachment 266662 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/516979 New failing tests: js/string-toLocaleLowerCase.html

Created attachment 266668 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 266676 [details] Patch

Comment on attachment 266676 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266676&action=review > Source/JavaScriptCore/runtime/StringPrototype.cpp:1469 > + int sSize = s.length(); > + if (!sSize) You can use if (s.isEmpty()) > Source/JavaScriptCore/runtime/StringPrototype.cpp:1471 > + RELEASE_ASSERT(sSize >= 0); The size of a string is unsigned. This assertion can never be false. > Source/JavaScriptCore/runtime/StringPrototype.cpp:1502 > + const char* localeChars = locale.utf8().data(); This is unsafe. locale.utf8() returns a CString on the stack. That CString holds a buffer to the characters in memory. Then you get a pointer "localeChars" to the characters. After the pointer has been assigned, the CString goes out of scope and is destroyed. Its buffer is deleted, and localeChars points to deleted memory. You need to keep the value live on the stack as long as you use the pointer: CString utf8LocalBuffer = locale.utf8(); const char* localeChars = utf8LocalBuffer.data(); ... > Source/JavaScriptCore/runtime/StringPrototype.cpp:1506 > + const UChar* utf16view = view.upconvertedCharacters(); Similar problem here, this is not safe as there is nothing telling the compiler to keep the buffer alive.

Created attachment 266683 [details] Patch

Comment on attachment 266683 [details] Patch Attachment 266683 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/517401 New failing tests: js/string-toLocaleLowerCase.html

Created attachment 266692 [details] Archive of layout-test-results from ews112 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5

Created attachment 266699 [details] Patch

Created attachment 266701 [details] Patch

Comment on attachment 266701 [details] Patch Looks good to me. I did not see any other use-after-free.

Comment on attachment 266701 [details] Patch Clearing flags on attachment: 266701 Committed r193611: <http://trac.webkit.org/changeset/193611>

All reviewed patches have been landed. Closing bug.

Comment on attachment 266701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=266701&action=review > Source/JavaScriptCore/runtime/IntlObject.h:67 > +String bestAvailableLocale(const HashSet<String>&, const String&); I think we need argument names here. It’s not obvious from the function name and the alone what the arguments mean.

(In reply to comment #14) > Comment on attachment 266701 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=266701&action=review > > > Source/JavaScriptCore/runtime/IntlObject.h:67 > > +String bestAvailableLocale(const HashSet<String>&, const String&); > > I think we need argument names here. It’s not obvious from the function name > and the alone what the arguments mean. I can make the change in the patch for bug 147609, if you'd like.