1205298 - Date.parse() should accept a wider range of potential formats

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

'YYYY-MM-DD HH:mm:ss' is a common time format in China, and since Chrome support that format, some of the websites start relying on this behavior, which cause compatibility issue for us.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: testcase

This page lists some cases that are not accepted by Firefox but accepted by Chrome and Edge.

(Note that this page requires Edge 13+, and doesn't work on the current Safari release)

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

According to a friend working in Alibaba, "90% of compatibility issues of Firefox on our internal system is due to this". [1]

[1] https://twitter.com/ayanamist/status/694854889730486272 (Chinese)

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

jorendorff, any chance we can get this fixed and make it into the ES spec?

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Related usage from Stack Overflow:
http://stackoverflow.com/questions/3257460/new-date-is-working-in-chrome-but-not-firefox
http://stackoverflow.com/questions/32672329/converting-date-in-javascript-is-not-working-in-firefox
http://stackoverflow.com/questions/15862967/cannot-use-date-parse-on-firefox-it-works-on-chrome
http://stackoverflow.com/questions/26182020/firefox-refuses-to-parse-date-even-when-properly-formatted

Anyway, I think Edge and Blink being interoperable on these cases is a signal for us to also support it and have it speced.

Comment 5

[Jan de Mooij [:jandem]]

FWIW, V8's Date parser is here:

https://github.com/v8/v8/blob/007e14ce4ba444531e9ec27b3e2a760f80cb801c/src/dateparser-inl.h#L15

And Chakra's:

https://github.com/Microsoft/ChakraCore/blob/9229c3387b695b2e2fb247681b26d6e6514bc6d1/lib/Runtime/Library/DateImplementation.cpp#L648

We could just port one of these, or compare them to our parser and see where the differences are. Sounds like a fun project actually.

Comment 6

[Jan de Mooij [:jandem]]

And JSC:

https://github.com/WebKit/webkit/blob/67985c34ffc405f69995e8a35f9c38618625c403/Source/JavaScriptCore/runtime/JSDateMath.cpp#L241

Both JSC and Chakra have a 1-slot cache for this. I'm not aware of any benchmarks where that would help, but might be interesting to investigate that as well.

Comment 7

[Morgan Phillips [:mrrrgn]]

Attachment: date.diff

I'm going to sit on this patch over the weekend. Right now we parse dates very conservatively according to the W3C "NOTE-datetime" specification, a restricted subset of the ISO 8601 format. I agree that we should open this up, but only a small amount. Other implementations have implemented heuristics that are nothing short of foot guns in my opinion.

Given that, I'm proposing two tiny changes to our current parsing:

1.) Allow single digit months and days: "2016-1-1" will be treated like "2016-01-01"

2.) Allow a space to act in place of a 'T' to begin the time part of a date: "2016-01-01T01:01:01" will be treated like "2016-01-01 01:01:01".

These small changes should solve many of the issues users are having if I'm reading things correctly.

Comment 8

[Morgan Phillips [:mrrrgn]]

Attachment: date.diff

Comment 9

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8716517 [details] [diff] [review]
date.diff

Review of attachment 8716517 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/ecma_6/Date/non-iso.js
@@ +4,5 @@
> + */
> +
> +/*
> + * For the sake of cross compatibility with other implementations we
> + * make two exceptions to the standard: months and days may be either

I should say which standard.

Comment 10

[Morgan Phillips [:mrrrgn]]

Attachment: date.diff

Comment 11

[Morgan Phillips [:mrrrgn]]

Comment on attachment 8716562 [details] [diff] [review]
date.diff

Review of attachment 8716562 [details] [diff] [review]:
-----------------------------------------------------------------

How do we feel about these changes?

Comment 12

[Jan de Mooij [:jandem]]

Thanks for working on this!

With this patch I still get a number of failures when I open the attached testcase (I ran it in the shell but that shouldn't matter). How hard would it be to fix the remaining issues?

Comment 13

[Morgan Phillips [:mrrrgn]]

(In reply to Jan de Mooij [:jandem] from comment #12)
> Thanks for working on this!
> 
> With this patch I still get a number of failures when I open the attached
> testcase (I ran it in the shell but that shouldn't matter). How hard would
> it be to fix the remaining issues?

Discussing this with jorendorff, I'm not sure we should support all of the cases. For instance, allowing two digit years opens up a huge can of worms. I was shooting for a very conservative change that should cover most use cases.

If we want to implement a date parser just like what, say, v8 has it will take a good bit of extra work I think. There's a real gray area around date parsing, and I'm not sure I like what other engines are doing. For instance, try running this in Chrome: https://jsfiddle.net/6288uhh4/3/

Comment 14

[Morgan Phillips [:mrrrgn]]

What I mean up above is, in many cases perhaps "Invalid Date" really is the correct result.

Comment 15

[Morgan Phillips [:mrrrgn]]

Hmm, that said, I can easily expand this patch to cover single digits hours and minutes. I definitely don't feel good about two digit years.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Morgan Phillips [:mrrrgn] from comment #15)
> Hmm, that said, I can easily expand this patch to cover single digits hours
> and minutes.

I think that would be nice for consistency with the month and days.

> I definitely don't feel good about two digit years.

Ah, I just tried your test in Safari and it totally doesn't match Chrome :(

  new Date(0-01-01): Sat Jan 01 0000 01:00:00 GMT+0100 (CET)
  ...
  new Date(99-01-01): Thu Jan 01 0099 01:00:00 GMT+0100 (CET)

Since engines disagree on this anyway, I agree we should keep Invalid Date for now.

Comment 17

[Morgan Phillips [:mrrrgn]]

Attachment: date.diff

Thank you for the quick feedback ! :) I've expanded the changes to cover hours, minutes, and seconds as well.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

I'm actually a bit concerned about the function name "ParseISODate", and that was why I didn't try to fix this issue myself. Should we rename this function to reflect the fact that it no longer only parses the ISO date string?

Comment 19

[Morgan Phillips [:mrrrgn]]

(In reply to Xidorn Quan [:xidorn] (UTC+8) (less responsive until Feb 22) from comment #18)
> I'm actually a bit concerned about the function name "ParseISODate", and
> that was why I didn't try to fix this issue myself. Should we rename this
> function to reflect the fact that it no longer only parses the ISO date
> string?

Perhaps we could change it to something like ParseISOStyleDate? We still follow the standard fairly closely with a few exceptions now introduced. I left a comment explaining the exceptions in the function and in new test cases.

Comment 20

[Jan de Mooij [:jandem]]

Comment on attachment 8717112 [details] [diff] [review]
date.diff

Review of attachment 8717112 [details] [diff] [review]:
-----------------------------------------------------------------

Great! Renaming to ParseISOStyleDate or something makes sense.

::: js/src/jsdate.cpp
@@ +745,5 @@
>   *   00:00 UTC.  If the time is present but the time zone field is
>   *   missing then we use local time.
>   *
> + * For the sake of cross compatibility with other implementations we
> + * make a few exceptions to the standard: months, days, hours, mintues

Nit: minutes, typo

@@ +781,3 @@
>   *   hh   = two digits of hour (00 through 23) (am/pm NOT allowed)
>   *   mm   = two digits of minute (00 through 59)
>   *   ss   = two digits of second (00 through 59)

Nit: update the hh/mm/ss lines as well

@@ +834,3 @@
>  
>   done_date:
> +    if (PEEK('T') || PEEK(' ')) {

Nit: single-line then/else bodies don't get {}

@@ +842,2 @@
>      NEED(':');
> +    NEED_NDIGITS_OR_LESS(2, min);

Nit: maybe add a test to ensure "n digits or less" does not include 0 digits. Something like "1997-03-08 11::20". A future refactoring could get that wrong.

Comment 21

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #20)
> Nit: single-line then/else bodies don't get {}

(That's a bit ambiguous, more correct: iff each of the condition + then block + else block fit on a single line, the if/else statement doesn't get braces.)

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e504b40d6f8

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e504b40d6f8

Comment 24

[Jason Orendorff [:jorendorff]]

Passing this ni? on to efaust, who can decide whether this is worth standardizing.

Comment 25

[Domenic Denicola]

It would be really great to get this standardized; nobody has made the time to do the necessary testing to find intersection semantics yet, but if someone has that time the world would become a better place.