Bug 131248

Summary: Date object needs to check for ES5 15.9.1.14 TimeClip limit
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: REOPENED ---    
Severity: Normal CC: barraclough, benjamin, cmarcelo, commit-queue, fpizlo, gary, ggaren, mhahnenberg, mmirman, msaboff, oliver
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch.
none
patch 2: addressed MarkH's feedback. mhahnenberg: review+, commit-queue: commit-queue-

Description Mark Lam 2014-04-04 16:16:52 PDT
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.
Comment 1 Mark Lam 2014-04-04 17:17:23 PDT
Created attachment 228635 [details]
the patch.
Comment 2 Mark Lam 2014-04-04 17:19:02 PDT
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.
Comment 3 Mark Lam 2014-04-04 17:20:00 PDT
<rdar://problem/16513974>
Comment 4 Mark Hahnenberg 2014-04-04 17:31:55 PDT
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
Comment 5 Mark Lam 2014-04-04 17:41:07 PDT
Created attachment 228638 [details]
patch 2: addressed MarkH's feedback.
Comment 6 Mark Hahnenberg 2014-04-04 17:43:51 PDT
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 :-)
Comment 7 Mark Hahnenberg 2014-04-04 17:44:49 PDT
(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?
Comment 8 Mark Hahnenberg 2014-04-04 18:04:19 PDT
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.
Comment 9 Mark Lam 2014-04-04 18:05:54 PDT
(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.
Comment 10 WebKit Commit Bot 2014-04-04 18:35:59 PDT
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
Comment 11 Mark Lam 2014-04-07 11:26:10 PDT
Thanks for the review.  svn up’ed and landed in r166876: <http://trac.webkit.org/r166876>.
Comment 12 Geoffrey Garen 2014-06-25 14:49:27 PDT
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
Comment 13 Geoffrey Garen 2014-06-25 14:52:15 PDT
Reverted r166876 for reason:

Caused some ECMA test262 failures

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