Bug 214223 – Intl.Locale maximize, minimize should return Intl.Locale instead of String

• Component: JavaScriptCore
• Status: RESOLVED FIXED

Created attachment 404057 [details]
Spec compliant output of Intl.Locale implemented properties/methods. On Edge, Chrome and Firefox

The functions implemented in the Intl.Locale class do not function as expected in the iOS 14 Beta 2 version of Safari. 

When no property is accessed it returns the locale using the toString function instead of a Locale instance (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale).

When you attempt to get a property from what should be a regular Locale instance, it instead returns undefined instead of the property's expected value.

I came across this bug as I was polyfilling Intl.Locale for browsers that didn't support Intl.Locale in a feature release and it was suddenly breaking (used maximize().region) with these undefined return values for iOS users using our staging pre-release builds.

Example:
const locale = new Intl.Locale('en')
const max = locale.maximize().baseName// undefined in iOS 14 Dev 2. Correct behaviour is 'en-Latn-US'
const min = locale.minimize().baseName// undefined in iOS 14 Dev 2. Correct behaviour is 'en'

Example 2:
const locale = new Intl.Locale('ja-Jpan-JP-u-ca-japanese-hc-h23-kf-lower-co-emoji-nu-jpan')
const max = locale.maximize() 
// Returns 'ja-Jpan-JP-u-ca-japanese-hc-h23-kf-lower-co-emoji-nu-jpan' in iOS 14 Dev 2. 
// Correct behaviour is returning a Locale object

Live Demo:
https://6f6bm.csb.app/

Attached is the spec compliant output and the iOS 14 Dev 2 output. This is identical on:
- Edge 83, 
- Edge 85 (Dev), 
- Chrome 83
- Chrome 86 (Canary)
- Firefox 78
- Firefox 79 (Developer)

Created attachment 404058 [details]
Output in iOS 14 Dev 2

[Radar WebKit Bug Importer]

<rdar://problem/65413620>

Relevant sections of the specification:

https://tc39.es/ecma402/#sec-Intl.Locale.prototype.maximize
https://tc39.es/ecma402/#sec-Intl.Locale.prototype.minimize

[Yusuke Suzuki]

Created attachment 404078 [details]
Patch

[Ross Kirsling]

Comment on attachment 404078 [details]
Patch

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

r=me, thanks!

> Source/JavaScriptCore/runtime/IntlLocale.h:56
> -    const String& maximize();
> -    const String& minimize();
> +    const String& maximal();
> +    const String& minimal();

I'm not sure that we need to change the method names here just because we're changing the field names -- toString currently returns m_fullString, say.

(Doesn't matter too much either way though since this is internal.)

> Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:141
> +    const String& fullString = locale->toString();
> +    RELEASE_AND_RETURN(scope, JSValue::encode(fullString.isEmpty() ? jsUndefined() : jsNontrivialString(vm, fullString)));

Hmm, my reasoning for not empty-checking these three is that we should have failed to construct an Intl.Locale at all if these can't be produced -- i.e. it would be ICU disagreeing *with itself* if these fail.

> JSTests/ChangeLog:9
> +        * stress/intl-locale-maximize-minimize.js: Added.

I think we should just add to the relevant part of intl-locale.js, since this is a mistake in the basic behavior of these two methods.

[Yusuke Suzuki]

Comment on attachment 404078 [details]
Patch

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

>> Source/JavaScriptCore/runtime/IntlLocale.h:56
>> +    const String& minimal();
> 
> I'm not sure that we need to change the method names here just because we're changing the field names -- toString currently returns m_fullString, say.
> 
> (Doesn't matter too much either way though since this is internal.)

I think this is better since IntlLocale#maximize() is confusing e.g. it looks like it is implementing Intl.Locale#maximize(). But this is not correct, and it is accessing maximal field of Intl.Locale. And Intl.Locale#maximize is doing the actual thing, which is wrapping Intl.Locale.

>> Source/JavaScriptCore/runtime/IntlLocalePrototype.cpp:141
>> +    RELEASE_AND_RETURN(scope, JSValue::encode(fullString.isEmpty() ? jsUndefined() : jsNontrivialString(vm, fullString)));
> 
> Hmm, my reasoning for not empty-checking these three is that we should have failed to construct an Intl.Locale at all if these can't be produced -- i.e. it would be ICU disagreeing *with itself* if these fail.

Discussed with Ross, I think using `jsString` in these Intl.Locale prototype functions is better since this is a boundary between third-party library (ICU) and JSC, and we are not guaranteeing our invariant in ICU. So being protective would be better.

>> JSTests/ChangeLog:9
>> +        * stress/intl-locale-maximize-minimize.js: Added.
> 
> I think we should just add to the relevant part of intl-locale.js, since this is a mistake in the basic behavior of these two methods.

Moved.

[Yusuke Suzuki]

Created attachment 404082 [details]
Patch for landing

[EWS]

Committed r264275: <https://trac.webkit.org/changeset/264275>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404082 [details].