Don't use StoreIC_ArrayLength on frozen arrays

Don't use StoreIC_ArrayLength on frozen arrays

The code previously assumed that an array with fast properties must have
a writable length property. But Object.freeze() now exposes a way to make
length read-only without moving the object into slow mode. This patch
simply adds a !is_frozen check to the IC code. Any future optimizations
to attribute-setting on JSArrays will need to make similar accomodations.

R=danno
BUG=v8:2711, 259548
Committed: https://code.google.com/p/v8/source/detail?r=15651

[ihab.awad, 2013-07-12 20:41:29]

https://codereview.chromium.org/19115002/diff/1/test/mjsunit/regress/regress-...
File test/mjsunit/regress/regress-2711.js (right):

https://codereview.chromium.org/19115002/diff/1/test/mjsunit/regress/regress-...
test/mjsunit/regress/regress-2711.js:30: a.push(2);
Fwiw, my testing on 30.0.1562.0 canary showed that:

  aFrozenArray.push(2);

respected frozen-ness, but, enigmatically:

  Array.prototype.push.call(aFrozenArray, 2);

did *not* respect frozen-ness; it bumped the 'length' property but did not
actually insert an element into the array.

[adamk, 2013-07-12 20:53:28]

On 2013/07/12 20:41:29, ihab.awad wrote:
>
https://codereview.chromium.org/19115002/diff/1/test/mjsunit/regress/regress-...
> File test/mjsunit/regress/regress-2711.js (right):
> 
>
https://codereview.chromium.org/19115002/diff/1/test/mjsunit/regress/regress-...
> test/mjsunit/regress/regress-2711.js:30: a.push(2);
> Fwiw, my testing on 30.0.1562.0 canary showed that:
> 
>   aFrozenArray.push(2);
> 
> respected frozen-ness, but, enigmatically:
> 
>   Array.prototype.push.call(aFrozenArray, 2);
> 
> did *not* respect frozen-ness; it bumped the 'length' property but did not
> actually insert an element into the array.

Indeed, the latter behavior is what I'd expect given the actual bug: the problem
is that once optimized, length-setting code would keep operating, while the
frozen-ness of the rest of the properties would be honored.  A simpler repro
case for me was:

a = Object.freeze([1]);
a.length = 2;
a.length = 2;

[Mark Miller, 2013-07-12 20:56:57]

https://codereview.chromium.org/19115002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/19115002/diff/1/src/ic.cc#newcode1672
src/ic.cc:1672: !receiver->map()->is_frozen()) {
The issue also arises if the .length property is non-writable, even if the
object as a whole isn't frozen.

[adamk, 2013-07-12 21:00:09]

https://codereview.chromium.org/19115002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/19115002/diff/1/src/ic.cc#newcode1672
src/ic.cc:1672: !receiver->map()->is_frozen()) {
On 2013/07/12 20:56:57, Mark Miller wrote:
> The issue also arises if the .length property is non-writable, even if the
> object as a whole isn't frozen.

See the comment above, and my note in the CL description. Before my
Object.freeze optimization, the HasFastProperties() check was sufficient to
detect the possibility that 'length' had been reconfigured. This is because the
code that did that, such as:

Object.defineProperty(arr, 'length', {writable: false})

Would have forced the object into dictionary ("slow") mode. The reason this bug
popped up is that Object.freeze() now causes length to become non-writable
without forcing the object into slow mode (that's actually the whole point of
the change).

[ihab.awad, 2013-07-12 21:02:00]

Np, just pointing out what I saw but you know the ramifications best, of course!
Thanks!

[Mark Miller, 2013-07-12 21:08:02]

https://codereview.chromium.org/19115002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/19115002/diff/1/src/ic.cc#newcode1672
src/ic.cc:1672: !receiver->map()->is_frozen()) {
Gotcha. Thanks for the clarification.

Does that mean that a cheaper workaround for browsers predating this fix would
be to wrap Object.freeze to check if the object about to be frozen is an array,
and if so, to defineProperty its length to non-writable before performing the
freeze as a whole?

The reason I ask is that Chrome may not go to beta with this bug, and we know
from previous experiences that wrapping or replacing .push is terribly expensive
for some of our important uses.

[Mark Miller, 2013-07-12 21:09:39]

https://codereview.chromium.org/19115002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/19115002/diff/1/src/ic.cc#newcode1672
src/ic.cc:1672: !receiver->map()->is_frozen()) {
On 2013/07/12 21:08:02, Mark Miller wrote:
> The reason I ask is that Chrome may not go to beta with this 

I meant "...may *now* go to beta with..."

[adamk, 2013-07-12 21:14:28]

https://codereview.chromium.org/19115002/diff/1/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/19115002/diff/1/src/ic.cc#newcode1672
src/ic.cc:1672: !receiver->map()->is_frozen()) {
On 2013/07/12 21:08:02, Mark Miller wrote:
> Gotcha. Thanks for the clarification.
> 
> Does that mean that a cheaper workaround for browsers predating this fix would
> be to wrap Object.freeze to check if the object about to be frozen is an
array,
> and if so, to defineProperty its length to non-writable before performing the
> freeze as a whole?
> 
> The reason I ask is that Chrome may not go to beta with this bug, and we know
> from previous experiences that wrapping or replacing .push is terribly
expensive
> for some of our important uses.

Replacing .push won't even help you; the underlying bug is that it will always
be possible to write to .length. But yes, a workaround for this bug would be to
freeze length separately using

Object.defineProperty(arr, 'length', {writable: false, configurable: false});
Object.freeze(arr);

That'll force the array to return false for HasFastProperties(), and thus avoid
this bug.

[danno, 2013-07-14 22:02:45]

lgtm