Don't use StoreIC_ArrayLength on frozen arrays

src/ic.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 1644 matching lines @@
   }
 
   // Observed objects are always modified through the runtime.
   if (FLAG_harmony_observation && receiver->map()->is_observed()) {
     return JSReceiver::SetPropertyOrFail(
         receiver, name, value, NONE, strict_mode, store_mode);
   }
 
   // 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);
     TRACE_IC("StoreIC", name, state, *stub);
     return JSReceiver::SetPropertyOrFail(
         receiver, name, value, NONE, strict_mode, store_mode);
   }
 
   if (receiver->IsJSGlobalProxy()) {
     if (FLAG_use_ic && kind() != Code::KEYED_STORE_IC) {
@@ skipping 1429 matching lines @@
 #undef ADDR
 };
 
 
 Address IC::AddressFromUtilityId(IC::UtilityId id) {
   return IC_utilities[id];
 }
 
 
 } }  // namespace v8::internal