1617562 - new Date("Fri, 31-Jan-2020 18:16:08 GMT") produces bad parse "Thu Jan 31 -2020 20:46:25 ..."

Status: VERIFIED FIXED

(Core :: JavaScript: Standard Library, defect, P3)

Description

[velavokr]

Attachment: Screenshot from 2020-02-24 14-32-16.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0

Steps to reproduce:

Enter in browser console
new Date("Fri, 31-Jan-2020 18:16:08 GMT")

Actual results:

Thu Jan 31 -2020 20:46:25 GMT+0230 (Moscow Standard Time)

Note the negative year.

Expected results:

Fri Jan 31 2020 21:16:08 GMT+0300 (Moscow Standard Time)

Comment 1

[velavokr]

Perhaps related to https://bugzilla.mozilla.org/show_bug.cgi?id=1439800

Comment 2

[Tooru Fujisawa [:arai]]

likely a same issue as bug 1515318

Comment 4

[Tom S [:evilpie]]

This is not quite a dupe of bug 1439800, because we only parse dashed dates at the beginning.

Comment 5

[Vinny Diehl]

With numeric dashed dates added in bug 1557650, we're encountering the same issue with day of week prefixes. Since I think both of these issues could (should) be fixed with the same patch, I won't open a separate bug for it.

This is happening because these dashed date formats defer to separate functions TryParseDashedDatePrefix and TryParseNumericDashedDatePrefix which try to parse out the format from the beginning of the string. If this fails, the rest of ParseDate continues to run, resulting in the bugged out negative year that you see here, since ParseDate does this when faced with a dashed date that wasn't accepted by TryParseDashedDatePrefix (like in bug 1858851). For the numeric dashed dates, the format with a weekday gets rejected entirely, but these are different symptoms of the same problem; the day of week needs to be parsed out before the dashed date prefix.

Comment 6

[Vinny Diehl]

I've been looking into this one, an approach that we might consider, rather than adding an extra day of week check to the beginning, is to move the day of week check to the beginning entirely. We currently support day of week in some strange places where other engines do not:

Format	JSC	SM	V8
"Sep 26 1995 Tues"	❌	✔️	❌
"Sep 26 Tues 1995"	❌	✔️	❌
"Sep 26 1995 10:Tues:00"	❌	✔️	❌

Moving support to only the beginning makes sense, with the benefit of improved performance as there are 7 less keywords to check for every word encountered. However, this would drop support for "Sep Tues 26 1995". V8 allows a fair amount of slop with words in that position:

d8> new Date("Sep you can put anything here 26 1995")
Tue Sep 26 1995 00:00:00 GMT-0700 (Mountain Standard Time)

and so, "Sep Tues 26 1995" is supported. Explicitly supporting this edge case would require a fair amount of extra complexity, methinks. What do you think we should do, arai?

Comment 7

[Tooru Fujisawa [:arai]]

Thank you for looking into this!

If we're to drop support for many possible syntax, it's better adding telemetry first and see how often the syntax is used (in other words, how many website will break if we drop the support).
Once we confirmed that the syntax is almost never used, then it should be fine to drop those syntax.
Both (add telemetry, drop support) need to be done in separate bug than this.

For telemetry, the "use counter" feature in UsageStatistics.h can be used for this case.
bug 1647791 has a patch that adds a "use counter" telemetry, and you can use it as an example.

The flow around telemetry + drop support is following:

1. create a patch to add telemetry
2. get code review, and also data review https://wiki.mozilla.org/Data_Collection
3. land the patch, and wait for the telemetry data
4. if there are many usage, either
   • add warning and wait until the usage decreases, or
   • stop here, remove the telemetry and close the bug
5. if there are almost no usage:
   1. remove the telemetry and then drop the support for the day of week syntax

Given that the above requires some amount of time, it would be better doing all of these work completely separately as this bug.
(to be clear, this is for the case if it's really necessary to drop the support for the syntax)

For this bug, it would be easier to just allow the optional day of week at the beginning, without touching the day of week handling in the other places, both for simplicity and also for backward/web compatibility.

Also, about the V8's behavior around allowing random words, we don't have to copy the behavior,
but for the "Sep Tues 26 1995" syntax that both SpiderMonkey and V8 allows now, we should be careful about dropping it.

Comment 8

[Vinny Diehl]

Thanks for the input! In that case I'll hack this patch together, and then I'll work on running some telemetry to see if we can simplify by dropping explicit support for day of week later in the pattern.

I've been sleeping on the allowance of random words, and I think that this behavior might be to simplify internationalization? Being loose about this will make bug 1730155 easier to tackle. It will also make the previously discussed format of "Sep foo 26 1995" much less complex to accept, by simply modifying this conditional. Safari also allows random words at the beginning, though not after the month:

Format	JSC	SM	V8
"foo bar Sep 26 1995"	✔️	❌	✔️
"Sep foo 26 1995"	❌	❌	✔️
"foo Sep bar 26 1995"	❌	❌	✔️

I don't think it hurts to accept them in both locations, especially if we're considering accepting internationalized dates?

Comment 9

[Tooru Fujisawa [:arai]]

Good point :)

I've checked the behavior around non-English month name, and apparently JSC and V8 perform the prefix match with "3 characters of month name" against the input.
SpiderMonkey does the opposite, it does the prefix match with input against full month name.

Format	JSC	SM	V8
"8 Sep 2021"	✔️	✔️	✔️
"8 Se 2021"	❌️️	✔️	❌️️
"8 Sepp 2021"	✔️	❌️	✔️
"8 Septembe 2021"	✔️	✔️️	✔️
"8 Septembee 2021"	✔️	❌️️️	✔️
"8 Septembre 2021"	✔️	❌️	✔️
"8 Settembre 2021"	❌️️	❌️	❌️️
"8 Avril 2021"	❌️️	❌️	❌️️

So, French month name "Septembre" (the bug 1730155's case) is accepted on JSC/V8 simply by luck.
Other month names, and month names in other language can be rejected.
For example, Italian month name "Settembre" above is rejected because the first 3 characters are "Set", not "Sep",
and also French month name "Avril" (April) is also rejected because the first 3 characters are "Avr", not "Apr".
So I don't think this is actually for internationalization. But lowering the requirement for the month name won't harm indeed, especially if JSC and V8 agreen on the behavior.

Then, about random words, if JSC and V8 agree on accepting random words before month name, then I agree making SpiderMonkey also accept them at the beginning.

Comment 10

[Vinny Diehl]

Attachment: Bug 1617562 - Allow unknown words at beginning of Date string r?arai,ochameau

They are also allowed after the month name, e.g. "Sep foo 26 1995".

These "unknown words" are supposed to be day of week, or abbreviations
of them, but they can be anything really.

We need to run telemetry before dropping explicit support for day of
week in later parts of the date string.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/74d7322a0dc4
Allow unknown words at beginning of Date string r=arai

Comment 12

[Sandor Molnar[:smolnar]]

Backed out for causing xpc failures on test_natural-sort.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/8be76292bf3f6acbc6d7d87935f7eb147bbf27ca

Push with failures

Failure log -> TEST-UNEXPECTED-FAIL | devtools/shared/tests/xpcshell/test_natural-sort.js | compareOptions - [compareOptions : 906] string first

Failure log Reftest -> SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/worker/checkouts/gecko/js/src/jsdate.cpp:1411:25 in ParseDate<char16_t>

Comment 13

[Vinny Diehl]

Fixed!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2516d38fcdfa
Allow unknown words at beginning of Date string r=arai,devtools-reviewers,ochameau

Comment 15

[Natalia Csoregi [:nataliaCs]]

Backed out for causing leaks on jsdate.cpp

• backout: https://hg.mozilla.org/integration/autoland/rev/7a3978f0626b31f7d311cbc700a8ad6eef3c38a7
• push: https://treeherder.mozilla.org/jobs?repo=autoland&revision=2516d38fcdfa499e0c7ff0a8aaea0a4322a9d6cb
• push where failures were seen: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=UJbtsktOQDq6MxaRcpKqVQ.0&revision=5f6163941f402e507f0d384b9904f2a425a28d57&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Casan%2Copt%2Cmochitests%2Ctest-linux1804-64-asan-qr%2Fopt-mochitest-devtools-chrome%2Cdt5
• failure log: https://treeherder.mozilla.org/logviewer?job_id=435262296&repo=autoland&lineNumber=26107

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/945a8040fc5a
Allow unknown words at beginning of Date string r=arai,devtools-reviewers,ochameau

Comment 17

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/945a8040fc5a

Comment 18

[Vinny Diehl]

Relnoted here: https://github.com/mdn/content/pull/30234

Comment 19

[Ardelean Oana, Desktop QA [:oardelean]]

Reproducible on a 2023-11-05 Nightly build on Windows 10.
Verified as fixed on Firefox 121.0b8 and Nightly 122.0a1 on Windows 10, macOS 12, Ubuntu 22.