591845 - New Zealand timezones incorrect

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Geoff Lankow (:darktrojan)]

STR:
* Set Windows timezone to New Zealand (seems to be a Windows-only thing)
* javascript:alert(new Date())

Expected: Sun Aug 29 2010 14:12:21 GMT+1200 (New Zealand Standard Time)
Got:      Sun Aug 29 2010 14:12:21 GMT-1200 (New Zealand Daylight Time)

Discussed this on IRC, and we suspect bug 578259.

Comment 1

[Geoff Lankow (:darktrojan)]

> Expected: Sun Aug 29 2010 14:12:21 GMT+1200 (New Zealand Standard Time)

I'm really on top of things today. I meant: 
Mon Aug 30 2010 14:12:21 GMT+1200 (New Zealand Standard Time)

Comment 2

[Justin Dolske [:Dolske]]

Blair confirmed that Date.now() reports about the same value on his Windows and OS X boxes (+/- a few seconds), but that that the |new Date()| string date was 12 hours off as reported above.

Comment 3

[Brian Hackett [Laid off!]]

Is there a Windows box available for testing fixes?  I only have an OSX box, and it seems to be doing the right thing:

Mon Aug 30 2010 15:54:52 GMT+1200 (NZST)

Comment 4

[Brian Hackett [Laid off!]]

Windows and OS X behave differently on this because they disagree over whether New Zealand was observing daylight savings time on January 1, 1970.  The basic problem, though, is that bug 578259 is not accounting correctly for locations that were observing daylight savings time at the epoch.  It's also doing the wrong thing for the Kingdom of Tonga, which is at UTC+1300.  Working on a fix for both of these.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: fix

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 470598 [details] [diff] [review]
fix

>+/* Get the local time. localtime_r is preferred as it is reentrant. */
>+static inline bool
>+computeLocalTime(time_t local, struct tm *ptm)

Nit: prevailing style for static helpers (deviations in jstracer.cpp aside) uses StudlyCaps not camelCaps.

>+{
>+#ifdef HAVE_LOCALTIME_R
>+    return localtime_r(&local, ptm);
>+#else
>+    struct tm *otm = localtime(&local);
>+    if (!otm)

Is localtime fallible? My Mac OS X man page says it always returns the address of its static buffer.

>     /*
>      * Get the difference between this time zone and GMT, by checking the local
>-     * time at the epoch.
>+     * time for days 0 and 180 of 1970, using a date for which daylight
>+     * savings time was not in effect.
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

Nit: rewrap so savings goes on the same line as daylight, it fits.

>      */
>-    time_t local = 0;
>+    JSInt32 day = 0;

Nit: do not use the JSInt32, etc. types, they are a historical relic from 1998. Use int32 if width matters (it doesn't here, right?) or just int (for best performance in general for locals, assuming sane compiler and target arch).

>+        if (!computeLocalTime(PRMJ_DAY_SECONDS * day, &tm))
>+            return 0;

Failure here means epoch start time, but I still don't know how localtime can fail (return null).

>-#ifndef HAVE_LOCALTIME_R
>-    struct tm *ptm = localtime(&local);
>-    if (!ptm)
>+    if (!computeLocalTime(static_cast<time_t>(localTimeSeconds), &tm))
>         return 0;

Ditto re: fallibility.

r=me with fixes.

/be

Comment 7

[Brian Hackett [Laid off!]]

I've been using:

http://www.opengroup.org/onlinepubs/000095399/functions/localtime.html

Which says localtime and localtime_r can both return NULL.  I don't know whether that ever actually occurs.

Comment 8

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/tracemonkey/rev/bb8eeb9890a7

localtime return value checks are still there, talked about this with Brendan on IRC.

bhackett: brendan: also, did you look at that localtime thing?
[2:43pm] bhackett: opengroup.org says it can EOVERFLOW
[2:43pm] brendan: bhackett: oops, behind on bugmail
[2:43pm] brendan: bhackett: ok, i gave r+ modulo fixes
[2:43pm] brendan: sounds like no fix needed there (hard to believe)
[2:44pm] bhackett: I don't think it's really possible
[2:44pm] bhackett: still don't want to SEGV if the OS is being weird

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/bb8eeb9890a7

Comment 11

[Paul Biggar]

Did this bug fix the test failures from bug 530974? If not, how was it tested?

Comment 12

[Brian Hackett [Laid off!]]

I don't know if this fixes 530974.  This is fixing a regression from bug 578259, which postdates 530974; I tested by building on a Windows machine (which is where this bug manifests), making sure that 'new Date()' produces the correct time for New Zealand and nearby timezones, and that the date-format-* gave the same set of times.  Timezone-related bugs are hard to write tests for, as JS always gets the timezone from the OS.