Bug 44632 – Date.parse not ES5 compliant, cannot parse standard Date Time String format

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Nathan Vander Wilt]

WebKit's Date.parse method cannot handle datetime strings in ECMAScript 5's RFC 3339/ISO 8601–style date interchange format (formally defined in ECMA-262-5 § 15.9.1.15).

This breaks compliance with ECMA-262-5, because in according to § 15.9.4.2 the following comparison should yield true:

x = new Date();
Date.parse(x.toString()) === Date.parse(x.toISOString())


As of WebKit r65825, the comparison yields false because Date.parse((new Date).toISOString()) returns NaN instead of the correct UTC milliseconds.


Notes:
Date.parse correctly handles the English readable RFC 822–style date formats dumped by Date.prototype.toString(), but these are not suitable for interchange (and standards-wise parsing them only represents an allowable implementation-specific fallback heuristic).

[Nathan Vander Wilt]

Created attachment 70673 [details]
Implemented ECMAScript/RFC3339 date parser in JavaScriptCore

I've implemented the missing Date.parse/constructor logic for RFC 3339–style timestrings.

[Darin Adler]

Comment on attachment 70673 [details]
Implemented ECMAScript/RFC3339 date parser in JavaScriptCore

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

review- because of the decimal separator issue

> JavaScriptCore/wtf/DateMath.cpp:566
> +    static unsigned int daysPerMonth[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};

We normally use “unsigned”, not “unsigned int”.

> JavaScriptCore/wtf/DateMath.cpp:569
> +    unsigned int month = 0, day = 0, hours = 0, minutes = 0;

We normally use “unsigned”, not “unsigned int”. We don’t declare multiple variables on a single line.

> JavaScriptCore/wtf/DateMath.cpp:574
> +    // NOTE: this is a bit more lenient on the year string than ES5 specifies:
> +    //       instead of restricting to 4 digits (or 6 digits with mandatory +/-),
> +    //       it accepts any integer value. Consider this an implementation fallback.

We use sentence format for comments, meaning the first word should be capitalized.

We don’t put NOTE in front of comments.

We don’t indent subsequent lines of comments in this fashion.

> JavaScriptCore/wtf/DateMath.cpp:576
> +    int numFieldsParsed = sscanf(dateString, "%d-%2u-%2u T %2u:%2u:%lf %6s",
> +                                 &year, &month, &day, &hours, &minutes, &seconds, timeZoneBuffer);

We don’t indent subsequent lines in this fashion.

Typically sscanf has poor performance. Would you be willing to write the parsing without using that function?

The sscanf function will determine which decimal point character based on the POSIX locale, and I presume we do not want that behavior here, so the seconds parsing is a bit of a problem.

[Nathan Vander Wilt]

Thanks for looking at this, Darin. The locale issue would definitely have been a problem, so I will look into using the conversion helper functions for the existing ("fallback") implementation instead.

[Nathan Vander Wilt]

Created attachment 70798 [details]
Rewrote patch based on feedback

Rewrote patch based on review feedback. Added additional tests for issues discovered during rewrite.

[Darin Adler]

Comment on attachment 70798 [details]
Rewrote patch based on feedback

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

I’d like to see still-more test cases. I don’t think these tests cover every branch in the function. I believe I can remove some of the code and still have all the tests pass.

This is OK to land as-is, but I’d prefer to see an even better version of the patch with more tests and more-conventional WebKit coding style.

> JavaScriptCore/wtf/DateMath.cpp:567
> +    static unsigned daysPerMonth[] = {31, 29, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31};

Should be “static const”. Also would be nice to specify 12 here as the array size. Our normal style is to put spaces inside the braces.

> JavaScriptCore/wtf/DateMath.cpp:577
> +    long year;
> +    long month;
> +    long day;
> +    long hours;
> +    long minutes;
> +    long intSeconds;
> +    double seconds;
> +    
> +    char* postParsePosition;

Would be nicer to define these right where they are initialized rather than defining them all at the top.

> JavaScriptCore/wtf/DateMath.cpp:586
> +    dateString = postParsePosition + 1;

It’s a little strange to keep moving this pointer along, but call it “date string”, since it’s a pointer to the remainder of the string, not the entire string. Might be better to give this a different name.

> JavaScriptCore/wtf/DateMath.cpp:628
> +        long numFracDigits;

This especially should be defined on the line where it’s initialized.

> JavaScriptCore/wtf/DateMath.cpp:643
> +    if (day < 1 || day > daysPerMonth[month-1])

We put spaces around operators like the "-" here.

[Nathan Vander Wilt]

Created attachment 70814 [details]
Adds more tests (and stricter input checking) and fixes more style issues.

Besides the new style issues, I've added some more tests for corner cases and made the invalid date checking a bit stricter.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 70814 [details]:

[u'webarchive/test-link-rel-icon.html']

Please file bugs against the tests.  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 70814 [details]
Adds more tests (and stricter input checking) and fixes more style issues.

Clearing flags on attachment: 70814

Committed r69833: <http://trac.webkit.org/changeset/69833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.