Bug 49989 – REGRESSION: Date.parse("Tue Nov 23 20:40:05 2010 GMT") returns NaN

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

Date.parse("Tue Nov 23 20:40:05 2010 GMT") returns NaN in Safari 5, but not in Safari 4 (or other browsers).

[Alexey Proskuryakov]

Confirmed as a difference with Firefox 3.6.

[Alexey Proskuryakov]

<rdar://problem/8895888>

[Geoffrey Garen]

The bug here is that the parser parses the timezone as "2010 GMT".

[chris reiss]

reproducible in QtWebBrowser as well but not Chrome.  Taking a look ...

[chris reiss]

Created attachment 82464 [details]
simple fix

[WebKit Review Bot]

Attachment 82464 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/wtf/DateMath.cpp:927:  Missing spaces around =  [whitespace/operators] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris reiss]

Created attachment 82467 [details]
simple fix

[Darin Adler]

Comment on attachment 82467 [details]
simple fix

View in context: https://bugs.webkit.org/attachment.cgi?id=82467&action=review

I’m not going to say review- just for a comment, but I don’t feel comfortable doing commit-queue+ on this so for now I am doing commit-queue-. Someone else can reverse that if we discuss and decide to land this exactly as-is.

> LayoutTests/fast/js/script-tests/date-parse-test.js:130
> +testDateParse("Wed Dec 25 1:30 1995 GMT", "819855000000");

I am concerned that we are adding only this single test case that is a well formatted date. I am more interested in more ambiguous cases that end up being treated as years. We need to make sure we cover not only legal cases but unusual ones as well.

Can someone add some more test cases?

> Source/JavaScriptCore/wtf/DateMath.cpp:924
> +    // the year may be before the timezone

WebKit project style uses a sentence style for comments. And this one is a sentence. I suggest this comment instead.

    // The year may be after the time but before the time zone.

[chris reiss]

Created attachment 82835 [details]
comment fix and a few extra tests added

[Andreas Kling]

Comment on attachment 82835 [details]
comment fix and a few extra tests added

r=me, and thanks for adding more tests.

[WebKit Commit Bot]

Comment on attachment 82835 [details]
comment fix and a few extra tests added

Clearing flags on attachment: 82835

Committed r79060: <http://trac.webkit.org/changeset/79060>

[Adele Peterson]

This broke searching for flights at United Airlines.  I recommend rolling this out while we investigate further.

[Adele Peterson]

Filed: https://bugs.webkit.org/show_bug.cgi?id=63003

[Adele Peterson]

Rollout patch posted: https://bugs.webkit.org/attachment.cgi?id=97829&action=review

[Oliver Hunt]

This broke united airlines and has been rolled out in r89281

[Andy Estes]

Here is the string that united.com attempts to parse:

Mon Jun 20 11:01:37 CDT 2011

Parsing this string via Date.parse() fails after r79060 but should not (works in Safari 5 and Firefox).

If someone tries to re-land a patch for this bug, please make sure strings such as the one above and the one in <https://bugs.webkit.org/show_bug.cgi?id=55228> can be parsed.

[Oliver Hunt]

(In reply to comment #16)
> Here is the string that united.com attempts to parse:
> 
> Mon Jun 20 11:01:37 CDT 2011
> 
> Parsing this string via Date.parse() fails after r79060 but should not (works in Safari 5 and Firefox).
> 
> If someone tries to re-land a patch for this bug, please make sure strings such as the one above and the one in <https://bugs.webkit.org/show_bug.cgi?id=55228> can be parsed.

I added this string to the date parsing tests.

[Darin Adler]

Comment on attachment 82467 [details]
simple fix

Clear review flag since this was rolled out.

[Gavin Barraclough]

Created attachment 131077 [details]
Updated version of Chris's patch, allow year in either position.

[Gavin Barraclough]

Fixed in r110331