444979 - switch statement considers -0 to be different from 0

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[László Jánszky]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; hu; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; hu; rv:1.9) Gecko/2008052906 Firefox/3.0

When you are trying to process with a switch statement -0, then case 0 doesn't works, so -0 is not equal to 0 in the switch.


Reproducible: Always

Steps to Reproduce:
<script>
var y=-0;
switch (y)
{
 case 0:
  alert("y=0")
  break;
 default:
  alert("y!=0")
}
</script>
Actual Results:  
alerts "y!=0"

Expected Results:  
alerts "y=0"

Comment 1

[Igor Bukanov]

This is a bug in the implementation of optimized JSOP_TABLESWITCH/JSOP_LOOKUPSWITCH bytecodes. Adding for example, "case Math:" right before the default: label will give the "y==0"alert.

Comment 2

[Igor Bukanov]

The bug is not a regression: the switch code simply has never accounted for the possibility of -O. Few initial revisions in CVS from 1998 of jsinterp.c do not have the bug, but that is because they effectively implemented == semantics, not ===, when comparing the switch value against the case labels. When the code was added on 1998-10-14 implementing the ECMA-specified === logic, it missed that -0 === 0.

Comment 3

[Andrei Saprykin]

Attachment: Fix

The patch fixes the following test code (that is a modification of the original test case provided in the bug comments):

var shortSwitch = 'var y=-0;switch (y){case 0:  print("y=0");  break; default:  print("y!=0")}';
eval(shortSwitch);
print("Short switch completed");
var longSwitch = 'var y=-0;var t=0;switch(y){case -1:';
for (var i = 0; i < 64000; i++) {
	longSwitch += ' t++;';
}
longSwitch += ' break; case 0: print("y=0"); break; default: print("y!=0")}'
eval(longSwitch);
print("Long switch completed");

with the patch applied the output of the test code is
y=0
Short switch completed
y=0
Long switch completed
This effectively means the JS value -0 falls into the switch case 0.

Comment 4

[Igor Bukanov]

Comment on attachment 339960 [details] [diff] [review]
Fix

>diff -r 15aecdaab936 js/src/jsinterp.cpp
>             rval = POP_OPND();
>-            if (!JSVAL_IS_INT(rval))
>-                DO_NEXT_OP(len);
>-            i = JSVAL_TO_INT(rval);
>+            if (!JSVAL_IS_INT(rval)&&!JSVAL_IS_DOUBLE(rval))
>+                DO_NEXT_OP(len);
>+            if (JSVAL_IS_INT(rval)) {
>+                i = JSVAL_TO_INT(rval);
>+            } else {
>+                if (*(JSVAL_TO_DOUBLE(rval)) == 0) {
>+                    i = 0;
>+                } else {
>+                    DO_NEXT_OP(len);
>+                }
>+            }

The code here uss JSVAL_IS_INT(rval) twice. Although a good compiler would be able to optimize that, it is important not to rely on that and ensure the optimal performance even for the price of extra source complexity. So the suggestion is to move checking JSVAL_IS_DOUBLE(rval) to the else part.

Comment 5

[Andrei Saprykin]

Attachment: Updated fix

Updated according to Igor's comments

Comment 6

[Andrei Saprykin]

Attachment: Updated fix

Comment 7

[Andrei Saprykin]

Attachment: Updated fix

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 340148 [details] [diff] [review]
Updated fix

>+            } else if (JSVAL_IS_DOUBLE(rval) && *(JSVAL_TO_DOUBLE(rval)) == 0) {

Drive-by nit: don't overparenthesize operand of unary-*.

/be

Comment 9

[Andrei Saprykin]

Attachment: Updated fix

Comment 10

[Igor Bukanov]

landed - http://hg.mozilla.org/mozilla-central/rev/5c76e04f1354

Comment 11

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/ecma_3/Statements/regress-444979.js,v  <--  regress-444979.js
initial revision: 1.1

http://hg.mozilla.org/mozilla-central/rev/f0e9fd501e63

Comment 12

[Bob Clary [:bc] (inactive)]

v 1.9.1