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 688 matching lines @@
         storage_(isolate->global_handles()->Create(*storage)),
         index_offset_(0u),
         bit_field_(FastElementsField::encode(fast_elements) |
                    ExceedsLimitField::encode(false) |
                    IsFixedArrayField::encode(storage->IsFixedArray())) {
     DCHECK(!(this->fast_elements() && !is_fixed_array()));
   }
 
   ~ArrayConcatVisitor() { clear_storage(); }
 
   bool visit(uint32_t i, Handle<Object> elm) {

[adamk, 2016/03/17 21:31:31]

Can you add a MUST_USE_RESULT on the front of this? That'll keep future code
from making the same mistake.

[Dan Ehrenberg, 2016/03/17 22:11:01]

Done

     uint32_t index = index_offset_ + i;
 
     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, Object::THROW_ON_ERROR);
+      if (!success.IsJust()) return false;

[adamk, 2016/03/17 21:31:31]

The usual way of writing this is:

MAYBE_RETURN(JSReceiver::CreateDataProperty(...), false);

That won't get you the DCHECK, but I think that's OK, since THROW_ON_ERROR as
part of the API implies that IsJust implies success anyway.

[Dan Ehrenberg, 2016/03/17 22:11:01]

Done (I'm sad about the lost DCHECK though)

+      DCHECK(success.FromJust());
       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();

[adamk, 2016/03/17 21:31:31]

The pattern elsewhere in this file is:

if (!SomethingThatMightThrow()) return isolate->heap()->exception()

For example, the IterateElements call just above. Please use that model here
too.

[Dan Ehrenberg, 2016/03/17 22:11:01]

Done (but I'm also sad about this DCHECK)

[adamk, 2016/03/17 22:16:25]

Not sure what's really to be done here, this pattern shows up in a million
places throughout the codebase, so one DCHECK isn't going to save us.

I think it's mostly just a consequence of trying to do exceptions
without...using exceptions.

       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