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..5e6ec4e4feacda86b72cb4c10e907bf4bd21fc83 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, 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;
     }
 
@@ -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();
       visitor.increase_index_offset(1);
     }
   }