Closed
Bug 649575
Opened 12 years ago
Closed 12 years ago
Date.prototype.toISOString must throw a RangeError for non-finite dates
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla6
People
(Reporter: Waldo, Assigned: evilpie)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
3.02 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
So say most of the tests from http://samples.msdn.microsoft.com/ietestcenter/Javascript/ES15.9.html that fail.
Assignee | ||
Updated•12 years ago
|
Assignee: general → evilpies
Assignee | ||
Comment 1•12 years ago
|
||
nyan nyan
Assignee | ||
Updated•12 years ago
|
Attachment #527107 -
Flags: review?(jwalden+bmo)
Reporter | ||
Comment 2•12 years ago
|
||
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-
Assignee | ||
Comment 3•12 years ago
|
||
>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)
Reporter | ||
Comment 4•12 years ago
|
||
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+
Assignee | ||
Comment 5•12 years ago
|
||
Yeah, that happens when writing patches at 2 am :(
Reporter | ||
Comment 6•12 years ago
|
||
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
Comment 7•12 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f9d134c5f744
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 8•11 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.
Description
•