[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}

src/builtins.cc

@@ -222,12 +222,12 @@ inline bool PrototypeHasNoElements(PrototypeIterator* iter) {
     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 @@ inline bool IsJSArrayFastElementMovingAllowed(Isolate* isolate,
       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 @@ inline bool EnsureJSArrayWithWritableFastElements(Isolate* isolate,
   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 @@ bool IterateElements(Isolate* isolate, Handle<JSReceiver> receiver,
     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 @@ bool IterateElements(Isolate* isolate, Handle<JSReceiver> receiver,
       // 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 @@ bool IterateElements(Isolate* isolate, Handle<JSReceiver> receiver,
     }
 
     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 @@ bool IterateElements(Isolate* isolate, Handle<JSReceiver> receiver,
 #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 IterateElements(Isolate* isolate, Handle<JSReceiver> receiver,
 
 
 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 @@ Object* Slow_ArrayConcat(Arguments* args, Handle<Object> species,
       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 @@ Object* Slow_ArrayConcat(Arguments* args, Handle<Object> species,
         } 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 @@ Object* Slow_ArrayConcat(Arguments* args, Handle<Object> species,
       }
     }
     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 @@ Object* Slow_ArrayConcat(Arguments* args, Handle<Object> species,
 
 
 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>();
       }

src/elements.cc

@@ -547,6 +547,16 @@ class ElementsAccessorBase : public ElementsAccessor {
                *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 @@ class DictionaryElementsAccessor
     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 @@ class TypedElementsAccessor
     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 @@ class SloppyArgumentsElementsAccessor
     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 @@ class SlowStringWrapperElementsAccessor
       : StringWrapperElementsAccessor<
             SlowStringWrapperElementsAccessor, DictionaryElementsAccessor,
             ElementsKindTraits<SLOW_STRING_WRAPPER_ELEMENTS>>(name) {}
+
+  static bool HasAccessorsImpl(JSObject* holder,
+                               FixedArrayBase* backing_store) {
+    return DictionaryElementsAccessor::HasAccessorsImpl(holder, backing_store);
+  }
 };
 
 }  // namespace

src/elements.h

@@ -55,6 +55,7 @@ class ElementsAccessor {
   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

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];

test/mjsunit/es6/array-concat.js

@@ -267,30 +267,22 @@ function testConcatTypedArray(type, elems, modulo) {
 }
 
 (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);
 })();
 
 

test/mjsunit/regress/regress-crbug-594574-concat-leak-1.js

+// 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);

test/mjsunit/regress/regress-crbug-594574-concat-leak-2.js

+// 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);