830304 - JS Date tests failing in UK

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

When I run the JS tests I see a whole bunch of failures related to Date.  My machine is set to the GMT/BST timezone.

I believe this is due to the same issue as brought up in bug 827816, the fact that the UK had year-round DST from 1969-1971.  Spidermonkey ignores the host system's concept of DST before 1st Jan 1970, creating a situation where DST appears to begin at midnight 1st Jan 1970, confusing the tests:

js> new Date(-1).toString()
"Wed Dec 31 1969 23:59:59 GMT+0000 (BST)"
js> new Date(0).toString()
"Thu Jan 01 1970 01:00:00 GMT+0100 (BST)"

The failing tests in full:

    ecma_3/Date/15.9.5.6.js
    ecma/Date/15.9.5.24-6.js
    ecma/Date/15.9.5.24-3.js
    ecma/Date/15.9.5.35-1.js
    ecma/Date/15.9.5.36-1.js
    ecma/Date/15.9.3.8-2.js
    ecma/Date/15.9.5.36-5.js
    ecma/Date/15.9.5.26-1.js
    ecma/Date/15.9.5.28-1.js
    ecma/Date/15.9.5.31-1.js
    ecma/Date/15.9.5.24-8.js
    ecma/Date/15.9.5.23-17.js
    ecma/Date/15.9.5.24-1.js
    ecma/Date/15.9.5.23-15.js
    ecma/Date/15.9.5.25-1.js
    ecma/Date/15.9.5.30-1.js
    ecma/Date/15.9.5.24-4.js
    ecma/Date/15.9.5.36-6.js
    ecma/Date/15.9.5.24-7.js
    ecma/Date/15.9.5.22-3.js
    ecma/Date/15.9.5.37-1.js
    ecma/Date/15.9.5.22-1.js
    ecma/Date/15.9.5.33-1.js
    ecma/Date/15.9.5.29-1.js
    ecma/Date/15.9.3.8-1.js
    ecma/Date/15.9.5.32-1.js
    ecma/Date/15.9.5.23-1.js
    ecma/Date/15.9.5.36-4.js
    ecma/Date/15.9.5.24-5.js
    ecma/Date/15.9.5.36-2.js
    ecma/Date/15.9.3.2-1.js
    ecma/Date/15.9.5.36-7.js
    ecma/Date/15.9.5.24-2.js
    ecma/Date/15.9.5.36-3.js
    ecma/Date/15.9.5.27-1.js
    ecma/Date/15.9.3.1-1.js
    ecma/Date/15.9.5.8.js
    ecma_5/Date/15.9.4.2.js

Comment 1

[André Bargull [:anba]]

Last time I've checked the Date tests, they contained code like `new Date(<str>).getTime() === <millis>`, where <str> and <milli> are some constants. So this is not really about GMT/BST timezone settings, but rather a general restriction of the test suite to use a specific timezone ("America/Los_Angeles" IIRC).

Comment 2

[Jon Coppeard (:jonco)]

(In reply to André Bargull from comment #1)

I believe the tests do pass in other places than the West coast of the US.

In fact there is a correction applied the for the test machine's time zone - see adjustResultArray() in js/src/tests/ecma/shell.js.

Comment 3

[Jeff Walden [:Waldo]]

What's test behavior now that bug 830257 is fixed?

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)

This is the behaviour after that fix :-(

Comment 5

[Jeff Walden [:Waldo]]

Probably it's just the longstanding behavior of assuming the tests are running in a particular time zone, then.  Too bad; I'd hoped this excess of failures (I saw far fewer the last time I ran out-of-P?T, in CST and EST) was implicated by the London quirk.

Comment 6

[André Bargull [:anba]]

Attachment: LocalTZA must not include DST

The computed LocalTZA value included DST (not allowed per 15.9.1.7), because mktime() normalizes its input, that means the |local| variable was altered. Using a fresh copy prevents this problem. And also only apply the DST offset in computeDSTOffsetMilliseconds() when the supplied time is in DST. 

This should drop the failure count for the GMT/BST timezone to a reasonable number.

Comment 7

[Jon Coppeard (:jonco)]

(In reply to André Bargull from comment #6)

> This should drop the failure count for the GMT/BST timezone to a reasonable
> number.

I can confirm this - now only 5 tests are failing with the patch, down from 40 without :-)

    ecma_3/Date/15.9.5.6.js
    js1_5/Regress/regress-58116.js
    js1_5/extensions/toLocaleFormat-02.js
    js1_5/extensions/toLocaleFormat-01.js
    ecma/Date/15.9.5.35-1.js
    ecma/Date/15.9.5.8.js

Comment 8

[André Bargull [:anba]]

Short explanation why those remaining tests still fail:
- js1_5/extensions/toLocaleFormat-0x and ecma_3/Date/15.9.5.6 are local dependent date format errors (i.e. not timezone related)
- js1_5/Regress/regress-58116 tests whether DST is the same on the following dates (*): 01/Aug/2005, 01/Aug/1970, 01/Aug/1965, 01/Aug/0000
- ecma/Date/15.9.5.8 can only be fixed when EU-DST rules instead of US-DST rules are used
- ecma/Date/15.9.5.35-1 will be fixed if the patch in bug 836404 gets accepted.

(*) this is a complete mess, see bug 58116 and bug 351066, comment 20

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 707641 [details] [diff] [review]
LocalTZA must not include DST

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

::: js/src/vm/DateTime.cpp
@@ +69,4 @@
>          // If |local| wasn't DST, we can use the same time.
>          currentNoDST = currentMaybeWithDST;
>      } else {
> +        // Create a fresh copy of |local|, because mktime() normalizes its input

Let's be more precise than "normalizes" (line-broken appropriately).

// Create a fresh copy of |local|, because mktime() will reset tm_isdst = 1 and will adjust tm_hour and tm_hour accordingly.

@@ +167,5 @@
>      struct tm tm;
>      if (!ComputeLocalTime(static_cast<time_t>(localTimeSeconds), &tm))
>          return 0;
> +    if (tm.tm_isdst == 0)
> +        return 0;

I'm not sure I buy this.  I'm happy with the first hunk, modulo the comment improvement; could you post a patch with just that in it for checkin?  We can get back to this change in a second patch (and I'll get to it in a second comment now).

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 707641 [details] [diff] [review]
LocalTZA must not include DST

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

::: js/src/vm/DateTime.cpp
@@ +167,5 @@
>      struct tm tm;
>      if (!ComputeLocalTime(static_cast<time_t>(localTimeSeconds), &tm))
>          return 0;
> +    if (tm.tm_isdst == 0)
> +        return 0;

So, computeDSTOffsetMilliseconds.  The algorithm it uses is this:

1. Given an input number of UTC seconds (btw, note your patch isn't against tip, and specifically not against the variable-renaming I did recently).
2. Get a broken-down local time for it.  This will include a DST component, possibly 0.
3. Get the seconds' offset into a day of (utcSeconds + LocalTZA), i.e. the local *standard* time, as dayoff.  
4. Get the seconds' offset into a day of the broken-down local time as tmoff.
5. This should hold: dayoff + DST offset == tmoff (mod SecondsPerDay).
6. So to get DST offset, compute tmoff - dayoff, possibly biased by SecondsPerDay.

Why should it be necessary to check for tm_isdst and abort early?  Seems to me that the result of the more elaborate algorithm will come to the same thing.  And for clarity purposes, I think we only want one way to do it here.  Date code's not perf-critical at this low level, given our DST caching.

Comment 11

[André Bargull [:anba]]

But that algorithm does not handle situations when year-round DST was observed, just as in UK during 1969-1971 (comment 0). Basically year-round DST means UK was shifted from LocalTZA = 0:00 to LocalTZA = +1:00 in 1969-1971.

Comment 12

[André Bargull [:anba]]

A related issue can be found for the timezone Africa/Bissau. The timezone is in 0:00, but until 1975 it was in -1:00. Applying the current computation will yield an DST offset of 23 hours.

Comment 13

[Jeff Walden [:Waldo]]

Yes, I'm quite familiar with London's situation in 1970 by now.  :-)  Could you point out *specifically* what's wrong in comment 10's algorithm WRT the UK's situtation in 1970?  I'm still not seeing anything.

Comment 14

[Jon Coppeard (:jonco)]

These test are passing since bug 1089745 landed.

Comment 15

[André Bargull [:anba]]

The underlying issue is still not fixed, see bug 1093130 for a recent bug report about this problem.

Comment 16

[André Bargull [:anba]]

Reopening per comment #15, see bug 1284507 for another recent bug report caused by UTCToLocalStandardOffsetSeconds returning the wrong offset.

Comment 17

[André Bargull [:anba]]

Attachment: tzoffset.patch

Updated patch.

This won't solve the start of the epoch issue for Europe/London, because that (probably) requires moving to ICU for historical time zone computations.

Comment 18

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> Why should it be necessary to check for tm_isdst and abort early?  Seems to
> me that the result of the more elaborate algorithm will come to the same
> thing.  And for clarity purposes, I think we only want one way to do it
> here.  Date code's not perf-critical at this low level, given our DST
> caching.

I no longer remember why I thought it's necessary to add this check. :-(

Comment 19

[André Bargull [:anba]]

(In reply to André Bargull from comment #17)
> Created attachment 8788540 [details] [diff] [review]
> tzoffset.patch
> 
> Updated patch.

Two things worth mentioning:
(1) The new patch depends on the changes from bug 1300825.
(2) Updating UTCToLocalStandardOffsetSeconds uncovered more time zone related issues, so I also needed to change ToPRMJTime and UTC (both in jsdate.cpp) to avoid regressions. ("two wrongs make a right" applied here, so when I fixed one wrong, other things broke. *sigh*)

Comment 20

[André Bargull [:anba]]

Attachment: bug830304.patch

Updated the patch to apply cleanly on current central (JS_ReportError -> JS_ReportErrorASCII).

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

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 8798749 [details] [diff] [review]
bug830304.patch

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

I wish I'd reviewed this awhile ago, and now I'm out of time to do so now.  I'll clear this so someone else who isn't going to be absent for four months can review this, if so desired.  Otherwise feel free to reassign review to me on my return.

Comment 23

[Jeff Walden [:Waldo]]

Comment on attachment 8798749 [details] [diff] [review]
bug830304.patch

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

> I'll clear this so someone else who isn't going to be absent for four months
> can review this, if so desired.  Otherwise feel free to reassign review to
> me on my return.

You were supposed to have someone else review this while I was gone, not take my invitation seriously to throw this back at me when I returned!  :-P

::: js/src/builtin/TestingFunctions.cpp
@@ +3544,5 @@
> +#ifdef XP_UNIX
> +static bool
> +GetTimeZone(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    using namespace std;

Mild preference for std::-qualifying stuff rather than just picking it up unexpectedly on implicit things.

@@ +3563,5 @@
> +        if (localtime_r(&now, &local)) {
> +#if defined(HAVE_TM_ZONE_TM_GMTOFF)
> +            tz = local.tm_zone;
> +#else
> +            tz = tzname[local.tm_isdst > 0];

Where is this def...oh, it's std::tzname.  Didn't realize there was a global variable like this in <ctime>.  This is just the reason why we should be std::-qualifying these names -- or at least |using std::tzname;| and so on to clearly import specific definitions.

@@ +4146,5 @@
> +
> +    JS_FN_HELP("setTimeZone", SetTimeZone, 1, 0,
> +"setTimeZone(tzname)",
> +"  Set the 'TZ' environment variable to the given time zone and applies the new time zone.\n"
> +"  An empty string or undefined resets the time zone to its default value."),

Note that the string is used verbatim, with zero validity checking to verify that it actually is a time zone and not just garbage input.

::: js/src/tests/ecma_6/Date/time-zone-pst.js
@@ +2,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Note: The default time zone is set to PST8PDT for all jstests (when run in shell).

Possibly worth a comment that this test depends on the current DST rules as of $now, and that if Congress decides to change things -- and we haven't changed SpiderMonkey to expose real DST behavior, not faked-up modern rules applied historically -- this test might need to change.

::: js/src/tests/ecma_6/Date/time-zones.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const hourToMilli = 60 * 60 * 1000;

Mild preference in these tests for using the standard's names for them, e.g. msPerHour (?) and such.

@@ +5,5 @@
> +const hourToMilli = 60 * 60 * 1000;
> +
> +// setTimeZone() is not available on Windows.
> +if (typeof setTimeZone === "function") {
> +    function inTimeZone(tzname, fn) {

I'd probably minimize indentation by putting an early-return inside this function, rather than wrapping the entire test in a big |if|.

@@ +16,5 @@
> +    }
> +
> +    // bug 158328
> +    inTimeZone("Europe/London", () => {
> +        let dt1 = new Date(2002, 6, 19, 16, 10, 55);

Please use |7 - 1| and so on for all the months in this file, and likewise in the other if there are any like so.

Comment 24

[André Bargull [:anba]]

(In reply to Jeff Walden [:Waldo] (I'm baaaaaaack...) from comment #23)
> Comment on attachment 8798749 [details] [diff] [review]
> bug830304.patch
> 
> Review of attachment 8798749 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > I'll clear this so someone else who isn't going to be absent for four months
> > can review this, if so desired.  Otherwise feel free to reassign review to
> > me on my return.
> 
> You were supposed to have someone else review this while I was gone, not
> take my invitation seriously to throw this back at me when I returned!  :-P

Hah! :-)


> ::: js/src/builtin/TestingFunctions.cpp
> @@ +3544,5 @@
> > +#ifdef XP_UNIX
> > +static bool
> > +GetTimeZone(JSContext* cx, unsigned argc, Value* vp)
> > +{
> > +    using namespace std;
> 
> Mild preference for std::-qualifying stuff rather than just picking it up
> unexpectedly on implicit things.

Done.

> 
> @@ +3563,5 @@
> > +        if (localtime_r(&now, &local)) {
> > +#if defined(HAVE_TM_ZONE_TM_GMTOFF)
> > +            tz = local.tm_zone;
> > +#else
> > +            tz = tzname[local.tm_isdst > 0];
> 
> Where is this def...oh, it's std::tzname.  Didn't realize there was a global
> variable like this in <ctime>.  This is just the reason why we should be
> std::-qualifying these names -- or at least |using std::tzname;| and so on
> to clearly import specific definitions.

"tzname" is a global variable or at least from what I've read, portable POSIX-code requires no "std" namespace qualifier for it. 

> 
> @@ +4146,5 @@
> > +
> > +    JS_FN_HELP("setTimeZone", SetTimeZone, 1, 0,
> > +"setTimeZone(tzname)",
> > +"  Set the 'TZ' environment variable to the given time zone and applies the new time zone.\n"
> > +"  An empty string or undefined resets the time zone to its default value."),
> 
> Note that the string is used verbatim, with zero validity checking to verify
> that it actually is a time zone and not just garbage input.

Done.

> 
> ::: js/src/tests/ecma_6/Date/time-zone-pst.js
> @@ +2,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +// Note: The default time zone is set to PST8PDT for all jstests (when run in shell).
> 
> Possibly worth a comment that this test depends on the current DST rules as
> of $now, and that if Congress decides to change things -- and we haven't
> changed SpiderMonkey to expose real DST behavior, not faked-up modern rules
> applied historically -- this test might need to change.

The test only uses dates in 2016. And I really hope the Congress doesn't decide one day to apply different DST rules for past dates. :-)
Well, there's the issue of Windows not really supporting past DST rules, but adding a comment about that issue is probably more confusing than it helps...

> 
> ::: js/src/tests/ecma_6/Date/time-zones.js
> @@ +1,5 @@
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +const hourToMilli = 60 * 60 * 1000;
> 
> Mild preference in these tests for using the standard's names for them, e.g.
> msPerHour (?) and such.

Done.

> 
> @@ +5,5 @@
> > +const hourToMilli = 60 * 60 * 1000;
> > +
> > +// setTimeZone() is not available on Windows.
> > +if (typeof setTimeZone === "function") {
> > +    function inTimeZone(tzname, fn) {
> 
> I'd probably minimize indentation by putting an early-return inside this
> function, rather than wrapping the entire test in a big |if|.

Well, it turned out it actually makes sense to add support for "setTimeZone" on Windows (caught a bug where DateTimeInfo::computeDSTOffsetMilliseconds computed a DST offset of 86400 seconds, so one full day, on Windows systems). We'll only be able test EST5EDT, CST6CDT, MST7MDT, and PST8PDT (cf. bug 1330149) in a cross-platform compatible way, but I guess that's better than no tests at all.

> 
> @@ +16,5 @@
> > +    }
> > +
> > +    // bug 158328
> > +    inTimeZone("Europe/London", () => {
> > +        let dt1 = new Date(2002, 6, 19, 16, 10, 55);
> 
> Please use |7 - 1| and so on for all the months in this file, and likewise
> in the other if there are any like so.

I've added an object to act as a poor man's enum to map from month names to month indices.

Comment 25

[André Bargull [:anba]]

Attachment: bug830304.patch

Changes:
- Applied review comments.
- Fixed a bug in DateTimeInfo::computeDSTOffsetMilliseconds where the DST offset was computed as 86400 seconds (one day). 
- Updated getTimeZone/setTimeZone testing functions to work on Windows.
- Moved setTimeZone to FuzzingUnsafeTestingFunctions instead of testing |fuzzingSafe| when setTimeZone was called.
- Added more regression tests to tests/ecma_6/Date/time-zones.js (bugs 294908, 610183, 637244, 802627, 879261, 994086, 1303306, 1317364, 1335818, 1355272).
- Added tests/ecma_6/Date/time-zones-posix.js based on time-zones.js for time zones which can be expressed as of one EST5EDT, CST6CDT, MST7MDT, or PST8PDT.
- time-zones-posix.js can be run on Windows, but shell-only. For some reason the setTimeZone() function doesn't work in Windows browser environments in automation. I wasn't able to reproduce that issue in local Windows builds. :-/


Optimistically carrying r+ from Waldo. :-)

Comment 26

[André Bargull [:anba]]

OS X 10.10 doesn't seem to use the correct DST data for Asia/Novosibirsk [1]. `new Date(1984, 4-1, 1).toString()` should be "Sun Apr 01 1984 01:00:00 GMT+0800", shouldn't it?


Excerpt from tzdata2017b

# Rule	NAME	FROM	TO	TYPE	IN	ON	AT	SAVE	LETTER/S
Rule	Russia	1981	1984	-	Apr	 1	 0:00	1:00	S
Rule	Russia	1985	2010	-	Mar	lastSun	 2:00s	1:00	S

# Zone	NAME		GMTOFF	RULES	FORMAT	[UNTIL]
Zone Asia/Novosibirsk	 5:31:40 -	LMT	1919 Dec 14  6:00
			 6:00	-	+06	1930 Jun 21
			 7:00	Russia	+07/+08	1991 Mar 31  2:00s

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc6a24ec10173c5f262c52d57fecaf874013911d

Comment 27

[André Bargull [:anba]]

Attachment: bug830304.patch

Moved the Asia/Novosibirsk test case (comment #26) into a separate file to unblock this patch from landing. No other changes, therefore carrying r+.

Comment 28

[André Bargull [:anba]]

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

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ef07909cc4
Compute correct time zone offsets for Date methods. r=Waldo

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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

Comment 31

[Jon Coppeard (:jonco)]

\o/