Last Comment Bug 649575 - Date.prototype.toISOString must throw a RangeError for non-finite dates
: Date.prototype.toISOString must throw a RangeError for non-finite dates
Status: VERIFIED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Tom Schuster [:evilpie]
:
Mentors:
Depends on:
Blocks: es5
  Show dependency treegraph
 
Reported: 2011-04-12 23:28 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-07-28 06:22 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
added err def and tests (3.05 KB, patch)
2011-04-19 14:36 PDT, Tom Schuster [:evilpie]
jwalden+bmo: review-
Details | Diff | Splinter Review
v2 (3.02 KB, patch)
2011-04-21 16:18 PDT, Tom Schuster [:evilpie]
jwalden+bmo: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-12 23:28:35 PDT
So say most of the tests from http://samples.msdn.microsoft.com/ietestcenter/Javascript/ES15.9.html that fail.
Comment 1 Tom Schuster [:evilpie] 2011-04-19 14:36:19 PDT
Created attachment 527107 [details] [diff] [review]
added err def and tests

nyan nyan
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-21 15:19:57 PDT
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] 2011-04-21 16:18:39 PDT
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] (remove +bmo to email) 2011-04-22 13:16:21 PDT
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] 2011-04-22 14:13:16 PDT
Yeah, that happens when writing patches at 2 am :(
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-04-22 19:57:57 PDT
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) 2011-04-26 15:17:14 PDT
http://hg.mozilla.org/mozilla-central/rev/f9d134c5f744
Comment 8 George Carstoiu 2011-07-28 06:22:42 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.