freeze/loop because Mozilla parses dates differently than IE

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
13 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Unassigned)

Tracking

Trunk
x86
Windows XP
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
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

13 years ago
*** Bug 272716 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 3

13 years ago
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

13 years ago
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

13 years ago
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.
Attachment #181352 - Flags: review?(brendan)
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 == ':') {
> 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

13 years ago
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.
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

13 years ago
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

13 years ago
s/8-9/8-10/, MSIE treats 8-10 as invalid dates.
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
Attachment #181352 - Flags: review?(brendan)
Attachment #181352 - Flags: review+
Attachment #181352 - Flags: approval1.8b2+
Fixed.  Thanks, Bob!

/be
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

13 years ago
Flags: testcase+
(Reporter)

Comment 14

13 years ago
I forgot to verify. Works fine now. Great job!
Status: RESOLVED → VERIFIED
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

12 years ago
(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.
File a followup bug, please -- bc or mrbkap, you willing to take it?

/be

Comment 18

12 years ago
Bug 301738 filed.

Comment 19

11 years ago
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
You need to log in before you can comment on or make changes to this bug.