Bug 106750 – String(new Date(2010,10,1)) is wrong in KRAT, YAKT

• Component: JavaScriptCore
• Status: RESOLVED CONFIGURATION CHANGED

[john]

I've tested on Safari 6.0.2 and latest Webkit nightly in Mountain Lion.
I'm using Krasnoyarsk Standard Time (KRAT). I've tested in China Standard Time (CST), the bug still exists.

new Date(2010, 10, 1) returns
Sun Oct 31 2010 01:00:00 GMT+0800 (KRAT)

This is not the date(Nov 1, 2010) I want.


When the year is not 2010, everything's fine. Like

new Date(2011, 10, 1) returns
Tue Nov 01 2011 00:00:00 GMT+0800 (KRAT)

new Date(2012, 10, 1) returns
Thu Nov 01 2012 00:00:00 GMT+0800 (KRAT)


I've tested on Google Chrome 24.0.1312.45 beta (Mac) and Safari 5.1.5 (Windows), new Date(2010, 10, 1) returns Nov 1 as expected.
So I assume it's a JavaScriptCore bug.

[john]

I changed time zone a couple of times, in KRAT, the bug still exists. But in CST, I could no longer reproduce this bug on Safari 6.0.2.

[john]

According to
http://www.timeanddate.com/worldclock/timezone.html?n=372&syear=2000
http://www.timeanddate.com/worldclock/timezone.html?n=372&syear=2010

KRAT uses daylight saving time in 2000~2009, then after Sunday, 31 October, 03:00 of 2010, KRAT sets to UTC+8h permanently.

So if I set the hour to anything larger that 2, then
new Date(2010,10,1,2)

yields correct result
Mon Nov 01 2010 02:00:00 GMT+3100 (KRAST)

A big thank to @fossilet for pointing it out.

[john]

Perhaps, the Date function treats KRAT time differently before and after the DST change.
So when the time is before the Sunday, 31 October, 02:00 of 2010, like,
new Date(2010,10,1,1,59,59)

it returns
Sun Oct 31 2010 02:59:59 GMT+0800 (KRAT)

when adding one second to the previous example
new Date(2010,10,2)

it returns
Mon Nov 01 2010 02:00:00 GMT+3100 (KRAST)

While the date is right,
the timezone (GMT+3100) is not quite right (maybe), it should be GMT+0800.

[Gavin Barraclough]

Updated the title – the Date object itself is fine, it's the toString conversion that is going wrong.

There are many things going wrong here.  ECMA requires that we do not take historically accurate timezone information into account, and to apply present DST rules to dates in the past.

In retrieving timezone information for a date, the system will provide historically accurate timezone information, which is not what we want.  So we try to extract timezone information from an "equivalent" year – a year that present timezone rules will be applied to, but where key characteristics match the year in question (specifically the equivalent year should have the same leap-year-ness, and should start on the same weekday since DST rules often key off this).

Here comes the first bug.  In trying to find an equivalent year, we assume that there is a leap year every four years, and based on this that adding 28 to the year will map to an equivalent year.  This is broken.

The second bug relates to the fact that the world ends in 2038 (for 32-bit machines).  As such we'll look for an equivalent year no later than the 28 year span 2010-2037.  Since 2010 is in the past, this restriction means that for timezones (including KRAT, YAKT) that have made DST changes since 2010, we'll fail to map to an equivalent date that follows present DST rules, and will incorrectly incorporate historically accurate information.

Third bug, in calculateDSTOffsetSimple, we do: "if (diff < 0) diff += secondsPerDay;".  This is converting the timezone offset -01:00 to +23:00, which is what makes the date leap forwards from just after midnight Oct 31 to just after midnight Nov 01, wham only an hour has passed.

Finally, in the YAKT timezone I notice that the dates pre the timezone change are shown as being YAKT, and the dates post the timezone change are shown as being YAKST - which is backwards.  I haven't yet debugged this last issue further.

[Gavin Barraclough]

Okay, the last two issues are closely related, and also ultimately rise from the 2038 problem.

In determining the local time offset, we separately calculate the offset from UTC to local standard time, and from standard time to DST.  We check for the DST offset being non-zero to indicate daylight saving being in affect.

The code does the "+= secondsPerDay" because in calculating the offset from standard to DST it sloppily just calculates the difference in hours/minutes/seconds – ignoring days/months/years, and assumes that if the clock has moved backwards that is because we've rolled around past midnight.  This is probably usually true, but makes the code pretty fragile.

Since UTC calculation is not dependent on finding an "equivalent year" we always use the current year (assuming, as the spec does, that UTC offset for a given timezone never changes – which is of course wrong).  Since the DST offset should also be calculated using current data these should always be in sync – however the 2038 bug means that for years from 2010 we can't find an equivalent year, so we'll stick with the historic data.  This means we can end up using a mix of current (UTC) and historic (DST) data.

In 2010 KRAT summertime was at +8, and standard time was at +7.  Since mid 2011 standard time has been at +8 (and there has been no DST).  Comparing these values, we see the summer of 2010 as being standard time since +8 == +8 (whereas in reality it was DST), and the period that should be standard time we treat as being DST with an offset of -1 (resulting in the YAKT/YAKST timezone names being backwards).  The DST offset of -1 trips over the fragile "+= secondsPerDay", and gets converted into a DST offset of +23.

The really crazy thing here is that we basically don't need to be doing any of this.  We're calculating the UTC-to-standard and standard-to-DST offsets separately, but they're always just added together to calculated the offset from UTC to local time – we just need to know this combined offset, and whether DST is in force at a given time.  But (on most systems) we can just get these values straight from localtime_r  – much of the work done to calculate these values is completely unnecessary.

[Gavin Barraclough]

Created attachment 203020 [details]
Fix part 1 - simplify handling of local time offsets

[WebKit Commit Bot]

Attachment 203020 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/runtime/JSDateMath.cpp', u'Source/JavaScriptCore/runtime/VM.cpp', u'Source/JavaScriptCore/runtime/VM.h', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/DateMath.cpp', u'Source/WTF/wtf/DateMath.h', u'Source/WTF/wtf/GregorianDateTime.cpp']" exit_code: 1
Source/WTF/wtf/DateMath.cpp:481:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WTF/wtf/DateMath.cpp:485:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WTF/wtf/DateMath.h:114:  Please declare integral type bitfields with either signed or unsigned.  [runtime/bitfields] [5]
Source/WTF/wtf/DateMath.h:115:  Please declare integral type bitfields with either signed or unsigned.  [runtime/bitfields] [5]
Total errors found: 4 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 203020 [details]
Fix part 1 - simplify handling of local time offsets

Attachment 203020 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/703205

[Gavin Barraclough]

Created attachment 203024 [details]
Fix part 1 v2 - fix style issues, windows build fix pt 1.

[Build Bot]

Comment on attachment 203024 [details]
Fix part 1 v2 - fix style issues, windows build fix pt 1.

Attachment 203024 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/724144

[Gavin Barraclough]

Created attachment 203026 [details]
Fix part 1 v3 - more windows build fix

[Darin Adler]

Comment on attachment 203026 [details]
Fix part 1 v3 - more windows build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=203026&action=review

> Source/JavaScriptCore/ChangeLog:13
> +        (JSC):

Please remove bogus lines like this one when buggy prepare-ChangeLog adds them.

> Source/JavaScriptCore/ChangeLog:21
> +        (JSC::VM::VM):

Please remove bogus lines like this one when buggy prepare-ChangeLog adds them.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:135
> +static LocalTimeOffset getLocalTimeOffset(ExecState* exec, double ms)

WebKit coding style would call this localTimeOffset, not getLocalTimeOffset. By convention, we aim to use get exclusively for functions that use out arguments.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:197
> +    // convert to UTC

Probably should dump this comment. The other function doesn’t have this.

> Source/JavaScriptCore/runtime/JSDateMath.cpp:207
> +    LocalTimeOffset localTimeOffset(false, 0);

Would be nicer to just have the LocalTimeOffset default constructor start with these values so that the (false, 0) can be omitted.

> Source/JavaScriptCore/runtime/VM.h:107
> +            : offset(false, 0)

This explicit initializer would not be needed if the default constructor for LocalTimeOffset existed, and I do suggest adding that. Also, this constructor sets the offset twice, because the reset function sets the offset.

> Source/JavaScriptCore/runtime/VM.h:114
> +            offset = LocalTimeOffset(false, 0);

It would be more readable to reset the two members individually.

If you do want to use struct assignment, I’d suggest using the default constructor here instead of the “false, 0”.

I’m not sure what members the reset function needs to affect; are these ever inspected without writing values into them? It seems that to invalidate the cache, only the key would need to be overwritten.

> Source/WTF/ChangeLog:13
> +        (WTF):

More bogus prepare-ChangeLog unhappiness.

> Source/WTF/ChangeLog:28
> +        (WTF):

More.

> Source/WTF/wtf/DateMath.cpp:473
> +    else if (localTimeSeconds < 0) // Go ahead a day to make localtime work (does not work with 0)

Would be idiomatic to put a period on this comment.

> Source/WTF/wtf/DateMath.cpp:475
> +    // FIXME: time_t has a potential problem in 2038

Would be idiomatic to put a period on this comment.

> Source/WTF/wtf/DateMath.cpp:476
> +    time_t localTime = static_cast<time_t>(localTimeSeconds);

Seems unnecessary to involve time_t at all in the !HAVE(TM_GMTOFF) case; can we isolate this type cast to that side of the #if branch?

> Source/WTF/wtf/DateMath.h:102
> +struct LocalTimeOffset {

It would be better to have the struct at the top of the file rather than right before the function that returns it.

> Source/WTF/wtf/DateMath.h:103
> +    LocalTimeOffset(bool isDST, int offset)

Could just put "= false" and "= 0" here and make this a default constructor too.

> Source/WTF/wtf/DateMath.h:109
> +    bool operator==(const LocalTimeOffset& other)

Typically we do != as well as ==, but I suppose it would be dead code.

> Source/WTF/wtf/DateMath.h:118
> +WTF_EXPORT_PRIVATE LocalTimeOffset calculateLocalTimeOffset(double);

Needs an argument name. I suggest “localTimeInMilliseconds” if that is accurate.

[Gavin Barraclough]

> I’m not sure what members the reset function needs to affect; are these ever inspected without writing values into them? It seems that to invalidate the cache, only the key would need to be overwritten.

Since the cache covers a date range, I think the key is effectively formed by all of start, end and increment.  We could probably avoid resetting offset, but since this is set by the constructor it would be asymmetric not to reset it in the reset method.  I'm inclined to leave as is...

> > Source/WTF/wtf/DateMath.cpp:476
> > +    time_t localTime = static_cast<time_t>(localTimeSeconds);
> 
> Seems unnecessary to involve time_t at all in the !HAVE(TM_GMTOFF) case; can we isolate this type cast to that side of the #if branch?

The time_t is actually necessary in the !HAVE(TM_GMTOFF) branch - if HAVE(TM_GMTOFF) we call getLocalTime directly, !HAVE(TM_GMTOFF) we call indirectly via calculateDSTOffset.  Structuring the code this way was deliberate to enable part 2 of this fix - which is to change how we 

> > Source/WTF/wtf/DateMath.h:103
> > +    LocalTimeOffset(bool isDST, int offset)
> 
> Could just put "= false" and "= 0" here and make this a default constructor too.

I added a default constructor.  Default parameters can led to bugs with implicit constructor calls. :-(

> > Source/WTF/wtf/DateMath.h:118
> > +WTF_EXPORT_PRIVATE LocalTimeOffset calculateLocalTimeOffset(double);
> 
> Needs an argument name. I suggest “localTimeInMilliseconds” if that is accurate.

*nod.  utcInMilliseconds.

All other issues fixed – committed revision 150833.
This should addresses the latter two issues outlined above, but does not yet fix the equivalent year calculation.

[Tim Horton]

I don't think the version you landed had that last Windows build fix: http://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/49132/steps/compile-webkit/logs/stdio and http://trac.webkit.org/changeset/150833/trunk/Source/JavaScriptCore/JavaScriptCore.vcproj/JavaScriptCore/JavaScriptCoreExports.def

[Tim Horton]

Also, you seem to have broken a test: http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r150833%20(10397)/fast/js/date-DST-time-cusps-pretty-diff.html

[Tim Horton]

(In reply to comment #15)
> Also, you seem to have broken a test: http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r150833%20(10397)/fast/js/date-DST-time-cusps-pretty-diff.html

I filed https://bugs.webkit.org/show_bug.cgi?id=116895 about that.

[Karl Dubost]

new Date(2010, 10, 1)

Safari:     Mon Nov 01 2010 00:00:00 GMT+0900 (Japan Standard Time)
Firefox:   Mon Nov 01 2010 00:00:00 GMT+0900 (Japan Standard Time)
Chrome: Mon Nov 01 2010 00:00:00 GMT+0900 (Japan Standard Time)

This has been fixed elsewhere probably.

[Karl Dubost]

Oh not tested in KRAT, or YAKT

[Karl Dubost]

Ok tested with KRAT and still working.
All browsers are sending. 
Mon Nov 01 2010 00:00:00 GMT+0700 (Krasnoyarsk Standard Time)

[Karl Dubost]

Ok tested with KRAT and still working.
All browsers are sending. 
Mon Nov 01 2010 00:00:00 GMT+0700 (Krasnoyarsk Standard Time)