Bug 154530

Summary: New intl-numberformat.js test fails on many Linux platforms
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andy, bugs-noreply, cgarcia, clopez, commit-queue, darin, gns, gyuyoung.kim, jh718.park, keith_miller, mark.lam, mcatanzaro, msaboff, ossy, sbarati, sukolsak, ysuzuki
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=158066
Bug Depends on: 158417    
Bug Blocks: 147605    
Attachments:
Description Flags
Patch
none
Patch for GTK+ darin: review+

Description Csaba Osztrogonác 2016-02-22 02:20:52 PST
intl-numberformat.js introduced in http://trac.webkit.org/changeset/196850,
but fails on EFL ARM and GTK x86_64 buildbots:
- GTK bot: https://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20%28Tests%29/builds/13917
- EFL ARM bot: https://build.webkit.org/builders/EFL%20Linux%20ARMv7%20Thumb2%20Release/builds/17225

It's strange that it passes on the EFL x86_64 bot. I don't know anything about this code path.
Is it possible that some 3rd party libraries with different version cause this failure?
Comment 1 Sukolsak Sakshuwong 2016-02-22 08:54:40 PST
This is likely the same issue as Bug 152448. It is because Intl uses POSIX locale.

    $ LANG=en_US.UTF-8 ./jsc -e "print(Infinity.toLocaleString())"
    ∞
    $ LANG=en_US_POSIX.UTF-8 ./jsc -e "print(Infinity.toLocaleString())"
    INF

The proposed patch in Bug 152448 should fix it.
Comment 2 Andy VanWagoner 2016-02-27 17:30:33 PST
The patch for Bug 152448 likely hasn't resolved this issue, because it only provided a hook for defaultLanguage that is implemented in WebCore.
Comment 3 Darin Adler 2016-02-27 17:45:24 PST
There are two separate questions here:

One is how to make WebKit and JavaScriptCore properly choose a default locale for each platform. I don’t know what the right way to do that is for Linux in general. Fixing that for WebCore is what bug 152448 is all about. But it almost certainly needs to be fixed for direct JavaScriptCore use too. At least I know it does need to be done on the Apple platforms. So we may need a bug to track that.

The other is how to make the test harness set a consistent language so that tests are reliable, rather than being sensitive to the language the user running the tests happens to prefer. That's something we need to address separately. There are lots of calls to setenv in various test tools, so it could be as simple as doing a setenv("LANG") in the right place for the appropriate platforms in some of the test tools.
Comment 4 Carlos Alberto Lopez Perez 2016-02-29 07:34:45 PST
(In reply to comment #3)
> There are two separate questions here:
> 
> One is how to make WebKit and JavaScriptCore properly choose a default
> locale for each platform. I don’t know what the right way to do that is for
> Linux in general. Fixing that for WebCore is what bug 152448 is all about.
> But it almost certainly needs to be fixed for direct JavaScriptCore use too.
> At least I know it does need to be done on the Apple platforms. So we may
> need a bug to track that.
> 


With glibc (GNU/Linux) the default locale is defined on the environment variable LANG. If LANG is not defined, then the locale defaults to 'C' (also known as 'POSIX') which is the default portable locale.

This default locale can be overridden with the environment variable LC_ALL.

So perhaps we can just do setenv("LC_ALL", "C") before starting the tests. This will override any locale defined by the user, and it will be portable across operating systems

Some quick tests:

# Linux
$ LANG=es_ES.UTF-8 WebKitBuild/Release/bin/jsc -e "print(Infinity.toLocaleString())"
∞
$ LC_ALL=POSIX LANG=es_ES.UTF-8 WebKitBuild/Release/bin/jsc -e "print(Infinity.toLocaleString())"
INF
$ LC_ALL=C LANG=es_ES.UTF-8 WebKitBuild/Release/bin/jsc -e "print(Infinity.toLocaleString())"
INF


# MacOS
$ LANG=es_ES.UTF-8 WebKitBuild//Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Infinity.toLocaleString())"
∞
$ LC_ALL=POSIX LANG=es_ES.UTF-8 WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Infinity.toLocaleString())"
INF
$ LC_ALL=C LANG=es_ES.UTF-8 WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Infinity.toLocaleString())"
INF


> The other is how to make the test harness set a consistent language so that
> tests are reliable, rather than being sensitive to the language the user
> running the tests happens to prefer. That's something we need to address
> separately. There are lots of calls to setenv in various test tools, so it
> could be as simple as doing a setenv("LANG") in the right place for the
> appropriate platforms in some of the test tools.

As explained above, just doing setenv("LC_ALL", "C") before starting the tests should work for all use cases.

More info: http://man7.org/linux/man-pages/man3/setlocale.3.html and https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/locale.1.html
Comment 5 Carlos Alberto Lopez Perez 2016-02-29 09:52:56 PST
Seems that my previous suggestion of setting LC_ALL is not going to help.

This test js/intl-numberformat.html is manually setting the locales and trying some numeric and monetary conversions and checking that it returns the right values.

For example see the follow examples:

linux # WebKitBuild/Release/bin/jsc -e "print(Intl.NumberFormat('es').format(1234.567))"
1 234,567
linux # $ WebKitBuild/Release/bin/jsc -e "print(Intl.NumberFormat('en').format(1234.567))"
1,234.567


macos # WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Intl.NumberFormat('es').format(1234.567))"
1.234,567
macos # WebKitBuild/Release/JavaScriptCore.framework/Versions/A/Resources/jsc -e "print(Intl.NumberFormat('en').format(1234.567))"
1,234.567


So it seems there is fundamental difference here.
In MacOS we are getting a '.' in the thousands decimal mark for the spanish locale (right), meanwhile in Linux we are getting an space (wrong).

The above checks are part of the test js/intl-numberformat.html therefore on Linux we are getting:

-PASS Intl.NumberFormat('es').format(1234.567) is '1.234,567'
+FAIL Intl.NumberFormat('es').format(1234.567) should be 1.234,567. Was 1 234,567.


Not sure what is wrong, at least with printf on Linux I get the right values

LC_NUMERIC=en_US.UTF8 /usr/bin/printf "%'.2f\n" '1234.567'
1,234.57
LC_NUMERIC=es_ES.UTF8 /usr/bin/printf "%'.2f\n" '1234.567'
1.234,57

So this don't seems a problem on the Linux glibc, but at first it looks that something on jsc is not picking/setting the right values for the locales when running on Linux


This is the full text difference for the test js/intl-numberformat.html with WebKitGTK+ (Linux):

--- /home/clopez/webkit/webkit/layout-test-results/js/intl-numberformat-expected.txt
+++ /home/clopez/webkit/webkit/layout-test-results/js/intl-numberformat-actual.txt
@@ -140,7 +140,7 @@
 PASS Intl.NumberFormat('en').format(Number.MIN_VALUE) is '0'
 PASS Intl.NumberFormat('en').format(Number.MAX_VALUE) is '179,769,313,486,232,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000,000'
 PASS Intl.NumberFormat('en').format(1234.567) is '1,234.567'
-PASS Intl.NumberFormat('es').format(1234.567) is '1.234,567'
+FAIL Intl.NumberFormat('es').format(1234.567) should be 1.234,567. Was 1 234,567.
 PASS Intl.NumberFormat('fr').format(1234.567) is '1 234,567'
 PASS Intl.NumberFormat('en-u-nu-latn').format(1234.567) is '1,234.567'
 PASS Intl.NumberFormat('en-u-nu-fullwide').format(1234.567) is '1,234.567'
@@ -156,14 +156,14 @@
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'USD'}).format(Infinity) is '$∞'
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'JPY'}).format(4.2) is '¥4'
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'xXx'}).format(4.2) is 'XXX4.20'
-PASS Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'code'}).format(4) is 'USD4.00'
+FAIL Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'code'}).format(4) should be USD4.00. Threw exception Error: Failed to format a number.
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'symbol'}).format(4) is '$4.00'
-PASS Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'name'}).format(4) is '4.00 US dollars'
-PASS Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'code'}).format(-4.2) is '-JPY4'
+FAIL Intl.NumberFormat('en', {style: 'currency', currency: 'USD', currencyDisplay: 'name'}).format(4) should be 4.00 US dollars. Threw exception Error: Failed to format a number.
+FAIL Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'code'}).format(-4.2) should be -JPY4. Threw exception Error: Failed to format a number.
 PASS Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'symbol'}).format(-4.2) is '-¥4'
-PASS Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'name'}).format(-4.2) is '-4 Japanese yen'
-PASS Intl.NumberFormat('fr', {style: 'currency', currency: 'USD', currencyDisplay: 'name'}).format(4) is '4,00 dollars des États-Unis'
-PASS Intl.NumberFormat('fr', {style: 'currency', currency: 'JPY', currencyDisplay: 'name'}).format(4) is '4 yens japonais'
+FAIL Intl.NumberFormat('en', {style: 'currency', currency: 'JPY', currencyDisplay: 'name'}).format(-4.2) should be -4 Japanese yen. Threw exception Error: Failed to format a number.
+FAIL Intl.NumberFormat('fr', {style: 'currency', currency: 'USD', currencyDisplay: 'name'}).format(4) should be 4,00 dollars des États-Unis. Threw exception Error: Failed to format a number.
+FAIL Intl.NumberFormat('fr', {style: 'currency', currency: 'JPY', currencyDisplay: 'name'}).format(4) should be 4 yens japonais. Threw exception Error: Failed to format a number.
 PASS Intl.NumberFormat('en', {minimumIntegerDigits: 4}).format(12) is '0,012'
 PASS Intl.NumberFormat('en', {minimumIntegerDigits: 4}).format(12345) is '12,345'
 PASS Intl.NumberFormat('en', {minimumFractionDigits: 3}).format(1) is '1.000'
@@ -184,7 +184,7 @@
 PASS Intl.NumberFormat('en', {maximumSignificantDigits: 4}).format(0.1234567) is '0.1235'
 PASS Intl.NumberFormat('en', {maximumSignificantDigits: 4}).format(1234567) is '1,235,000'
 PASS Intl.NumberFormat('en', {useGrouping: true}).format(1234567.123) is '1,234,567.123'
-PASS Intl.NumberFormat('es', {useGrouping: true}).format(1234567.123) is '1.234.567,123'
+FAIL Intl.NumberFormat('es', {useGrouping: true}).format(1234567.123) should be 1.234.567,123. Was 1 234 567,123.
 PASS Intl.NumberFormat('fr', {useGrouping: true}).format(1234567.123) is '1 234 567,123'
 PASS Intl.NumberFormat('en', {useGrouping: false}).format(1234567.123) is '1234567.123'
 PASS Intl.NumberFormat('es', {useGrouping: false}).format(1234567.123) is '1234567,123'
Comment 6 Carlos Garcia Campos 2016-05-25 04:13:13 PDT
Many other tests started to fail after r201066, but the problem seems to be the same. run-javascriptcore-tests does $ENV{LANG}="en_US.UTF-8"; but we are not actually honoring the environment variables at all when using jsc binary. We are using setlocale with a nullptr locale to get the current one, but the current one is always "C", because to set the locale according to the environment variables we need to call setlocale with an empty string as locale. That's done by  gtk_init() which is called by all our binaries (web process, network process, etc.), but not by jsc (because jsc doesn't depend on GTK+). The reason why it has always worked for EFL is because they call ecore_init() in jsc that calls setlocale. I'll submit a patch to fix this and the new tests failing.
Comment 7 Carlos Garcia Campos 2016-05-25 04:22:34 PDT
Created attachment 279758 [details]
Patch

I'm not sure this is also needed for jsc-only port. In that case it would be a matter of updating the ifdef
Comment 8 Darin Adler 2016-05-25 04:26:20 PDT
Comment on attachment 279758 [details]
Patch

OK. Not sure this is the right way to do things. Seems unfortunate to have significant platform dependencies in a command line tool. Unpleasant to have iOS, EFL, Windows, and GTK specifics. I suppose I should be grateful we don’t have anything like this for Mac yet?
Comment 9 Carlos Garcia Campos 2016-05-25 05:20:25 PDT
hmm, I was wrong, even with this patch intl-numberformat.js is filing, but all others that started to fail after r201066 are passing now, so I'll submit a separate bug.
Comment 10 Csaba Osztrogonác 2016-05-25 05:22:19 PDT
(In reply to comment #7)
> Created attachment 279758 [details]
> Patch
> 
> I'm not sure this is also needed for jsc-only port. In that case it would be
> a matter of updating the ifdef

I'll check if it fixes failures on jsc-only ARM bots. 
( without guard, of course :) )

Could you wait ~2-3 hours for my testing before landing?
Comment 11 Carlos Garcia Campos 2016-05-25 05:27:14 PDT
Comment on attachment 279758 [details]
Patch

Clearing flags, I moved the patch to bug #158066
Comment 12 Carlos Garcia Campos 2016-05-25 05:28:01 PDT
(In reply to comment #10)
> (In reply to comment #7)
> > Created attachment 279758 [details]
> > Patch
> > 
> > I'm not sure this is also needed for jsc-only port. In that case it would be
> > a matter of updating the ifdef
> 
> I'll check if it fixes failures on jsc-only ARM bots. 
> ( without guard, of course :) )
> 
> Could you wait ~2-3 hours for my testing before landing?

Sure
Comment 13 Carlos Garcia Campos 2016-05-25 05:31:22 PDT
intl-numberformat.js works for me locally, but not in the bots
Comment 14 Carlos Garcia Campos 2016-05-25 05:57:05 PDT
In my laptop:

$ LANG=en_US.UTF-8 bin/jsc -e "print(Intl.NumberFormat('es', {useGrouping: true}).format(1234567.123))"
1.234.567,123

In the bot:

$ LANG=en_US.UTF-8 bin/jsc -e "print(Intl.NumberFormat('es', {useGrouping: true}).format(1234567.123))"
1 234 567,123

There must be something else, doesn't look like a WebKit problem.
Comment 15 Carlos Garcia Campos 2016-05-25 06:00:28 PDT
Are we using ICU for this? could it be the ICU version? it's the only (relevant) thing that looks different between my laptop and the bot
Comment 16 Carlos Garcia Campos 2016-05-26 06:32:54 PDT
Created attachment 279882 [details]
Patch for GTK+

So it was indeed the ICU version installed in the bots. Confirmed that using a newer version in the bots fixes the tests.
Comment 17 Carlos Garcia Campos 2016-05-26 07:43:03 PDT
I tried the patch on a clean build in a tests bot we have and it worked, so I'm not sure about he EWS failure, maybe a clan build would fix it.
Comment 18 Darin Adler 2016-05-26 08:47:29 PDT
Comment on attachment 279882 [details]
Patch for GTK+

Please look at the compile failure in GTK EWS.
Comment 19 Carlos Garcia Campos 2016-05-26 08:51:23 PDT
Darin, I think it's a clean build issue, because I've tried in a test bot and it worked. I'll trigger clean builds in the bots and watch them after landing.
Comment 20 Carlos Garcia Campos 2016-05-27 00:17:40 PDT
Committed r201449: <http://trac.webkit.org/changeset/201449>
Comment 21 Yusuke Suzuki 2016-05-27 20:14:23 PDT
While your change solves the JSC tests (Great!!), the layout-tests in GTK ports pose many new failing tests instead. Do you already know the reason?
Comment 22 Yusuke Suzuki 2016-05-27 20:43:29 PDT
Ah, no, these tests are previously failing. Sorry.
Comment 23 Csaba Osztrogonác 2016-05-29 03:52:11 PDT
It broke the GTK ARM bot.
Comment 24 Carlos Garcia Campos 2016-05-29 23:11:46 PDT
(In reply to comment #23)
> It broke the GTK ARM bot.

I already talked to Gustavo, who was going to look at it. For some reason wget is failing to download from sourceforge in the ARM bot. In any case, it's a problem of the bot itself, not the patch.
Comment 25 Carlos Alberto Lopez Perez 2016-05-31 03:33:20 PDT
Committed r201509: <http://trac.webkit.org/changeset/201509>