Throw as per spec when modifying an Array with builtin methods

BUG=v8:3684
LOG=n

Review URL: https://codereview.chromium.org/726773002

Cr-Commit-Position: refs/heads/master@{#25364}

src/builtins.cc

@@ -420,6 +420,10 @@ BUILTIN(ArrayPop) {
   int len = Smi::cast(array->length())->value();
   if (len == 0) return isolate->heap()->undefined_value();
 
+  if (JSArray::HasReadOnlyLength(array)) {
+    return CallJsBuiltin(isolate, "ArrayPop", args);
+  }
+
   ElementsAccessor* accessor = array->GetElementsAccessor();
   int new_length = len - 1;
   Handle<Object> element =
@@ -451,6 +455,10 @@ BUILTIN(ArrayShift) {
   int len = Smi::cast(array->length())->value();
   if (len == 0) return heap->undefined_value();
 
+  if (JSArray::HasReadOnlyLength(array)) {
+    return CallJsBuiltin(isolate, "ArrayShift", args);
+  }
+
   // Get first element
   ElementsAccessor* accessor = array->GetElementsAccessor();
   Handle<Object> first =
@@ -756,6 +764,11 @@ BUILTIN(ArraySplice) {
     return CallJsBuiltin(isolate, "ArraySplice", args);
   }
 
+  if (new_length != len && JSArray::HasReadOnlyLength(array)) {
+    AllowHeapAllocation allow_allocation;
+    return CallJsBuiltin(isolate, "ArraySplice", args);
+  }
+
   if (new_length == 0) {
     Handle<JSArray> result = isolate->factory()->NewJSArrayWithElements(
         elms_obj, elements_kind, actual_delete_count);

src/objects.cc

@@ -13097,18 +13097,21 @@ bool JSArray::IsReadOnlyLengthDescriptor(Handle<Map> jsarray_map) {
 }
 
 
+bool JSArray::HasReadOnlyLength(Handle<JSArray> array) {
+  LookupIterator it(array, array->GetIsolate()->factory()->length_string(),
+                    LookupIterator::OWN_SKIP_INTERCEPTOR);
+  CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
+  CHECK(it.IsFound());
+  CHECK_EQ(LookupIterator::ACCESSOR, it.state());
+  return it.IsReadOnly();
+}
+
+
 bool JSArray::WouldChangeReadOnlyLength(Handle<JSArray> array,
                                         uint32_t index) {
   uint32_t length = 0;
   CHECK(array->length()->ToArrayIndex(&length));
-  if (length <= index) {
-    LookupIterator it(array, array->GetIsolate()->factory()->length_string(),
-                      LookupIterator::OWN_SKIP_INTERCEPTOR);
-    CHECK_NE(LookupIterator::ACCESS_CHECK, it.state());
-    CHECK(it.IsFound());
-    CHECK_EQ(LookupIterator::ACCESSOR, it.state());
-    return it.IsReadOnly();
-  }
+  if (length <= index) return HasReadOnlyLength(array);
   return false;
 }
 

src/objects.h

@@ -10184,10 +10184,13 @@ class JSArray: public JSObject {
                                            uint32_t index,
                                            Handle<Object> value);
 
-  static bool IsReadOnlyLengthDescriptor(Handle<Map> jsarray_map);
+  static bool HasReadOnlyLength(Handle<JSArray> array);
   static bool WouldChangeReadOnlyLength(Handle<JSArray> array, uint32_t index);
   static MaybeHandle<Object> ReadOnlyLengthError(Handle<JSArray> array);
 
+  // TODO(adamk): Remove this method in favor of HasReadOnlyLength().
+  static bool IsReadOnlyLengthDescriptor(Handle<Map> jsarray_map);
+
   // Initialize the array with the given capacity. The function may
   // fail due to out-of-memory situations, but only if the requested
   // capacity is non-zero.

test/mjsunit/array-push-unshift-read-only-length.js → test/mjsunit/array-methods-read-only-length.js

@@ -4,14 +4,12 @@
 
 // Flags: --allow-natives-syntax
 
-function test(mode) {
+function testAdd(mode) {
   var a = [];
   Object.defineProperty(a, "length", { writable : false});
 
   function check(f) {
-    try {
-      f(a);
-    } catch(e) { }
+    assertThrows(function() { f(a) }, TypeError);
     assertFalse(0 in a);
     assertEquals(0, a.length);
   }
@@ -37,11 +35,67 @@ function test(mode) {
   check(unshift);
   %OptimizeFunctionOnNextCall(unshift);
   check(unshift);
+
+  function splice(a) {
+    a.splice(0, 0, 3);
+  }
+
+  check(splice);
+  check(splice);
+  check(splice);
+  %OptimizeFunctionOnNextCall(splice);
+  check(splice);
 }
 
-test("fast properties");
+testAdd("fast properties");
+
+testAdd("normalized");
+
+function testRemove(mode) {
+  var a = [1, 2, 3];
+  Object.defineProperty(a, "length", { writable : false});
+
+  function check(f) {
+    assertThrows(function() { f(a) }, TypeError);
+    assertEquals(3, a.length);
+  }
+
+  if (mode == "fast properties") %ToFastProperties(a);
 
-test("normalized");
+  function pop(a) {
+    a.pop();
+  }
+
+  check(pop);
+  check(pop);
+  check(pop);
+  %OptimizeFunctionOnNextCall(pop);
+  check(pop);
+
+  function shift(a) {
+    a.shift();
+  }
+
+  check(shift);
+  check(shift);
+  check(shift);
+  %OptimizeFunctionOnNextCall(shift);
+  check(shift);
+
+  function splice(a) {
+    a.splice(0, 1);
+  }
+
+  check(splice);
+  check(splice);
+  check(splice);
+  %OptimizeFunctionOnNextCall(splice);
+  check(splice);
+}
+
+testRemove("fast properties");
+
+testRemove("normalized");
 
 var b = [];
 Object.defineProperty(b.__proto__, "0", {
@@ -97,11 +151,6 @@ try {
   b.unshift(3, 4, 5);
 } catch(e) { }
 
-// TODO(ulan): According to the ECMA-262 unshift should throw an exception
-// when moving b[0] to b[3] (see 15.4.4.13 step 6.d.ii). This is difficult
-// to do with our current implementation of SmartMove() in src/array.js and
-// it will regress performance. Uncomment the following line once acceptable
-// solution is found:
-// assertFalse(2 in b);
-// assertFalse(3 in b);
-// assertEquals(2, b.length);
+assertFalse(2 in b);
+assertFalse(3 in b);
+assertEquals(2, b.length);