Closed Bug 273292 Opened 20 years ago Closed 19 years ago

freeze/loop because Mozilla parses dates differently than IE

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: martijn.martijn, Unassigned)

References

(
URL
)

Details

Attachments

(2 files)

Testcase
339 bytes, text/html
Details
fix
977 bytes, patch
brendan
: review+
brendan
: approval1.8b2+
Details | Diff | Splinter Review 
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.
Attached file Testcase — 
This testcase shows that 11/69/2004 is succesfully parsed as a date, but
11/70/2004 not.
*** Bug 272716 has been marked as a duplicate of this bug. ***
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.
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.
Attached patch fix — — Splinter Review 
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
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
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. 
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
Closed: 19 years ago
Resolution: --- → FIXED
Flags: testcase+
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
(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
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.

Attachment

General

Creator:
Created:
Updated:
Size: