1265136 - new Date(<mm/dd/yy>) behaves differently in Firefox vs Chrome/Safari

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Johnny Stenback (:jst)]

Attachment: foo.html

Using new Date() to parse a date string in the format mm/dd/yy behaves differently in Firefox than it does in Chrome and Safari. For example using 04/16/17 yields a date object for Apr 16 1917 in Firefox and Apr 16 2017 in Chrome and Safari. IE and Edge appear to match what Firefox does.

I'm assuming this is per spec so we should figure out if we can get Chrome and Safari to fix their implementations in a timely manner, or we should consider matching their behavior and arguing for a spec update to match reality.

Comment 1

[Jason Orendorff [:jorendorff]]

This is nonstandard, and it's possible it's one of those things where no one can change.

Cc-ing our standards folks and Morgan who recently hacked our behavior a little in bug 1205298.

Comment 2

[Morgan Phillips [:mrrrgn]]

(In reply to Johnny Stenback  (:jst, [e-mail]) from comment #0)
> Created attachment 8742016 [details]
> foo.html
> 
> Using new Date() to parse a date string in the format mm/dd/yy behaves
> differently in Firefox than it does in Chrome and Safari. For example using
> 04/16/17 yields a date object for Apr 16 1917 in Firefox and Apr 16 2017 in
> Chrome and Safari. IE and Edge appear to match what Firefox does.
> 
> I'm assuming this is per spec so we should figure out if we can get Chrome
> and Safari to fix their implementations in a timely manner, or we should
> consider matching their behavior and arguing for a spec update to match
> reality.

Sadly, the standard purposefully allowed this behavior to be implementation specific. There are differences between Chrome and Safari for some other formats/date-ranges but in this case they do agree. It seems to be the case that any two digit year <= 50 is converted to a 21st century year, and any above 50 is converted to a 20th century year. Because our behavior is in the minority I believe we should modify it to match the majority (users could care less about the spec).

Date formats have caused a lot of problems for users. We need to make a spec proposal. I'll work with jorendorff to try doing that myself.

Comment 3

[Johnny Stenback (:jst)]

Thanks Morgan. And I agree that we should align with majority behavior here, and defining sane behavior here, or even less than sane, would be better for developers than the current undefined behavior.

Comment 4

[Morgan Phillips [:mrrrgn]]

Okay, I have a little laundry list of changes to make:

"<Month> dd yy" and "<Month> yy dd", where <Month> is an english word, should be supported anywhere that dd is > 0 and < 32 and yy is > 0 and < 100.

yy/mm/dd , mm/dd/yy, and yy/dd/mm should be supported for all valid ranges of dd and mm -- currently it is not, and has weird behaviors because of that (e.g. new Date("49/12/01").toString() === "Thu Jan 12 1905 00:00:00 GMT-0800 (PST)" with our current rules).

Comment 5

[Morgan Phillips [:mrrrgn]]

Attachment: two-digits.diff

Just attaching this WIP for now. Will look over it again in the morning and then flag for review.

Comment 6

[Morgan Phillips [:mrrrgn]]

Attachment: two-digits.diff

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8745019 [details] [diff] [review]
two-digits.diff

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

Given how mon/mday are computed in the 150 lines prior, this is somewhat rubberstampy, because I have approximately zero idea what those named variables correspond to in the comments' cheeky "f/m/l" nomenclature, and it's fairly hard to figure out reading the code.  We really should just write up a spec algorithm and get it standardized, so we don't have this inscrutable implementation behavior to maintain (and sometimes modify, like here).  Perhaps doing that at the same time we write a self-hosted implementation makes sense.

::: js/src/jsdate.cpp
@@ +1126,5 @@
> +     *         b. If 31 < f and 0 < m <= 12 and 0 < l <= 31 f/m/l is
> +     *         interpreted as year/month/day
> +     *            i.  If year < 50, it is the number of years after 2000
> +     *            ii. If year >= 50, it is the number of years after 1900.
> +     *           iii. If year >= 100, it is the number of years after 0.

Move these year-test bits outside the two cases entirely, since we're applying them to all cases (see below).  Or rather, move the paragraph of prose in case 1, looks like.

@@ +1131,3 @@
>       */
>      if (seenMonthName) {
> +        if (mday > 100 && mon > 100)

>100 doesn't agree with the >= 100 in the comment above.  <=31 later means it doesn't matter, but still: best be consistent so no confusion.

@@ +1138,5 @@
>              year = mday;
>              mday = temp;
>          }
> +
> +        if (mday == 0 || mday > 31)

I know we excluded mday < 0 above, but since we're really checking for in-[0, 31] range, I'd rather the condition also discarded negative mday.

  if (mday < 0 || mday > 31)

@@ +1148,5 @@
>              year += 1900;
>          }
> +
> +    /* (a) month/day/year */
> +    } else if (mon <= 12 && mon > 0 && mday <= 31 && mday > 0) {

Mild preference for reading these in x < y < z order, so:

  (0 < mon && mon <= 12 && 0 < mday && mday <= 31)

@@ +1156,4 @@
>              year += 1900;
>          }
> +
> +    /* (b) year/month/day */

Move this inside the subsequent if?  There's not a *good* place to put this with C++'s semantics, but inside the actual block in question seems preferable to inside a quasi-adjacent block.

@@ +1164,5 @@
> +            if (year < 50) {
> +                year += 2000;
> +            } else if (year >= 50 && year < 100) {
> +                year += 1900;
> +            }

So if I read this correctly, this if-else is present at the end of every arm of this if, excepting only the early |return false| below.  So can't you move this if-else outside the entire if-else if-else sequence, to just before the |month -= 1| below?

Oh, excessive braces on this if, because condition/both consequents are one-liners.

::: js/src/tests/ecma_6/Date/two-digit-years.js
@@ +16,5 @@
> +for (let year of Array(100).keys()) {
> +    for (let month of Array(12).keys()) {
> +        for (let day of Array(31).keys()) {
> +            let fullYear = year >= 50 ? year + 1900 : year + 2000;
> +            let fullDate = new Date(`${month + 1}/${day + 1}/${fullYear}`);

new Date(month, day + 1, fullYear) to avoid the excessive parsing dependence, IMO.

@@ +20,5 @@
> +            let fullDate = new Date(`${month + 1}/${day + 1}/${fullYear}`);
> +
> +            // mm/dd/yy
> +            let d1 = new Date(`${month + 1}/${day + 1}/${year}`);
> +            assertEq(d1.toString(), fullDate.toString())

Rather than compare toString() with its complex/slow behavior, compare the actual internal values using getTime().

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/138521110469

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/138521110469

Comment 10

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/date-parse-handling-of-2-digit-years-has-been-changed-to-be-chrome-compatible/

Comment 11

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/49#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/parse#Compatibility_notes