Bug 3366 – Handle more of invalid date formats

• Status: RESOLVED
• Resolution: WORKSFORME
• Component: JavaScriptCore

[John C. Tull]

Qixo.com, a best airfare finder consolidator, has persistently failed in Safari in all versions, including the 
newly opened source build for the webkit framework. When entering a search, then proceeding, a 
javascript error warning that fares cannot be found within 12 hours is displayed, regardless of the dates 
entered. I cannot generate any debug output to Console.app or the javascript console.

[Mark Rowe (bdash)]

Created attachment 2158 [details]
A reduced test case

The incorrect behaviour of the search on Qixo.com appears to be due to a bug in
date parsing that results dates of the format "mm/dd/yyyy hh:mm:ss" being
handled incorrectly.

[Mark Rowe (bdash)]

Created attachment 2161 [details]
Updated testcase that's not timezone specific.

[Nate Cook]

Created attachment 2200 [details]
Proposed fix

This patch adds more extensive fallback code for the Date() constructor when
passed a string like:
	'12/25/1995 12:21:43 PST'

Currently only the date is expected, so the year field is corrupt and causes
mktime to return an invalid date.

[Nate Cook]

Created attachment 2217 [details]
Proposed fix (rev)

Revised to handle input error.

[Darin Adler]

Looks pretty good. Here are some patch review comments:

The patch has tabs in it. That needs to be fixed. The coding guidelines mention that issue.

Typically, CoreFoundation calls that create new objects have "Copy" or "Create" in their names, which 
applies to GetTimeZoneWithName.

I'd like to see more than just the single test case added; perhaps test cases that are almost in this format, 
but not quite, say with more than one space in various places, etc. One nice approach would be to add the 
test cases to date-parse-test.html.

[Nate Cook]

Created attachment 2233 [details]
Updated date-parse-test.html with alternate formats

This also fixes a bug where the second test fails if you aren't in Cupertino.
:)

[Nate Cook]

Created attachment 2234 [details]
Patch for date-parse-test.html

[Nate Cook]

Created attachment 2235 [details]
Proposed fix (rev2)

Comments above incorporated. This executes the revised date-parse-test.html
properly -- let me know if that test is too strenuous / weird.

[Darin Adler]

Looking great to me. Just a couple more issues I can see:

1) Where's the delete to match this new (from the patch)?

+            t.tm_zone = new char[3];

2) You're calling UString::ascii, which says:

    This method should only be used for *debugging* purposes

Instead you should use UTF8String or just copy a character at a time instead of trying to do all three at 
once with strcpy.

And then one less important thing:

3) Seems slightly bad style to have those hardcoded 3 values in various places for the maximum length 
of a time zone string.

The test cases in date-parse-test.html look fine (part of the patch is already taken care of, since I 
updated date-parse-test.html last night). I'd also like to see test cases with various bad characters in 
the date (like spaces before the "/" characters, or crazy characters in there) eventually, even if not right 
now.

[Harri Porten]

I get like 10 test failures when running the test case in Firefox 1.0.5. 
Problem in Firefox or the test case? 

[Nate Cook]

Created attachment 2311 [details]
Proposed fix (rev3)

Incorporated comments from above and expanded the parsing flexibility to handle
more cases.

I also realized that when Date.parse() fails, it should return NaN, and
anInvalidDate.toString() should return "Invalid Date," not "Fri Dec 13 1901
14:45:52 GMT-0600." This is fixed too, which I think means failing on random
screwed up input is much more okay than it was.

[Nate Cook]

Created attachment 2312 [details]
A meaner date_parse_test.html

This adds uglier dates to parse, some of which are obviously bad enough to
cause parsing failure.

Re: differences between this and Firefox ... in my opinion Safari will be the
better off after this patch, since it will be more liberal at parsing erroneous
input. Out of 16 tests in this new test that Firefox fails, there are only 3
that it parses that Safari doesn't. One of the cases where FF parses is when
you put in a date like 12/56/1995 -- it comes back with January 25, 1996 (or
something), Safari says "nope." Thoughts?

[Alexey Proskuryakov]

qixo.com works in a current nightly, apparently due to the many date parsing changes that were made in WebKit.

Some of the examples in the test are parsed by neither WinIE nor Firefox, yet fixing some sounds reasonable:

Dec 25 1995 1:30AM (PASS in IE)
Dec 25 1995 1:30PM (PASS in IE)
Dec 25 1995 13:30 PM GMT (PASS in IE)
12/25/1995 :: (PASS in IE and Firefox)
12/25/1995 3: (PASS in IE and Firefox)
12 / 25 / 1995 13:30:30 GMT (PASS in IE and Firefox)
12 / 25 /1995 13:30:30 GMT (PASS in IE and Firefox)
asdf 12/ 25/ 1995 13:30:30 GMT (PASS in IE and Firefox - they cannot parse this, as expected)
12/56/1995 13:30:30 GMT (FAIL in IE and Firefox - they parse this successfully)

[Joe Gillespie]

Please support native gmtime() format

The date format

"Tue Feb 13 00:55:41 2007 GMT"

as generated by a Perl script using

my $expDate = gmtime(time()." GMT";

The JavaScript code would be...

expdate="Tue Feb 13 00:55:41 2007 GMT";
exp3=Date.parse(expdate);
alert(exp3);

Every other browser I've tried returns a number like

1170455193910

Safari returns nothing and the console shows no error

[Adam Barth]

As far as I can tell, our current behavior matches Firefox.