Fixed Array.prototype.reverse() behavior when array is frozen or array elements are not writable an…

Fixed Array.prototype.reverse() behavior when array is frozen or array elements are not writable and configurable.


BUG=v8:1992
TEST=mjsunit/sparse-array-reverse

[ioseb, 2012-05-24 18:19:38]

Functionality is aligned to those found in recent stable version of FF and
WebKit nightly. 

If patch is ok, i volunteer to fix other methods as well i.e. sort(), push(),
pop() etc... these methods need to be fixed same they too. I already implemented
several reusable methods for this.

[Michael Starzinger, 2012-05-25 07:53:10]

This patch is functionally correct, but it looks like it is introducing a huge
overhead into the inner loop of Array.prototype.reverse() using
Object.getOwnPropertyDescriptor() is too expensive for these checks. We need to
find a more efficient way.

[ioseb, 2012-05-25 08:08:56]

On 2012/05/25 07:53:10, Michael Starzinger wrote:
> This patch is functionally correct, but it looks like it is introducing a huge
> overhead into the inner loop of Array.prototype.reverse() using
> Object.getOwnPropertyDescriptor() is too expensive for these checks. We need
to
> find a more efficient way.

Are there known ways to improve this? I tried to find already implemented
functionality for descriptor retrieval and found only this:
ObjectGetOwnPropertyDescriptor()

I'll look inside again, maybe i missed something and it's possible to do same
thing in more efficient manner.

[Michael Starzinger, 2012-05-25 08:48:10]

I don't think V8 has support for fast access of property attributes at the
moment. But I have the feeling that duplicating all of the attribute checks in
the builtin functions is the wrong approach since we are able do them in the ICs
already.

So for this particular instance you can just switch Array.prototype.reverse into
strict mode and get the desired behavior. But I don't want to land a change that
switches just Array.prototype.reverse into strict mode before we thought through
all affected builtin functions first.

Also it's not trivial to just switch all our builtin functions into strict mode.
See the following issue for details:

http://code.google.com/p/v8/issues/detail?id=1955

[ioseb, 2012-05-25 09:27:40]

I agree, this doesn't look like a solid solution. 

> But I don't want to land a change that switches just Array.prototype.reverse
into strict mode before we thought through all affected builtin functions first.

We have following mutator methods on array object: pop(), push(), shift(),
unshift(), splice(), reverse() and sort()

As I identified(maybe there are some other details) only reverse() and sort()
require element level lookup for property descriptors (frozen state check is
same as for all other mutator methods).

All other methods throw TypeErrors when array is frozen. See examples below:

// initialize and freeze array
var a = [1, 2, 3, 4];
Object.freeze(a);

// Array.prototype.pop
// FF TypeError: property a.pop() is non-configurable and can't be deleted
// WebKit TypeError: Attempted to assign to readonly property.
a.pop();

// Array.prototype.push
// FF TypeError: a.push(0) is not extensible
// WebKit TypeError: Attempted to assign to readonly property.
a.push(0); 

// Array.prototype.shift
// FF TypeError: a.shift() is read-only
// WebKit TypeError: Attempted to assign to readonly property.
a.shift();

// Array.prototype.unshift
// FF TypeError: a.unshift(1) is not extensible
// WebKit TypeError: Attempted to assign to readonly property.
a.unshift(1); 

// Array.prototype.splice
// FF TypeError: a.splice(0, 1) is read-only
// WebKit TypeError: Attempted to assign to readonly property.
a.splice(0, 1);

[Michael Starzinger, 2012-07-03 12:57:09]

Code Review Spring Cleaning: Since we agreed that this particular fix is not
efficient enough, I am closing this CL now.