Bug 147608

Summary:

[INTL] Implement String.prototype.toLocaleLowerCase in ECMA-402

Product:

WebKit

Reporter:

Andy VanWagoner <andy>

Component:

JavaScriptCore

Assignee:

Andy VanWagoner <andy>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, commit-queue, keith_miller, mark.lam, msaboff, saam

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

90906

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from ews114 for mac-yosemite	none
Patch	none
Patch	none
Archive of layout-test-results from ews112 for mac-yosemite	none
Patch	none
Patch	none

Andy VanWagoner

Reported 2015-08-03 17:30:26 PDT

Implement ECMA-402 2.0 13.1.2 String.prototype.toLocaleLowerCase ([locales]) http://ecma-international.org/publications/standards/Ecma-402.htm This definition supersedes the definition provided in ES2015, 21.1.3.20.

Attachments
Patch (23.30 KB, patch) 2015-12-04 14:42 PST, Andy VanWagoner	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews114 for mac-yosemite (843.88 KB, application/zip) 2015-12-04 15:34 PST, Build Bot	no flags	Details
Patch (23.31 KB, patch) 2015-12-04 16:00 PST, Andy VanWagoner	no flags	Details Formatted Diff Diff
Patch (23.23 KB, patch) 2015-12-04 16:54 PST, Andy VanWagoner	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews112 for mac-yosemite (1.15 MB, application/zip) 2015-12-04 17:34 PST, Build Bot	no flags	Details
Patch (24.32 KB, patch) 2015-12-04 19:59 PST, Andy VanWagoner	no flags	Details Formatted Diff Diff
Patch (24.36 KB, patch) 2015-12-04 20:45 PST, Andy VanWagoner	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Andy VanWagoner

Comment 1 2015-12-04 14:42:54 PST

Created attachment 266662 [details] Patch

Build Bot

Comment 2 2015-12-04 15:34:33 PST

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

Build Bot

Comment 3 2015-12-04 15:34:35 PST

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

Andy VanWagoner

Comment 4 2015-12-04 16:00:48 PST

Created attachment 266676 [details] Patch

Benjamin Poulain

Comment 5 2015-12-04 16:42:41 PST

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.

Andy VanWagoner

Comment 6 2015-12-04 16:54:24 PST

Created attachment 266683 [details] Patch

Build Bot

Comment 7 2015-12-04 17:34:54 PST

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

Build Bot

Comment 8 2015-12-04 17:34:56 PST

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

Andy VanWagoner

Comment 9 2015-12-04 19:59:48 PST

Created attachment 266699 [details] Patch

Andy VanWagoner

Comment 10 2015-12-04 20:45:39 PST

Created attachment 266701 [details] Patch

Benjamin Poulain

Comment 11 2015-12-06 21:30:12 PST

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

WebKit Commit Bot

Comment 12 2015-12-06 22:13:45 PST

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

WebKit Commit Bot

Comment 13 2015-12-06 22:13:49 PST

All reviewed patches have been landed. Closing bug.

Darin Adler

Comment 14 2015-12-07 14:29:51 PST

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.

Andy VanWagoner

Comment 15 2015-12-07 14:33:17 PST

(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.

Note You need to log in before you can comment on or make changes to this bug.