451906 - Regression: Array index has different results

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Steve Roussey (:sroussey)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: 

Indexing an array by number, if that number is then quoted, results in undefined, where all previous versions of Firefox (as well as IE, Safari, etc) implicitly convert the number string into a number for the index and return that part of the array.

Reproducible: Always

Steps to Reproduce:
var s=[1,2,3]
console.log(s['1')
Actual Results:  
undefined

Expected Results:  
2

Comment 2

[Brendan Eich [:brendan]]

This should block 1.9.1.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: fix

if (is_special_case) {
  special_case();
} else {
  normal_case();
}

when you want to split special_case() into a more special version of itself and a variation on normal_case() becomes

do {
  if (is_special_case) {
    if (extra_special) {
      special_case();
      break;
    }
    setup_variation_on_normal_case();
  }
  normal_case();
} while (0);

Maximum code sharing without goto.

/be

Comment 4

[Brendan Eich [:brendan]]

Want this in the next possible patch release too.

/be

Comment 5

[Mike Shaver (:shaver emeritus)]

Comment on attachment 335403 [details] [diff] [review]
fix

r=shaver; need test coverage here, for sure.

Comment 7

[Brendan Eich [:brendan]]

Fixed:

http://hg.mozilla.org/mozilla-central/index.cgi/rev/b11ad5053d28

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 335403 [details] [diff] [review]
fix

Can this make it for 1.9.0.2?

/be

Comment 9

[Brendan Eich [:brendan]]

bc: you should holler harder when we break stuff ;-).

Sam: this is covered by the js testsuite -- we just didn't act on the bug bc filed, due to heads-down TM hacking. This is a pretty bad regression, and the fix should go into the next Firefox 3.0.x.

/be

Comment 10

[Bob Clary [:bc] (inactive)]

actually, js1_5/Array/regress-107138.js didn't show a failure on 1.9.0 for me. :-(

I figured you saw my bug mail about the regressions on MC/TM. But I'll speak up more.

Comment 11

[Samuel Sidler (old account; do not CC)]

Comment on attachment 335403 [details] [diff] [review]
fix

Approved for 1.9.0.2. Please land in CVS ASAP (code freeze is tonight at midnight PDT). a=ss

Note: I'm approving this knowing that any new tests that need to be created for it will get created shortly by Bob.

Comment 12

[Brian Crowder]

Lands cleanly with no more than a name-change, so that's what I did:

Checking in jsparse.c;
/cvsroot/mozilla/js/src/jsparse.c,v  <--  jsparse.c
new revision: 3.349; previous revision: 3.348
done

Comment 13

[Bob Clary [:bc] (inactive)]

Steve, this was with Firebug correct? I can't reproduce this on mac or windows with Firefox 3.0.1 and Firebug 1.2.

Does anyone have a shell test that illustrates the bug on 1.9.0?

Comment 14

[Steve Roussey (:sroussey)]

Wasn't in 3.0.1 or I think a lot of people would have complained. ;) Was in Minefield when i discovered it (3.1a2pre of the bug report date).

Comment 15

[Bob Clary [:bc] (inactive)]

That wasn't clear from the build info in the original report. The regression on 1.9.1 is covered by the duped bug's test. Brendan, what did you fix on 1.9.0?

Comment 16

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Array/regress-451906.js,v  <--  regress-451906.js
initial revision: 1.1

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

Comment 17

[Bob Clary [:bc] (inactive)]

v 1.9.0, 1.9.1 1.9.2