Revision: 21423
Author: [email protected]
Date: Thu May 22 08:09:57 2014 UTC
Log: Fix Array.prototype.push and Array.prototype.unshift for
read-only length.
BUG=
[email protected], [email protected]
Review URL: https://codereview.chromium.org/279773002
http://code.google.com/p/v8/source/detail?r=21423
Added:
/branches/bleeding_edge/test/mjsunit/array-push-unshift-read-only-length.js
Modified:
/branches/bleeding_edge/src/array.js
/branches/bleeding_edge/src/builtins.cc
/branches/bleeding_edge/src/hydrogen.cc
/branches/bleeding_edge/src/ic.cc
/branches/bleeding_edge/src/objects.cc
/branches/bleeding_edge/src/objects.h
/branches/bleeding_edge/test/webkit/fast/js/Object-defineProperty-expected.txt
=======================================
--- /dev/null
+++
/branches/bleeding_edge/test/mjsunit/array-push-unshift-read-only-length.js
Thu May 22 08:09:57 2014 UTC
@@ -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);
=======================================
--- /branches/bleeding_edge/src/array.js Wed May 14 08:51:10 2014 UTC
+++ /branches/bleeding_edge/src/array.js Thu May 22 08:09:57 2014 UTC
@@ -628,7 +628,7 @@
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);
=======================================
--- /branches/bleeding_edge/src/builtins.cc Mon May 19 10:47:00 2014 UTC
+++ /branches/bleeding_edge/src/builtins.cc Thu May 22 08:09:57 2014 UTC
@@ -382,15 +382,17 @@
}
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 @@
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 @@
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 @@
// 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);
=======================================
--- /branches/bleeding_edge/src/hydrogen.cc Wed May 21 08:51:29 2014 UTC
+++ /branches/bleeding_edge/src/hydrogen.cc Thu May 22 08:09:57 2014 UTC
@@ -7880,6 +7880,7 @@
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
=======================================
--- /branches/bleeding_edge/src/ic.cc Fri May 9 13:01:50 2014 UTC
+++ /branches/bleeding_edge/src/ic.cc Thu May 22 08:09:57 2014 UTC
@@ -1793,6 +1793,15 @@
}
}
}
+
+ 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()) {
@@ -1803,15 +1812,7 @@
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;
}
=======================================
--- /branches/bleeding_edge/src/objects.cc Thu May 22 05:36:27 2014 UTC
+++ /branches/bleeding_edge/src/objects.cc Thu May 22 08:09:57 2014 UTC
@@ -13258,6 +13258,14 @@
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:
@@ -13541,6 +13549,41 @@
array->set_length(*len);
}
}
+
+
+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(
=======================================
--- /branches/bleeding_edge/src/objects.h Thu May 22 07:32:59 2014 UTC
+++ /branches/bleeding_edge/src/objects.h Thu May 22 08:09:57 2014 UTC
@@ -10338,6 +10338,10 @@
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.
=======================================
---
/branches/bleeding_edge/test/webkit/fast/js/Object-defineProperty-expected.txt
Thu Jul 25 19:54:24 2013 UTC
+++
/branches/bleeding_edge/test/webkit/fast/js/Object-defineProperty-expected.txt
Thu May 22 08:09:57 2014 UTC
@@ -142,8 +142,8 @@
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.
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
