Bug 49989

Summary: REGRESSION: Date.parse("Tue Nov 23 20:40:05 2010 GMT") returns NaN
Product: WebKit Reporter: ravager
Component: JavaScriptCoreAssignee: Gavin Barraclough <barraclough>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, aestes, ap, christopher.reiss, commit-queue, edwardjsabol, ggaren, kling, oliver, webkit.review.bot
Priority: P1 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.6   
Bug Depends on:    
Bug Blocks: 55228    
Attachments:
Description Flags
simple fix
none
simple fix
darin: commit-queue-
comment fix and a few extra tests added
none
Updated version of Chris's patch, allow year in either position. oliver: review+

Description ravager 2010-11-23 13:14:27 PST
Date.parse("Tue Nov 23 20:40:05 2010 GMT") returns NaN in Safari 5, but not in Safari 4 (or other browsers).
Comment 1 Alexey Proskuryakov 2010-11-23 23:07:33 PST
Confirmed as a difference with Firefox 3.6.
Comment 2 Alexey Proskuryakov 2011-01-20 16:47:07 PST
<rdar://problem/8895888>
Comment 3 Geoffrey Garen 2011-01-20 17:00:36 PST
The bug here is that the parser parses the timezone as "2010 GMT".
Comment 4 chris reiss 2011-02-14 12:35:53 PST
reproducible in QtWebBrowser as well but not Chrome.  Taking a look ...
Comment 5 chris reiss 2011-02-15 08:48:47 PST
Created attachment 82464 [details]
simple fix
Comment 6 WebKit Review Bot 2011-02-15 08:52:42 PST
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.
Comment 7 chris reiss 2011-02-15 08:56:30 PST
Created attachment 82467 [details]
simple fix
Comment 8 Darin Adler 2011-02-15 08:59:19 PST
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.
Comment 9 chris reiss 2011-02-17 11:27:14 PST
Created attachment 82835 [details]
comment fix and a few extra tests added
Comment 10 Andreas Kling 2011-02-18 02:58:42 PST
Comment on attachment 82835 [details]
comment fix and a few extra tests added

r=me, and thanks for adding more tests.
Comment 11 WebKit Commit Bot 2011-02-18 17:43:59 PST
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>
Comment 12 Adele Peterson 2011-06-20 11:28:58 PDT
This broke searching for flights at United Airlines.  I recommend rolling this out while we investigate further.
Comment 13 Adele Peterson 2011-06-20 11:35:13 PDT
Filed: https://bugs.webkit.org/show_bug.cgi?id=63003
Comment 14 Adele Peterson 2011-06-20 11:41:29 PDT
Rollout patch posted: https://bugs.webkit.org/attachment.cgi?id=97829&action=review
Comment 15 Oliver Hunt 2011-06-20 12:49:03 PDT
This broke united airlines and has been rolled out in r89281
Comment 16 Andy Estes 2011-06-20 12:54:39 PDT
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.
Comment 17 Oliver Hunt 2011-06-20 13:00:22 PDT
(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.
Comment 18 Darin Adler 2011-07-16 09:16:33 PDT
Comment on attachment 82467 [details]
simple fix

Clear review flag since this was rolled out.
Comment 19 Gavin Barraclough 2012-03-09 12:47:27 PST
Created attachment 131077 [details]
Updated version of Chris's patch, allow year in either position.
Comment 20 Gavin Barraclough 2012-03-09 14:32:12 PST
Fixed in r110331