Fix Array.prototype.push and Array.prototype.unshift for read-only length.

src/builtins.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "v8.h"
 
 #include "api.h"
 #include "arguments.h"
 #include "bootstrapper.h"
 #include "builtins.h"
@@ skipping 364 matching lines @@
   HandleScope scope(isolate);
   Handle<Object> receiver = args.receiver();
   MaybeHandle<FixedArrayBase> maybe_elms_obj =
       EnsureJSArrayWithWritableFastElements(isolate, receiver, &args, 1);
   Handle<FixedArrayBase> elms_obj;
   if (!maybe_elms_obj.ToHandle(&elms_obj)) {
     return CallJsBuiltin(isolate, "ArrayPush", args);
   }
 
   Handle<JSArray> array = Handle<JSArray>::cast(receiver);
+  int len = Smi::cast(array->length())->value();
+  int to_add = args.length() - 1;
+  if (to_add > 0 && JSArray::WouldChangeReadOnlyLength(array, len + to_add)) {
+    RETURN_FAILURE_ON_EXCEPTION(
+        isolate,
+        JSArray::ReadOnlyLengthError(array));

[Michael Starzinger, 2014/05/14 08:30:48]

As discussed offline: Instead of handling this corner-case here in the optimized
version, could we instead call out to the JS builtin reference implementation
instead? This would make the optimized path simpler.

[ulan, 2014/05/15 11:15:54]

Replaced RETURN_FAILURE_ON_EXCEPTION(..) with call to JS builtin. I thought
about simplifying the condition as well (only checking for read-only length),
but left it unchanged because it allows to avoid length lookup when the index is
within bounds.

+  }
   ASSERT(!array->map()->is_observed());
 
   ElementsKind kind = array->GetElementsKind();
 
   if (IsFastSmiOrObjectElementsKind(kind)) {
     Handle<FixedArray> elms = Handle<FixedArray>::cast(elms_obj);
-
-    int len = Smi::cast(array->length())->value();
-    int to_add = args.length() - 1;
     if (to_add == 0) {
       return Smi::FromInt(len);
     }
     // Currently fixed arrays cannot grow too big, so
     // we should never hit this case.
     ASSERT(to_add <= (Smi::kMaxValue - len));
 
     int new_length = len + to_add;
 
     if (new_length > elms->length()) {
@@ skipping 18 matching lines @@
     }
 
     if (*elms != array->elements()) {
       array->set_elements(*elms);
     }
 
     // Set the length.
     array->set_length(Smi::FromInt(new_length));
     return Smi::FromInt(new_length);
   } else {
-    int len = Smi::cast(array->length())->value();
     int elms_len = elms_obj->length();
-
-    int to_add = args.length() - 1;
     if (to_add == 0) {
       return Smi::FromInt(len);
     }
     // Currently fixed arrays cannot grow too big, so
     // we should never hit this case.
     ASSERT(to_add <= (Smi::kMaxValue - len));
 
     int new_length = len + to_add;
 
     Handle<FixedDoubleArray> new_elms;
@@ skipping 134 matching lines @@
   }
   Handle<FixedArray> elms = Handle<FixedArray>::cast(elms_obj);
 
   int len = Smi::cast(array->length())->value();
   int to_add = args.length() - 1;
   int new_length = len + to_add;
   // Currently fixed arrays cannot grow too big, so
   // we should never hit this case.
   ASSERT(to_add <= (Smi::kMaxValue - len));
 
+  if (to_add > 0 && JSArray::WouldChangeReadOnlyLength(array, len + to_add)) {
+    RETURN_FAILURE_ON_EXCEPTION(
+        isolate,
+        JSArray::ReadOnlyLengthError(array));

[Michael Starzinger, 2014/05/14 08:30:48]

Likewise.

+  }
+
   JSObject::EnsureCanContainElements(array, &args, 1, to_add,
                                      DONT_ALLOW_DOUBLE_ELEMENTS);
 
   if (new_length > elms->length()) {
     // New backing storage is needed.
     int capacity = new_length + (new_length >> 1) + 16;
     Handle<FixedArray> new_elms =
         isolate->factory()->NewUninitializedFixedArray(capacity);
 
     ElementsKind kind = array->GetElementsKind();
@@ skipping 1106 matching lines @@
 }
 BUILTIN_LIST_C(DEFINE_BUILTIN_ACCESSOR_C)
 BUILTIN_LIST_A(DEFINE_BUILTIN_ACCESSOR_A)
 BUILTIN_LIST_H(DEFINE_BUILTIN_ACCESSOR_H)
 BUILTIN_LIST_DEBUG_A(DEFINE_BUILTIN_ACCESSOR_A)
 #undef DEFINE_BUILTIN_ACCESSOR_C
 #undef DEFINE_BUILTIN_ACCESSOR_A
 
 
 } }  // namespace v8::internal