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

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).

(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.

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3) This is the behaviour after that fix :-(

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.

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.

(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

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 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 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.

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.

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.

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.

These test are passing since bug 1089745 landed.

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

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

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.

(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. :-(

(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*)

Updated the patch to apply cleanly on current central (JS_ReportError -> JS_ReportErrorASCII). Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58477b66af97127f02063372492e915ae0816189

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 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.

(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.

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. :-)

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

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

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

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b7ef07909cc4 Compute correct time zone offsets for Date methods. r=Waldo

\o/