Bug 171480 – [INTL] Support dashed values in unicode locale extensions

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andy VanWagoner]

[INTL] Support dashed values in unicode locale extensions

[Andy VanWagoner]

Created attachment 308675 [details]
Patch

[Build Bot]

Comment on attachment 308675 [details]
Patch

Attachment 308675 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3645788

New failing tests:
media/modern-media-controls/macos-fullscreen-media-controls/macos-fullscreen-media-controls-buttons-styles.html

[Build Bot]

Created attachment 308698 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Andy VanWagoner]

Created attachment 308878 [details]
Patch

[JF Bastien]

Comment on attachment 308878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308878&action=review

> Source/JavaScriptCore/runtime/IntlObject.cpp:519
> +    for (unsigned k = 0; k < length; ++k) {

unsigned is too small because toLength can return up to 2**53 - 1.
uint64_t would do, though it seems weird to also keep the double value for length up top.

> Source/JavaScriptCore/runtime/IntlObject.cpp:707
> +    subtags.append(extension.substring(valueStart, extensionLength - valueStart));

Can `extensionLength < valueStart` here?

[Andy VanWagoner]

Comment on attachment 308878 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308878&action=review

>> Source/JavaScriptCore/runtime/IntlObject.cpp:519
>> +    for (unsigned k = 0; k < length; ++k) {
> 
> unsigned is too small because toLength can return up to 2**53 - 1.
> uint64_t would do, though it seems weird to also keep the double value for length up top.

Ok, that makes sense. I had changed it since hasProperty() and get() take an unsigned. It seemed weird to me to use a double for something that will always have an integer value.
But toLength returns a double so I'll just put k back to double.

>> Source/JavaScriptCore/runtime/IntlObject.cpp:707
>> +    subtags.append(extension.substring(valueStart, extensionLength - valueStart));
> 
> Can `extensionLength < valueStart` here?

valueStart = 3. It's possible, but only if extension doesn't start with -u-, which shouldn't happen.
valueStart = index + 1, where index < extensionLength. Can't happen in that assignment.
valueStart = subtagStart, where extensionLength - subtagStart == 2. Can't happen here either.

I can harden this a little more making the first bailout if (extensionLength < 3) instead of !extensionLength.

[Andy VanWagoner]

Created attachment 308903 [details]
Patch

[JF Bastien]

Comment on attachment 308903 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 308903 [details]
Patch

Clearing flags on attachment: 308903

Committed r216122: <http://trac.webkit.org/changeset/216122>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.