Bug 131248 – Date object needs to check for ES5 15.9.1.14 TimeClip limit

• Status: REOPENED
• Component: JavaScriptCore

[Mark Lam]

The current Date object code does not adequately checks for the ES5 15.9.1.14 TimeClip limit.  As a result, some calculations can underflow / overflow and produce unexpected results.

[Mark Lam]

Created attachment 228635 [details]
the patch.

[Mark Lam]

FYI, the spec for ES5 15.9.1.14 can be viewed here: http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.14.

[Mark Lam]

<rdar://problem/16513974>

[Mark Hahnenberg]

Comment on attachment 228635 [details]
the patch.

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

> Source/JavaScriptCore/ChangeLog:8
> +        The current Date object code does not adequately checks for the ES5

adequately check

> Source/JavaScriptCore/ChangeLog:12
> +        For example, we were getting an assertion failures in

failure

> Source/JavaScriptCore/ChangeLog:19
> +        The changes has no noticeable impact on benchmark results.

have

> LayoutTests/js/script-tests/regress-131248.js:2
> +"This tests checks date values at the limits set by the ES5 15.9.1.14 TimeClip specification and ensures that we don't crash on any assertions."

This test

[Mark Lam]

Created attachment 228638 [details]
patch 2: addressed MarkH's feedback.

[Mark Hahnenberg]

Comment on attachment 228638 [details]
patch 2: addressed MarkH's feedback.

I'm not an expert on the Date code, so I'm not sure what I should be looking for in this review. Why did you put ASSERTs in the places you did? Why did you choose to return NaN in some places? Why did you call timeClip (which looks like it already existed) where you did? I'm not saying you're wrong, I just don't know what methodology you used to get here :-)

[Mark Hahnenberg]

(In reply to comment #6)
> (From update of attachment 228638 [details])
> I'm not an expert on the Date code, so I'm not sure what I should be looking for in this review. Why did you put ASSERTs in the places you did? Why did you choose to return NaN in some places? Why did you call timeClip (which looks like it already existed) where you did? I'm not saying you're wrong, I just don't know what methodology you used to get here :-)

I guess a better way to ask this question is: how do we know that we've covered all our bases?

[Mark Hahnenberg]

Comment on attachment 228638 [details]
patch 2: addressed MarkH's feedback.

Okay, so the answer is that we're not 100% sure that we'll do the right thing always, but the test covers this particular bug. You should file a bug for us to revisit this with a more comprehensive solution that probably involves leveraging the type system in a creative way to make these sorts of bugs even less likely.

[Mark Lam]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 228638 [details] [details])
> > I'm not an expert on the Date code, so I'm not sure what I should be looking for in this review. Why did you put ASSERTs in the places you did? Why did you choose to return NaN in some places? Why did you call timeClip (which looks like it already existed) where you did? I'm not saying you're wrong, I just don't know what methodology you used to get here :-)
> 
> I guess a better way to ask this question is: how do we know that we've covered all our bases?

Here's what did do so far:
1. I looked thru JSDateMath.cpp and analyzed all the code paths that uses the milliseconds time value.  Based on that I added checked where I saw a need.
2. I added assertions in WTF::DateMath and at other relevant places to ensure that the milliseconds time value is sane where we would expect it to be.  I added checks to places where the assertions revealed an issue.
3. I added some layout tests that specifically exercise milliseconds to Date (and vice versa) with values at the TimeClip limits, and verified that the conversion works as expected in these edge cases.

I do know that this patch has improved the quality of the code bug does not guarantee that we've exhaustively fix all possible issues pertaining to 15.9.1.14.  I've filed https://bugs.webkit.org/show_bug.cgi?id=131250 for later if we want to do an exhaustive approach of solving this.

[WebKit Commit Bot]

Comment on attachment 228638 [details]
patch 2: addressed MarkH's feedback.

Rejecting attachment 228638 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 228638, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
chanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 8] nodename nor servname provided, or not known>

Full output: http://webkit-queues.appspot.com/results/6177313318240256

[Mark Lam]

Thanks for the review.  svn up’ed and landed in r166876: <http://trac.webkit.org/r166876>.

[Geoffrey Garen]

This patch caused a few test262 tests to fail <http://test262.ecmascript.org/>:

15.9.5.43-0-10	Date.prototype.toISOString - RangeError is not thrown when value of date is Date(1970, 0, -99999999, 0, 0, 0, 1), the time zone is UTC(0)	Fail

15.9.5.43-0-9	Date.prototype.toISOString - RangeError is not thrown when value of date is Date(1970, 0, -99999999, 0, 0, 0, 0), the time zone is UTC(0)	Fail

[Geoffrey Garen]

Reverted r166876 for reason:

Caused some ECMA test262 failures

Committed r170440: <http://trac.webkit.org/changeset/170440>