430717 - Dense arrays don't inherit deleted elements from Array.prototype

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Test case:

  Array.prototype[2] = "two";
  var x = [0, 1, 2, 3, 4, 5];
  delete x[2];
  if (x[2] != "two") throw "bug";

In trunk, x[2] === undefined.

Comment 1

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

La, la, la.  This is caused by using rval to store the sampled obj->dslots[i], which is then used to fill id -- making a jsid from JSVAL_HOLE is bad.

Patch coming.  Blocking: embarrassingly core language regression.

Comment 2

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

Attachment: reload rval if we have a hole

Comment 3

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

My stylistic choices, as I saw them:

A: load obj->dslots[i] twice, and hope the compiler doesn't mind

if (obj->dslots[i] != JSVAL_HOLE) {
    rval = obj->dslots[i];
    goto end_getelem;
}

B: add a temporary

C: just reload rval in the uncommon case, to avoid any penalty to the common case

I went with door C.

Thanks to crowder for recognizing instantly that (jsval)22 was JSVAL_HOLE; saved me a bunch of time.

Comment 4

[Mike Connor [:mconnor]]

Comment on attachment 317589 [details] [diff] [review]
reload rval if we have a hole

a=mconnor on behalf of 1.9 drivers

Comment 5

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

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.491; previous revision: 3.490
done

The test looks like:

Array.prototype[2] = "two";
var a = [0,1,2,3];
delete a[2];
assert(a[2] === "two");

bclary: where should the test for this go?  In with the ECMA tests for looking in prototype?  I couldn't find those at a glance, and wasn't 100% sure it was the right place anyway.

Comment 6

[Bob Clary [:bc] (inactive)]

The best location is debatable imo. The best option is to find the section of the specification describing the feature and place the test appropriately. Trying to tie a test to a specific location in the hierarchy is difficult especially considering that ecma/ == ecma 262-1st edition, ecma_2/ == ecma 262 2nd edition, etc and that the section names in the documents have changed between editions. 

I normally don't worry too much about the purity of the final location of the test and in this case would have probably chosen ecma_3/Array, but if the inheritance is more important then js1_3/inherit would be a better location. I prefer not to put anything in js1_3 though unless you have a gun pointed at me. 

I prefer ecma_3 over earlier ecma editions as the older tests have a degree of funkiness which I do not care for. I prefer js1_5 over lower js versions (unless a feature requiring a higher version is required) for the same reason and might have chosen js1_5/Array over ecma_3/Array.

Comment 7

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/Array/regress-430717.js

I'll check in when the tree reopens

Comment 8

[Bob Clary [:bc] (inactive)]

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

Comment 9

[Bob Clary [:bc] (inactive)]

v 1.9.0