98901 - mozilla crashes on http://www.sony.com/productregistration - N620 [@js_emit] [@ js_EmitTree]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Mike Palczewski]

In the location bar type the url and hit enter mozilla will soon crash.

This is mozilla 0.9.3 on linux.

Comment 1

[Ryan McLoughlin]

Also happens for me on Win95 on build 2001 09 04 08.

MOZILLA caused a stack fault in module JS3250.DLL at 0157:60d001c0.
Registers:
EAX=00000013 CS=0157 EIP=60d001c0 EFLGS=00010212
EBX=0eb52900 SS=015f ESP=00591f58 EBP=00592460
ECX=00000000 DS=015f ESI=0068ee74 FS=124f
EDX=00000000 ES=015f EDI=0fdc9260 GS=0000
Bytes at CS:EIP:
53 56 8b 75 0c 8b 5d 10 57 8b 7d 08 8b 4e 40 8b 
Stack dump:
00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000 00000000 00000000 00000000 00000000 

Comment 2

[Matthias Versen [:Matti]]

win2k build 20010905 (cvs debug)

-> JS Engine

A part of the stack : 
This lines are 100x reapeated and MSC++ reports a stack overflow,

js_EmitTree(JSContext * 0x03757218, JSCodeGenerator * 0x0012ec78, JSParseNode * 
0x042c4570) line 924 + 5 bytes
js_EmitTree(JSContext * 0x03757218, JSCodeGenerator * 0x0012ec78, JSParseNode * 
0x042c45d0) line 2444 + 20 bytes
js_EmitTree(JSContext * 0x03757218, JSCodeGenerator * 0x0012ec78, JSParseNode * 
0x042c4958) line 2444 + 20 bytes

Comment 3

[Matthias Versen [:Matti]]

Attachment: Full Stack Trace

Comment 4

[André Dahlqvist]

Attachment: Testcase. This is the file that crashes mozilla.

Comment 5

[Phil Schwartau]

The code defines a lot of variables:

EMAL="";
PASS="";
PASV="";
FNAM="";
LNAM="";
ADD1="";
ADD2="";
CITY="";
STAT="";
STATi=0;
ZIPC="";

  etc.
  etc.


And then a function to concatenate them all:


function formData() {
 age_header="19";
 if (isNaN(AGES)||AGES.length==0) {age_header="";}
 data="";

 data+="&UNID="+UNID
+"&DIstat="+DI_Reg_Code
+"&EMAL="+EMAL
+"&PASS="+PASS
+"&PASV="+PASV
+"&FNAM="+FNAM
+"&LNAM="+LNAM
+"&ADD1="+ADD1
+"&ADD2="+ADD2
+"&CITY="+CITY
+"&STAT="+STAT
+"&ZIPC="+ZIPC

      etc. 
      etc.


}


Result: stack overflow. cc'ing Brendan for advice. 
Is this a duplicate of one of these bugs?

bug 56940  O(n**2) and O(n**3) growth too easy with JS string concat
bug 80981  Need extended jump bytecode to avoid "script too large" errors, etc.


Or is this bug a separate issue? Thanks - 
Note: NN4.7, IE4.7 have no trouble with this site -

Comment 6

[Mike Palczewski]

Netscape 4.7 under linux does crash on this page.

Comment 7

[Phil Schwartau]

Mike is right - I have completely different experiences with this
site on my Linux box vs. on my WinNT, Mac:

NN4.7 WinNT --> loads the site in less than 1 second
NN4.7 Mac 9 --> loads the site in less than 1 second
NN4.7 Linux --> wait, wait, CRASH


Here's what happens with Mozilla trunk builds 20010909xx:

Moz WinNT  --> CRASH
Moz Mac 9  --> loads the site in about 1 second
Moz Linux  --> CRASH

Comment 8

[Brendan Eich [:brendan]]

Simple stack overflow due to incredibly unmaintainable, unreadable, etc. program
structure (yeah, I'm blaming the customer), or (to blame the engine) due to KISS
use of recursion in the JS engine's code generator.

Phil, this bug is not related to the "string concat is O(bad)" and "SpiderMonkey
needs an extended jump bytecode" bugs.

/be

Comment 9

[Phil Schwartau]

*** Bug 106046 has been marked as a duplicate of this bug. ***

Comment 10

[Phil Schwartau]

Copying keywords and status whiteboard from duped bug 106046 

Comment 11

[chris hofmann]

brendan> Simple stack overflow due to incredibly unmaintainable, unreadable, 
brendan> etc. program structure (yeah, I'm blaming the customer)

so, should we evangelize the site?

Comment 12

[bobj]

And evangelize the other sites mentioned in bug xxx?:

> talkback shows quite a few frustrated sony customers trying to register and
> also a handfull of other sites that generate this crash.
>
>
http://danseider.com/bin/web/real_estate?acnt=AR38351&ZKEY=&action=ACTIVATE_FRAMES&linkout=%2Fbin%2Fweb%2Freal_estate%3FZKEY%3D%26acnt%3DAR38351%26action%3DHOME_SEARCH%26hs_action%3DVIEW_DETAIL%26listing_id%3DREAA1577122
>
>
http://www.neopets.com/games/pyramids/pyramids.phtml?action=play&position=6.neopets.com/games/pyramids/pyramids.phtml?action=play&position=6
>
> http://www.angelfire.com/ct2/yalesville/officers.htm
>
> http://www.sagu.edu/sde
>
> http://www.kobra.ktu.lt/~vytis

Also, the Sony site works with IE5.5 on Win95.

Comment 13

[bobj]

FYI, I looked at this:

data+="&UNID="+UNID
+"&DIstat="+DI_Reg_Code
+"&EMAL="+EMAL
+"&PASS="+PASS
+"&PASV="+PASV
+"&FNAM="+FNAM
+"&LNAM="+LNAM
+"&ADD1="+ADD1
+"&ADD2="+ADD2
+"&CITY="+CITY
+"&STAT="+STAT
+"&ZIPC="+ZIPC

      etc. 
      etc.

and it goes on for 1654 lines.

Comment 14

[Arun Ranganathan]

We can try, in parallel, to tell Sony* to make their code better and less
sloppy.  But this is absolutely a product bug and not merely an evangelism bug,
so this bug ought to remain open and assigned.  Evangelism will not alter the
fact that this web page *crashes* the browser.  Yes, I agree that the JavaScript
in it is ugly and ought to be cleaned up, but the code is not guilty of anything
other than ugliness.  The browser ought to fail gracefully, and not crash.  A
crash is a sign of something suspect in the JS Engine.
Does anyone here *really* think that the task of finding someone to talk to at
Sony and telling them that their web page crashes our browser and not any other
browser is likely to win us any respect, along with the requisite code change?

Comment 15

[bobj]

I agree that it doesn't seem right to evangelize changing poor bug correct
code because we crash.  Any user hitting one of these pages crashes -- much
worse than the page just not working.

Looking at the attached stack trace, it appears we recursively called 
js_EmitTree() 596 times before crashing.  I don't know if that is a reasonable
limitation even if we degraded more gracefully.

Comment 16

[Jay Patel [:jay]]

Adding N620 and  [@ js_EmitTree] to summary, since this has been a topcrasher on
recent branch builds.

Comment 17

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 18

[Brendan Eich [:brendan]]

I'll take this in exchange for an r= in return.  Patch comments:

(1) The main problem here is the stack consumed by recursive js_EmitTree, which
exceeds the parallel recursion that the parser's various functions go through. 
Recursion in the parser could blow the stack too, but we haven't seen bugs
there, so to fix just the main problem, this patch flattens left-heavy trees of
like binary operators into a list.  It does this during constant folding, to
allow 1+2 and "hi "+"there" to be combined at compile time.  This change
required adding a  left-associative operator format flag.

(2) While testing, I noticed that the js shell was compiling twice, when it
should have been parsing, then compiling once it had assembled a compileable
unit in a single buffer that parsed.  Fixing that entailed making the jsparse.c
friend method js_ParseTokenStream work when called from elsewhere in the engine
without knowledge of its internals (its need to disable the GC).
/be

Comment 19

[Brendan Eich [:brendan]]

Comment on attachment 55155 [details] [diff] [review]
proposed fix

The main fix is good, but I didn't minimize mccabe's code in JS_BufferIsCompilableUnit enough
(hitEOF is singly-used, doubly-set; pn is singly-used; both must die).

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 55155 [details] [diff] [review]
proposed fix

The main fix is good, but I didn't minimize mccabe's code in JS_BufferIsCompilableUnit enough
(hitEOF is singly-used, doubly-set; pn is singly-used; both must die).

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: proposed fix, with side order of cleanup

Comment 22

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 55161 [details] [diff] [review]
proposed fix, with side order of cleanup

Yeah, that's what I was hoping to see. sr=shaver

Comment 23

[John Bandhauer]

Comment on attachment 55161 [details] [diff] [review]
proposed fix, with side order of cleanup

r=jband
I *thought* I marked this reviewed yesterday!

Comment 24

[Brendan Eich [:brendan]]

Fix is in, thanks for the reviews.

/be

Comment 25

[Phil Schwartau]

Using Mozilla trunk binaries 20011030xx on WinNT, Linux, Mac9.1. 
The given URL loads fine every time:

          http://www.sony.com/productregistration


The URL from duped bug 106047 loads fine on my WinNT and Linux boxes,
but crashes my Mac9.1 build:

          http://www.sony.com/productregistration/indexl.html


Before I reopen the other bug, however, I need to check with Mac developers
to see if my memory allocations are correct. On the same box, NN4.7 also
crashes on this URL.

I would also like to ask bobj and others what their experiences
are like at each of these sites now -

Comment 26

[Phil Schwartau]

Typo: that's duped bug 106046 
(not 106047)

Comment 27

[Phil Schwartau]

Marking Verified - 

Comment 28

[John Bandhauer]

*** Bug 129287 has been marked as a duplicate of this bug. ***

Comment 29

[Susie Wyshak]

please make sure this is embedded...

Comment 30

[Phil Schwartau]

Susie: what is the exact branch name to check in CVS?

Comment 31

[Phil Schwartau]

i.e. is it "MOZILLA_0_9_9_BRANCH" we should check in CVS?

Comment 32

[chris hofmann]

won't need this on the 0.9.9 branch if its not there already.. (looks like it
might be if fix was checked in before march 02).

Comment 33

[bmartin]

I verified that http://www.sony.com/productregistration loads without crashing
within the latest mfcEmbed 2002-04-03-23-099ec  ( embed-win32.zip located at
ftp://ftp.mozilla.org/pub/mozilla/nightly/2002-04-03-23-0.9.9ec/ )

Comment 34

[Bob Clary [:bc] (inactive)]

Checking in regress-98901.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-98901.js,v  <--  regress-98901.js
initial revision: 1.1
done