Bug 32304 - JSC::gregorianDateTimeToMS() returns negative values for huge years
Summary: JSC::gregorianDateTimeToMS() returns negative values for huge years
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-08 21:40 PST by Kent Tamura
Modified: 2009-12-11 07:39 PST (History)
4 users (show)

See Also:


Attachments
Proposed patch (3.16 KB, patch)
2009-12-08 22:18 PST, Kent Tamura
darin: review-
Details | Formatted Diff | Diff
Proposed patch (rev.2) (6.12 KB, patch)
2009-12-10 06:34 PST, Kent Tamura
commit-queue: commit-queue-
Details | Formatted Diff | Diff
sunspider-compare-results output (3.44 KB, text/plain)
2009-12-10 17:29 PST, Kent Tamura
no flags Details
Proposed patch (rev.3) (6.95 KB, patch)
2009-12-10 18:22 PST, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2009-12-08 21:40:24 PST
WTF:gregorianDateTimeToMS() is in JavaScriptCore/wtf/DateMath.cpp.

It returns negative values for huge years.
For example, it  returns -1.85543E+17 for GregorianDateTime with .year=11798100.

This issue doesn't cause any real bugs for now because JSC Date instances are created with WTF::timeClip(), which maps a huge value to NaN.  However this issue might cause a real bug in the future.
Comment 1 Kent Tamura 2009-12-08 22:13:32 PST
Oh, it's JSC::, not WTF::.
Comment 2 Kent Tamura 2009-12-08 22:18:58 PST
Created attachment 44511 [details]
Proposed patch
Comment 3 WebKit Review Bot 2009-12-08 22:21:43 PST
style-queue ran check-webkit-style on attachment 44511 [details] without any errors.
Comment 4 Darin Adler 2009-12-09 10:05:43 PST
Comment on attachment 44511 [details]
Proposed patch

This patch does not include a test case. Why not?

> -static int dateToDayInYear(int year, int month, int day)
> +static double dateToDayInYear(int year, int month, int day)

This seems wrong. A "day in year" value can never be out of range for an int: There are a maximum of 366 days in a year. So I don't see how the change in return type is helpful unless there is a bug inside the dateToDayInYear function. You'll have to explain why changing to double is helpful.

> +    // 8.64E15 is a value near to the maximum integral number that
> +    // double can represent without precision loss.
>      if (fabs(t) > 8.64E15)

That comment seems to me like it's probably right right, but the best way to document such things is typically to use a named constant. Also, I believe this number comes straight from the ECMAScript specification, and should not be changed. The comment makes it seem like you could come up with a value even closer to the maximum number, and improve the code, but I believe that is not true and would cause us to differ from the specification if we changed it.
Comment 5 Kent Tamura 2009-12-10 06:34:28 PST
Created attachment 44611 [details]
Proposed patch (rev.2)

(In reply to comment #4)
> (From update of attachment 44511 [details])
> This patch does not include a test case. Why not?

I added a test to the updated patch.
If 'yearday' in dateToDayInYear() is int, the test causes an assertion failure.

> > -static int dateToDayInYear(int year, int month, int day)
> > +static double dateToDayInYear(int year, int month, int day)
> 
> This seems wrong. A "day in year" value can never be out of range for an int:
> There are a maximum of 366 days in a year. So I don't see how the change in
> return type is helpful unless there is a bug inside the dateToDayInYear
> function. You'll have to explain why changing to double is helpful.

This function returns the number of days from 1970-01-01 to the specified date. You know, (year - 1970) * 365 can be larger than INT_MAX.
The function name is very wrong. I renamed it in the updated patch.

> > +    // 8.64E15 is a value near to the maximum integral number that
> > +    // double can represent without precision loss.
> >      if (fabs(t) > 8.64E15)
> 
> That comment seems to me like it's probably right right, but the best way to
> document such things is typically to use a named constant. Also, I believe this
> number comes straight from the ECMAScript specification, and should not be
> changed. The comment makes it seem like you could come up with a value even
> closer to the maximum number, and improve the code, but I believe that is not
> true and would cause us to differ from the specification if we changed it.

ok, I introduced a named constant.
Comment 6 WebKit Review Bot 2009-12-10 06:38:01 PST
style-queue ran check-webkit-style on attachment 44611 [details] without any errors.
Comment 7 Geoffrey Garen 2009-12-10 11:18:34 PST
Please test SunSpider with and without your patch, and post results.
Comment 8 Kent Tamura 2009-12-10 17:29:16 PST
Created attachment 44652 [details]
sunspider-compare-results output

> Please test SunSpider with and without your patch, and post results.

Here it is.

The baseline is r51974 without this patch, and the competitor is r51974 + this patch.
It seems there are no significant differences.
Comment 9 WebKit Commit Bot 2009-12-10 17:45:03 PST
Comment on attachment 44611 [details]
Proposed patch (rev.2)

Rejecting patch 44611 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11764 test cases.
fast/js/date-daysfrom1970-overflow.html -> failed

Exiting early after 1 failures. 7083 tests run.
113.75s total testing time

7082 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
2 test cases (<1%) had stderr output
Comment 10 Kent Tamura 2009-12-10 17:59:10 PST
> fast/js/date-daysfrom1970-overflow.html -> failed

The test depends on the local timezone.  I'll fix it.
Comment 11 Kent Tamura 2009-12-10 18:22:54 PST
Created attachment 44655 [details]
Proposed patch (rev.3)

* Fix a timezone-dependency of the test
Comment 12 WebKit Review Bot 2009-12-10 18:26:25 PST
style-queue ran check-webkit-style on attachment 44655 [details] without any errors.
Comment 13 WebKit Commit Bot 2009-12-11 07:39:37 PST
Comment on attachment 44655 [details]
Proposed patch (rev.3)

Clearing flags on attachment: 44655

Committed r51986: <http://trac.webkit.org/changeset/51986>
Comment 14 WebKit Commit Bot 2009-12-11 07:39:42 PST
All reviewed patches have been landed.  Closing bug.