485013 - jsdate.cpp AdjustTime() breaks London (GMT+0) dates when DST activates Mar 29 2009

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew Paprocki]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.7) Gecko/2009021910 Firefox/3.0.7
Build Identifier: mozilla-central

Previously, AdjustTime() used to be a macro:

#define AdjustTime(t)   fmod(LocalTZA + DaylightSavingTA(t), msPerDay)

It was changed to a function in the following rev:
http://hg.mozilla.org/mozilla-central/diff/d2b395fc397e/js/src/jsdate.cpp

static jsdouble
AdjustTime(jsdouble date)
{
    jsdouble t = DaylightSavingTA(date) + LocalTZA;
    t = (LocalTZA > 0) ? fmod(t, msPerDay) : -fmod(msPerDay - t, msPerDay);
    return t;
}

If the LocalTZA is 0 or -0, the -fmod case will be used, causing the date offset to be incorrect. From what I can tell, this will occur for all UK users when DST goes into effect there Mar 29, 2009 at 2am.

Reproducible: Always

Steps to Reproduce:
In order to reproduce this easily in the US, simple hacks to prmjtime.cpp can be made to simulate London with DST enabled:

JSInt32
PRMJ_LocalGMTDifference()
{
    return 0;
}

JSInt64
PRMJ_DSTOffset(JSInt64 local_time)
{
    JSInt64 retval;
    JSLL_I2L(retval, 60*60);
    JSLL_MUL(retval, retval, 1000);
    JSLL_MUL(retval, retval, 1000);
    return retval;
}
Actual Results:  
With the above prmjtime.cpp hacks in place:

js> new Date(2009, 3, 1)
Wed Apr 01 2009 00:00:00 GMT-2300

Expected Results:  
With the above prmjtime.cpp hacks in place as well as a (dumb dumb) patch to jsdate.cpp to illustrate the expected value:

    if (LocalTZA == 0 || LocalTZA == -0)
        t = fmod(t, msPerDay);
    else
        t = (LocalTZA > 0) ? fmod(t, msPerDay) : -fmod(msPerDay - t, msPerDay);

js> new Date(2009, 3, 1)
Wed Apr 01 2009 00:00:00 GMT+0100

Comment 1

[Brendan Eich [:brendan]]

We can't regress this if it's valid. Marking NEW for Andrew.

/be

Comment 2

[Brian Crowder]

So we should just be using LocalTZA >= 0, instead of LocalTZA > 0 ....  right?  Or am I missing something?

Comment 3

[Andrew Paprocki]

I wasn't sure if LocalTZA >= 0 would catch the -0 case. It appears to in a simple test, so that should work.

Comment 4

[Andreas Gal :gal]

-0 == 0, so yes, >= 0 should work (assuming LocalTZA is a jsdouble).

Comment 5

[Brian Crowder]

Attachment: the fix

This doesn't regress the Venezuela bug, either...  yay.  :)

Comment 6

[Brian Crowder]

And yes, all this math is done with jsdoubles.  Thanks for the reassurance, Andreas.

Comment 7

[Brendan Eich [:brendan]]

Let's get this into tm first, m-c next, and 1.9.1 when sayrer says so or does the deed. Main thing is tm first. Thanks,

/be

Comment 8

[Robert Sayre]

http://hg.mozilla.org/tracemonkey/rev/88e89529299c

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/88e89529299c

Comment 10

[Brian Crowder]

This was fixed in 1.9.1 in bug 411726.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified fixed with testcase in comment 0 with the following debug builds:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre)
Gecko/20090522 Minefield/3.6a1pre ID:20090522133810

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre)
Gecko/20090522 Shiretoko/3.5pre ID:20090522153422

Haven't we tests which checks for time correctness?