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

BUG=
R=mstarzinger@chromium.org, mvstanton@chromium.org

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

git-svn-id: https://v8.googlecode.com/svn/branches/bleeding_edge@21423 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/array.js b/src/array.js
index 5b036e6db41ce76926ca3cd2e070b00ff2b5b390..60a4025c017c370d482cd5906501016b61e125ee 100644 (file)
--- a/src/array.js
+++ b/src/array.js
@@ -628,7 +628,7 @@ function ArrayUnshift(arg1) {  // length == 1
   var num_arguments = %_ArgumentsLength();
   var is_sealed = ObjectIsSealed(array);
 
-  if (IS_ARRAY(array) && !is_sealed) {
+  if (IS_ARRAY(array) && !is_sealed && len > 0) {
     SmartMove(array, 0, 0, len, num_arguments);
   } else {
     SimpleMove(array, 0, 0, len, num_arguments);

diff --git a/src/builtins.cc b/src/builtins.cc
index 50eaa6f7ceec75ff66d6d00c90888af0cd980209..6cdd4a953088bfe013535f9a8c2a7fb3efddc6bc 100644 (file)
--- a/src/builtins.cc
+++ b/src/builtins.cc
@@ -382,15 +382,17 @@ BUILTIN(ArrayPush) {
   }
 
   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 CallJsBuiltin(isolate, "ArrayPush", args);
+  }
   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);
     }
@@ -429,10 +431,7 @@ BUILTIN(ArrayPush) {
     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);
     }
@@ -578,8 +577,6 @@ BUILTIN(ArrayUnshift) {
   if (!array->HasFastSmiOrObjectElements()) {
     return CallJsBuiltin(isolate, "ArrayUnshift", args);
   }
-  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;
@@ -587,6 +584,12 @@ BUILTIN(ArrayUnshift) {
   // we should never hit this case.
   ASSERT(to_add <= (Smi::kMaxValue - len));
 
+  if (to_add > 0 && JSArray::WouldChangeReadOnlyLength(array, len + to_add)) {
+    return CallJsBuiltin(isolate, "ArrayUnshift", args);
+  }
+
+  Handle<FixedArray> elms = Handle<FixedArray>::cast(elms_obj);
+
   JSObject::EnsureCanContainElements(array, &args, 1, to_add,
                                      DONT_ALLOW_DOUBLE_ELEMENTS);
 

diff --git a/src/hydrogen.cc b/src/hydrogen.cc
index b296b9372379bf8bcfc91809d7750d0d9207c03d..e1fc1ba7d012d79af2111b3b69454cd14e6f96b6 100644 (file)
--- a/src/hydrogen.cc
+++ b/src/hydrogen.cc
@@ -7880,6 +7880,7 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
       ElementsKind elements_kind = receiver_map->elements_kind();
       if (!IsFastElementsKind(elements_kind)) return false;
       if (receiver_map->is_observed()) return false;
+      if (JSArray::IsReadOnlyLengthDescriptor(receiver_map)) return false;
       ASSERT(receiver_map->is_extensible());
 
       // If there may be elements accessors in the prototype chain, the fast

diff --git a/src/ic.cc b/src/ic.cc
index a6efdab56b49f041ca9c53cbf49dead9c2f7a65b..81022ba8e45be5d46543ebb9c8d07536fadac0f4 100644 (file)
--- a/src/ic.cc
+++ b/src/ic.cc
@@ -1794,6 +1794,15 @@ MaybeHandle<Object> KeyedStoreIC::Store(Handle<Object> object,
     }
   }
 
+  if (store_handle.is_null()) {
+    ASSIGN_RETURN_ON_EXCEPTION(
+        isolate(),
+        store_handle,
+        Runtime::SetObjectProperty(
+            isolate(), object, key, value, NONE, strict_mode()),
+        Object);
+  }
+
   if (!is_target_set()) {
     if (*stub == *generic_stub()) {
       TRACE_GENERIC_IC(isolate(), "KeyedStoreIC", "set generic");
@@ -1803,15 +1812,7 @@ MaybeHandle<Object> KeyedStoreIC::Store(Handle<Object> object,
     TRACE_IC("StoreIC", key);
   }
 
-  if (!store_handle.is_null()) return store_handle;
-  Handle<Object> result;
-  ASSIGN_RETURN_ON_EXCEPTION(
-      isolate(),
-      result,
-      Runtime::SetObjectProperty(
-          isolate(), object, key, value,  NONE, strict_mode()),
-      Object);
-  return result;
+  return store_handle;
 }
 
 

diff --git a/src/objects.cc b/src/objects.cc
index 2be60d8ec8a519edae2784da34a9c28676481723..5665889c17762ad243d845aa7c5f50e0ceba0701 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -13258,6 +13258,14 @@ MaybeHandle<Object> JSObject::SetElementWithoutInterceptor(
       CheckArrayAbuse(object, "elements write", index, true);
     }
   }
+  if (object->IsJSArray() && JSArray::WouldChangeReadOnlyLength(
+      Handle<JSArray>::cast(object), index)) {
+    if (strict_mode == SLOPPY) {
+      return value;
+    } else {
+      return JSArray::ReadOnlyLengthError(Handle<JSArray>::cast(object));
+    }
+  }
   switch (object->GetElementsKind()) {
     case FAST_SMI_ELEMENTS:
     case FAST_ELEMENTS:
@@ -13543,6 +13551,41 @@ void JSArray::JSArrayUpdateLengthFromIndex(Handle<JSArray> array,
 }
 
 
+bool JSArray::IsReadOnlyLengthDescriptor(Handle<Map> jsarray_map) {
+    Isolate* isolate = jsarray_map->GetIsolate();
+    ASSERT(!jsarray_map->is_dictionary_map());
+    LookupResult lookup(isolate);
+    Handle<Name> length_string = isolate->factory()->length_string();
+    jsarray_map->LookupDescriptor(NULL, *length_string, &lookup);
+    return lookup.IsReadOnly();
+}
+
+
+bool JSArray::WouldChangeReadOnlyLength(Handle<JSArray> array,
+                                        uint32_t index) {
+  uint32_t length = 0;
+  CHECK(array->length()->ToArrayIndex(&length));
+  if (length <= index) {
+    Isolate* isolate = array->GetIsolate();
+    LookupResult lookup(isolate);
+    Handle<Name> length_string = isolate->factory()->length_string();
+    array->LocalLookupRealNamedProperty(length_string, &lookup);
+    return lookup.IsReadOnly();
+  }
+  return false;
+}
+
+
+MaybeHandle<Object> JSArray::ReadOnlyLengthError(Handle<JSArray> array) {
+  Isolate* isolate = array->GetIsolate();
+  Handle<Name> length = isolate->factory()->length_string();
+  Handle<Object> args[2] = { length, array };
+  Handle<Object> error = isolate->factory()->NewTypeError(
+      "strict_read_only_property", HandleVector(args, ARRAY_SIZE(args)));
+  return isolate->Throw<Object>(error);
+}
+
+
 MaybeHandle<Object> JSObject::GetElementWithInterceptor(
     Handle<JSObject> object,
     Handle<Object> receiver,

diff --git a/src/objects.h b/src/objects.h
index a161b0b7c51b015e11bb4df059957ca424d176a0..f702bb9347e30c01ae5955495a75042d62a06e1c 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -10338,6 +10338,10 @@ class JSArray: public JSObject {
                                            uint32_t index,
                                            Handle<Object> value);
 
+  static bool IsReadOnlyLengthDescriptor(Handle<Map> jsarray_map);
+  static bool WouldChangeReadOnlyLength(Handle<JSArray> array, uint32_t index);
+  static MaybeHandle<Object> ReadOnlyLengthError(Handle<JSArray> array);
+
   // 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.

diff --git a/test/mjsunit/array-push-unshift-read-only-length.js b/test/mjsunit/array-push-unshift-read-only-length.js
new file mode 100644 (file)
index 0000000..67aa397
--- /dev/null
+++ b/test/mjsunit/array-push-unshift-read-only-length.js
@@ -0,0 +1,107 @@
+// Copyright 2014 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.
+
+// Flags: --allow-natives-syntax
+
+function test(mode) {
+  var a = [];
+  Object.defineProperty(a, "length", { writable : false});
+
+  function check(f) {
+    try {
+      f(a);
+    } catch(e) { }
+    assertFalse(0 in a);
+    assertEquals(0, a.length);
+  }
+
+  function push(a) {
+    a.push(3);
+  }
+
+  if (mode == "fast properties") %ToFastProperties(a);
+
+  check(push);
+  check(push);
+  check(push);
+  %OptimizeFunctionOnNextCall(push);
+  check(push);
+
+  function unshift(a) {
+    a.unshift(3);
+  }
+
+  check(unshift);
+  check(unshift);
+  check(unshift);
+  %OptimizeFunctionOnNextCall(unshift);
+  check(unshift);
+}
+
+test("fast properties");
+
+test("normalized");
+
+var b = [];
+Object.defineProperty(b.__proto__, "0", {
+  set : function(v) {
+    b.x = v;
+    Object.defineProperty(b, "length", { writable : false });
+  },
+  get: function() {
+    return b.x;
+  }
+});
+
+b = [];
+try {
+  b.push(3, 4, 5);
+} catch(e) { }
+assertFalse(1 in b);
+assertFalse(2 in b);
+assertEquals(0, b.length);
+
+b = [];
+try {
+  b.unshift(3, 4, 5);
+} catch(e) { }
+assertFalse(1 in b);
+assertFalse(2 in b);
+assertEquals(0, b.length);
+
+b = [1, 2];
+try {
+  b.unshift(3, 4, 5);
+} catch(e) { }
+assertEquals(3, b[0]);
+assertEquals(4, b[1]);
+assertEquals(5, b[2]);
+assertEquals(1, b[3]);
+assertEquals(2, b[4]);
+assertEquals(5, b.length);
+
+b = [1, 2];
+
+Object.defineProperty(b.__proto__, "4", {
+  set : function(v) {
+    b.z = v;
+    Object.defineProperty(b, "length", { writable : false });
+  },
+  get: function() {
+    return b.z;
+  }
+});
+
+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);

diff --git a/test/webkit/fast/js/Object-defineProperty-expected.txt b/test/webkit/fast/js/Object-defineProperty-expected.txt
index 7a303f2c5e76c6aac9c0cd9d42a04117b162ca3d..118f9dddf278b463a6819029d416bd5785bd8dae 100644 (file)
--- a/test/webkit/fast/js/Object-defineProperty-expected.txt
+++ b/test/webkit/fast/js/Object-defineProperty-expected.txt
@@ -142,8 +142,8 @@ PASS 'use strict'; var o = {}; o.readOnly = false; o.readOnly threw exception Ty
 PASS Object.getOwnPropertyDescriptor(Object.defineProperty(Object.defineProperty({}, 'foo', {get: function() { return false; }, configurable: true}), 'foo', {value:false}), 'foo').writable is false
 PASS Object.getOwnPropertyDescriptor(Object.defineProperty(Object.defineProperty({}, 'foo', {get: function() { return false; }, configurable: true}), 'foo', {value:false, writable: false}), 'foo').writable is false
 PASS Object.getOwnPropertyDescriptor(Object.defineProperty(Object.defineProperty({}, 'foo', {get: function() { return false; }, configurable: true}), 'foo', {value:false, writable: true}), 'foo').writable is true
-FAIL var a = Object.defineProperty([], 'length', {writable: false}); a[0] = 42; 0 in a; should be false. Was true.
-FAIL 'use strict'; var a = Object.defineProperty([], 'length', {writable: false}); a[0] = 42; 0 in a; should throw an exception. Was true.
+PASS var a = Object.defineProperty([], 'length', {writable: false}); a[0] = 42; 0 in a; is false
+PASS 'use strict'; var a = Object.defineProperty([], 'length', {writable: false}); a[0] = 42; 0 in a; threw exception TypeError: Cannot assign to read only property 'length' of [object Array].
 PASS var a = Object.defineProperty([42], '0', {writable: false}); a[0] = false; a[0]; is 42
 PASS 'use strict'; var a = Object.defineProperty([42], '0', {writable: false}); a[0] = false; a[0]; threw exception TypeError: Cannot assign to read only property '0' of [object Array].
 PASS var a = Object.defineProperty([], '0', {set: undefined}); a[0] = 42; a[0]; is undefined.