1330149 - Regressions on ecma_3\Date\15.9.5.5.js 15.9.5.7.js

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, defect, P3)

Description

[Ted Campbell [:tcampbell]]

> cd js/src/build_DBG.OBJ
> ../configure --enable-debug --disable-optimize
> mozmake
> ../tests/jstests.py dist/bin/js.exe "ecma_3\Date"

Fails as follows:

## ecma_3\Date\15.9.5.7.js: rc = 0, run time = 1.039
15.9.5.7 Date.prototype.toLocaleTimeString()
 PASSED! typeof (now.toLocaleTimeString()) = string expected: string
 PASSED! Date.prototype.toLocaleTimeString.length = 0 expected: 0
 FAILED! d = new Date(0); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Wed Dec 31 1969 19:00:00 GMT-0800 (PDT) expected: Wed Dec 31 1969 16:00:00 GMT-0800 (PDT)
 FAILED! d = new Date(-28800000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Wed Dec 31 1969 11:00:00 GMT-0800 (PST) expected: Wed Dec 31 1969 08:00:00 GMT-0800 (PST)
 FAILED! d = new Date(-2208988800000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Sun Dec 31 1899 19:00:00 GMT-0800 (PST) expected: Sun Dec 31 1899 16:00:00 GMT-0800 (PST)
 FAILED! d = new Date(-2208960000000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Mon Jan 01 1900 03:00:00 GMT-0800 (PST) expected: Mon Jan 01 1900 00:00:00 GMT-0800 (PST)
 FAILED! d = new Date(946684800000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Fri Dec 31 1999 19:00:00 GMT-0800 (PST) expected: Fri Dec 31 1999 16:00:00 GMT-0800 (PST)
 FAILED! d = new Date(946713600000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Sat Jan 01 2000 03:00:00 GMT-0800 (PST) expected: Sat Jan 01 2000 00:00:00 GMT-0800 (PST)
 FAILED! d = new Date(951782400000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Mon Feb 28 2000 19:00:00 GMT-0800 (PST) expected: Mon Feb 28 2000 16:00:00 GMT-0800 (PST)
 FAILED! d = new Date(951782399000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Mon Feb 28 2000 18:59:59 GMT-0800 (PST) expected: Mon Feb 28 2000 15:59:59 GMT-0800 (PST)
 FAILED! d = new Date(951811200000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Tue Feb 29 2000 03:00:00 GMT-0800 (PST) expected: Tue Feb 29 2000 00:00:00 GMT-0800 (PST)
 FAILED! d = new Date(1484090367588); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Tue Jan 10 2017 18:19:27 GMT-0800 (PST) expected: Tue Jan 10 2017 15:19:27 GMT-0800 (PST)
 FAILED! d = new Date(1484119167588); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Tue Jan 10 2017 02:19:27 GMT-0800 (PST) expected: Tue Jan 10 2017 23:19:27 GMT-0800 (PST)
 FAILED! d = new Date(1104537600000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Fri Dec 31 2004 19:00:00 GMT-0800 (PST) expected: Fri Dec 31 2004 16:00:00 GMT-0800 (PST)
 FAILED! d = new Date(1104537599000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Fri Dec 31 2004 18:59:59 GMT-0800 (PST) expected: Fri Dec 31 2004 15:59:59 GMT-0800 (PST)
 FAILED! d = new Date(1104566400000); d == new Date(d.toDateString() + " " + d.toLocaleTimeString()) = Sat Jan 01 2005 03:00:00 GMT-0800 (PST) expected: Sat Jan 01 2005 00:00:00 GMT-0800 (PST)
REGRESSION - ecma_3\Date\15.9.5.7.js
[5|1|0|0]  66% ===================================>                   |   2.2s
## ecma_3\Date\15.9.5.5.js: rc = 0, run time = 1.18
15.9.5.5 Date.prototype.toLocaleString()
 PASSED! typeof (now.toLocaleString()) = string expected: string
 PASSED! Date.prototype.toLocaleString.length = 0 expected: 0
 FAILED! Math.abs(Date.parse(now.toLocaleString('en-US')) - now.valueOf()) < 1000 = false expected: true
 FAILED! Date.parse(Wed Dec 31 1969 16:00:00 GMT-0800 (PDT)).toLocaleString("en-US")) = 10800000 expected: 0
 FAILED! Date.parse(Wed Dec 31 1969 08:00:00 GMT-0800 (PST)).toLocaleString("en-US")) = -18000000 expected: -28800000
 FAILED! Date.parse(Sun Dec 31 1899 16:00:00 GMT-0800 (PST)).toLocaleString("en-US")) = -2208978000000 expected: -2208988800000
 FAILED! Date.parse(Mon Jan 01 1900 00:00:00 GMT-0800 (PST)).toLocaleString("en-US")) = -2208949200000 expected: -2208960000000
 FAILED! Date.parse(Fri Dec 31 1999 16:00:00 GMT-0800 (PST)).toLocaleString("en-US")) = 946695600000 expected: 946684800000
 FAILED! Date.parse(Sat Jan 01 2000 00:00:00 GMT-0800 (PST)).toLocaleString("en-US")) = 946724400000 expected: 946713600000
 FAILED! Date.parse(Mon Feb 28 2000 16:00:00 GMT-0800 (PST)).toLocaleString("en-US")) = 951793200000 expected: 951782400000
 FAILED! Date.parse(Mon Feb 28 2000 15:59:59 GMT-0800 (PST)).toLocaleString("en-US")) = 951793199000 expected: 951782399000
 FAILED! Date.parse(Tue Feb 29 2000 00:00:00 GMT-0800 (PST)).toLocaleString("en-US")) = 951822000000 expected: 951811200000
 FAILED! Date.parse(Fri Dec 31 2004 16:00:00 GMT-0800 (PST)).toLocaleString("en-US")) = 1104548400000 expected: 1104537600000
 FAILED! Date.parse(Fri Dec 31 2004 15:59:59 GMT-0800 (PST)).toLocaleString("en-US")) = 1104548399000 expected: 1104537599000
 FAILED! Date.parse(Sat Jan 01 2005 00:00:00 GMT-0800 (PST)).toLocaleString("en-US")) = 1104577200000 expected: 1104566400000
REGRESSION - ecma_3\Date\15.9.5.5.js

Comment 1

[Ted Campbell [:tcampbell]]

As :anba pointed out on #jsapi, this fails in the simple case of

$ TZ=PST8PDT build_DBG.OBJ/dist/bin/js.exe
js> print(`tz=${Intl.DateTimeFormat().resolvedOptions().timeZone}, offset=${new Date().getTimezoneOffset()}`)
tz=America/Toronto, offset=480

The TZ variable not working correctly

Comment 2

[André Bargull [:anba]]

Attachment: bug1330149.patch

Do you think this approach makes sense?

Comment 3

[André Bargull [:anba]]

Cc'ed Arthur Edelstein because https://trac.torproject.org/projects/tor/ticket/20981 seems to be related to this bug report.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8826359 [details] [diff] [review]
bug1330149.patch

Review of attachment 8826359 [details] [diff] [review]:
-----------------------------------------------------------------

This doesn't seem inconceivable to me.  It would be nice if this included definitive comments explaining just why it is those handful of time zone strings are the *only* ones that require special treatment, tho.  (And it would be nice if upstream handled it somehow, instead.)

::: js/src/vm/DateTime.cpp
@@ +9,5 @@
> +#if defined(XP_WIN)
> +#include "mozilla/UniquePtr.h"
> +
> +#include <stdlib.h>
> +#endif

Add /* defined(XP_WIN) */.

@@ +387,5 @@
>      if (guard.get() == IcuTimeZoneStatus::NeedsUpdate) {
> +        bool recreate = true;
> +#if defined(XP_WIN)
> +        const char* tz = getenv("TZ");
> +        if (IsOlsonCompatibleWindowsPosixId(tz)) {

Mild preference for |tz && IOCWPI(tz)| with no null-check in IOCWPI.

Comment 5

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important) from comment #4)
> This doesn't seem inconceivable to me.  It would be nice if this included
> definitive comments explaining just why it is those handful of time zone
> strings are the *only* ones that require special treatment, tho.  (And it
> would be nice if upstream handled it somehow, instead.)

It doesn't seem likely that ICU is going to use TZ on Windows, or at least this is the impression I get from the current comment at http://bugs.icu-project.org/trac/ticket/13130 (spun off from bug 1333916).

Comment 6

[Ted Campbell [:tcampbell]]

This is coming up again as we look to migrate spidermonkey jobs to taskcluster machines running in UTC instead of PDT. Perhaps we should disable these tests on Windows for now.

Comment 7

[Ted Campbell [:tcampbell]]

- ecma_3\Date\15.9.6.5.js is also failing now
- js1_5/extensions/toLocaleFormat-02.js is an UNEXPECTED-PASS on Windows in UTC

Comment 8

[Ted Campbell [:tcampbell]]

Attachment: 0001-Bug-1330149-Disable-tests-that-are-sensitive-to-host.patch

Disable the tests on Windows for now to unblock taskcluster migration.

Comment 9

[Ted Campbell [:tcampbell]]

Comment on attachment 8897533 [details] [diff] [review]
0001-Bug-1330149-Disable-tests-that-are-sensitive-to-host.patch

These are known, but low priority issues. What do you think about disabling the tests on Windows for now?

Comment 10

[André Bargull [:anba]]

Comment on attachment 8897533 [details] [diff] [review]
0001-Bug-1330149-Disable-tests-that-are-sensitive-to-host.patch

Review of attachment 8897533 [details] [diff] [review]:
-----------------------------------------------------------------

I think we still want to look into applying the other patch, because it fixes our inconsistent handling of the TZ environment variable on Windows (cf. comment #1), but to unblock TC migration it should be okay to skip these tests for now. 

js1_5/extensions/toLocaleFormat-02.js
- Date.prototype.toLocaleFormat will be removed in the near future (bug 818634), so it shouldn't really matter to skip this file on Windows.

ecma_3/Date/15.9.5.5.js
ecma_3/Date/15.9.5.6.js
ecma_3/Date/15.9.5.7.js
- Nowadays these tests are effectively testing the date formatting functions from ICU. Assuming that ICU formats dates the same way across all platforms, it should be okay to skip these tests on Windows, because we still get coverage from the other non-Windows platforms. 


Can you set the leave-open flag, so this bug doesn't get closed when your patch gets merged? Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e82c11723fcf
Disable tests sensitive to host TZ on Windows. r=anba

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e82c11723fcf

Comment 13

[Ted Campbell [:tcampbell]]

We had to disable tests as a temp work-around to unblock taskcluster migration work. This should be revisited with a real fix.

Comment 14

[André Bargull [:anba]]

Attachment: bug1330149-part1-TZ-on-windows.patch

Expanded the comments per the review feedback.

Comment 15

[André Bargull [:anba]]

Attachment: bug1330149-part2-reenable-tests.patch

Only re-enables the tests disabled in comment #12.

Comment 16

[André Bargull [:anba]]

Comment on attachment 8924120 [details] [diff] [review]
bug1330149-part1-TZ-on-windows.patch

I wonder why "TZ=UTC" works [1] even though it's not listed as supported in [2]. Well whatever, I guess I just update part 1 to allow "UTC". Clearing review for now.

[1] https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/toolkit/components/resistfingerprinting/nsRFPService.cpp#275
[2] https://msdn.microsoft.com/en-us//library/90s5c885.aspx#Anchor_1

Comment 17

[André Bargull [:anba]]

Attachment: bug1330149-part1-TZ-on-windows.patch

Updated to also allow "UTC", "UCT", and "GMT".

---

George [1] didn't add support for "GMT0", "GMT+0", and "GMT-0" to ICU, so the test case just ignores these ids... :-/

[1] https://searchfox.org/mozilla-central/rev/aa1343961fca52e8c7dab16788530da31aab7c8d/intl/icu/source/common/putil.cpp#728

Comment 18

[André Bargull [:anba]]

Check-in needed for 
- bug1330149-part1-TZ-on-windows.patch
- bug1330149-part2-reenable-tests.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f833d5c6182a3a05402fc7e5926759b2276cf0a1

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26a088e04dc6
Part 1: Set time zone from TZ environment variable as default ICU time zone if it's a valid time zone name. r=Waldo
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e51201d771e
Part 2: Re-enable time zone dependent tests on Windows. r=Waldo

Comment 20

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/26a088e04dc6
https://hg.mozilla.org/mozilla-central/rev/2e51201d771e