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

VERIFIED FIXED in mozilla6

Status

()

Core
JavaScript Engine
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: evilpie)

Tracking

Trunk
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

So say most of the tests from http://samples.msdn.microsoft.com/ietestcenter/Javascript/ES15.9.html that fail.
Assignee: general → evilpies
Created attachment 527107 [details] [diff] [review]
added err def and tests

nyan nyan
Attachment #527107 - Flags: review?(jwalden+bmo)
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.
Attachment #527107 - Flags: review?(jwalden+bmo) → review-
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. ;(
Attachment #527107 - Attachment is obsolete: true
Attachment #527678 - Flags: review?(jwalden+bmo)
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!
Attachment #527678 - Flags: review?(jwalden+bmo) → review+
Yeah, that happens when writing patches at 2 am :(
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.
Status: NEW → ASSIGNED
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla6
http://hg.mozilla.org/mozilla-central/rev/f9d134c5f744
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 8

6 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.