[builtins] Fix Array.prototype.concat bug
Array.prototype.concat did not work correct with complex elements on the
receiver or the prototype chain.
BUG=chromium:594574
LOG=y
Review URL: https://codereview.chromium.org/1804963002
Cr-Commit-Position: refs/heads/master@{#34798}
diff --git a/src/builtins.cc b/src/builtins.cc
index 930c1b3..1af71de 100644
--- a/src/builtins.cc
+++ b/src/builtins.cc
@@ -222,12 +222,12 @@
JSObject* current = iter->GetCurrent<JSObject>();
if (current->IsAccessCheckNeeded()) return false;
if (current->HasIndexedInterceptor()) return false;
+ if (current->HasStringWrapperElements()) return false;
if (current->elements()->length() != 0) return false;
}
return true;
}
-
inline bool IsJSArrayFastElementMovingAllowed(Isolate* isolate,
JSArray* receiver) {
DisallowHeapAllocation no_gc;
@@ -239,12 +239,43 @@
isolate->IsFastArrayConstructorPrototypeChainIntact()) {
return true;
}
-
// Slow case.
PrototypeIterator iter(isolate, receiver);
return PrototypeHasNoElements(&iter);
}
+inline bool HasSimpleElements(JSObject* current) {
+ if (current->IsAccessCheckNeeded()) return false;
+ if (current->HasIndexedInterceptor()) return false;
+ if (current->HasStringWrapperElements()) return false;
+ if (current->GetElementsAccessor()->HasAccessors(current)) return false;
+ return true;
+}
+
+inline bool HasOnlySimpleReceiverElements(Isolate* isolate,
+ JSReceiver* receiver) {
+ // Check that we have no accessors on the receiver's elements.
+ JSObject* object = JSObject::cast(receiver);
+ if (!HasSimpleElements(object)) return false;
+ // Check that ther are not elements on the prototype.
+ DisallowHeapAllocation no_gc;
+ PrototypeIterator iter(isolate, receiver);
+ return PrototypeHasNoElements(&iter);
+}
+
+inline bool HasOnlySimpleElements(Isolate* isolate, JSReceiver* receiver) {
+ // Check that ther are not elements on the prototype.
+ DisallowHeapAllocation no_gc;
+ PrototypeIterator iter(isolate, receiver,
+ PrototypeIterator::START_AT_RECEIVER);
+ for (; !iter.IsAtEnd(); iter.Advance()) {
+ if (iter.GetCurrent()->IsJSProxy()) return false;
+ JSObject* current = iter.GetCurrent<JSObject>();
+ if (!HasSimpleElements(current)) return false;
+ }
+ return true;
+}
+
// Returns |false| if not applicable.
MUST_USE_RESULT
inline bool EnsureJSArrayWithWritableFastElements(Isolate* isolate,
@@ -258,7 +289,7 @@
if (args != nullptr && !IsJSArrayFastElementMovingAllowed(isolate, *array)) {
return false;
}
- ElementsKind origin_kind = array->map()->elements_kind();
+ ElementsKind origin_kind = array->GetElementsKind();
if (IsDictionaryElementsKind(origin_kind)) return false;
if (array->map()->is_observed()) return false;
if (!array->map()->is_extensible()) return false;
@@ -1063,11 +1094,11 @@
if (!val->ToUint32(&length)) {
length = 0;
}
+ // TODO(cbruni): handle other element kind as well
+ return IterateElementsSlow(isolate, receiver, length, visitor);
}
- if (!receiver->IsJSArray()) {
- // For classes which are not known to be safe to access via elements alone,
- // use the slow case.
+ if (!HasOnlySimpleElements(isolate, *receiver)) {
return IterateElementsSlow(isolate, receiver, length, visitor);
}
Handle<JSObject> array = Handle<JSObject>::cast(receiver);
@@ -1081,7 +1112,7 @@
// to check the prototype for missing elements.
Handle<FixedArray> elements(FixedArray::cast(array->elements()));
int fast_length = static_cast<int>(length);
- DCHECK(fast_length <= elements->length());
+ DCHECK_LE(fast_length, elements->length());
for (int j = 0; j < fast_length; j++) {
HandleScope loop_scope(isolate);
Handle<Object> element_value(elements->get(j), isolate);
@@ -1141,14 +1172,6 @@
}
case DICTIONARY_ELEMENTS: {
- // CollectElementIndices() can't be called when there's a JSProxy
- // on the prototype chain.
- for (PrototypeIterator iter(isolate, array); !iter.IsAtEnd();
- iter.Advance()) {
- if (PrototypeIterator::GetCurrent(iter)->IsJSProxy()) {
- return IterateElementsSlow(isolate, array, length, visitor);
- }
- }
Handle<SeededNumberDictionary> dict(array->element_dictionary());
List<uint32_t> indices(dict->Capacity() / 2);
// Collect all indices in the object and the prototypes less
@@ -1189,6 +1212,7 @@
#define TYPED_ARRAY_CASE(Type, type, TYPE, ctype, size) case TYPE##_ELEMENTS:
TYPED_ARRAYS(TYPED_ARRAY_CASE)
#undef TYPED_ARRAY_CASE
+ return IterateElementsSlow(isolate, receiver, length, visitor);
case FAST_STRING_WRAPPER_ELEMENTS:
case SLOW_STRING_WRAPPER_ELEMENTS:
// |array| is guaranteed to be an array or typed array.
@@ -1201,7 +1225,6 @@
bool HasConcatSpreadableModifier(Isolate* isolate, Handle<JSArray> obj) {
- DCHECK(isolate->IsFastArrayConstructorPrototypeChainIntact());
Handle<Symbol> key(isolate->factory()->is_concat_spreadable_symbol());
Maybe<bool> maybe = JSReceiver::HasProperty(obj, key);
return maybe.FromMaybe(false);
@@ -1246,17 +1269,14 @@
length_estimate = static_cast<uint32_t>(array->length()->Number());
if (length_estimate != 0) {
ElementsKind array_kind =
- GetPackedElementsKind(array->map()->elements_kind());
+ GetPackedElementsKind(array->GetElementsKind());
kind = GetMoreGeneralElementsKind(kind, array_kind);
}
element_estimate = EstimateElementCount(array);
} else {
if (obj->IsHeapObject()) {
- if (obj->IsNumber()) {
- kind = GetMoreGeneralElementsKind(kind, FAST_DOUBLE_ELEMENTS);
- } else {
- kind = GetMoreGeneralElementsKind(kind, FAST_ELEMENTS);
- }
+ kind = GetMoreGeneralElementsKind(
+ kind, obj->IsNumber() ? FAST_DOUBLE_ELEMENTS : FAST_ELEMENTS);
}
length_estimate = 1;
element_estimate = 1;
@@ -1299,7 +1319,7 @@
} else {
JSArray* array = JSArray::cast(*obj);
uint32_t length = static_cast<uint32_t>(array->length()->Number());
- switch (array->map()->elements_kind()) {
+ switch (array->GetElementsKind()) {
case FAST_HOLEY_DOUBLE_ELEMENTS:
case FAST_DOUBLE_ELEMENTS: {
// Empty array is FixedArray but not FixedDoubleArray.
@@ -1351,14 +1371,7 @@
}
}
if (!failure) {
- Handle<JSArray> array = isolate->factory()->NewJSArray(0);
- Smi* length = Smi::FromInt(j);
- Handle<Map> map;
- map = JSObject::GetElementsTransitionMap(array, kind);
- array->set_map(*map);
- array->set_length(length);
- array->set_elements(*storage);
- return *array;
+ return *isolate->factory()->NewJSArrayWithElements(storage, kind, j);
}
// In case of failure, fall through.
}
@@ -1415,23 +1428,23 @@
MaybeHandle<JSArray> Fast_ArrayConcat(Isolate* isolate, Arguments* args) {
- if (!isolate->IsFastArrayConstructorPrototypeChainIntact()) {
- return MaybeHandle<JSArray>();
- }
int n_arguments = args->length();
int result_len = 0;
{
DisallowHeapAllocation no_gc;
- Object* array_proto = isolate->array_function()->prototype();
// Iterate through all the arguments performing checks
// and calculating total length.
for (int i = 0; i < n_arguments; i++) {
Object* arg = (*args)[i];
if (!arg->IsJSArray()) return MaybeHandle<JSArray>();
+ if (!HasOnlySimpleReceiverElements(isolate, JSObject::cast(arg))) {
+ return MaybeHandle<JSArray>();
+ }
+ // TODO(cbruni): support fast concatenation of DICTIONARY_ELEMENTS.
+ if (!JSObject::cast(arg)->HasFastElements()) {
+ return MaybeHandle<JSArray>();
+ }
Handle<JSArray> array(JSArray::cast(arg), isolate);
- if (!array->HasFastElements()) return MaybeHandle<JSArray>();
- PrototypeIterator iter(isolate, *array);
- if (iter.GetCurrent() != array_proto) return MaybeHandle<JSArray>();
if (HasConcatSpreadableModifier(isolate, array)) {
return MaybeHandle<JSArray>();
}
diff --git a/src/elements.cc b/src/elements.cc
index 7746d8d..86cc8d7 100644
--- a/src/elements.cc
+++ b/src/elements.cc
@@ -547,6 +547,16 @@
*holder, *backing_store, index, filter) != kMaxUInt32;
}
+ bool HasAccessors(JSObject* holder) final {
+ return ElementsAccessorSubclass::HasAccessorsImpl(holder,
+ holder->elements());
+ }
+
+ static bool HasAccessorsImpl(JSObject* holder,
+ FixedArrayBase* backing_store) {
+ return false;
+ }
+
Handle<Object> Get(Handle<JSObject> holder, uint32_t entry) final {
return ElementsAccessorSubclass::GetImpl(holder, entry);
}
@@ -1092,6 +1102,21 @@
obj->set_elements(*new_elements);
}
+ static bool HasAccessorsImpl(JSObject* holder,
+ FixedArrayBase* backing_store) {
+ SeededNumberDictionary* dict = SeededNumberDictionary::cast(backing_store);
+ if (!dict->requires_slow_elements()) return false;
+ int capacity = dict->Capacity();
+ for (int i = 0; i < capacity; i++) {
+ Object* key = dict->KeyAt(i);
+ if (!dict->IsKey(key)) continue;
+ DCHECK(!dict->IsDeleted(i));
+ PropertyDetails details = dict->DetailsAt(i);
+ if (details.type() == ACCESSOR_CONSTANT) return true;
+ }
+ return false;
+ }
+
static Object* GetRaw(FixedArrayBase* store, uint32_t entry) {
SeededNumberDictionary* backing_store = SeededNumberDictionary::cast(store);
return backing_store->ValueAt(entry);
@@ -1992,6 +2017,11 @@
return index < AccessorClass::GetCapacityImpl(*holder, *backing_store);
}
+ static bool HasAccessorsImpl(JSObject* holder,
+ FixedArrayBase* backing_store) {
+ return false;
+ }
+
static void SetLengthImpl(Isolate* isolate, Handle<JSArray> array,
uint32_t length,
Handle<FixedArrayBase> backing_store) {
@@ -2162,6 +2192,13 @@
return ArgumentsAccessor::HasEntryImpl(arguments, entry - length);
}
+ static bool HasAccessorsImpl(JSObject* holder,
+ FixedArrayBase* backing_store) {
+ FixedArray* parameter_map = FixedArray::cast(backing_store);
+ FixedArrayBase* arguments = FixedArrayBase::cast(parameter_map->get(1));
+ return ArgumentsAccessor::HasAccessorsImpl(holder, arguments);
+ }
+
static uint32_t GetIndexForEntryImpl(FixedArrayBase* parameters,
uint32_t entry) {
FixedArray* parameter_map = FixedArray::cast(parameters);
@@ -2599,6 +2636,11 @@
: StringWrapperElementsAccessor<
SlowStringWrapperElementsAccessor, DictionaryElementsAccessor,
ElementsKindTraits<SLOW_STRING_WRAPPER_ELEMENTS>>(name) {}
+
+ static bool HasAccessorsImpl(JSObject* holder,
+ FixedArrayBase* backing_store) {
+ return DictionaryElementsAccessor::HasAccessorsImpl(holder, backing_store);
+ }
};
} // namespace
diff --git a/src/elements.h b/src/elements.h
index 386fe57..0d06021 100644
--- a/src/elements.h
+++ b/src/elements.h
@@ -55,6 +55,7 @@
virtual Handle<Object> Get(Handle<JSObject> holder, uint32_t entry) = 0;
virtual PropertyDetails GetDetails(JSObject* holder, uint32_t entry) = 0;
+ virtual bool HasAccessors(JSObject* holder) = 0;
// Modifies the length data property as specified for JSArrays and resizes the
// underlying backing store accordingly. The method honors the semantics of
diff --git a/test/mjsunit/array-concat.js b/test/mjsunit/array-concat.js
index 97bd85a..6e25b5c 100644
--- a/test/mjsunit/array-concat.js
+++ b/test/mjsunit/array-concat.js
@@ -29,6 +29,19 @@
* @fileoverview Test concat on small and large arrays
*/
+
+(function testStringWrapperConcat() {
+ var concat = Array.prototype.concat;
+ var str = new String('abcd');
+ assertEquals([1,2,3,new String('abcd')], [1, 2, 3].concat(str));
+ assertEquals([new String("abcd")], concat.call(str));
+
+ var array = [1, 2, 3];
+ array.__proto__ = str;
+ array.length = 4;
+ assertEquals([1,2,3,'d'], concat.call(array));
+})()
+
var poses;
poses = [140, 4000000000];
diff --git a/test/mjsunit/es6/array-concat.js b/test/mjsunit/es6/array-concat.js
index 801c225..f22d038 100644
--- a/test/mjsunit/es6/array-concat.js
+++ b/test/mjsunit/es6/array-concat.js
@@ -267,30 +267,22 @@
}
(function testConcatSmallTypedArray() {
- var max = [Math.pow(2, 8), Math.pow(2, 16), Math.pow(2, 32), false, false];
- [
- Uint8Array,
- Uint16Array,
- Uint32Array,
- Float32Array,
- Float64Array
- ].forEach(function(ctor, i) {
- testConcatTypedArray(ctor, 1, max[i]);
- });
+ var length = 1;
+ testConcatTypedArray(Uint8Array, length, Math.pow(2, 8));
+ testConcatTypedArray(Uint16Array, length, Math.pow(2, 16));
+ testConcatTypedArray(Uint32Array, length, Math.pow(2, 32));
+ testConcatTypedArray(Float32Array, length, false);
+ testConcatTypedArray(Float64Array, length, false);
})();
(function testConcatLargeTypedArray() {
- var max = [Math.pow(2, 8), Math.pow(2, 16), Math.pow(2, 32), false, false];
- [
- Uint8Array,
- Uint16Array,
- Uint32Array,
- Float32Array,
- Float64Array
- ].forEach(function(ctor, i) {
- testConcatTypedArray(ctor, 4000, max[i]);
- });
+ var length = 4000;
+ testConcatTypedArray(Uint8Array, length, Math.pow(2, 8));
+ testConcatTypedArray(Uint16Array, length, Math.pow(2, 16));
+ testConcatTypedArray(Uint32Array, length, Math.pow(2, 32));
+ testConcatTypedArray(Float32Array, length, false);
+ testConcatTypedArray(Float64Array, length, false);
})();
diff --git a/test/mjsunit/regress/regress-crbug-594574-concat-leak-1.js b/test/mjsunit/regress/regress-crbug-594574-concat-leak-1.js
new file mode 100644
index 0000000..d5f51a4
--- /dev/null
+++ b/test/mjsunit/regress/regress-crbug-594574-concat-leak-1.js
@@ -0,0 +1,36 @@
+// Copyright 2016 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: --expose-gc
+
+array = new Array(10);
+array[0] = 0.1;
+// array[1] = THE_HOLE, reading through the prototype chain
+array[2] = 2.1;
+array[3] = 3.1;
+
+var copy = array.slice(0, array.length);
+
+// Change the array's prototype.
+var proto = {};
+array.__proto__ = proto;
+
+// Define [1] on the prototype to alter the array during concatenation.
+Object.defineProperty(
+ proto, 1, {
+ get() {
+ // Alter the array.
+ array.length = 1;
+ // Force gc to move the array.
+ gc();
+ return "value from proto";
+ },
+ set(new_value) { }
+});
+
+var concatted_array = Array.prototype.concat.call(array);
+assertEquals(concatted_array[0], 0.1);
+assertEquals(concatted_array[1], "value from proto");
+assertEquals(concatted_array[2], undefined);
+assertEquals(concatted_array[3], undefined);
diff --git a/test/mjsunit/regress/regress-crbug-594574-concat-leak-2.js b/test/mjsunit/regress/regress-crbug-594574-concat-leak-2.js
new file mode 100644
index 0000000..f359cfd
--- /dev/null
+++ b/test/mjsunit/regress/regress-crbug-594574-concat-leak-2.js
@@ -0,0 +1,35 @@
+// Copyright 2016 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: --expose-gc
+
+array = new Array(10);
+array[0] = 0.1;
+// array[1] = THE_HOLE, reading through the prototype chain
+array[2] = 2.1;
+array[3] = 3.1;
+
+var copy = array.slice(0, array.length);
+
+// Use the defaul array prototype.
+var proto = array.__proto__;
+
+// Define [1] on the prototype to alter the array during concatenation.
+Object.defineProperty(
+ proto, 1, {
+ get() {
+ // Alter the array.
+ array.length = 1;
+ // Force gc to move the array.
+ gc();
+ return "value from proto";
+ },
+ set(new_value) { }
+});
+
+var concatted_array = Array.prototype.concat.call(array);
+assertEquals(concatted_array[0], 0.1);
+assertEquals(concatted_array[1], "value from proto");
+assertEquals(concatted_array[2], undefined);
+assertEquals(concatted_array[3], undefined);
