Bug 13472 - Misparsing date in javascript leads to year value of -1
Summary: Misparsing date in javascript leads to year value of -1
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Minor
Assignee: Nobody
URL: javascript:void(alert(new Date( '24 M...
Keywords:
Depends on:
Blocks:
 
Reported: 2007-04-24 08:05 PDT by John McKerrell
Modified: 2007-10-03 08:45 PDT (History)
1 user (show)

See Also:


Attachments
First attempt (1.44 KB, patch)
2007-09-18 12:09 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Updated patch (1.85 KB, patch)
2007-09-18 12:54 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Now with testcase (5.62 KB, patch)
2007-09-18 23:56 PDT, Rob Buis
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John McKerrell 2007-04-24 08:05:08 PDT
Simple test case given in the URL, copied here in case of problems:
   javascript:void(alert(new Date( '24 May,2007 15:35:00' ) ) )

If a space is added after the comma, the parse succeeds, without the space it gives the incorrect year.
Comment 1 David Kilzer (:ddkilzer) 2007-04-24 09:32:01 PDT
Confirmed with a local debug build of WebKit r21060 on Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

Firefox 2.0.0.3 parses it correctly, as does Mac IE 5.2.3 and Opera 9.10.

OmniWeb 5.5.3 suffers from the same bug as WebKit.

Comment 2 David Kilzer (:ddkilzer) 2007-04-24 09:33:36 PDT
Not a regression as the same misparsing occurs in shipping Safari 2.0.4 (419.3) on Mac OS X 10.4.9 (8P135).

Comment 3 Rob Buis 2007-09-18 12:09:45 PDT
Created attachment 16315 [details]
First attempt

This patch should fix the problem. However there are some things I am wondering about:

- mozilla is very liberal with ',', basically making it act as whitespace and things like this work:
javascript:void(alert(new Date( '24 May,,,,,,2007 15:35:00' )))
I wonder if we want to go that way?

- not sure where to add/change a testcase for this?

Cheers,

Rob.
Comment 4 Darin Adler 2007-09-18 12:43:00 PDT
Comment on attachment 16315 [details]
First attempt

I suggest putting the test case in LayoutTests/fast/js.
Comment 5 Rob Buis 2007-09-18 12:54:28 PDT
Created attachment 16316 [details]
Updated patch

Updating the patch with a fix for bug 14176, since they are pretty similar.
Cheers,

Rob.
Comment 6 David Kilzer (:ddkilzer) 2007-09-18 13:07:35 PDT
You should also run WebKitTools/Scripts/run-javascript-tests to make sure there are no regressions and to update the results if there are progressions!

Comment 7 Rob Buis 2007-09-18 23:56:42 PDT
Created attachment 16321 [details]
Now with testcase

I updated the patch with some added tests in the directory Darin pointed out.
Cheers,

Rob.
Comment 8 Adam Roben (:aroben) 2007-09-22 14:46:01 PDT
Comment on attachment 16321 [details]
Now with testcase

+            while (*dateString && (*dateString != '-' && *dateString != ',') && !isspace(*dateString))

The parentheses around (*dateString != '-' && *dateString != ',') should not be necessary.

+            if (*dateString != '-' && *dateString != '/' && !isspace(*dateString) && *dateString != ',')

It would be a little nicer to put the comma check after the slash check, but that's a real nitpick.
Comment 9 Adam Roben (:aroben) 2007-09-28 11:47:28 PDT
Comment on attachment 16321 [details]
Now with testcase

r=me even if the above comments aren't fixed.

Please land this on feature-branch.
Comment 10 Rob Buis 2007-09-29 01:39:36 PDT
Hi Adam,

(In reply to comment #9)
> (From update of attachment 16321 [details] [edit])
> r=me even if the above comments aren't fixed.
> 
> Please land this on feature-branch.

I just did, including taking your comments into account. Unfortunately I had to tweak it slightly
to make it pass the test, I am not sure why, but the same fix works on trunk.
Cheers,

Rob.
Comment 11 Rob Buis 2007-10-03 08:45:05 PDT
I guess this can be closed now.
Cheers,

Rob.