Closed Bug 649575 Opened 12 years ago Closed 12 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: evilpie)



(Whiteboard: fixed-in-tracemonkey)


(1 file, 1 obsolete file)

So say most of the tests from that fail.
Assignee: general → evilpies
Attached patch added err def and tests (obsolete) — Splinter Review
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');

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-
Attached patch v2Splinter Review
>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]

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

Turns out there was more than just that to fix, ecma_5/Date/ 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.
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla6
Closed: 12 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.