[runtime] Let native setters have a return value.

Native setters (see AccessorInfo in accessors.h) didn't have the ability
to return a result value. As a consequence of this, for instance, Reflect.set
on the length property of arrays had the wrong behavior:

var y = [];
Object.defineProperty(y, 0, {value: 42, configurable: false})
Reflect.set(y, 'length', 0)

The Reflect.set call used to return true. Now it returns false as
required by the spec.

BUG=v8:5401

Review-Url: https://codereview.chromium.org/2397603003
Cr-Commit-Position: refs/heads/master@{#40579}

src/accessors.cc

@@ -19,13 +19,9 @@
 namespace v8 {
 namespace internal {
 
-
 Handle<AccessorInfo> Accessors::MakeAccessor(
-    Isolate* isolate,
-    Handle<Name> name,
-    AccessorNameGetterCallback getter,
-    AccessorNameSetterCallback setter,
-    PropertyAttributes attributes) {
+    Isolate* isolate, Handle<Name> name, AccessorNameGetterCallback getter,
+    AccessorNameBooleanSetterCallback setter, PropertyAttributes attributes) {
   Factory* factory = isolate->factory();
   Handle<AccessorInfo> info = factory->NewAccessorInfo();
   info->set_property_attributes(attributes);
@@ -106,7 +102,7 @@ MUST_USE_RESULT MaybeHandle<Object> ReplaceAccessorWithDataProperty(
 
 void Accessors::ReconfigureToDataProperty(
     v8::Local<v8::Name> key, v8::Local<v8::Value> val,
-    const v8::PropertyCallbackInfo<void>& info) {
+    const v8::PropertyCallbackInfo<v8::Boolean>& info) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(info.GetIsolate());
   HandleScope scope(isolate);
   Handle<Object> receiver = Utils::OpenHandle(*info.This());
@@ -116,7 +112,11 @@ void Accessors::ReconfigureToDataProperty(
   Handle<Object> value = Utils::OpenHandle(*val);
   MaybeHandle<Object> result =
       ReplaceAccessorWithDataProperty(isolate, receiver, holder, name, value);
-  if (result.is_null()) isolate->OptionalRescheduleException(false);
+  if (result.is_null()) {
+    isolate->OptionalRescheduleException(false);
+  } else {
+    info.GetReturnValue().Set(true);
+  }
 }
 
 //
@@ -160,11 +160,9 @@ void Accessors::ArrayLengthGetter(
   info.GetReturnValue().Set(Utils::ToLocal(Handle<Object>(result, isolate)));
 }
 
-
 void Accessors::ArrayLengthSetter(
-    v8::Local<v8::Name> name,
-    v8::Local<v8::Value> val,
-    const v8::PropertyCallbackInfo<void>& info) {
+    v8::Local<v8::Name> name, v8::Local<v8::Value> val,
+    const v8::PropertyCallbackInfo<v8::Boolean>& info) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(info.GetIsolate());
   HandleScope scope(isolate);
 
@@ -180,17 +178,21 @@ void Accessors::ArrayLengthSetter(
 
   JSArray::SetLength(array, length);
 
-  if (info.ShouldThrowOnError()) {
-    uint32_t actual_new_len = 0;
-    CHECK(array->length()->ToArrayLength(&actual_new_len));
-    // Throw TypeError if there were non-deletable elements.
-    if (actual_new_len != length) {
+  uint32_t actual_new_len = 0;
+  CHECK(array->length()->ToArrayLength(&actual_new_len));
+  // Fail if there were non-deletable elements.
+  if (actual_new_len != length) {
+    if (info.ShouldThrowOnError()) {
       Factory* factory = isolate->factory();
       isolate->Throw(*factory->NewTypeError(
           MessageTemplate::kStrictDeleteProperty,
           factory->NewNumberFromUint(actual_new_len - 1), array));
       isolate->OptionalRescheduleException(false);
+    } else {
+      info.GetReturnValue().Set(false);
     }
+  } else {
+    info.GetReturnValue().Set(true);
   }
 }
 
@@ -225,7 +227,7 @@ void Accessors::ModuleNamespaceEntryGetter(
 
 void Accessors::ModuleNamespaceEntrySetter(
     v8::Local<v8::Name> name, v8::Local<v8::Value> val,
-    const v8::PropertyCallbackInfo<void>& info) {
+    const v8::PropertyCallbackInfo<v8::Boolean>& info) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(info.GetIsolate());
   HandleScope scope(isolate);
   Factory* factory = isolate->factory();
@@ -238,7 +240,7 @@ void Accessors::ModuleNamespaceEntrySetter(
         i::Object::TypeOf(isolate, holder), holder));
     isolate->OptionalRescheduleException(false);
   } else {
-    info.GetReturnValue().Set(Utils::ToLocal(factory->ToBoolean(false)));
+    info.GetReturnValue().Set(false);
   }
 }
 
@@ -748,11 +750,9 @@ void Accessors::FunctionPrototypeGetter(
   info.GetReturnValue().Set(Utils::ToLocal(result));
 }
 
-
 void Accessors::FunctionPrototypeSetter(
-    v8::Local<v8::Name> name,
-    v8::Local<v8::Value> val,
-    const v8::PropertyCallbackInfo<void>& info) {
+    v8::Local<v8::Name> name, v8::Local<v8::Value> val,
+    const v8::PropertyCallbackInfo<v8::Boolean>& info) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(info.GetIsolate());
   HandleScope scope(isolate);
   Handle<Object> value = Utils::OpenHandle(*val);
@@ -760,6 +760,8 @@ void Accessors::FunctionPrototypeSetter(
       Handle<JSFunction>::cast(Utils::OpenHandle(*info.Holder()));
   if (SetFunctionPrototype(isolate, object, value).is_null()) {
     isolate->OptionalRescheduleException(false);
+  } else {
+    info.GetReturnValue().Set(true);
   }
 }
 
@@ -1262,9 +1264,9 @@ void Accessors::ErrorStackGetter(
   info.GetReturnValue().Set(value);
 }
 
-void Accessors::ErrorStackSetter(v8::Local<v8::Name> name,
-                                 v8::Local<v8::Value> val,
-                                 const v8::PropertyCallbackInfo<void>& info) {
+void Accessors::ErrorStackSetter(
+    v8::Local<v8::Name> name, v8::Local<v8::Value> val,
+    const v8::PropertyCallbackInfo<v8::Boolean>& info) {
   i::Isolate* isolate = reinterpret_cast<i::Isolate*>(info.GetIsolate());
   HandleScope scope(isolate);
   Handle<JSObject> obj =

src/accessors.h

@@ -71,7 +71,7 @@ class Accessors : public AllStatic {
 
 #define ACCESSOR_SETTER_DECLARATION(name)                                \
   static void name(v8::Local<v8::Name> name, v8::Local<v8::Value> value, \
-                   const v8::PropertyCallbackInfo<void>& info);
+                   const v8::PropertyCallbackInfo<v8::Boolean>& info);
   ACCESSOR_SETTER_LIST(ACCESSOR_SETTER_DECLARATION)
 #undef ACCESSOR_SETTER_DECLARATION
 
@@ -100,12 +100,21 @@ class Accessors : public AllStatic {
   static bool IsJSObjectFieldAccessor(Handle<Map> map, Handle<Name> name,
                                       int* object_offset);
 
+  // Create an AccessorInfo. The setter is optional (can be nullptr).
+  //
+  // Note that the type of setter is AccessorNameBooleanSetterCallback instead
+  // of v8::AccessorNameSetterCallback.  The difference is that the former can
+  // set a (boolean) return value. The setter should roughly follow the same
+  // conventions as many of the internal methods in objects.cc:
+  // - The return value is unset iff there was an exception.
+  // - If the ShouldThrow argument is true, the return value must not be false.
+  typedef void (*AccessorNameBooleanSetterCallback)(
+      Local<v8::Name> property, Local<v8::Value> value,
+      const PropertyCallbackInfo<v8::Boolean>& info);
+
   static Handle<AccessorInfo> MakeAccessor(
-      Isolate* isolate,
-      Handle<Name> name,
-      AccessorNameGetterCallback getter,
-      AccessorNameSetterCallback setter,
-      PropertyAttributes attributes);
+      Isolate* isolate, Handle<Name> name, AccessorNameGetterCallback getter,
+      AccessorNameBooleanSetterCallback setter, PropertyAttributes attributes);
 };
 
 }  // namespace internal

src/api-arguments.h

@@ -88,7 +88,7 @@ class PropertyCallbackArguments
         Smi::FromInt(should_throw == Object::THROW_ON_ERROR ? 1 : 0);
 
     // Here the hole is set as default value.
-    // It cannot escape into js as it's remove in Call below.
+    // It cannot escape into js as it's removed in Call below.
     values[T::kReturnValueDefaultValueIndex] =
         isolate->heap()->the_hole_value();
     values[T::kReturnValueIndex] = isolate->heap()->the_hole_value();

src/objects.cc

@@ -1417,12 +1417,20 @@ Maybe<bool> Object::SetPropertyWithAccessor(LookupIterator* it,
       return Nothing<bool>();
     }
 
-    v8::AccessorNameSetterCallback call_fun =
-        v8::ToCData<v8::AccessorNameSetterCallback>(info->setter());
-    // TODO(verwaest): We should not get here anymore once all AccessorInfos are
-    // marked as special_data_property. They cannot both be writable and not
-    // have a setter.
-    if (call_fun == nullptr) return Just(true);
+    // The actual type of call_fun is either v8::AccessorNameSetterCallback or
+    // i::Accesors::AccessorNameBooleanSetterCallback, depending on whether the
+    // AccessorInfo was created by the API or internally (see accessors.cc).
+    // Here we handle both cases using GenericNamedPropertySetterCallback and
+    // its Call method.
+    GenericNamedPropertySetterCallback call_fun =
+        v8::ToCData<GenericNamedPropertySetterCallback>(info->setter());
+
+    if (call_fun == nullptr) {
+      // TODO(verwaest): We should not get here anymore once all AccessorInfos
+      // are marked as special_data_property. They cannot both be writable and
+      // not have a setter.
+      return Just(true);
+    }
 
     if (info->is_sloppy() && !receiver->IsJSReceiver()) {
       ASSIGN_RETURN_ON_EXCEPTION_VALUE(
@@ -1432,9 +1440,15 @@ Maybe<bool> Object::SetPropertyWithAccessor(LookupIterator* it,
 
     PropertyCallbackArguments args(isolate, info->data(), *receiver, *holder,
                                    should_throw);
-    args.Call(call_fun, name, value);
+    Handle<Object> result = args.Call(call_fun, name, value);
+    // In the case of AccessorNameSetterCallback, we know that the result value
+    // cannot have been set, so the result of Call will be null.  In the case of
+    // AccessorNameBooleanSetterCallback, the result will either be null
+    // (signalling an exception) or a boolean Oddball.
     RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
-    return Just(true);
+    if (result.is_null()) return Just(true);
+    DCHECK(result->BooleanValue() || should_throw == DONT_THROW);
+    return Just(result->BooleanValue());
   }
 
   // Regular accessor.
@@ -5782,17 +5796,10 @@ Maybe<bool> JSObject::DefineOwnPropertyIgnoreAttributes(
             it->TransitionToAccessorPair(accessors, attributes);
           }
 
-          Maybe<bool> result =
-              JSObject::SetPropertyWithAccessor(it, value, should_throw);
-
-          if (current_attributes == attributes || result.IsNothing()) {
-            return result;
-          }
-
-        } else {
-          it->ReconfigureDataProperty(value, attributes);
+          return JSObject::SetPropertyWithAccessor(it, value, should_throw);
         }
 
+        it->ReconfigureDataProperty(value, attributes);
         return Just(true);
       }
       case LookupIterator::INTEGER_INDEXED_EXOTIC:

test/cctest/heap/test-alloc.cc

@@ -116,11 +116,8 @@ void TestGetter(
       v8::internal::HeapTester::TestAllocateAfterFailures()));
 }
 
-
-void TestSetter(
-    v8::Local<v8::Name> name,
-    v8::Local<v8::Value> value,
-    const v8::PropertyCallbackInfo<void>& info) {
+void TestSetter(v8::Local<v8::Name> name, v8::Local<v8::Value> value,
+                const v8::PropertyCallbackInfo<v8::Boolean>& info) {
   UNREACHABLE();
 }
 

test/mjsunit/es6/reflect.js

@@ -273,6 +273,14 @@ function prepare(target) {
 })();
 
 
+(function testReflectSetArrayLength() {
+  var y = [];
+  Object.defineProperty(y, 0, {value: 42, configurable: false});
+  assertFalse(Reflect.set(y, 'length', 0));
+  assertTrue(Reflect.set(y, 'length', 2));
+})();
+
+
 
 ////////////////////////////////////////////////////////////////////////////////
 // Reflect.has
@@ -352,6 +360,14 @@ function prepare(target) {
 })();
 
 
+(function testReflectDefinePropertyArrayLength() {
+  var y = [];
+  Object.defineProperty(y, 0, {value: 42, configurable: false});
+  assertFalse(Reflect.defineProperty(y, 'length', {value: 0}));
+  assertTrue(Reflect.defineProperty(y, 'length', {value: 2}));
+})();
+
+
 // See reflect-define-property.js for further tests.
 
 

test/mjsunit/modules-namespace1.js

@@ -32,8 +32,8 @@ assertEquals(
     Reflect.getOwnPropertyDescriptor(foo, "yo"));
 assertFalse(Reflect.deleteProperty(foo, "yo"));
 assertTrue(Reflect.has(foo, "yo"));
-// TODO(neis): The next three should be False.
-assertTrue(Reflect.set(foo, "yo", true));
+assertFalse(Reflect.set(foo, "yo", true));
+// TODO(neis): The next two should be False.
 assertTrue(Reflect.defineProperty(foo, "yo",
     Reflect.getOwnPropertyDescriptor(foo, "yo")));
 assertTrue(Reflect.defineProperty(foo, "yo", {}));