Bug 16753

Summary: date set methods with no args should result in NaN (Acid3 bug)
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.4   
Bug Depends on:    
Bug Blocks: 17064    
Attachments:
Description Flags
Abstract all dateObject.set* functions into shared static functions
none
Add fast/js/date-set-to-nan test
none
patch, good enough
none
Patch to handle NaN, Inf and missing arguments
none
Patch to handle NaN, Inf and missing arguments
eric: review-
Patch to handle NaN, Inf and missing arguments
darin: review+
revised version of the patch darin: review+

Description Eric Seidel (no email) 2008-01-05 14:25:18 PST
date set methods with no args should result in NaN (acid3 bug)

    function () {
      // test 90: a Date object's method called with no arguments
      var d = new Date();
      assert(isNaN(d.setMilliseconds()), "calling setMilliseconds() with no arguments didn't result in NaN");
      assert(isNaN(d), "date wasn't made NaN");
      assert(isNaN(d.getDay()), "date wasn't made NaN");
      return 6;
    },
Comment 1 Eric Seidel (no email) 2008-01-05 14:59:32 PST
This is the spec:

15.9.5.29 Date.prototype.setUTCMilliseconds(ms)
1. Let t be this time value.
2. Call ToNumber(ms).
3. Compute MakeTime(HourFromTime(t), MinFromTime(t), SecFromTime(t), Result(2)).
4. Compute MakeDate(Day(t), Result(3)).
5. Set the [[Value]] property of the this value to TimeClip(Result(4)).
6. Return the value of the [[Value]] property of the this value.

I'm not sure I fully understand why this turns date into NaN yet.
Comment 2 Eric Seidel (no email) 2008-01-05 15:02:43 PST
Never mind.  It's quite clear from the spec we should be setting the Date internal value to be NaN.  Our implementation works differently than the spec does, which is our problem.
Comment 3 Eric Seidel (no email) 2008-01-06 17:23:01 PST
I'll fix this one.
Comment 4 Eric Seidel (no email) 2008-01-06 19:15:45 PST
Created attachment 18308 [details]
Abstract all dateObject.set* functions into shared static functions

 JavaScriptCore/ChangeLog           |   26 +++
 JavaScriptCore/kjs/date_object.cpp |  293 ++++++------------------------------
 2 files changed, 74 insertions(+), 245 deletions(-)
Comment 5 Eric Seidel (no email) 2008-01-06 19:15:47 PST
Created attachment 18309 [details]
Add fast/js/date-set-to-nan test

 LayoutTests/fast/js/date-set-to-nan.html         |   13 +++++
 LayoutTests/fast/js/resources/date-set-to-nan.js |   53 ++++++++++++++++++++++
 2 files changed, 66 insertions(+), 0 deletions(-)
Comment 6 Eric Seidel (no email) 2008-01-06 19:18:34 PST
Comment on attachment 18308 [details]
Abstract all dateObject.set* functions into shared static functions

This can stand for review by itself. Then when it's OK'd I'll land it and clear the flag.
Comment 7 David Kilzer (:ddkilzer) 2008-01-06 20:11:29 PST
(In reply to comment #4)
> Created an attachment (id=18308) [edit]
> Abstract all dateObject.set* functions into shared static functions

Sam, I think this patch may address <rdar://problem/5595552>.
Comment 8 David Kilzer (:ddkilzer) 2008-01-06 20:34:42 PST
Comment on attachment 18308 [details]
Abstract all dateObject.set* functions into shared static functions

Thanks, Eric!  r=me
Comment 9 Eric Seidel (no email) 2008-01-07 07:02:07 PST
At this point, the difficult part is not the fix, it's the test case.  I'm not certain the test case I've posted is enough.  I think for functions which take more arguments, I may need to test correct handling of NaN/null/undefined/inf, at different argument numbers to make sure NaN is produced in appropriate cases.
Comment 10 Eric Seidel (no email) 2008-01-07 07:02:43 PST
Comment on attachment 18308 [details]
Abstract all dateObject.set* functions into shared static functions

This landed as:
http://trac.webkit.org/projects/webkit/changeset/29217
Clearing review flag.
Comment 11 Eric Seidel (no email) 2008-01-10 16:53:54 PST
I'm distracted by other things at the moment.  Returning this back to unassigned.
Comment 12 Darin Adler 2008-02-03 20:11:26 PST
Created attachment 18897 [details]
patch, good enough

It'd be even better to fix cases like undefined and null. But to fix enough to pass Acid3, this patch is all it will take.
Comment 13 Eric Seidel (no email) 2008-02-04 08:07:47 PST
Comment on attachment 18897 [details]
patch, good enough

Looking at the original Ecma algorithm, there are other ways to get a NaN during processing.  I don't think we support any of those either.
Comment 14 Michael Knaup 2008-02-18 14:09:15 PST
Created attachment 19197 [details]
Patch to handle NaN, Inf and missing arguments

The fast/js/date-set-to-nan test runs successfully after applying this patch.

But there seems to be an additional problem with the DST correction for years smaller 1917 (at least when using UTC-1)
Comment 15 Michael Knaup 2008-02-18 22:12:04 PST
Created attachment 19206 [details]
Patch to handle NaN, Inf and missing arguments

There was still a bug with recovering from NaN, by using setMilliseconds() which lead to a buggy date.
Comment 16 Eric Seidel (no email) 2008-02-18 22:41:30 PST
Comment on attachment 19206 [details]
Patch to handle NaN, Inf and missing arguments

Thanks for the patch!

A few comments.  One, you should familiarize yourself with the WebKit coding style guidelines:
http://webkit.org/coding/coding-style.html

A few "violations" I see in this patch include:
if (foo)
{

(The { should be on the same line as the if)

and:

if (0 != args.size())

should be:

if (args.size()) (we avoid == 0 and != 0 when the meaning is otherwise clear from context.  e.g. comparing pointers to NULL or using size() for isEmpty()... in this case, there migth even be an isEmpty() which could be more clear).

Single line ifs should not have { }

Wouldn't:
ok = !(isinf(millis) || isnan(millis));
be equivalent to:
ok = isfinite(mills); ?

Also, the above if (ok) block, generally we prefer an early-return style in webkit.  In this instance you would use:
if (!ok)
   return false;

instead of wrapping that section in an if (ok) block.

Likewise, it would be more clear (or at least more inline with webkit style) to do:

if (!args.size()) {
   result = jsNaN();
   date->setInternalValue(result);
   return result;
} 

as an early return, instead of adding a long if block.

We'll need to of course run SunSpider with this change to make sure there is not speed regression due to added branching in these functions.

r- for the style issues.

Ideallly your patch would include the diff for the test case, since someone else will be landing this for you (unless you have commit-bit and I simply don't recognize your name).

Thanks again for the patch!
Comment 17 Michael Knaup 2008-02-19 14:11:10 PST
Created attachment 19214 [details]
Patch to handle NaN, Inf and missing arguments

I changed the code to match the code style guidelines and I added a modified the testcase which also checks for NaN "recovery" using the setTime, setYear, setFullYear and setUTCFullYear methods. For all other methods it is not possible to recover from an NaN date
Comment 18 Darin Adler 2008-02-20 09:37:55 PST
Comment on attachment 19214 [details]
Patch to handle NaN, Inf and missing arguments

+    if (3 == numArgsToUse && isnan(milli))

I know that in some projects people think that putting the constant on the left side of an "==" is good style because it helps you avoid the mistake of using "=" by mistake, but we really don't believe in that, and I think it's strange to have this once instance of it mixed in with the other code.

The test is great. It's thorough. I'd prefer it if a lot more of the test cases and results were written out; it's better if the results file shows a lot more of what's being tested rather than just the conclusions.

If you look at other tests that make heavy use of shouldBe you'll see how that can work.

+function makeIEHappy (functionNameRoot, value)

We don't put spaces after function names.

+function testDateFunctionWithValueExpectingNaN1(functionNameRoot) {

We put the braces on the next line, not the same line as the function.

r=me as-is, but please consider making improvements as well.
Comment 19 Michael Knaup 2008-02-20 13:50:50 PST
Created attachment 19240 [details]
revised version of the patch

Changed the testcase based on darin comments.
Comment 20 Darin Adler 2008-02-20 14:24:10 PST
Comment on attachment 19240 [details]
revised version of the patch

r=me
Comment 21 Sam Weinig 2008-02-20 20:54:03 PST
Landed in r30453.