Closed Bug 1422415 Opened 7 years ago Closed 6 years ago

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


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




Tracking Status
firefox60 --- fixed


(Reporter: zbraniecki, Assigned: anba)




(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/ 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');`

Assignee: nobody → andrebargull
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.

- `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]

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]

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
Part 1: Remove HAVE_SETLOCALE because setlocale() is available on all supported platforms. r=glandium
Part 2: Call _sprintf_l with C-locale to ensure result doesn't contain locale-dependent decimal separator. r=Waldo
Keywords: checkin-needed
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.