Date object is created wrongly for leap year On dates between 3.1.2034 and 31.12.2099 date object is incorrect: For example Mar 1 2034 05:23:52 ==> Tue Feb 28 2034
Created attachment 226502 [details] Patch
Comment on attachment 226502 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226502&action=review Can you add a regression test to show that the old code is broken and to confirm that the new code is working? > Source/WTF/wtf/DateMath.cpp:520 > + int mday = firstDayOfMonth[(isLeapYear(year) ? 1 : 0)][mon -1 ]; You don’t need the ?: expression. "isLeapYear(year)” is adequate. http://en.cppreference.com/w/cpp/language/implicit_cast states that a bool will be converted to a 0 or a 1. Also, the webkit style guide says you should have a space between “mon” and “1” in “mon -1”. “[mon -1 ]” should be “[mon - 1]”.
Created attachment 226583 [details] Patch
@Mark, Thanks for the review.
Comment on attachment 226583 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226583&action=review Much better, but I think this still need some changes. First of all, out of curiosity, how did you determine that the erroneous date range is between 3.1.2034 and 31.12.2099? The reason I ask is because I would like to know the added regression test to test the appropriate range to ensure that things are correct. The test that you added only tests one value and may not necessarily catch future regressions. > LayoutTests/js/script-tests/date-2034-03-01.js:7 > +description( > +"This tests new Date('03/01/2034') equals to 'Wed Mar 01 2034'." > +); > + > +shouldBe("(new Date('03/01/2034')).getMonth()", "2"); > +shouldBe("(new Date('03/01/2034')).getDate()", "1"); > +shouldBe("(new Date('03/01/2034')).getFullYear()", "2034"); There is already a js/date-constructor.html test that test conversion values. If you look in date-constructor.js, you’ll see mention of a regression test for another bug. Instead of adding new test files, I suggest you add your test there as a regression test for this bug. Also, the purpose of adding the regression test is to catch possible regressions in date parsing. This bug was file because the date parsing actually worked for some date ranges but not for others. Hence, I think the regression test that we add needs to test a range of dates instead of just doing one specific date. Why don’t you do something like this to get more coverage: === BEGIN === // Regression test for ... function () { var monthNames = [ “January”, “February”, “March”, ... “December” ]; function testDate(year, month, date) { var success = true; var dateString = monthNames[month] + “ “ + date + “, “ + year; var dateObj = new Date(dateString); if (dateObj.getFullYear() != year) { shouldBe("(new Date(‘“ + dateString + “‘).)getFullYear()”, year); success = false; } if (dateObj.getMonth() != month) { shouldBe("(new Date(‘“ + dateString + “‘).)getMonth()”, month); success = false; } if (dateObj.getDate() != date) { shouldBe("(new Date(‘“ + dateString + “‘).)getDate()”, date); success = false; } return success; } var success = false; var year = 1000; var month = 0; var date = 1; while (year < 9999) { success ||= testDate(year, month, date); year += 37; month = (month + 7) % 12; date = (date + 13) % 28 + 1; } success ||= testDate(9999, month, date); if (success) debug(“PASS Dates from year 1000 to 9999 parsed correctly”); } (); === END === Some details about why I suggest the above: 1. According to http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getFullYear, the full year ranges from 1000 to 9999. I figured we could just test some dates scattered over that range of years. 2. To keep the dates varying, I incremented the year, month, day by primes. For simplicity, I ignored dates >28 so that I don’t have to deal with leap years, and varying last date of the months. 3. If we specify a date as “03/01/2034”, I wonder if there are localization implications resulting in this sometimes meaning mm/dd/yyyy and sometimes meaning dd//mm/yyyy. So, I suggested using a text string for the month so that it is not ambiguous. However, I’m not sure if that would end up exercising the code path that we want to test in the fix. Please check if this is appropriate. If not, please address the localization issue if needed. 4. I came up with this test without knowing how you determine that the erroneous date range is between 3.1.2034 and 31.12.2099. Is there anything special about that date range that we need to add additional tests? Feel free to implement an alternative if you have a better suggestion.
Comment on attachment 226583 [details] Patch @Mark, Thanks for the kind comments. Actually I didn't fully verified when the errors get recovered. Which was my bad. And today I have found that even after 2099, the error still remains. So, I will digging it more and prepare a advised test cases and updates the summary etc,...
Since 1970, date errors happen periodically as following: 2034 3/1 ~ 2100 2/28 2167 3/1 ~ 2200 2/28 2434 3/1 ~ 2500 2/28 2567 3/1 ~ 2600 2/28 ….. 9634 3/1 ~ 9700 2/28 9767 3/1 ~ 9800 2/28
Created attachment 226727 [details] Patch
Created attachment 226728 [details] Patch
Comment on attachment 226728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226728&action=review > LayoutTests/js/script-tests/date-constructor.js:73 > +var monthNames = ["January", "February", "March", "April", "May", "June", "July", "August", "September", "October", "November", "December" ]; Please remove the space between “December” and “];”. > LayoutTests/js/script-tests/date-constructor.js:81 > + if (dateObj.getFullYear() != year) { > + success = false; Please add: shouldBe(“new Date(“ + dateString + “).getFullYear()”, year); That way, if it fails, we’ll know what values it failed on, and lessen the effort to get a reproduction case for debugging the issue. > LayoutTests/js/script-tests/date-constructor.js:83 > + } if (dateObj.getMonth() != month) { > + success = false; Ditto. Please add: shouldBe(“new Date(“ + dateString + “).getMonth()”, month); > LayoutTests/js/script-tests/date-constructor.js:85 > + } if (dateObj.getDate() != date) { > + success = false; Ditto. Please add: shouldBe(“new Date(“ + dateString + “).getDate()”, date); > LayoutTests/js/script-tests/date-constructor.js:100 > +var leapTestResult = true; > +var year = 1970; > +var month = 2; > +var date = 1; > + > +// Check value of March 1 day since 1970 till 9999 > +while (year < 10000) { > + leapTestResult = testDate(year, month, date); > + if(!leapTestResult) break; > + year += 1; > +} In my original suggestion, I had the month and date incrementing by a prime number so that we can test different months and dates as well: month = (month + 7) % 12; date = (date + 13) % 28; Is there a reason you opted to go only with "March 1”? I feel that this actually lessens the coverage of the test. If you don’t have a good reason for this, please add back the date and month increments so that we can test different values. If there’s something special about “March 1”, then please add a line to test it explicitly (in addition to the one that tests the varying date values) with a comment to describe why it is : leapTestResult ||= testDate(year, 2, 1); // <reason why you’re explicitly testing this month and date …> In my original suggestion, I also incremented the year by a prime number greater than 1. The reason was so that the test will take less time to run. But I supposed any modern computer should be able to blow thru 10000 iterations in no time. So, incrementing the year by 1 is fine. According to http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getFullYear, the full year ranges from 1000 to 9999. So, we might as well test Date ranges in the past as well. A quick test on FF, Chrome, and Safari all reveals that they can take years going back to year 100. On Safari and Chrome, years less than that get aliases e.g. 99 become 1999, 10 becomes 2010. On FF, 99 becomes 1999, 10 returns a NaN. How about starting the year with year 100?
Created attachment 226779 [details] Patch
@Mark, thanks for the review. I have update it. FYI, there are no compound assignment operators. http://programmers.stackexchange.com/questions/189479/why-there-are-no-compound-assignment-operators-for-logical-operators-such-as
Comment on attachment 226779 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=226779&action=review Almost there. =) Please fix the remaining issues. For your updated patch, mark it cq? if you need it to be committed via the commit-queue. > LayoutTests/js/script-tests/date-constructor.js:81 > + shouldBe(ânew Date(â + dateString + â).getFullYear()â, year); Please use the ascii double quote character ‘“' instead of 'â’. > LayoutTests/js/script-tests/date-constructor.js:84 > + shouldBe(ânew Date(â + dateString + â).getMonth()â, month); Ditto. > LayoutTests/js/script-tests/date-constructor.js:87 > + shouldBe(ânew Date(â + dateString + â).getDate()â, date); Ditto.
Created attachment 226783 [details] Patch
Comment on attachment 226783 [details] Patch r=me
@Mark, I really appreciate your help!
Comment on attachment 226783 [details] Patch Clearing flags on attachment: 226783 Committed r165667: <http://trac.webkit.org/changeset/165667>
All reviewed patches have been landed. Closing bug.
*** Bug 129308 has been marked as a duplicate of this bug. ***
*** Bug 106720 has been marked as a duplicate of this bug. ***