Handle changes to the Object prototype in fast handling of arrays

Handle changes to the Object prototype in fast handling of arrays

R=ager@chromium.org

BUG=v8:1403
TEST=test/mjsunit/regress/regress-1403.js

Committed: http://code.google.com/p/v8/source/detail?r=8032

[Mads Ager (chromium), 2011-05-24 08:59:49]

LGTM, Anton, could you double check that the behavior will be correct with this
change?

[antonm, 2011-05-24 10:38:20]

LGTM too.

http://codereview.chromium.org/7067019/diff/1/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/7067019/diff/1/src/builtins.cc#newcode369
src/builtins.cc:369: // This method depends on non writability of Object and
Array prototype
Sigh, but this comment should now go away.  I wonder why I thought that way,
probably it's ECMAScript 5 which doesn't allow to play with protos.  And we
cannot make them not-writable for bug-compatibility I gather :(

http://codereview.chromium.org/7067019/diff/1/src/builtins.cc#newcode379
src/builtins.cc:379: if (array_proto !=
global_context->initial_object_prototype()) return false;
while you're here, may you swap these two lines?  I am concerned with the
following case: Array.prototype.__proto__ = 2;

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
File test/mjsunit/regress/regress-1403.js (right):

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
test/mjsunit/regress/regress-1403.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=1403
do we have similar test case when Array.prototype.__proto__ is mutated?

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
test/mjsunit/regress/regress-1403.js:30: a = new Array();
nit: a = [] might be easier to read

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
test/mjsunit/regress/regress-1403.js:33: Object.prototype.__proto__ = p;
nit: maybe (for compactness): Object.prototype.__proto__ = { __proto__: null };

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
test/mjsunit/regress/regress-1403.js:34: a.shift();
I would consider adding indexed properties on Object.prototype.__proto__ to
check they are handled correctly.

[Søren Thygesen Gjesse, 2011-05-24 12:25:52]

http://codereview.chromium.org/7067019/diff/1/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/7067019/diff/1/src/builtins.cc#newcode369
src/builtins.cc:369: // This method depends on non writability of Object and
Array prototype
On 2011/05/24 10:38:20, antonm wrote:
> Sigh, but this comment should now go away.  I wonder why I thought that way,
> probably it's ECMAScript 5 which doesn't allow to play with protos.  And we
> cannot make them not-writable for bug-compatibility I gather :(

Done.

http://codereview.chromium.org/7067019/diff/1/src/builtins.cc#newcode379
src/builtins.cc:379: if (array_proto !=
global_context->initial_object_prototype()) return false;
On 2011/05/24 10:38:20, antonm wrote:
> while you're here, may you swap these two lines?  I am concerned with the
> following case: Array.prototype.__proto__ = 2;

Not sure which two lines you mean. Also

  Array.prototype.__proto__ = 2

does not have any effect.

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
File test/mjsunit/regress/regress-1403.js (right):

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
test/mjsunit/regress/regress-1403.js:28: // See:
http://code.google.com/p/v8/issues/detail?id=1403
On 2011/05/24 10:38:20, antonm wrote:
> do we have similar test case when Array.prototype.__proto__ is mutated?

Don't know. Added one here.

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
test/mjsunit/regress/regress-1403.js:30: a = new Array();
On 2011/05/24 10:38:20, antonm wrote:
> nit: a = [] might be easier to read

Done.

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
test/mjsunit/regress/regress-1403.js:33: Object.prototype.__proto__ = p;
On 2011/05/24 10:38:20, antonm wrote:
> nit: maybe (for compactness): Object.prototype.__proto__ = { __proto__: null
};

Done.

http://codereview.chromium.org/7067019/diff/1/test/mjsunit/regress/regress-14...
test/mjsunit/regress/regress-1403.js:34: a.shift();
On 2011/05/24 10:38:20, antonm wrote:
> I would consider adding indexed properties on Object.prototype.__proto__ to
> check they are handled correctly.

There are tests of this in array-elements-from-array-prototype-chain.js

[antonm, 2011-05-24 12:33:59]

Still LGTM

http://codereview.chromium.org/7067019/diff/1/src/builtins.cc
File src/builtins.cc (right):

http://codereview.chromium.org/7067019/diff/1/src/builtins.cc#newcode379
src/builtins.cc:379: if (array_proto !=
global_context->initial_object_prototype()) return false;
Sorry, I mean JSObject::cast and != global_context->initial_object_prototype()
check.  But if don't allow anything except for null and JSObject as a prototype
value, then it's not mandatory.

On 2011/05/24 12:25:52, Søren Gjesse wrote:
> On 2011/05/24 10:38:20, antonm wrote:
> > while you're here, may you swap these two lines?  I am concerned with the
> > following case: Array.prototype.__proto__ = 2;
> 
> Not sure which two lines you mean. Also
> 
>   Array.prototype.__proto__ = 2
> 
> does not have any effect.

http://codereview.chromium.org/7067019/diff/3002/src/builtins.cc#newcode369
src/builtins.cc:369: // This method depends on non writability of Object and
Array prototype
just fyi, this comment is still here