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

src/builtins.cc

Index: src/builtins.cc
diff --git a/src/builtins.cc b/src/builtins.cc
index 1af71de0f19d39a7d7a952171d5ce800cf94bbea..ecad63adcc4b3dec6229379134c6115378623700 100644
--- a/src/builtins.cc
+++ b/src/builtins.cc
@@ -718,10 +718,11 @@ 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<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;
     }
 
@@ -823,9 +824,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());
@@ -1409,7 +1408,9 @@ Object* Slow_ArrayConcat(Arguments* args, Handle<Object> species,
         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);
     }
   }