Closed Bug 1422415 Opened 7 years ago Closed 7 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: zbraniecki, Assigned: anba)

References

Details

Attachments

(2 files, 2 obsolete files)

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 :(
Andre - any chance you can help me here?
Flags: needinfo?(andrebargull)
Blocks: 1201232
Priority: -- → P4
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
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Flags: needinfo?(andrebargull)
Summary: NumberFormat returns NaN if RegionalPrefsLocales is fa-IN and value has decimals → NumberFormat returns NaN if RegionalPrefsLocales is fa-IR and value has decimals
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"
Always enable HAVE_SETLOCALE for MSVC environments to ensure the default locale handling in the SpiderMonkey shell matches the behaviour of Firefox.
Attachment #8934998 - Flags: review?(mh+mozilla)
Patches ICU to use _sprintf_l with the C-locale in Windows environments.
Attachment #8935000 - Flags: review?(jwalden+bmo)
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.
Attachment #8934998 - Flags: review?(mh+mozilla)
Updated part 1 per review comment #7 to remove the compiler check for setlocale and instead unconditionally call setlocale on all platforms.
Attachment #8934998 - Attachment is obsolete: true
Attachment #8935758 - Flags: review?(mh+mozilla)
Attachment #8935758 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8935000 - Flags: review?(jwalden+bmo) → review+
Rebased part 1 to apply cleanly on inbound. Carrying r+.
Attachment #8935758 - Attachment is obsolete: true
Attachment #8945824 - Flags: review+
Pushed by rgurzau@mozilla.com: 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
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: