554338 - Incorrect time zone abbreviation string given by js Date objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Karl Norby]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.2) Gecko/20100316 Firefox/3.6.2

On Firefox 3.5 and higher on Linux (Ubuntu in every case of different version, some using Ubuntu packaged version, and some Mozilla), I see the incorrect time zone abbreviation. After the recent time zone shift, I should see the time zone abbreviation as CDT (GMT-0500), but I see CST. CST is GMT-0600, but I see the correct offset of GMT-0500. Every other application on my system (including Thunderbird 2.0.0.23) gives the time zone as CDT ('date +%Z` gives CDT as well). I have also tested this on firefox 3.5 on Solaris, and I get CDT. As I said, the time and the GMT offset are both correct, just the abbreviation is wrong.  

Reproducible: Always

Steps to Reproduce:
1. Create a new javascript date object ("javascript:new%20Date();" or other means).
Actual Results:  
Tue Mar 23 2010 10:45:44 GMT-0500 (CST)

Expected Results:  
Tue Mar 23 2010 10:45:44 GMT-0500 (CDT)

I believe some web apps use the abbreviation instead of offset (I think gmail is one), which is how this problem came to my attention.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I see this on Mac too.  Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f54a6c9a0316&tochange=670a3b50dfe0 (back in Jan 2009).

Looks like a regression from bug 411726 to me at first glance.  It's using the timezone of time_t == 0 in all cases, looks like.

Blake, you reviewed the patch in that bug, right?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Note that PRMJ_FormatTime only seems to be used to format the timezone name, and it now does that completely wrong half of the year...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: This sorta fixes it, but looks.... wrong to me and jorendorff

In general, seems like the caller should just have more context than we're getting here, or something.

Comment 5

[David Mandelin [:dmandelin]]

(In reply to comment #3)
> Created attachment 434453 [details] [diff] [review]
> This sorta fixes it, but looks.... wrong to me and jorendorff
> 
> In general, seems like the caller should just have more context than we're
> getting here, or something.

I assume you mean the caller should be telling you if it's DST instead of making PRMJ_FormatTime puzzle it out?

I took a look at it, and I think this code is bogus:

#if defined(HAVE_LOCALTIME_R) && defined(HAVE_TM_ZONE_TM_GMTOFF)
    {
        struct tm td;
        time_t bogus = 0;
        localtime_r(&bogus, &td);
        a.tm_gmtoff = td.tm_gmtoff;
        a.tm_zone = td.tm_zone;
    }
#endif

There is a comment suggesting that Linux and SunOS 4 crash unless tm_zone is filled in. But that comment predates the code block above. And Bill tried disabling this code on his system, and it didn't crash--in fact it fixed this bug. Is that block necessary to get Caracas time right, or what?

Comment 6

[David Mandelin [:dmandelin]]

Attachment: Patch (slightly cleaned up version of prev)

I thought about this for a while, looked at the previous patch, and decided it's basically right. Dumb reason why: that's how V8 does it. Also, based on the man page for localtime, it should in fact work, as briefly explained by the comment in my revision on the patch.

I'm guessing the unease expressed in comment 3 is related to this: The source of the problem is that PRMJTime is meant to be a copy of |struct tm|, but some systems store timezone information in |struct tm| that is not in PRMJTime. So, on those systems PRMJTime is a bit 'wrong' without those things. An alternative solution would be to add those fields to PRMJTime where they occur. That seems kind of annoying just to fix one date-formatting function. And the original hack is correct, AFAICT. If you think that's really wrong or something, I guess adding fields to PRMJTime is the way to go. But hopefully not.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 492742 [details] [diff] [review]
Patch (slightly cleaned up version of prev)

OK, I'll buy that. I hope Caracas doesn't suffer because of it...

Comment 8

[David Mandelin [:dmandelin]]

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

I did verify the Caracas test case still works with this version.

Comment 9

[Robert Sayre]

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