Don't use StoreIC_ArrayLength on frozen arrays

src/ic.cc

Index: src/ic.cc
diff --git a/src/ic.cc b/src/ic.cc
index a15863e1415955cb2824093df138acaf43e1cca2..f84b3b9248eae2b7061ef2f181ab24214cc9426f 100644
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -1662,12 +1662,14 @@ MaybeObject* StoreIC::Store(State state,
 
   // Use specialized code for setting the length of arrays with fast
   // properties. Slow properties might indicate redefinition of the length
-  // property.
+  // property. Note that when redefined using Object.freeze, it's possible
+  // to have fast properties but a read-only length.
   if (FLAG_use_ic &&
       receiver->IsJSArray() &&
       name->Equals(isolate()->heap()->length_string()) &&
       Handle<JSArray>::cast(receiver)->AllowsSetElementsLength() &&
-      receiver->HasFastProperties()) {
+      receiver->HasFastProperties() &&
+      !receiver->map()->is_frozen()) {

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

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]

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).

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

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]

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

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

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.

     Handle<Code> stub =
         StoreArrayLengthStub(kind(), strict_mode).GetCode(isolate());
     set_target(*stub);