date set methods with no args should result in NaN (Acid3 bug)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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;
    },

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I'll fix this one.

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

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(-)

[Eric Seidel (no email)]

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.

[David Kilzer (:ddkilzer)]

(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>.

[David Kilzer (:ddkilzer)]

Comment on attachment 18308 [details]
Abstract all dateObject.set* functions into shared static functions

Thanks, Eric!  r=me

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

I'm distracted by other things at the moment.  Returning this back to unassigned.

[Darin Adler]

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.

[Eric Seidel (no email)]

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.

[Michael Knaup]

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)

[Michael Knaup]

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.

[Eric Seidel (no email)]

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!

[Michael Knaup]

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

[Darin Adler]

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.

[Michael Knaup]

Created attachment 19240 [details]
revised version of the patch

Changed the testcase based on darin comments.

[Darin Adler]

Comment on attachment 19240 [details]
revised version of the patch

r=me

[Sam Weinig]

Landed in r30453.