Closed Bug 730838 Opened 8 years ago Closed 3 years ago

Date.parse accepts non-zero milliseconds for ISO-8601 midnight representation

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: anba, Assigned: tephra)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120215223356

Steps to reproduce:

js> Date.parse("1970-01-01T24:00:00.500Z")


Actual results:

The number `86400500` is returned


Expected results:

Date.parse() should return `NaN` in this case.

Cf. ISO 8601:2004(E), "4.2.3 Midnight":
---
[...] To represent midnight the representations may be expanded with a decimal fraction containing only zeros in accordance with 4.2.2.4.
---

Non-zero values for the minute or second field do return `NaN`:
js> Date.parse("1970-01-01T24:00:01.000Z")
NaN
js> Date.parse("1970-01-01T24:01:00.000Z")
NaN

(The current behaviour is allowed by the spec since Date.parse() can parse the input string in an implementation dependent way after trying the ISO 8601 format. But it'd still be nice to have consistent behaviour...)
Assignee: nobody → general
Component: General → JavaScript Engine
QA Contact: general → general
Assignee: general → nobody
Component: JavaScript Engine → JavaScript: Standard Library
This issue is now impacting the score on the "ECMAScript 5 Compatibility Table" (http://kangax.github.io/compat-table/es5/).  Is there a meta bug for that site
Status: UNCONFIRMED → NEW
Component: JavaScript: Standard Library → JavaScript Engine
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86_64 → All
Component: JavaScript Engine → JavaScript: Standard Library
We forgot to check that the fraction part of second in the date was zero
if the time is midnight.

Review commit: https://reviewboard.mozilla.org/r/55398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55398/
Attachment #8756779 - Flags: review?(evilpies)
Attachment #8756780 - Flags: review?(evilpies)
Looks like we forgot to check for the fraction part in the error checking for midnight.

First time using mozreview so not sure if I did everything correctly.
Attachment #8756779 - Flags: review?(evilpies) → review+
Comment on attachment 8756779 [details]
MozReview Request: Tests for Bug 730838

https://reviewboard.mozilla.org/r/55398/#review52526

Thanks, nice work. I didn't actually check André's references, but I am going to trust he is correct. And matches Chrome.
Comment on attachment 8756780 [details]
MozReview Request: Bug 730838 - Date.parse accepts non-zero milliseconds for ISO-8601 midnight representation

https://reviewboard.mozilla.org/r/55400/#review52524

::: js/src/tests/ecma_5/Date/15.9.4.2.js:133
(Diff revision 1)
>    // 00:00 and 24:00
>    check("2009-07-23T00:00:00.000-07:00", dd(2009,7,23,7,0,0,0));
>    check("2009-07-23T24:00:00.000-07:00", dd(2009,7,24,7,0,0,0));
>  
> +  // Bug 730838 - non-zero fraction part for midnight should produce NaN
> +  printBugNumber("730838");

Don't think printBugNumber is necessary here.
Attachment #8756780 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #5)
> Thanks, nice work. I didn't actually check André's references, but I am
> going to trust he is correct. And matches Chrome.

Yes the reference is correct and Chrome gives NaN for this :)
(In reply to Tom Schuster [:evilpie] from comment #6)
> Comment on attachment 8756780 [details]
> MozReview Request: Test for bug 730838
> 
> https://reviewboard.mozilla.org/r/55400/#review52524
> 
> ::: js/src/tests/ecma_5/Date/15.9.4.2.js:133
> (Diff revision 1)
> >    // 00:00 and 24:00
> >    check("2009-07-23T00:00:00.000-07:00", dd(2009,7,23,7,0,0,0));
> >    check("2009-07-23T24:00:00.000-07:00", dd(2009,7,24,7,0,0,0));
> >  
> > +  // Bug 730838 - non-zero fraction part for midnight should produce NaN
> > +  printBugNumber("730838");
> 
> Don't think printBugNumber is necessary here.

I added this since we do a printBugNumber at the beginning of the test and thought it would make sense to do it here so that if this test fail sometime we don't get confused by the first printBugNumber output.
Comment on attachment 8756780 [details]
MozReview Request: Bug 730838 - Date.parse accepts non-zero milliseconds for ISO-8601 midnight representation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55400/diff/1-2/
Attachment #8756779 - Attachment description: MozReview Request: Bug 730838 - Date.parse accepts non-zero milliseconds for ISO-8601 midnight representation → MozReview Request: Review comment fix: unneccesarry printBugNumber
Comment on attachment 8756779 [details]
MozReview Request: Tests for Bug 730838

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55398/diff/1-2/
Seems like I messed up somehow when pushing my review fix to mozreview so that the actual fix disappeared. No sure what actually happened but repushing it to review and try-server.
Comment on attachment 8756780 [details]
MozReview Request: Bug 730838 - Date.parse accepts non-zero milliseconds for ISO-8601 midnight representation

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55400/diff/2-3/
Attachment #8756780 - Attachment description: MozReview Request: Test for bug 730838 → MozReview Request: Bug 730838 - Date.parse accepts non-zero milliseconds for ISO-8601 midnight representation
Attachment #8756779 - Attachment description: MozReview Request: Review comment fix: unneccesarry printBugNumber → MozReview Request: Tests for Bug 730838
Comment on attachment 8756779 [details]
MozReview Request: Tests for Bug 730838

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55398/diff/2-3/
Okay, really not sure what happened but was able to get everything in the correct state again.

Tom, I've never pushed to try before. Would love some pointers on how to decide which tests to run on treeherder, so I don't mess that up :).
Flags: needinfo?(evilpies)
I would usually just run everything on linux/linux64 especially during weekends. http://trychooser.pub.build.mozilla.org/ is useful. You probably at least want jsreftest and jittests.
Flags: needinfo?(evilpies)
Given that I can't assign bugs to my self and I'm unable to set the checkin-needed flag. Can you help me there Tom? (Sorry for all the hassle)
Flags: needinfo?(evilpies)
(In reply to Eric Skoglund [:tephra] from comment #18)
> Given that I can't assign bugs to my self and I'm unable to set the
> checkin-needed flag

Yes you can.  ;-)
Flags: needinfo?(evilpies)
Assignee: nobody → eric
Keywords: checkin-needed
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> (In reply to Eric Skoglund [:tephra] from comment #18)
> > Given that I can't assign bugs to my self and I'm unable to set the
> > checkin-needed flag
> 
> Yes you can.  ;-)

Great, thanks :)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fca24589e966
Date.parse accepts non-zero milliseconds for ISO-8601 midnight representation. r=evilpies
https://hg.mozilla.org/integration/mozilla-inbound/rev/90a4cdcb559e
Tests for Bug 730838. r=evilpies
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fca24589e966
https://hg.mozilla.org/mozilla-central/rev/90a4cdcb559e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.