How to reproduce:
1) make sure the computer has the time set to central european time (CET/CEST)
2) run this line of code at the console prompt:
var a = new Date(2014,2,30); new Date(a.setHours(1))
Expected result:
Sun Mar 30 2014 01:00:00 GMT+0100 (CET) (one o'clock)
Actual result:
Sun Mar 30 2014 00:00:00 GMT+0100 (CET) (midnight)
Created attachment 229139[details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 229140[details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
@Tibor, I have tested with your patch and could not get correct result.
Please check my patch and update your opinion it.
@Mark, Could you please review current patch?
Comment on attachment 229431[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=229431&action=review> Source/JavaScriptCore/ChangeLog:10
> + String(new Date(Mar 30 2014 01:00:00)) is wrong in CET
> + https://bugs.webkit.org/show_bug.cgi?id=130967
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + According to calculateLocalTimeOffset define,
> + it accepts utcInMilliseconds as an Argument
> + So, we need to calculate UTC instead of localTime milliseconds
What is the issue? I tried evaluating "String(new Date("Mar 30 2014 01:00:00 UTC+0100”))” in Safari, Chrome, and Firefox (note: CET is UTC+1 according to http://www.timeanddate.com/library/abbreviations/timezones/), and they all show the same string. You need a test case that demonstrates the issue and will serve as a regression test in the future. If appropriate, please add the test case to an existing Date test.
Also, I fail to follow your logic here. What does calculateLocalTimeOffset() have to do with your change in parseDateFromNullTerminatedCharacters()?
> Source/WTF/wtf/DateMath.cpp:470
> +double getUTCOffset()
> +{
> + return calculateUTCOffset();
> +}
If we really need to export this, then let’s export calculateUTCOffset(). No need to create another function.
> Source/WTF/wtf/DateMath.h:159
> +using WTF::getUTCOffset;
> using WTF::calculateLocalTimeOffset;
This shows that we should export WTF::calculateTimeOffset (if needed) instead of creating a WTF::getUTCOffset just to wrap it but adds nothing.
Comment on attachment 229431[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=229431&action=review>> Source/JavaScriptCore/ChangeLog:10
>> + So, we need to calculate UTC instead of localTime milliseconds
>
> What is the issue? I tried evaluating "String(new Date("Mar 30 2014 01:00:00 UTC+0100”))” in Safari, Chrome, and Firefox (note: CET is UTC+1 according to http://www.timeanddate.com/library/abbreviations/timezones/), and they all show the same string. You need a test case that demonstrates the issue and will serve as a regression test in the future. If appropriate, please add the test case to an existing Date test.
>
> Also, I fail to follow your logic here. What does calculateLocalTimeOffset() have to do with your change in parseDateFromNullTerminatedCharacters()?
Could you please test it without setting Timezone explicitly while setting your system timezone to Spain(Madrid)?
It will shows you "Sun Mar 30 2014 00:00:00 GMT+0100 (CET)" instead of "Sun Mar 30 2014 01:00:00 GMT+0100 (CET)."
This issue came from parseDateFromNullTerminatedCharacters just convert "Mar 30 2014 01:00:00" to milliseconds without considering UTC when no TimeZone information provided.
>> Source/WTF/wtf/DateMath.cpp:470
>> +}
>
> If we really need to export this, then let’s export calculateUTCOffset(). No need to create another function.
I will do it.
>> Source/WTF/wtf/DateMath.h:159
>> using WTF::calculateLocalTimeOffset;
>
> This shows that we should export WTF::calculateTimeOffset (if needed) instead of creating a WTF::getUTCOffset just to wrap it but adds nothing.
I will do it.
(In reply to comment #10)
> Could you please test it without setting Timezone explicitly while setting your system timezone to Spain(Madrid)?
> It will shows you "Sun Mar 30 2014 00:00:00 GMT+0100 (CET)" instead of "Sun Mar 30 2014 01:00:00 GMT+0100 (CET)."
>
> This issue came from parseDateFromNullTerminatedCharacters just convert "Mar 30 2014 01:00:00" to milliseconds without considering UTC when no TimeZone information provided.
OK, I can see the issue if I change my machine time zone. Can you think of a way to write a test for this that does not require a machine time zone change in order to verify the issue?
We couldn't make a unit test for it, because there are no JavaScript API to specify System TimeZone.
It only possible to do manual test by creating Date() and see if it shows correctly.
I have checked that PDT/PST also has same issue.
ex) Date('Mar 09 2014 03:00:00') ~ Date('Mar 09 2014 09:59:59')
Created attachment 229614[details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 229622[details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 229629[details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
In order to write a unit test for that a wrapper function could be used in the lower level. It returns the system time and when run from a unit test it uses the datetime provided as a parameter.
Comment on attachment 229741[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=229741&action=review> Source/JavaScriptCore/ChangeLog:9
> + There are duplicataions on WTF and JavaScript Core,
> + so we remove the Timezone conversion and call the function in WTF.
You misspelled "duplications" here. In addition, from looking at the code, I'm not sure that it is really a duplication because the underlying implementation of local time offset computation is different between what's in JSDateMath and WTF::DateMath.
> Source/JavaScriptCore/runtime/JSDateMath.cpp:236
> -static double parseDateFromNullTerminatedCharacters(VM& vm, const char* dateString)
> +static double parseDateFromNullTerminatedCharacters(const char* dateString)
> {
> - bool haveTZ;
> - int offset;
> - double ms = WTF::parseDateFromNullTerminatedCharacters(dateString, haveTZ, offset);
> + double ms = WTF::parseDateFromNullTerminatedCharacters(dateString);
> if (std::isnan(ms) || fabs(ms) > WTF::maxECMAScriptTime)
> return std::numeric_limits<double>::quiet_NaN();
>
> - // fall back to local timezone
> - if (!haveTZ)
> - offset = localTimeOffset(vm, ms).offset / WTF::msPerMinute;
> -
> - return ms - (offset * WTF::msPerMinute);
> + return ms;
Here, you are effectively delegating the localTimeOffset computation to the WTF implementation. However, at least on the surface, I see that the WTF implementation of calculateLocalTimeOffset() differs from the JSDateMath localTimeOffset(). I learned that ES5 has different rules for how JS is supposed to handle day light savings time computations, and I think the local time offset is responsible for that difference. I'm not too familiar with what the ES5 spec says specifically here, but I'd be reluctant to go with this change unless we can have a greater understanding of what the ES5 spec says in this area, and are able to show that the WTF implementation satisfies the ES5 requirements.
Another thing that clued me into this issue is that, after applying your patch, there are still other parts of JSDateMath that uses the JSDateMath localTineOffset() and not the WTF version. If it can be shown that the WTF implementation is correct and spec compliant for ES5, then everything should be switched over so that we don't have inconsistent behavior in different parts of the code.
Created attachment 238786[details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 238788[details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 238793[details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 238894[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=238894&action=review
Could you detail, in the bug reported issue, the outcome after applying the patch? I.e. what happens at new Date(2014, 3, 30, 2, 0) and new Data(2014, 3, 30, 2, 10)? Both cases getHours should return 3.
> LayoutTests/ChangeLog:9
> + DST starts at April 25, 1982 at 3:00 am, so it is correct to return 2:00 am.
The meaning of this is: DST starts at 3:00 am. So at 2am, it started to be 3am. So it should return 3:00am PDT. The original test is right.
> LayoutTests/js/dom/script-tests/date-DST-time-cusps.js:12
> + testCases.push(["(new Date(1982, 2, 14, 2)).getHours()", "2"]);
Original test was right. At 2am it became 3am, and DST started. So the right outcome is 3.
> LayoutTests/js/dom/script-tests/date-DST-time-cusps.js:15
> }
Likely a good idea would be adding new sets of tests for other timezones, so build bots with different timezones would still exercise this.
What I mean is that DST starts at April 25, 1982, so March 14, 1982 couldn't be applicable for DST time conversion because it is before DST start time.
Created attachment 239083[details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 239085[details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 239088[details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 239091[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=239091&action=review> LayoutTests/js/dom/script-tests/date-DST-time-cusps.js:12
> + testCases.push(["(new Date('Mar 09 2014 03:00:00')).getTimezoneOffset()", "420"]);
Here you should exercise Mar 09 2014 02:00:00, and 02:10:00 and getHours should return 3 in both cases (as in the original test case).
Comment on attachment 239091[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=239091&action=review>> LayoutTests/js/dom/script-tests/date-DST-time-cusps.js:12
>> + testCases.push(["(new Date('Mar 09 2014 03:00:00')).getTimezoneOffset()", "420"]);
>
> Here you should exercise Mar 09 2014 02:00:00, and 02:10:00 and getHours should return 3 in both cases (as in the original test case).
On Firefox the outcome is 3. On Chrome it is 1. I think both are valid. So we can proceed with this. The ES standard is not clear about how to handle this case anyway.
Comment on attachment 239314[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=239314&action=review
Concept of the fix looks fine. Not sure I find the use of a boolean here sufficiently clear.
> Source/JavaScriptCore/ChangeLog:10
> + So, it gives errornous results while calculating offset of DST boundary time.
Typo: errornous instead of erroneous.
> Source/JavaScriptCore/runtime/JSDateMath.cpp:135
> +static LocalTimeOffset localTimeOffset(VM& vm, double ms, bool inputIsUTC = true)
I don’t think a boolean is a good way to express the “inputIsUTC”. It doesn’t make it clear what the input is if not UTC. I think we should use an enum instead with one value being LocalTime and the other being UTC.
> Source/JavaScriptCore/runtime/VM.h:136
> + , isutc(true)
isutc -> isUTC, but also should use that same enum
> Source/WTF/wtf/DateMath.cpp:364
>
> -#if !HAVE(TM_GMTOFF)
>
Should not leave double blank lines around. Please delete one.
> Source/WTF/wtf/DateMath.cpp:407
> +#if !HAVE(TM_GMTOFF)
> #if OS(WINDOWS)
Should have a blank line after the HAVE(TM_GMTOFF) line.
> Source/WTF/wtf/DateMath.cpp:466
> + double utcOffset = 0;
Do we really need to initialize this to zero? Seems like it always gets set before being used.
I would write this:
#if HAVE(TM_GMTOFF)
// Optimize case where the input is already UTC by not even calculating the offset.
double utcOffset = inputIsUTC ? 0 : calculateUTCOffset();
#else
double utcOffset = calculateUTCOffset();
#endif
// Convert local time milliseconds to UTF milliseconds.
if (!inputIsUTC)
ms -= utcOffset;
> Source/WTF/wtf/DateMath.cpp:468
> + // Convert current milliseconds to UTC milliseconds
Extra space after "Convert". Missing period after "milliseconds".
> Source/WTF/wtf/DateMath.cpp:503
> - double utcOffset = calculateUTCOffset();
> + if (inputIsUTC)
> + utcOffset = calculateUTCOffset();
Then I’d just delete this code entirely.
> Source/WTF/wtf/DateMath.cpp:1091
> bool haveTZ;
> int offset;
> +
> double ms = parseDateFromNullTerminatedCharacters(dateString, haveTZ, offset);
Why add this blank line? The old code intentionally groups the definition of the out arguments with the line of code that sets them.
> Source/WTF/wtf/DateMath.cpp:1097
> + offset = calculateLocalTimeOffset(ms, false).offset / msPerMinute; // ms value is not UTC milliseconds
The "false" argument here is really mysterious. And the comment isn’t that great either, because it says what the milliseconds value is *not*, but does not say what it *is*!
Comment on attachment 239986[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=239986&action=review
Almost there. Please fix the remaining issues. Thanks.
> Source/JavaScriptCore/ChangeLog:11
> + By adding a argument to distinguish UTC and local time, we can get correct offset.
nit: “we can get correct offset” ==> "we can get the correct offset."
> Source/WTF/ChangeLog:11
> + By adding a argument to distinguish UTC and local time, we can get correct offset
ditto.
> Source/JavaScriptCore/runtime/JSDateMath.cpp:197
> double result = (day * WTF::msPerDay) + ms;
Rather than adding the comment below, I suggest renaming “result” to “localTimeResult”. I think the code will be clearer that way.
> Source/JavaScriptCore/runtime/JSDateMath.cpp:232
> double ms = WTF::parseDateFromNullTerminatedCharacters(dateString, haveTZ, offset);
Ditto: rename “ms” to “localTimeMS”. With this the added comment below becomes unnecessary.
> Source/JavaScriptCore/runtime/VM.h:152
> + WTF::InputTimeType inputTimeType;
I would express this as:
WTF::TimeType typeTime;
I think that the fact that it was used as the input value to some time function is irrelevant to the nature of the type. Hence, I don’t think we should be using “Input” in its name.
> Source/WTF/wtf/DateMath.cpp:474
> +LocalTimeOffset calculateLocalTimeOffset(double ms, InputTimeType inputTimeType)
You can leave the argument name here as “inputTimeType”. I agree that it’s appropriate in this context. But please replace the argument type with “TimeType”.
> Source/WTF/wtf/DateMath.cpp:479
> + // Convert local time milliseconds to UTC milliseconds, if input time type is LocalTime.
> + double inputTimeOffset = inputTimeType == LocalTime ? calculateUTCOffset() : 0;
> + if (inputTimeOffset)
> + ms -= inputTimeOffset;
I would remove this comment and rename “inputTimeOffset” to “localToUTCTimeOffset”.
> Source/WTF/wtf/DateMath.cpp:1104
> + offset = calculateLocalTimeOffset(ms, LocalTime).offset / msPerMinute; // ms value is local time milliseconds.
nit: “is local” ==> “is in local”.
> LayoutTests/ChangeLog:8
> + Set latest DST timezone bounday values on
typo: “bounday” ==> “boundary”?
Comment on attachment 239986[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=239986&action=review> Source/JavaScriptCore/runtime/JSDateMath.cpp:199
> if (!inputIsUTC)
Also, now that you have the WTF::TimeType, please change the gregorianDateTimeToMS() to take a "WTF::TimeType inputTimeType" arg instead of the "bool inputIsUTC".
Similarly, please change msToGregorianDateTime() to take a "WTF::TimeType outputTimeType" arg instead the bool.
Also change setNewValueFromTimeArgs() and setNewValueFromDateArgs() in DatePrototype.cpp accordingly.
Also remove "__ZN3JSC21msToGregorianDateTimeEPNS_9ExecStateEdbRN3WTF17GregorianDateTimeE" and "__ZN3JSC21gregorianDateTimeToMSEPNS_9ExecStateERKN3WTF17GregorianDateTimeEdb" from the JavaScriptCore.order file.
I think this will increase code clarity.
>> Source/WTF/wtf/DateMath.cpp:479
>> + // Convert local time milliseconds to UTC milliseconds, if input time type is LocalTime.
>> + double inputTimeOffset = inputTimeType == LocalTime ? calculateUTCOffset() : 0;
>> + if (inputTimeOffset)
>> + ms -= inputTimeOffset;
>
> I would remove this comment and rename “inputTimeOffset” to “localToUTCTimeOffset”.
Upon further review, I see what Darin was suggesting. Let's re-write this (as Darin suggested) as:
#if HAVE(TM_GMTOFF)
double utcOffset = inputTimeType == LocalTime ? calculateUTCOffset() : 0;
#else
double utfOffset = calculateUTCOffset();
#endif
if (inputTimeType == LocalTime)
ms -= utcOffset;
This way the utfOffset value is initialized in only one place (for better clarity), and there's no chance of calculateUTCOffset() being called 2 times (for when the local time is UTC time).
> Source/WTF/wtf/DateMath.cpp:511
> - double utcOffset = calculateUTCOffset();
> + // If we alredy calculated UTC offset to convert input time, then use it as an offset.
> + double utcOffset = inputTimeOffset ? inputTimeOffset : calculateUTCOffset();
You can remove this code with the above change.
@Mark, thanks for the review.
I have applied all of requests from you except following:
=========================================================================
> Source/JavaScriptCore/runtime/JSDateMath.cpp:197
> double result = (day * WTF::msPerDay) + ms;
Rather than adding the comment below, I suggest renaming “result” to “localTimeResult”. I think the code will be clearer that way.
The result value can be local time and UTC time according to InputTimeType.
So, I think it would be better to keep use the value name of result.
Comment on attachment 240259[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=240259&action=review> Source/JavaScriptCore/runtime/JSDateMath.cpp:200
> double result = (day * WTF::msPerDay) + ms;
>
> - if (!inputIsUTC)
> - result -= localTimeOffset(vm, result).offset;
> + if (inputTimeType == WTF::LocalTime)
> + result -= localTimeOffset(vm, result, inputTimeType).offset; // result is in local time milliseconds.
Previously, you said that "The result value can be local time and UTC time according to InputTimeType.” To clarify, what do you mean when in this added comment when you said that the "result is in local time milliseconds”? Did you mean that the previous result was in local time, and that you’re converting it to UTC time now? Or did you mean the previous result is in UTC time and you’re converting it to local time?
When the result passed in localTimeOffset(vm, result, inputTimeType), it is in the local time. After result value compensated by it's result by "result -= localTimeOffset(vm, result, inputTimeType).offset;", it is turned into UTC time.
Comment on attachment 240259[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=240259&action=review
In light of your comment, I'd like to request one more change (and threw in a few more nits while I was at it). Can you make these changes please? Thanks.
> Source/JavaScriptCore/runtime/DatePrototype.cpp:944
> + const WTF::TimeType inputTimeType = WTF::LocalTime;
> + return setNewValueFromTimeArgs(exec, 1, inputTimeType);
nit: This can be simplified to:
return setNewValueFromTimeArgs(exec, 1, WTF::LocalTime);
I don't think there's that much to be gained by using 2 statements here. Can you also apply this to the other functions below which is doing the same thing?
>> Source/JavaScriptCore/runtime/JSDateMath.cpp:200
>> + result -= localTimeOffset(vm, result, inputTimeType).offset; // result is in local time milliseconds.
>
> Previously, you said that "The result value can be local time and UTC time according to InputTimeType.” To clarify, what do you mean when in this added comment when you said that the "result is in local time milliseconds”? Did you mean that the previous result was in local time, and that you’re converting it to UTC time now? Or did you mean the previous result is in UTC time and you’re converting it to local time?
(In reply to comment #57)
> When the result passed in localTimeOffset(vm, result, inputTimeType), it is in the local time. After result value compensated by it's result by "result -= localTimeOffset(vm, result, inputTimeType).offset;", it is turned into UTC time.
It looks like the result is always expected to be in UTC time. In that case, let's rename "result" to "utcTimeResult" and remove the comment. I think that that should make it clear that we had a local time input here and we want a UTC time result.
> Source/WTF/wtf/DateMath.cpp:1105
> + offset = calculateLocalTimeOffset(ms, LocalTime).offset / msPerMinute; // ms value is local time milliseconds.
nit: "is local" ==> "is in local".
@Mark,
Since at the moment of calling "localTimeOffset", result value is localTime.
So I changed the value to localTimeResult and added localToUTCTimeOffset to get the offset value returned by localTimeOffset.
=============================================================================
197 double localTimeResult = (day * WTF::msPerDay) + ms;
198 double localToUTCTimeOffset = inputTimeType == LocalTime
199 ? localTimeOffset(vm, localTimeResult, inputTimeType).offset : 0;
2014-04-11 08:43 PDT, Tibor Mészáros
2014-04-11 09:32 PDT, Build Bot
2014-04-11 10:01 PDT, Build Bot
2014-04-15 23:58 PDT, Byungseon Shin
2014-04-17 18:24 PDT, Byungseon Shin
2014-04-17 19:42 PDT, Build Bot
2014-04-17 21:32 PDT, Build Bot
2014-04-17 22:41 PDT, Build Bot
2014-04-18 22:45 PDT, Byungseon Shin
2014-04-19 12:30 PDT, Byungseon Shin
2014-09-27 11:50 PDT, Byungseon(Sun) Shin
2014-09-27 12:09 PDT, Byungseon(Sun) Shin
2014-09-27 13:42 PDT, Build Bot
2014-09-27 14:23 PDT, Build Bot
2014-09-27 16:31 PDT, Build Bot
2014-09-29 16:15 PDT, Byungseon(Sun) Shin
2014-10-01 19:40 PDT, Byungseon(Sun) Shin
2014-10-01 21:58 PDT, Build Bot
2014-10-01 22:51 PDT, Build Bot
2014-10-01 23:57 PDT, Build Bot
2014-10-02 01:45 PDT, Byungseon(Sun) Shin
2014-10-05 23:40 PDT, Byungseon(Sun) Shin
2014-10-05 23:43 PDT, Byungseon(Sun) Shin
2014-10-16 18:05 PDT, Byungseon(Sun) Shin
2014-10-22 03:23 PDT, Byungseon(Sun) Shin
2014-10-22 17:06 PDT, Byungseon(Sun) Shin