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)
Core
JavaScript: Internationalization API
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: zbraniecki, Assigned: anba)
References
Details
Attachments
(2 files, 2 obsolete files)
3.66 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
2.19 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
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 :(
Reporter | ||
Comment 1•7 years ago
|
||
Andre - any chance you can help me here?
Flags: needinfo?(andrebargull)
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Comment 3•7 years ago
|
||
See Also: → https://ssl.icu-project.org/trac/ticket/13497
Assignee | ||
Comment 4•7 years ago
|
||
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"
Assignee | ||
Comment 5•7 years ago
|
||
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)
Assignee | ||
Comment 6•7 years ago
|
||
Patches ICU to use _sprintf_l with the C-locale in Windows environments.
Attachment #8935000 -
Flags: review?(jwalden+bmo)
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8935758 -
Flags: review?(mh+mozilla) → review+
Comment 9•7 years ago
|
||
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+
Assignee | ||
Comment 10•7 years ago
|
||
Rebased part 1 to apply cleanly on inbound. Carrying r+.
Attachment #8935758 -
Attachment is obsolete: true
Attachment #8945824 -
Flags: review+
Assignee | ||
Comment 11•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d527eabf54c144d4fe059df6166529e2fec6c6c2
Keywords: checkin-needed
Comment 12•7 years ago
|
||
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
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/68acdee69ce1
https://hg.mozilla.org/mozilla-central/rev/8b6bb4335d4b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in
before you can comment on or make changes to this bug.
Description
•