Last Comment Bug 273292 - freeze/loop because Mozilla parses dates differently than IE
: freeze/loop because Mozilla parses dates differently than IE
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: general
: Phil Schwartau
Mentors:
http://sports.sina.com.cn/nba/
: 272716 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2004-12-05 14:10 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-08-05 22:27 PDT (History)
6 users (show)
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (339 bytes, text/html)
2004-12-05 14:15 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
fix (977 bytes, patch)
2005-04-20 14:55 PDT, Bob Clary [:bc:]
brendan: review+
brendan: approval1.8b2+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2004-12-05 14:10:19 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041203 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041203 Firefox/1.0+

This was mentioned at:
http://forums.mozillazine.org/viewtopic.php?t=177249

Basically the url site is freezing, because of this (much simplified here) script:
<script>
for (d=3;;d++)
{
var ddd="11/"+d+"/2004"
var days= new Date(ddd);
month=days.getMonth()+1;
dayday=month+"/"+days.getDate()

if (dayday=="4/21") break;
}
</script> 
This loop never breaks, because dayday never becomes "4/21" in Mozilla.
There seems to be a "boundary" of where Mozilla still succesfully parses a date.
11/69/2004 is succesfully parsed into a date.
11/70/2004 is not anymore succesfully parsed into a date.

Well, they both seem wrong dates to me, but I don't know how Mozilla should
handle those dates. But I think it should handle them the same.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2004-12-05 14:15:08 PST
Created attachment 167954 [details]
Testcase

This testcase shows that 11/69/2004 is succesfully parsed as a date, but
11/70/2004 not.
Comment 2 Hermann Schwab 2004-12-05 14:22:09 PST
*** Bug 272716 has been marked as a duplicate of this bug. ***
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-04-20 12:40:54 PDT
The url doesn't cause a freeze anymore, so probably something has changed on the
site. The testcase stills shows the initial cause of the issue, though.
Comment 4 Bob Clary [:bc:] 2005-04-20 13:23:33 PDT
The 11/70/2004 syntax error is due to:
<http://lxr.mozilla.org/mozilla/source/js/src/jsdate.c#624>. For "11/70/2004",
once |mon| is set, this block fails to check if mday needs to be set, and ends
up creating a date with |mon| == 10, |mday| == -1, |year| == 1970. Let's see if
I can create a patch. 
Comment 5 Bob Clary [:bc:] 2005-04-20 14:55:29 PDT
Created attachment 181352 [details] [diff] [review]
fix

simple fix. Added test.

/cvsroot/mozilla/js/tests/ecma_3/Date/15.9.3.2-1.js,v  <--  15.9.3.2-1.js
initial revision: 1.1

No regressions found in full test suite.
Comment 6 Brendan Eich [:brendan] 2005-04-20 18:50:41 PDT
Comment on attachment 181352 [details] [diff] [review]
fix

><HTML><HEAD><STYLE>u { text-decoration:none!important; font-style:italic!important; }</STYLE></HEAD><BODY><PRE>Index: jsdate.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsdate.c,v
>retrieving revision 3.65
>diff -p -u -6 -r3.65 jsdate.c
>--- jsdate.c	24 Dec 2004 00:03:58 -0000	3.65
>+++ jsdate.c	20 Apr 2005 20:34:20 -0000
>@@ -620,13 +620,15 @@ date_parseString(JSString *str, jsdouble
>                     n = -n;
>                 if (tzoffset != 0 && tzoffset != -1)
>                     goto syntax;
>                 tzoffset = n;
>             } else if (n >= 70 ||
>                        (prevc == '/' && mon >= 0 && mday >= 0 && year < 0)) {

So this condition seems fishy.	This function was based on bad old Gosling
java.util.Date code from 1995, and I haven't laid hands on the original to
compare, but it's interesting to note similar code in the GNU classpath
project's Date.parse:
http://savannah.gnu.org/cgi-bin/viewcvs/classpath/classpath/java/util/Date.java
?annotate=1.17.2.2 (starting at line 810).

Bob, if you are game, can you dig up a java.util.Date.java source and see what
it does nowadays?

Just from first principles, I would rather see the if condition fixed so that
if mday is not yet set, control bypasses the then clause and follows the
else-if chain till it finds the normal place that checks for (mday < 0) and
sets mday.

/be

>-                if (year >= 0)
>+                if (mday < 0)
>+                    mday = n;
>+                else if (year >= 0)
>                     goto syntax;
>                 else if (c <= ' ' || c == ',' || c == '/' || i >= limit)
>                     year = n < 100 ? n + 1900 : n;
>                 else
>                     goto syntax;
>             } else if (c == ':') {
Comment 7 Brendan Eich [:brendan] 2005-04-20 18:53:50 PDT
> Just from first principles, I would rather see the if condition fixed so that
> if mday is not yet set, control bypasses the then clause and follows the
> else-if chain till it finds the normal place that checks for (mday < 0) and
> sets mday.

So I didn't mean that this complicated (n >= 70 || ...) condition should test
mday >= 0 explicitly in both operands of the || operator.  The classpath code
instead conjoins some punctuator tests with (n >= 70) that may or may not have
the right effect in all cases.  But it does seem like a bad idea to base
decisions at this point in the random-coded state machine based only on the
magnitude of n.

/be
Comment 8 Nicholas Allen 2005-04-20 19:20:05 PDT
The code for java.util.Date.parse looks very much the same with identical
bustage.  However it's been deprecated since '96.  The replacement (DateFormat)
works correctly.  DateFormat is heavily internationalized and object-orientated
so overkill for this problem.
Comment 9 Brendan Eich [:brendan] 2005-04-20 19:33:31 PDT
Yeah, I know the sorry Java history (the one native object source file I didn't
write for the original "Mocha" runtime was modate.c, which a helper translated
from java.util.Date -- which seemed like a good idea at the time.  It really put
the "Java" in "JavaScript" ;-).

Still looking for the right (or a good-enough) spot-fix here.

/be
Comment 10 Bob Clary [:bc:] 2005-04-20 20:18:25 PDT
Original condition: 
(n >= 70 || (prevc == '/' && mon >= 0 && mday >= 0 && year < 0))

1) move mday outside:
(mday >= 0 && (n >= 70 || (prevc == '/' && mon >= 0 && year < 0))

2) move mday and mon outside:
(mday >= 0 && mon >= 0 && (n >= 70 || (prevc == '/' && year < 0))

1) seemed closest to what I had before but 2) seemed to make more sense since
mon would probably have the same issues. But running the test suite gives:

Testcase ecma_3/Date/15.9.3.2-1.js failed
[ Top of Page ]
STATUS: 15.9.3.2 new Date(value)
Failure messages were:
FAILED! expected: Section 8 of test - 70/70/70 is invalid.
FAILED! expected: Expected value 'true', Actual value 'false'
FAILED! expected:
FAILED! expected: Section 9 of test - 70/70/1970 is invalid.
FAILED! expected: Expected value 'true', Actual value 'false'
FAILED! expected:
FAILED! expected: Section 10 of test - 70/70/2004 is invalid.
FAILED! expected: Expected value 'true', Actual value 'false'
FAILED! expected:

which all passed in the original version of the "fix". Note that I made sections
8-9 as invalid because MSIE agreed. Seems like they almost "emulated" us bug for
bug. ;-) I wonder if the original "fix" isn't best for compatibility's sake. I
can add more tests to 15.9.3.2-1.js to map out the behavior of the "fix" vs. MSIE. 
Comment 11 Bob Clary [:bc:] 2005-04-20 20:27:50 PDT
s/8-9/8-10/, MSIE treats 8-10 as invalid dates.
Comment 12 Brendan Eich [:brendan] 2005-04-20 20:48:09 PDT
Comment on attachment 181352 [details] [diff] [review]
fix

Ok, thanks -- I'm convinced.  Nits about else-after-goto suppressed as that
seems to be prevailing style, although not consistently observed (even the
context shows a then-clause goto with no else).

/be
Comment 13 Brendan Eich [:brendan] 2005-04-20 22:05:07 PDT
Fixed.  Thanks, Bob!

/be
Comment 14 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-04-28 16:35:39 PDT
I forgot to verify. Works fine now. Great job!
Comment 15 Blake Kaplan (:mrbkap) (please use needinfo!) 2005-07-18 16:40:21 PDT
For the record: this checkin broke parsing of dates such as 2005/04/25 (which
turns into a date at some point in 1920 or so). I'm not sure if it's worth
fixing since this matches IE's behavior, but it did break the web app found at
http://trimpath.com/demos/nextaction_static1/nextaction.htm
Comment 16 Bob Clary [:bc:] 2005-07-22 01:33:51 PDT
(In reply to comment #15)
javascript:new%20Date('2005/04/25')

MSIE6:Mon Apr 25 00:00:00 EDT 2005
DeerPark: Fri Sep 26 1930 00:00:00 GMT-0500 (Eastern Standard Time)

Seems like a compatibility issue to me.
Comment 17 Brendan Eich [:brendan] 2005-07-22 09:24:09 PDT
File a followup bug, please -- bc or mrbkap, you willing to take it?

/be
Comment 18 Bob Clary [:bc:] 2005-07-22 09:46:00 PDT
Bug 301738 filed.
Comment 19 Bob Clary [:bc:] 2006-08-28 22:55:54 PDT
note to self:  ecma_3/Date/15.9.3.2-1.js, result: FAILED TIMED OUT, firefox_1.8.0.7pre_2006082816_dbg, linux/pear

Note You need to log in before you can comment on or make changes to this bug.