Throw the right exceptions from setting elements in Array.prototype.concat

This patch fixes two bugs in Array.prototype.concat in conjunction with
subclassing Arrays:
- Create a new property rather than calling Set when adding elements to
  the output array. This means setters are not called.
- If there is an exception thrown from DefineProperty, propagate it
  outwards properly, rather than swallowing it. This can occur, e.g., with
  a Proxy as the new output array.

R=adamk
LOG=Y
BUG=chromium:595319

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

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

src/builtins.cc

@@ -706,7 +706,7 @@ class ArrayConcatVisitor {
 
   ~ArrayConcatVisitor() { clear_storage(); }
 
-  bool visit(uint32_t i, Handle<Object> elm) {
+  MUST_USE_RESULT bool visit(uint32_t i, Handle<Object> elm) {
     uint32_t index = index_offset_ + i;
 
     if (i >= JSObject::kMaxElementCount - index_offset_) {
@@ -718,10 +718,10 @@ class ArrayConcatVisitor {
     }
 
     if (!is_fixed_array()) {
-      Handle<Object> element_value;
-      ASSIGN_RETURN_ON_EXCEPTION_VALUE(
-          isolate_, element_value,
-          Object::SetElement(isolate_, storage_, index, elm, STRICT), false);
+      LookupIterator it(isolate_, storage_, index, LookupIterator::OWN);
+      MAYBE_RETURN(
+          JSReceiver::CreateDataProperty(&it, elm, Object::THROW_ON_ERROR),
+          false);
       return true;
     }
 
@@ -823,9 +823,7 @@ class ArrayConcatVisitor {
     set_fast_elements(false);
   }
 
-  inline void clear_storage() {
-    GlobalHandles::Destroy(Handle<Object>::cast(storage_).location());
-  }
+  inline void clear_storage() { GlobalHandles::Destroy(storage_.location()); }
 
   inline void set_storage(FixedArray* storage) {
     DCHECK(is_fixed_array());
@@ -1402,7 +1400,7 @@ Object* Slow_ArrayConcat(Arguments* args, Handle<Object> species,
         return isolate->heap()->exception();
       }
     } else {
-      visitor.visit(0, obj);
+      if (!visitor.visit(0, obj)) return isolate->heap()->exception();
       visitor.increase_index_offset(1);
     }
   }

test/mjsunit/regress/regress-595319.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.
+
+// https://bugs.chromium.org/p/chromium/issues/detail?id=595319
+// Ensure exceptions are checked for by Array.prototype.concat from adding
+// an element, and that elements are added to array subclasses appropriately
+
+// If adding a property does throw, the exception is propagated
+class MyException extends Error { }
+class NoDefinePropertyArray extends Array {
+  constructor(...args) {
+    super(...args);
+    return new Proxy(this, {
+      defineProperty() { throw new MyException(); }
+    });
+  }
+}
+assertThrows(() => new NoDefinePropertyArray().concat([1]), MyException);
+
+// Ensure elements are added to the instance, rather than calling [[Set]].
+class ZeroGetterArray extends Array { get 0() {} };
+assertArrayEquals([1], new ZeroGetterArray().concat(1));
+
+// Frozen arrays lead to throwing
+
+class FrozenArray extends Array {
+  constructor(...args) { super(...args); Object.freeze(this); }
+}
+assertThrows(() => new FrozenArray().concat([1]), TypeError);
+
+// Non-configurable non-writable zero leads to throwing
+class ZeroFrozenArray extends Array {
+  constructor(...args) {
+    super(...args);
+    Object.defineProperty(this, 0, {value: 1});
+  }
+}
+assertThrows(() => new ZeroFrozenArray().concat([1]), TypeError);