1422415 - NumberFormat returns NaN if RegionalPrefsLocales is fa-IR and value has decimals

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, enhancement, P4)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

This is a spin-off from my debugging of bug 1201232.

In that bug users report that if they set their Windows Regional Prefs locales to Persian (fa-IR), then numbers are formatter to NaN in DownloadUtils.jsm.

I looked into this and noticed that if I set Windows Regional Prefs locales to en-US, the following happens:

```
(269.711).toLocaleString('en-US'); // "269.711"
(269.7113).toLocaleString('en-US'); // "269.711"
```

but if I set my OS Regional Prefs Locales to "fa-IR" and restart Firefox Nightly, this happens:

```
(269.711).toLocaleString('en-US'); // "269.711"
(269.7113).toLocaleString('en-US'); // "NaN"
```

What's confusing is that we do not set `fa-IR` anywhere from Gecko to SpiderMonkey. In XPCLocale we set LocaleService::AppLocale, which is still en-US.

So my initial guess is that somehow ICU on its own picks up regional prefs locale `fa-IR` from the OS and in result messes with rounding? But I'm not sure how to find it since my debugging capabilities on Windows are very limited :(

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Andre - any chance you can help me here?

Comment 2

[André Bargull [:anba]]

The default decimal separator for fa-IR on Windows is "/", but ICU only handles "." and "," [1].

|sprintf| [2] uses the current default locale of the program. For Firefox the program default locale is set to the system default locale here [3], but SpiderMonkey doesn't call |setlocale| on Windows [4], because HAVE_SETLOCALE isn't set in js/src/old-configure.in. Sigh.

Oh, and at least four digits after the decimal point are needed to ensure this fast path [5] isn't taken.


STR for any locale on Windows:
- Go to Settings -> Time & Language -> Additional date, time, & regional settings -> Change date, time, or number formats
- Open "Additional settings..."
- Change "Decimal symbol" to any character except "." or ","
- Evaluate `(269.7113).toLocaleString('en-US');`

[1] https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/intl/icu/source/i18n/digitlst.cpp#858-864
[2] https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/intl/icu/source/i18n/digitlst.cpp#854
[3] https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/xpcom/build/XPCOMInit.cpp#543
[4] https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/js/src/shell/js.cpp#8691
[5] https://searchfox.org/mozilla-central/rev/0b613c3887789f7786cd3131dfe9648398f4a6ac/intl/icu/source/i18n/precision.cpp#228-251

Comment 3

[André Bargull [:anba]]

Filed https://ssl.icu-project.org/trac/ticket/13497

Comment 4

[André Bargull [:anba]]

This issue is theoretically also reproducible under Linux.

STR:
- `sudo vi /usr/share/i18n/locales/en_US` (pick whatever the current system locale is)
- Then change decimal_point under LC_NUMERIC to any value except "<U002E>" or "<U002C>"
- `sudo locale-gen`
- `js -e "print((269.7113).toLocaleString('en-US'))"` should now print "NaN"

Comment 5

[André Bargull [:anba]]

Attachment: bug1422415-part1-enable-setlocale-windows.patch

Always enable HAVE_SETLOCALE for MSVC environments to ensure the default locale handling in the SpiderMonkey shell matches the behaviour of Firefox.

Comment 6

[André Bargull [:anba]]

Attachment: bug1422415-part2-sprintf-c-locale.patch

Patches ICU to use _sprintf_l with the C-locale in Windows environments.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8934998 [details] [diff] [review]
bug1422415-part1-enable-setlocale-windows.patch

Review of attachment 8934998 [details] [diff] [review]:
-----------------------------------------------------------------

Gecko uses setlocale on all platforms except android without looking for the setlocale function during configure. I don't see a reason to keep HAVE_SETLOCALE in spidermonkey. Just remove the ifdefs, and the AC_CHECK_FUNC.

Comment 8

[André Bargull [:anba]]

Attachment: bug1422415-part1-remove-compile-check-for-setlocale.patch

Updated part 1 per review comment #7 to remove the compiler check for setlocale and instead unconditionally call setlocale on all platforms.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8935000 [details] [diff] [review]
bug1422415-part2-sprintf-c-locale.patch

Review of attachment 8935000 [details] [diff] [review]:
-----------------------------------------------------------------

A little bit rs=me on this, but okay.

Comment 10

[André Bargull [:anba]]

Attachment: bug1422415-part1-remove-compile-check-for-setlocale.patch

Rebased part 1 to apply cleanly on inbound. Carrying r+.

Comment 11

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d527eabf54c144d4fe059df6166529e2fec6c6c2

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68acdee69ce1
Part 1: Remove HAVE_SETLOCALE because setlocale() is available on all supported platforms. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6bb4335d4b
Part 2: Call _sprintf_l with C-locale to ensure result doesn't contain locale-dependent decimal separator. r=Waldo

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/68acdee69ce1
https://hg.mozilla.org/mozilla-central/rev/8b6bb4335d4b