83532 - Mozilla crashes immediately on loading page [@ RecycleTree]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[GAThrawn]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9+) Gecko/20010529
BuildID:    2001052904

Type the above URI into the URI bar, press enter.
Mozilla is redirected to http://www.mydigiguide.com/dgx/wbl.dll it starts to
render page background colours and then immediately crashes with the following
Windows error message:

"MOZILLA caused an invalid page fault in
module JS3250.DLL at 017f:60cc9b35."

Haven't visited the site for a while, but this used to work fine in 0.8.1 and
works fine in IE and NS4 but has been reproducable in Moz builds from the last 4
days or so. Have tried using fresh, blank profile but the same happens.

Talkback reports have been set, most recent is: TB31155221E

Reproducible: Always

Comment 1

[Matthias Versen [:Matti]]

confirming with win2k build 20010530.. (CVS opt)
I will try to get a stack (must update my debug build)

Comment 2

[Matthias Versen [:Matti]]

Stack from my opt build (my debug doesn´t crash !)

JS3250! 00d89b35()
JS3250! 00d895b9()
JS3250! 00d63ec1()
JS3250! 00d63e6f()
JS3250! 00d64605()
JSDOM! 00e129c5()
GKCONTENT! 00e6dda0()
GKCONTENT! 00e6db4b()
GKCONTENT! 00e6e517()
NECKO! 00cf75d5()
NECKO! 00cfd702()
NECKO! 00cfc6b2()
XPCOM! 1002a66c()
NTDLL! 778b0c24()

Comment 3

[Asa Dotzler [:asa]]

from talkback:

 Call Stack:     (Signature = RecycleTree 87f17709)
     RecycleTree [d:\builds\seamonkey\mozilla\js\src\jsparse.c, line 202]
     0x81d7adf8  
     js_CompileTokenStream [d:\builds\seamonkey\mozilla\js\src\jsparse.c, line 392]
     CompileTokenStream [d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 2786]
     JS_CompileUCScriptForPrincipals
[d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 2864]
     JS_EvaluateUCScriptForPrincipals
[d:\builds\seamonkey\mozilla\js\src\jsapi.c, line 3271]
     nsJSContext::EvaluateString
[d:\builds\seamonkey\mozilla\dom\src\base\nsJSEnvironment.cpp, line 604]
     nsScriptLoader::EvaluateScript
[d:\builds\seamonkey\mozilla\content\base\src\nsScriptLoader.cpp, line 565]
     nsScriptLoader::ProcessRequest
[d:\builds\seamonkey\mozilla\content\base\src\nsScriptLoader.cpp, line 478]
     nsScriptLoader::OnStreamComplete
[d:\builds\seamonkey\mozilla\content\base\src\nsScriptLoader.cpp, line 756]
     nsStreamLoader::OnStopRequest
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsStreamLoader.cpp, line 122]
     nsStreamListenerTee::OnStopRequest
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsStreamListenerTee.cpp, line 25]
     nsHttpChannel::OnStopRequest
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpChannel.cpp, line 2056]
     nsOnStopRequestEvent::HandleEvent
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsRequestObserverProxy.cpp, line 159]
     PL_HandleEvent [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 591]
     PL_ProcessPendingEvents
[d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c, line 524]
     _md_EventReceiverProc [d:\builds\seamonkey\mozilla\xpcom\threads\plevent.c,
line 1072]
     KERNEL32.DLL + 0x24407 (0xbff94407)  
     0x00688b5e  
 

Comment 4

[Phil Schwartau]

When I load this site in a Mozilla debug build (2001-05-31 WinNT),
I do not crash. I see errors like this in the JavaScript Console:

   Error: invalid return
   Source File: http://www.mydigiguide.com/dgx/wbl.dll
   Line: 2
   Source Code:
   return fnHighlight( this, 1 );



The source for this in the (oddly-named) wbl.dll file is:


<script language="JavaScript" type="text/javascript" 
                                    event="onmouseover" for="NavLink">
 
 return fnHighlight( this, 1 );

</script>


The "event" and "for" attributes in <SCRIPT> tags are IE-only,
as far as I know, although MS claims they are DOM 2 spec:

http://msdn.microsoft.com/workshop/author/dhtml/reference/properties/event.asp

Comment 5

[Phil Schwartau]

Recent WinNT nightly binaries all crash on loading this site.
However, Linux nightly binaries from the same dates do NOT crash.

On either platform, debug builds (2001-05-31) do NOT crash.
This is making it impossible for me to get a good stack trace.

cc'ing jst, jband for help here. Is there anything in the 
Talkback trace at 2001-05-31 15:42 above that offers a clue?
Do you think this has anything to do with the non-standard
<SCRIPT> tag in use here? Do you think this is JS Engine? 

Thanks -

Comment 6

[John Bandhauer]

This crashed for me in my NT "non-debug with symbols" build showing the same 
stack. It was actually working on a .js file at the crash point (the name of 
which I don't have handy). I telnet'd the .js file and it looked normal enough 
in the place where the scanner seemed to be pointing. The locals and params in 
the top couple of frames were completly whacky. I don't know if this reflected 
reality or the debugger's problems with non-debug builds. I'm trying to run it 
under Purify.

Comment 7

[John Bandhauer]

Well. This reduces to:

  function f(i){switch(i){case -1: case 0:}}

Compiling that line in a release build will crash. It compiles without complain 
in a debug build.

I'll attach a stack from the crash of this in xpcshell (same as if you stick it 
in <script> tags and use the browser).

The stack shows some pretty unlikely stuff. the param values are unlikely *and* 
the lines it shows don't really lineup with calls to the functions it claims to 
be running. Again, it is hard to tell if the debugger is lying or if the 
compiler did some very odd things in generating the x86 code.

I'll leave it to you JS compiler jocks to work this out. I'd have to speculate 
that the -1 and non-negative values are screwing up decisions about using a  
tableswitch v. lookupswitch or the tableswitch code just can't handle that value 
or some such. Or maybe there is something wrong with the optimized code? I 
dunno.

Comment 8

[John Bandhauer]

Attachment: xpcshell non-debug NT crash stack for simple testcase

Comment 9

[rogerl (gone)]

Mea Culpa since I reviewed this patch (#74474). The code to allocate a bitmap 
for duplicate case testing messes up if the first case is outside the range of 
the stack-allocated smaller bitmap. I don't have a build to test this on, could 
someone try this patch and see if it works?

Comment 10

[Phil Schwartau]

Mozilla also crashes on this:

<SCRIPT>
function f(){switch(1){case -1:}}
</SCRIPT>

but does NOT crash on this:

<SCRIPT>
var f = function(){switch(1){case -1:}}
</SCRIPT>

Comment 11

[Phil Schwartau]

Moz does not crash if we parenthesize the -1 in the above example:

<SCRIPT>
function f(){switch(1){case(-1):}}
</SCRIPT>

Does that make any sense?

Comment 12

[rogerl (gone)]

Attachment: hopeful fix

Comment 13

[Phil Schwartau]

It has been explained to me that when there is a bad bit being set,
whether you crash or not depends on the contextual parsing output and stack.
Setting a bad bit that doesn't matter would hide the the problem completely.
Or resetting a bit that's already been set would also mask the problem. 

This explains the variations I was seeing above - 

Comment 14

[Phil Schwartau]

Testcase added to JS test suite:

        js/tests/ecma_3/Statements/regress-83532-001.js

FWIW, I've never been able to crash on this in the JS shell.
Only in the browser - 

Comment 15

[Phil Schwartau]

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

Comment 16

[Jay Patel [:jay]]

Here's a few urls I found in the Talkback data in case you needed more testcases: 

- http://www.themarker.com/
- www.myspace.com.
- http://www.oddworld.com
- http://www.mydigiguide.com/dgx/wbl.dll
- www.themarker.co.il

Comment 17

[Phil Schwartau]

cc'ing beard, rginda; adding review keyword - 

Comment 18

[Phil Schwartau]

Attachment: Reduced HTML testcase

Comment 19

[Phil Schwartau]

If you try to load the testcase with a recent Mozilla nightly, 
you will crash immediately. 

If you load it with a Mozilla debug build or with NN4.x, you
load successfully. The page is deliberately blank. 

Comment 20

[Phil Schwartau]

NOTE: the HTML testcase above only seems to crash for me on my WinNT box.
      On my Linux box, it loads fine - 

Comment 21

[Phil Schwartau]

Another testcase added to JS test suite:

        js/tests/ecma_3/Statements/regress-83532-002.js

Unlike the first one above, this one crashes in the (optimized) JS shell.

JS32! js_Invoke + 1454 bytes
JS32! js_Interpret + 21517 bytes
JS32! js_Execute + 338 bytes
JS32! js_obj_toString + 706 bytes
JS32! js_Invoke + 985 bytes
JS32! js_Interpret + 21777 bytes
JS32! js_Interpret + 21777 bytes

Comment 22

[Phil Schwartau]

And again, FWIW, this crash only happens for me on WinNT, not on Linux.

Comment 23

[Phil Schwartau]

With patch id=36850 above applied, I no longer crash on WinNT in the 
optimized JS shell on js/tests/ecma_3/Statements/regress-83532-002.js

I have run the entire JS testsuite with this patch applied, and 
observed no regressions in either the debug or optimized JS engine -

Comment 24

[Phil Schwartau]

cc'ing shaver, as in the related bug 74474 -

Comment 25

[Brendan Eich [:brendan]]

Attachment: fix

Comment 26

[Brendan Eich [:brendan]]

Sorry about that (I would've gladly taken this bug; guess r= guilt spared me
:-).  The hopeful fix patch should has the right fix, but it also has a change
that seems wrong to me:

                 if (i < 0)
-                    i += JS_BIT(15);
+                    i += JS_BIT(16);

Code above this point has ensured that i is in [-32768,32767], and the i += ...
statement here is merely trying to translate i into non-negative domain.  Adding
65536 will overflow the worst-case bitmap if the case label is >= 0.

My "fix" patch does not change this line.  It does include the essential fix
that rogerl came up with, to test i against the stack-allocated bitmap's length
before just assuming it will fit.  How about quick testing, r=, sr=, and a=
love?  Thanks,

/be

Comment 27

[Brendan Eich [:brendan]]

s/should has/does have/

(/me cleans his glasses and rewires his typing neurons)

/be

Comment 28

[John Bandhauer]

I have the uncomfortable feeling that I'm on the hook for the sr chores here. 
I'll have to puzzle this out. It would be good if rogerl and brendan would come 
to an agreement about the JS_BIT(15) v. JS_BIT(16) issue to spare the 
uninitiated the anguish of working out this issue too.

Comment 29

[rogerl (gone)]

So, my thought was that if i is in [-32768,32767], then "if (i < 0) i+= 65536"
would result in i in [0,65536]. ?

Comment 30

[rogerl (gone)]

umm, s/[0,65536]/[0,65535]/

Comment 31

[Phil Schwartau]

With Brendan's patch (id=37258) applied, I have the same findings as with
Roger's patch: I no longer crash on WinNT in the optimized JS shell on 
js/tests/ecma_3/Statements/regress-83532-002.js

I have run the entire JS testsuite with this patch applied, and 
observed no regressions in either the debug or optimized JS engine.
I did this on both WinNT and Linux - 

Comment 32

[Brendan Eich [:brendan]]

>So, my thought was that if i is in [-32768,32767], then "if (i < 0) i+= 65536"
>would result in i in [0,65536]. ?

No, it results in [32768,98303].

/be

Comment 33

[John Bandhauer]

sr=jband on brendan's patch. I did figure out more about how this works and play 
with it in the debugger and under Purify with various values. It looks right. 
One nit... The comment about negative values requiring a malloc'd bitmap is a 
little misleading since negatives on the low end of the range (e.g. -32768) end 
up looking like small non-negative numbers. Then the issue is that they *may* 
end up colliding with actual small numbers thus requiring a lookupswitch. AFAICT 
those mechaisms work right. It might be worth pointing out in the comment that 
this is OK. I can see that the with more 'normal' mixes of negative and 
non-negative clustered around zero the code does require the malloc'd bitmap but 
does not need to fallback on a lookupswitch. I'm sure this is as intended and so 
justifies the cost of the malloc.

Comment 34

[chris hofmann]

is this 0.9.1 fodder if we try and respin?
who would put the changes on the branch?

Comment 35

[rogerl (gone)]

with i in [-32768,32767], then "if (i < 0) i += 65536"

let's look at some cases:
i = -32768 -->      i + 65536 = 32768
i = -1     -->      i + 65536 = 65535
i =  0     -->      i + 0     = 0
i = 32767  -->      i + 0     = 32767

therefore the result range is [0,65535]

Comment 36

[selmer (gone)]

Chofmann says Brendan has the fix for this.  Please check it in for the respin.

Comment 37

[Brendan Eich [:brendan]]

rogerl: right, sorry I was confused.  I like your patch better, because while it
costs a malloc for the bitmap for small negative constant case labels, it avoids
false dups, so still selects JSOP_TABLESWITCH.

jband: this seems better to me than making a switch with small negative and
non-negative constant case labels select JSOP_LOOKUPSWITCH.

I'm checking the full fix into branch and trunk, a=chofmann.

/be

Comment 38

[Brendan Eich [:brendan]]

Fix in branch, waiting for trunk to unblock.

/be

Comment 39

[Brendan Eich [:brendan]]

Fix is in trunk, too.

/be

Comment 40

[Phil Schwartau]

VERIFIED FIXED using trunk and branch0.9.1 nightly binaries 20010607xx
on WinNT, Linux, and Mac. I tried the given URL, the reduced HTML testcase,
and Jay's URLS at 2001-06-04 16:14 above. No crash on loading any of these -