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.
Created attachment 228635 [details] the patch.
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.
<rdar://problem/16513974>
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
Created attachment 228638 [details] patch 2: addressed MarkH's feedback.
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 :-)
(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 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.
(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 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
Thanks for the review. svn up’ed and landed in r166876: <http://trac.webkit.org/r166876>.
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
Reverted r166876 for reason: Caused some ECMA test262 failures Committed r170440: <http://trac.webkit.org/changeset/170440>