Bug 3477 – some US-centric date formats not parsed by JavaScript (clock at news8austin.com)

• Component: JavaScriptCore
• Status: VERIFIED FIXED

[Stuart Morgan]

The clock in the upper-left corner of news8austin.com always gives 12:45 PST as
the time in Safari, but works in IE and the Mozilla family.

Apple Bug: rdar://3444900

[Stuart Morgan]

10/7/03 9:31 AM Darin Adler:
The problem is that the site provides the date as a string, but it's a string
our version of Date can't parse.

    var now = new Date("10/7/2003 11:30:29 AM")

We'll have to add code to parse dates in this format to fix this.

[Carsten Guenther]

I will have a fix for that soon. Just needs some more testing.

[Carsten Guenther]

Created attachment 2337 [details]
Simple testcase

[Carsten Guenther]

Created attachment 2338 [details]
Proposed fix

[Darin Adler]

Instead of just taking this patch, we might want to take the KDE date_object.cpp, which I believe has 
improvements in handling of this and other similar date formats.

[Carsten Guenther]

I took a look at KHTML's date_object.cpp. They also support the YYYY/MM/DD format, other than that it 
looks not much different.

Let me know what you prefer, I can prepare a patch based on the KHTML source if you want.

[Nate Cook]

I think it would be best to use the KDE version, esp. since they've switched KJS::KRFCDate_parseDate to 
return double, and solved the maximum year 2038 problem as outlined in Bug 3296 and Bug 3417.

[Carsten Guenther]

Created attachment 2444 [details]
Merged patch from KDE

Attached new patch from the KDE code. This fixes this bug and 3296.

[Darin Adler]

Comment on attachment 2444 [details]
Merged patch from KDE

I think it's great to land this improved date code from KDE, presuming that all
the existing date tests we have continue to work.

For future improvements, it's probably worth checking out the FAQ at
<http://www.tondering.dk/claus/calendar.html>, which can answer questions like
the one in the yearFromTime function. In fact, I think
<http://www.tondering.dk/claus/cal/node3.html#SECTION003161000000000000000>
answers that question.

There seems to be a missing "static" in front of timeFromYear, yearFromTime,
and weekDay, which we should add.

I don't understand the need for extrac checks in the part of code that says "if
(utc)". As far as I can tell, ToGMTString and ToUTCString will always set utc
to true, and ToString will always set utc to false, so there's no need for the
additional checks.

The code calls localtime_r inside the __APPLE__ ifdef. If it's no longer
calling localtime() then we should get rid of the macro at the top of the file
changing localtime() calls to localtimeUsingCF() calls. The original reason we
called localtimeUsingCF() instead of localtime() is that localtime() hit the
disk every time; we'll need to test to see that is no longer the case. The
simplest thing to do may be to extend the macro so there's one for localtime_r
too.

I see some tabs in this patch. Please fix that and use only spaces for
indenting.

All the loops that say:

+	   while(*dateString && isspace(*dateString))

could leave out the 0 check. isspace will return false for the 0 character.

The calls to strtol should check the error; I don't think the patch should
remove our errno checking and any call to strtol should check errno after the
fact.

I'm not sure that calling isspace is right -- do we really want characters like
'\n' or '\t' to be allowed in dates, but not Unicode whitespace? I think our
isSpaceOrTab might have been better. We'd need test cases to be sure.

Seems close, but not quite ready to land.

[Carsten Guenther]

Here comes the improved version of the patch.

> There seems to be a missing "static" in front of timeFromYear, yearFromTime,
> and weekDay, which we should add.

Added the missing statics.

> I don't understand the need for extrac checks in the part of code that says "if
> (utc)". As far as I can tell, ToGMTString and ToUTCString will always set utc
> to true, and ToString will always set utc to false, so there's no need for the
> additional checks.

Not excactly sure what tests you mean. Can you give me the line numbers?

> The code calls localtime_r inside the __APPLE__ ifdef. If it's no longer
> calling localtime() then we should get rid of the macro at the top of the file
> changing localtime() calls to localtimeUsingCF() calls. The original reason we
> called localtimeUsingCF() instead of localtime() is that localtime() hit the
> disk every time; we'll need to test to see that is no longer the case. The
> simplest thing to do may be to extend the macro so there's one for localtime_r
> too.

I ran some tests with fs_usage and could not find any disk access when calling localtime(). However, I 
vote for keeping the macros and the core foundation code for now since according to a comment in the 
code it provides us with larger date ranges and I am not sure of all the consequences of removing 
them. This could be addressed in a separate bug.

I also rewrote the one localtime_r to be localtime.

> I see some tabs in this patch. Please fix that and use only spaces for
> indenting.

Cleaned up indendation.

> All the loops that say:
> 
> +          while(*dateString && isspace(*dateString))
> 
> could leave out the 0 check. isspace will return false for the 0 character.

Removed the *dateString test from those loops.

> The calls to strtol should check the error; I don't think the patch should
> remove our errno checking and any call to strtol should check errno after the
> fact.

Put back in the errno checks after calls to strtol.

> I'm not sure that calling isspace is right -- do we really want characters like
> '\n' or '\t' to be allowed in dates, but not Unicode whitespace? I think our
> isSpaceOrTab might have been better. We'd need test cases to be sure.

isSpaceOrTab does not check for more than spaces and tabs either. In addition we should also allow '\r' 
what isspace() does. I ran a quick test with Firefox and it allows both tabs and newlines in dates. So I 
think using isspace() is the right thing until we decide to support all unicode whitespace characters. 
And I am not sure if we should.

[Carsten Guenther]

Created attachment 2533 [details]
Improved patch

[Darin Adler]

Comment on attachment 2533 [details]
Improved patch

Setting the review flag on Carsten's behalf.

[Darin Adler]

Comment on attachment 2533 [details]
Improved patch

I'm not so happy with all the whitespace and indenting changes. There is lots
of code here that hasn't changed at all except that you seem to have run some
program's indenting on it (emacs, perhaps?).

Maybe we should do that in a separate pass, as it makes it harder to spot the
actual changes.

To me, some of the whitespace changes look wrong, too. Like the
strangely-indented body of DateProtoFuncImp::DateProtoFuncImp for example.

My (utc) comment above is about this code:

+    if ( (id == DateProtoFuncImp::ToGMTString) ||
+	  (id == DateProtoFuncImp::ToUTCString) )
+	 t = gmtime(&tv);
+    else if (id == DateProtoFuncImp::ToString)
+	 t = localtime(&tv);
+    else if (utc)
+	 t = gmtime(&tv);
+    else
+	 t = localtime(&tv);

I think the first two if statements are entirely unnecessary.

Because there were so many whitespace changes, I wasn't able to do a thorough
review of the new patch.

[Carsten Guenther]

Created attachment 2552 [details]
Changes without indentation cleanup

Added another patch with the above changes but without the indentation cleanup.


I think you are right about the if(utc) check. I removed the first two if
statements since they are redundant, utc got already set depending on the id
value.

[Darin Adler]

Comment on attachment 2552 [details]
Changes without indentation cleanup

Patch looks great now.

I think we need a better test case now, that covers a lot more cases that the
single test case attached to this bug.

Perhaps we can find one the KDE project is already using? Or maybe with these
changes we can enable the Mozilla date tests.

In any case, the only remaining issue here is that the test case is not good
enough. I will mark this review+, but we should get a much more thorough test
case that covers the new code paths before we land this patch.

In particular, my insistence that we check errno may have resulted in a
stricter function that behaves different than KDE's KJS on various date
strings, so we might end up deciding we need to change that.

[Carsten Guenther]

There is still a problem with dates < 1970, the mozilla test results in 2 regressions. I will look into that.

Also reassigning to myself.

[Carsten Guenther]

Created attachment 2635 [details]
Next interation of proposed fix

Here comes the (hopefully) final patch. It fixes all outstanding issues I could
find with dates that were not in the portable range.

This patch also fixes one of the date tests (ecma_3/Date/15.9.5.5.js), I will
also append the updated expected.html file.

In regards to testcases, I would like to enable all of the mozilla test cases
for dates (the ones in ecma/dates). I did a first run locally and it does not
look too bad. But to fully support those I need to add some code, for example
to create a date object from a bool value. I will open a separate bug for this.

[Carsten Guenther]

Created attachment 2636 [details]
Updated expected.html

[Darin Adler]

Comment on attachment 2635 [details]
Next interation of proposed fix

This line:

+  return seconds == -1 ? NaN : seconds * 1000.0;

should use the constant "invalidDate" rather than a hard-wired -1.

Otherwise, r=me, assuming that our testing is super-thorough. Ideally I'd want
a test date that exercises each branch within the date-parsing function.

Enabling the date tests in the Mozilla JavaScript test suite is a great idea.
I'm so happy this change will allow us to do that!

Harri Porten also showed me a pretty complete date test page the other day. I
suggest contacting him, because I'd like to land that as a layout test.

[Carsten Guenther]

Created attachment 2637 [details]
Final patch

Replaced -1 with invalidDate.

[Darin Adler]

Comment on attachment 2637 [details]
Final patch

r=me

[Geoffrey Garen]

This patch breaks:
     fast/js/date-parse-test

Changing status to reopened and rolling out patch.

[Harri Porten]

Here are the KDE tests:

http://websvn.kde.org/trunk/tests/khtmltests/regression/tests/js/Date.js?rev=424394&view=markup

Not as complete as the Mozilla tests but a) they test the features currently
supported by the implementation and b) test some special, non-standard cases
found on the Web.

[Harri Porten]

As I commented on in another br I think the date-parse-test contains some
questionnable tests. (My) Firefox and Konqueror have 10 failures, IE 9
(partially different ones). Could be that both browsers have their own bugs but
I'd be careful with the interpretation of the results from this test.

[Darin Adler]

The failures are probably due to items in the test that are incorrect. If so, we need to update expected 
results.

[Carsten Guenther]

Sorry about that, I did not run the layout tests after my latest changes. I will look into that later this week.

[Carsten Guenther]

Created attachment 2723 [details]
New patch including updated layout tests

Here comes a new patch that contains date_object.cpp|h, expected.html and a
modified fast/js/date-parse-test.

Let's go through the failed layout tests in detail since they fall into four
groups:

1. Five of the tests were failing because we are checking for errno after every
strtol operation. In one location this was wrong: when we have the year follow
by the timezone, without a time. I removed that check for errno and added a
comment.

2. Six of the tests contained invalid timezone identifiers. Those are invalid
dates and NaN should be returned so the testcases were wrong. In addition to
that our code was wrong too since it produces a number. I added that check for
an unknown timezone and also modified the testcases to have NaN as the expected
result. I also cross-checked with what Firefox does.

3. Two other testcases were wrong in that they assume that "Dec 25 1995 1:30AM"
and "Dec 25 1995 1:30PM" are valid dates. Both are invalid date descriptions,
note the missing space between the time and AM/PM. I modified the testcases to
expect NaN and also cross-checked with Firefox.

4. The remaining two testcases expected NaN althought they are valid: "Dec 25
1995 0:30 AM GMT" and "Dec 25 1995 0:30 PM GMT". Our code produced the right
numbers, again I cross-checked with the results from Firefox. I modified those
two testcases to expect the right results.

[Darin Adler]

Comment on attachment 2723 [details]
New patch including updated layout tests

r=me

[Harri Porten]

Created attachment 2724 [details]
Copyright years update

A one-liner that might make the patch work even better ;)

[Harri Porten]

Comment on attachment 2724 [details]
Copyright years update

Patch accidentally reversed.

[Joost de Valk (AlthA)]

reporter could you veriufy this one? thx.