308085 - JavaScript switch statement going to wrong case

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Henry Cejtin]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050825 Firefox/1.0.4 (Debian package 1.0.4-2sarge3)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

The attached html file, when run, first puts up an alert box showing 43 (which
is correct) but then, instead of putting up an alert showing CORRECT, it puts
up one showing BUG.  This indicates that the big switch statement, which has
cases for 0 through 244 as well as default, is going to the wrong place.

Note, as should be clear, the JavaScript code is machine generated.  It doesn't
take much of a change to cause the bug to go away (or atleast hide).

I marked this bug critical because clearly the JavaScript could do any thing,
including causing data to be lost.

Reproducible: Always

Steps to Reproduce:
1. load the html file
2. click ok on the first alert (showing 43)
3. observe the alert showing BUG

Actual Results:  
You see the BUG alert

Expected Results:  
You should see the CORRECT alert

Comment 1

[Henry Cejtin]

Attachment: Here is the html file that actually demonstrates the bug

Comment 2

[Bill Gianopoulos [:WG9s]]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1)
Gecko/20050911 Firefox/1.6a1 and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.9a1) Gecko/20050830 SeaMonkey/1.1a.

JavaScrpit looks OK to me.  Works as user expected in IE and Opera.

Comment 3

[OstGote!]

Works also with Mozilla 1.7.11. 

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Seeing as this is a regression from the 1.7 branch... blocking1.8b5?

Comment 5

[Brendan Eich [:brendan]]

I won't have time to look at this till later today, so mrbkap or shaver, feel
free to steal.

/be

Comment 6

[Igor Bukanov]

Attachment: Simpler test case for the shell

If one remove single line of 1; or add "default:" after "case 1:" in the test
case, then it printsexpected "CORRECT" instead of "BUG".

Comment 7

[Igor Bukanov]

The regression is caused by the fix for the bug 302439. Without attachment
190798 [details] [diff] [review] it works.

Comment 8

[Brendan Eich [:brendan]]

Attachment: fix the regression

This patch also fixes the bug that causes the code generator to emit bytecode
for all those useless '1;' statements.

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #8)
> This patch also fixes the bug that causes the code generator to emit bytecode
> for all those useless '1;' statements.

But only when the switch is in a function.  At top-level, the useless expression
eliminator is still lame, for fear of eliminating a wanted result communicated
to JS_EvaluateScript's caller via the *rval out param.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 196961 [details] [diff] [review]
fix the regression

Moving the null defense to the right place so we actually emit good bytecodes
sounds good to me!

r=mrbkap

Comment 11

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

Comment on attachment 196961 [details] [diff] [review]
fix the regression

sr=shaver

Comment 12

[Brendan Eich [:brendan]]

Attachment: fix checked in, good for 1.8 branch too

This is a regression in 1.8b4, new since Firefox 1.0 / Mozilla 1.7.

/be

Comment 13

[Brendan Eich [:brendan]]

Fixed on the trunk.

/be

Comment 14

[Brendan Eich [:brendan]]

This bug's fix caused bug 309695.

/be

Comment 15

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch, along with 309695.

/be

Comment 16

[Bob Clary [:bc] (inactive)]

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