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

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: tcampbell, Assigned: anba)

Tracking

Trunk
mozilla58
x86
Windows 10
Points:
---

Firefox Tracking Flags

(firefox53 affected, firefox58 fixed)

Details

(Whiteboard: [tor 20981])

Attachments

(3 attachments, 2 obsolete attachments)

> 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
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
Posted patch bug1330149.patch (obsolete) — Splinter Review
Do you think this approach makes sense?
Attachment #8826359 - Flags: feedback?(jwalden+bmo)
Cc'ed Arthur Edelstein because https://trac.torproject.org/projects/tor/ticket/20981 seems to be related to this bug report.
Assignee: nobody → andrebargull
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.
Attachment #8826359 - Flags: feedback?(jwalden+bmo) → feedback+
(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).
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.
- 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
Disable the tests on Windows for now to unblock taskcluster migration.
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?
Attachment #8897533 - Flags: review?(andrebargull)
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!
Attachment #8897533 - Flags: review?(andrebargull) → review+
Keywords: leave-open
Pushed by tcampbell@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e82c11723fcf
Disable tests sensitive to host TZ on Windows. r=anba
We had to disable tests as a temp work-around to unblock taskcluster migration work. This should be revisited with a real fix.
Priority: -- → P3
Expanded the comments per the review feedback.
Attachment #8826359 - Attachment is obsolete: true
Attachment #8924120 - Flags: review?(jwalden+bmo)
Only re-enables the tests disabled in comment #12.
Attachment #8924121 - Flags: review?(jwalden+bmo)
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
Attachment #8924120 - Flags: review?(jwalden+bmo)
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
Attachment #8924120 - Attachment is obsolete: true
Attachment #8924230 - Flags: review?(jwalden+bmo)
Attachment #8924121 - Flags: review?(jwalden+bmo) → review+
Attachment #8924230 - Flags: review?(jwalden+bmo) → review+
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
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Whiteboard: [tor 20981]
You need to log in before you can comment on or make changes to this bug.