Bug 32304 – JSC::gregorianDateTimeToMS() returns negative values for huge years

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Kent Tamura]

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.

[Kent Tamura]

Oh, it's JSC::, not WTF::.

[Kent Tamura]

Created attachment 44511 [details]
Proposed patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44511 [details] without any errors.

[Darin Adler]

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.

[Kent Tamura]

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.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44611 [details] without any errors.

[Geoffrey Garen]

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

[Kent Tamura]

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.

[WebKit Commit Bot]

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

[Kent Tamura]

> fast/js/date-daysfrom1970-overflow.html -> failed

The test depends on the local timezone.  I'll fix it.

[Kent Tamura]

Created attachment 44655 [details]
Proposed patch (rev.3)

* Fix a timezone-dependency of the test

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 44655 [details] without any errors.

[WebKit Commit Bot]

Comment on attachment 44655 [details]
Proposed patch (rev.3)

Clearing flags on attachment: 44655

Committed r51986: <http://trac.webkit.org/changeset/51986>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.