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

src/builtins.cc

 // Copyright 2012 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.
 
 #include "src/builtins.h"
 
 #include "src/api.h"
 #include "src/api-arguments.h"
 #include "src/api-natives.h"
 #include "src/base/once.h"
@@ skipping 700 matching lines @@
 
     if (i >= JSObject::kMaxElementCount - index_offset_) {
       set_exceeds_array_limit(true);
       // Exception hasn't been thrown at this point. Return true to
       // break out, and caller will throw. !visit would imply that
       // there is already a pending exception.
       return true;
     }
 
     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<bool> success = JSReceiver::CreateDataProperty(
+          &it, elm, AbstractCode::ShouldThrow::THROW_ON_ERROR);

[adamk, 2016/03/17 19:22:21]

"Object::THROW_ON_ERROR" is the usual way of naming this enum.

[Dan Ehrenberg, 2016/03/17 20:26:33]

Done

+      if (!success.IsJust()) return false;
+      DCHECK(success.FromJust());

[adamk, 2016/03/17 19:22:21]

I agree that this ought to be the case, but I'm not sure our CreateDataProperty
function agrees. See this code from JSObject::CreateDataProperty:

  if (it->IsFound()) {
    if (!it->IsConfigurable()) return Just(false);
  } else {
    if (!JSObject::IsExtensible(Handle<JSObject>::cast(it->GetReceiver())))
      return Just(false);
  }

Can you add tests cases for those two cases (one where '0' is present but
non-configurable and one where the returned object doesn't have '0' and is
non-extensible)?

From my reading of the code those should throw but will not (and your DCHECK
will fire).

It seems like we may need some fixes in objects.cc; hope no existing code
depends on this non-throwing behavior...

[Dan Ehrenberg, 2016/03/17 20:26:33]

For this patch, I just fixed it on this side, WDYT?

[adamk, 2016/03/17 20:33:59]

I'd prefer it be fixed properly, the CreateDataProperty API is just wrong at the
moment. I don't think the bug fix is urgent enough to want to do this
out-of-order.

[adamk, 2016/03/17 20:39:54]

I do think it would make sense to do the CreateDataProperty fix in a separate
patch, though; I just think it should land before this one.

[Dan Ehrenberg, 2016/03/17 21:15:53]

This new version depends on https://codereview.chromium.org/1809233002 to throw
instead.

       return true;
     }
 
     if (fast_elements()) {
       if (index < static_cast<uint32_t>(storage_fixed_array()->length())) {
         storage_fixed_array()->set(index, *elm);
         return true;
       }
       // Our initial estimate of length was foiled, possibly by
       // getters on the arrays increasing the length of later arrays
@@ skipping 81 matching lines @@
         if (!new_storage.is_identical_to(slow_storage)) {
           slow_storage = loop_scope.CloseAndEscape(new_storage);
         }
       }
     }
     clear_storage();
     set_storage(*slow_storage);
     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());
     storage_ = isolate_->global_handles()->Create(storage);
   }
 
   class FastElementsField : public BitField<bool, 0, 1> {};
   class ExceedsLimitField : public BitField<bool, 1, 1> {};
   class IsFixedArrayField : public BitField<bool, 2, 1> {};
 
@@ skipping 563 matching lines @@
   for (int i = 0; i < argument_count; i++) {
     Handle<Object> obj((*args)[i], isolate);
     Maybe<bool> spreadable = IsConcatSpreadable(isolate, obj);
     MAYBE_RETURN(spreadable, isolate->heap()->exception());
     if (spreadable.FromJust()) {
       Handle<JSReceiver> object = Handle<JSReceiver>::cast(obj);
       if (!IterateElements(isolate, object, &visitor)) {
         return isolate->heap()->exception();
       }
     } else {
-      visitor.visit(0, obj);
+      bool success = visitor.visit(0, obj);
+      DCHECK_EQ(success, !isolate->has_pending_exception());
+      if (isolate->has_pending_exception()) return isolate->heap()->exception();
       visitor.increase_index_offset(1);
     }
   }
 
   if (visitor.exceeds_array_limit()) {
     THROW_NEW_ERROR_RETURN_FAILURE(
         isolate, NewRangeError(MessageTemplate::kInvalidArrayLength));
   }
 
   if (is_array_species) {
@@ skipping 3123 matching lines @@
 BUILTIN_LIST_C(DEFINE_BUILTIN_ACCESSOR_C)
 BUILTIN_LIST_A(DEFINE_BUILTIN_ACCESSOR_A)
 BUILTIN_LIST_H(DEFINE_BUILTIN_ACCESSOR_H)
 BUILTIN_LIST_DEBUG_A(DEFINE_BUILTIN_ACCESSOR_A)
 #undef DEFINE_BUILTIN_ACCESSOR_C
 #undef DEFINE_BUILTIN_ACCESSOR_A
 
 
 }  // namespace internal
 }  // namespace v8