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

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

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...)

Comment 1

[Alexandre Folle de Menezes]

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

Comment 2

[Eric Skoglund [:tephra]]

Attachment: MozReview Request: Tests for Bug 730838

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/

Comment 3

[Eric Skoglund [:tephra]]

Attachment: MozReview Request: Bug 730838 - Date.parse accepts non-zero milliseconds for ISO-8601 midnight representation

Review commit: https://reviewboard.mozilla.org/r/55400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55400/

Comment 4

[Eric Skoglund [:tephra]]

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.

Comment 5

[Tom S [:evilpie]]

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 6

[Tom S [:evilpie]]

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.

Comment 7

[Eric Skoglund [:tephra]]

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

Comment 8

[Eric Skoglund [:tephra]]

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

[Eric Skoglund [:tephra]]

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/

Comment 10

[Eric Skoglund [:tephra]]

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/

Comment 11

[Eric Skoglund [:tephra]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5a83736fe37

Comment 12

[Eric Skoglund [:tephra]]

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 13

[Eric Skoglund [:tephra]]

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/

Comment 14

[Eric Skoglund [:tephra]]

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/

Comment 15

[Eric Skoglund [:tephra]]

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 :).

Comment 16

[Tom S [:evilpie]]

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.

Comment 17

[Eric Skoglund [:tephra]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=361e268719be

Comment 18

[Eric Skoglund [:tephra]]

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)

Comment 19

[Jeff Walden [:Waldo]]

(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.  ;-)

Comment 20

[Eric Skoglund [:tephra]]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fca24589e966
https://hg.mozilla.org/mozilla-central/rev/90a4cdcb559e