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

RESOLVED FIXED in Firefox 60

Status

()

P4
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: gandalf, Assigned: anba)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
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

a year ago
Andre - any chance you can help me here?
Flags: needinfo?(andrebargull)
(Reporter)

Updated

a year ago
Blocks: 1201232
Priority: -- → P4
(Assignee)

Comment 2

a year 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

a year 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 4

a year 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

a year ago
Created attachment 8934998 [details] [diff] [review]
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.
Attachment #8934998 - Flags: review?(mh+mozilla)
(Assignee)

Comment 6

a year ago
Created attachment 8935000 [details] [diff] [review]
bug1422415-part2-sprintf-c-locale.patch

Patches ICU to use _sprintf_l with the C-locale in Windows environments.
Attachment #8935000 - Flags: review?(jwalden+bmo)

Comment 7

11 months 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

11 months ago
Created attachment 8935758 [details] [diff] [review]
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.
Attachment #8934998 - Attachment is obsolete: true
Attachment #8935758 - Flags: review?(mh+mozilla)

Updated

11 months ago
Attachment #8935758 - Flags: review?(mh+mozilla) → review+

Comment 9

10 months 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

10 months ago
Created attachment 8945824 [details] [diff] [review]
bug1422415-part1-remove-compile-check-for-setlocale.patch

Rebased part 1 to apply cleanly on inbound. Carrying r+.
Attachment #8935758 - Attachment is obsolete: true
Attachment #8945824 - Flags: review+

Comment 12

10 months 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/68acdee69ce1
https://hg.mozilla.org/mozilla-central/rev/8b6bb4335d4b
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months 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.