649575 - Date.prototype.toISOString must throw a RangeError for non-finite dates

Status: VERIFIED FIXED

Description

[Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important)]

So say most of the tests from http://samples.msdn.microsoft.com/ietestcenter/Javascript/ES15.9.html that fail.

Comment 1

[Tom Schuster [:evilpie]]

Created attachment 527107 [details] [diff] [review]
added err def and tests

nyan nyan

Comment 2

[Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important)]

Comment on attachment 527107 [details] [diff] [review]
added err def and tests

Looks mostly reasonable, but there are enough nits that I'd like to get another patch for this, rather than make the necessary pre-landing tweaks myself.

>diff --git a/js/src/js.msg b/js/src/js.msg

>+MSG_DEF(JSMSG_INVALID_DATE,           270, 0, JSEXN_RANGEERR, "invalid date")
>\ No newline at end of file

Add one.


>diff --git a/js/src/jsdate.cpp b/js/src/jsdate.cpp

>     if (!JSDOUBLE_IS_FINITE(utctime))
>-        JS_snprintf(buf, sizeof buf, js_NaN_date_str);
>+        if (printFunc == print_iso_string) {
>+            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_INVALID_DATE);
>+            return false;
>+        }
>+        else
>+            JS_snprintf(buf, sizeof buf, js_NaN_date_str);
>     else
>         (*printFunc)(buf, sizeof buf, utctime);

You should put braces around the else line here.

This is pretty ugh the way this handling is all done, but whatever.  :-\


>diff --git a/js/src/tests/ecma_5/Date/jstests.list b/js/src/tests/ecma_5/Date/jstests.list

>+script toISOString.js
>\ No newline at end of file

Add one.


>diff --git a/js/src/tests/ecma_5/Date/toISOString.js b/js/src/tests/ecma_5/Date/toISOString.js

>+function throwsRangeError(a) {
>+    try {
>+        var date = new Date(a);
>+        date.toISOString();
>+    } catch (err) {
>+        assertEq(err instanceof RangeError, true, 'wrong error class');
>+        return;
>+    }
>+    assertEq(0, 1, 'not good, nyan, nyan');
>+}
>+
>+throwsRangeError(Infinity);
>+throwsRangeError(-Infinity);
>+throwsRangeError('99/70/1');
>+throwsRangeError('100/70/2');

This relies on unspecified behavior of |new Date(v)| when ToString(v) doesn't match the YYYY-MM-DDTHH:mm:ss.sssZ format.  Instead use |new Date()| followed by |date.setTime(a)| for fully-specified behavior.  With that you should get rid of the latter two tests, and I think those only produce NaN-backed dates, so it should be fine to replace them with |throwsRangeError(NaN)|.

>+if (typeof reportCompare === "function")
>+  reportCompare(true, true);
>\ No newline at end of file

Add one.

Comment 3

[Tom Schuster [:evilpie]]

Created attachment 527678 [details] [diff] [review]
v2

>I'd like to get another patch for this, rather than make the necessary >pre-landing tweaks myself.
Nobody expects that. :)

>This is pretty ugh the way this handling is all done, but whatever.  :-\
Didn't like it either, i am not totally guilty, Jason proposed that, too. ;(

Comment 4

[Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important)]

Comment on attachment 527678 [details] [diff] [review]
v2

Looks good except for it still doing |var date = new Date(a)| rather than |var date = new Date(); date.setTime(a)|, but I don't mind fixing that on push.  Thanks!

Comment 5

[Tom Schuster [:evilpie]]

Yeah, that happens when writing patches at 2 am :(

Comment 6

[Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important)]

http://hg.mozilla.org/tracemonkey/rev/f9d134c5f744

Turns out there was more than just that to fix, ecma_5/Date/15.9.4.2.js relied on toISOString returning "Invalid Date" for out-of-range dates.  :-(  I fixed up all the stuff in that test that relied on that and pushed with those changes.

Also, a much less substantive style issue, I hadn't noticed the original patch had an if (...) return; else ..., which I converted to if (...) return; ... to avoid else-after-return.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/f9d134c5f744

Comment 8

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

Run tests from Comment 0 on Ubuntu 11.04 x86, WinXP, Win 7 x86, Mac OS X 10.6.

Total tests: 22 Passed: 22 Failed: 0 Could not load: 0

Setting status to Verified Fixed.