[api] Default native data property setter to replace the setter if the property is writable.

BUG=chromium:580584
LOG=y

Review URL: https://codereview.chromium.org/1632603002

Cr-Commit-Position: refs/heads/master@{#33551}

src/accessors.h

@@ -44,6 +44,12 @@ class AccessorInfo;
   V(ScriptIsEmbedderDebugScript)  \
   V(StringLength)
 
+#define ACCESSOR_SETTER_LIST(V)        \
+  V(ReconfigureToDataProperty)         \
+  V(ObservedReconfigureToDataProperty) \
+  V(ArrayLengthSetter)                 \
+  V(FunctionPrototypeSetter)
+
 // Accessors contains all predefined proxy accessors.
 
 class Accessors : public AllStatic {
@@ -53,16 +59,18 @@ class Accessors : public AllStatic {
   static void name##Getter(                               \
       v8::Local<v8::Name> name,                           \
       const v8::PropertyCallbackInfo<v8::Value>& info);   \
-  static void name##Setter(                               \
-      v8::Local<v8::Name> name,                           \
-      v8::Local<v8::Value> value,                         \
-      const v8::PropertyCallbackInfo<void>& info);   \
   static Handle<AccessorInfo> name##Info(                 \
       Isolate* isolate,                                   \
       PropertyAttributes attributes);
   ACCESSOR_INFO_LIST(ACCESSOR_INFO_DECLARATION)
 #undef ACCESSOR_INFO_DECLARATION
 
+#define ACCESSOR_SETTER_DECLARATION(name)                                \
+  static void name(v8::Local<v8::Name> name, v8::Local<v8::Value> value, \
+                   const v8::PropertyCallbackInfo<void>& info);
+  ACCESSOR_SETTER_LIST(ACCESSOR_SETTER_DECLARATION)
+#undef ACCESSOR_SETTER_DECLARATION
+
   enum DescriptorId {
 #define ACCESSOR_INFO_DECLARATION(name) \
     k##name##Getter, \

src/api-natives.cc

@@ -202,14 +202,12 @@ MaybeHandle<JSObject> ConfigureInstance(Isolate* isolate, Handle<JSObject> obj,
   return obj;
 }
 
-
 MaybeHandle<JSObject> InstantiateObject(Isolate* isolate,
-                                        Handle<ObjectTemplateInfo> data) {
+                                        Handle<ObjectTemplateInfo> info) {
   // Enter a new scope.  Recursion could otherwise create a lot of handles.
   HandleScope scope(isolate);
   // Fast path.
   Handle<JSObject> result;
-  auto info = Handle<ObjectTemplateInfo>::cast(data);
   auto constructor = handle(info->constructor(), isolate);
   Handle<JSFunction> cons;
   if (constructor->IsUndefined()) {

src/api.cc

@@ -15,6 +15,7 @@
 #include "include/v8-experimental.h"
 #include "include/v8-profiler.h"
 #include "include/v8-testing.h"
+#include "src/accessors.h"
 #include "src/api-experimental.h"
 #include "src/api-natives.h"
 #include "src/assert-scope.h"
@@ -1118,20 +1119,23 @@ static i::Handle<i::AccessorInfo> SetAccessorInfoProperties(
   return obj;
 }
 
-
 template <typename Getter, typename Setter>
 static i::Handle<i::AccessorInfo> MakeAccessorInfo(
     v8::Local<Name> name, Getter getter, Setter setter, v8::Local<Value> data,
     v8::AccessControl settings, v8::PropertyAttribute attributes,
-    v8::Local<AccessorSignature> signature) {
+    v8::Local<AccessorSignature> signature, bool is_special_data_property) {
   i::Isolate* isolate = Utils::OpenHandle(*name)->GetIsolate();
   i::Handle<i::AccessorInfo> obj = isolate->factory()->NewAccessorInfo();
   SET_FIELD_WRAPPED(obj, set_getter, getter);
+  if (is_special_data_property && setter == nullptr) {
+    setter = reinterpret_cast<Setter>(&i::Accessors::ReconfigureToDataProperty);
+  }
   SET_FIELD_WRAPPED(obj, set_setter, setter);
   if (data.IsEmpty()) {
     data = v8::Undefined(reinterpret_cast<v8::Isolate*>(isolate));
   }
   obj->set_data(*Utils::OpenHandle(*data));
+  obj->set_is_special_data_property(is_special_data_property);
   return SetAccessorInfoProperties(obj, name, settings, attributes, signature);
 }
 
@@ -1277,22 +1281,18 @@ static inline i::Handle<i::TemplateInfo> GetTemplateInfo(
   return Utils::OpenHandle(object_template);
 }
 
-
-template<typename Getter, typename Setter, typename Data, typename Template>
-static bool TemplateSetAccessor(
-    Template* template_obj,
-    v8::Local<Name> name,
-    Getter getter,
-    Setter setter,
-    Data data,
+template <typename Getter, typename Setter, typename Data, typename Template>
+static bool TemplateSetAccessor(Template* template_obj, v8::Local<Name> name,
+                                Getter getter, Setter setter, Data data,
                                 AccessControl settings,
                                 PropertyAttribute attribute,
-    v8::Local<AccessorSignature> signature) {
+                                v8::Local<AccessorSignature> signature,
+                                bool is_special_data_property) {
   auto isolate = Utils::OpenHandle(template_obj)->GetIsolate();
   ENTER_V8(isolate);
   i::HandleScope scope(isolate);
   auto obj = MakeAccessorInfo(name, getter, setter, data, settings, attribute,
-                              signature);
+                              signature, is_special_data_property);
   if (obj.is_null()) return false;
   auto info = GetTemplateInfo(isolate, template_obj);
   i::ApiNatives::AddNativeDataProperty(isolate, info, obj);
@@ -1307,8 +1307,8 @@ void Template::SetNativeDataProperty(v8::Local<String> name,
                                      PropertyAttribute attribute,
                                      v8::Local<AccessorSignature> signature,
                                      AccessControl settings) {
-  TemplateSetAccessor(
-      this, name, getter, setter, data, settings, attribute, signature);
+  TemplateSetAccessor(this, name, getter, setter, data, settings, attribute,
+                      signature, true);
 }
 
 
@@ -1319,8 +1319,8 @@ void Template::SetNativeDataProperty(v8::Local<Name> name,
                                      PropertyAttribute attribute,
                                      v8::Local<AccessorSignature> signature,
                                      AccessControl settings) {
-  TemplateSetAccessor(
-      this, name, getter, setter, data, settings, attribute, signature);
+  TemplateSetAccessor(this, name, getter, setter, data, settings, attribute,
+                      signature, true);
 }
 
 
@@ -1342,8 +1342,8 @@ void ObjectTemplate::SetAccessor(v8::Local<String> name,
                                  v8::Local<Value> data, AccessControl settings,
                                  PropertyAttribute attribute,
                                  v8::Local<AccessorSignature> signature) {
-  TemplateSetAccessor(
-      this, name, getter, setter, data, settings, attribute, signature);
+  TemplateSetAccessor(this, name, getter, setter, data, settings, attribute,
+                      signature, i::FLAG_disable_old_api_accessors);
 }
 
 
@@ -1353,8 +1353,8 @@ void ObjectTemplate::SetAccessor(v8::Local<Name> name,
                                  v8::Local<Value> data, AccessControl settings,
                                  PropertyAttribute attribute,
                                  v8::Local<AccessorSignature> signature) {
-  TemplateSetAccessor(
-      this, name, getter, setter, data, settings, attribute, signature);
+  TemplateSetAccessor(this, name, getter, setter, data, settings, attribute,
+                      signature, i::FLAG_disable_old_api_accessors);
 }
 
 
@@ -3919,7 +3919,7 @@ static Maybe<bool> ObjectSetAccessor(Local<Context> context, Object* self,
       i::Handle<i::JSObject>::cast(Utils::OpenHandle(self));
   v8::Local<AccessorSignature> signature;
   auto info = MakeAccessorInfo(name, getter, setter, data, settings, attributes,
-                               signature);
+                               signature, i::FLAG_disable_old_api_accessors);
   if (info.is_null()) return Nothing<bool>();
   bool fast = obj->HasFastProperties();
   i::Handle<i::Object> result;

src/flag-definitions.h

@@ -867,6 +867,10 @@ DEFINE_INT(external_allocation_limit_incremental_time, 1,
            "Time spent in incremental marking steps (in ms) once the external "
            "allocation limit is reached")
 
+DEFINE_BOOL(disable_old_api_accessors, false,
+            "Disable old-style API accessors whose setters trigger through the "
+            "prototype chain")
+
 //
 // Dev shell flags
 //

src/lookup.h

@@ -237,6 +237,9 @@ class LookupIterator final BASE_EMBEDDED {
     DCHECK(has_property_);
     return property_details_;
   }
+  PropertyAttributes property_attributes() const {
+    return property_details().attributes();
+  }
   bool IsConfigurable() const { return property_details().IsConfigurable(); }
   bool IsReadOnly() const { return property_details().IsReadOnly(); }
   Representation representation() const {

src/objects.cc

@@ -1192,7 +1192,6 @@ bool AccessorInfo::IsCompatibleReceiverMap(Isolate* isolate,
       ->IsTemplateFor(*map);
 }
 
-
 Maybe<bool> Object::SetPropertyWithAccessor(LookupIterator* it,
                                             Handle<Object> value,
                                             ShouldThrow should_throw) {
@@ -1217,11 +1216,10 @@ Maybe<bool> Object::SetPropertyWithAccessor(LookupIterator* it,
 
     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);
-    // TODO(verwaest): Shouldn't this case be unreachable (at least in the long
-    // run?) Should we have AccessorInfo with missing setter that are
-    // "writable"? If they aren't writable, shouldn't we have bailed out already
-    // earlier?
 
     LOG(isolate, ApiNamedPropertyAccess("store", *holder, *name));
     PropertyCallbackArguments args(isolate, info->data(), *receiver, *holder,
@@ -1350,7 +1348,7 @@ Maybe<PropertyAttributes> JSObject::GetPropertyAttributesWithFailedAccessCheck(
   Handle<JSObject> checked = it->GetHolder<JSObject>();
   while (AllCanRead(it)) {
     if (it->state() == LookupIterator::ACCESSOR) {
-      return Just(it->property_details().attributes());
+      return Just(it->property_attributes());
     }
     DCHECK_EQ(LookupIterator::INTERCEPTOR, it->state());
     auto result = GetPropertyAttributesWithInterceptor(it);
@@ -4219,8 +4217,7 @@ Maybe<bool> Object::SetSuperProperty(LookupIterator* it, Handle<Object> value,
                                             should_throw);
 
       case LookupIterator::DATA: {
-        PropertyDetails details = own_lookup.property_details();
-        if (details.IsReadOnly()) {
+        if (own_lookup.IsReadOnly()) {
           return WriteToReadOnlyProperty(&own_lookup, value, should_throw);
         }
         return SetDataProperty(&own_lookup, value);
@@ -5268,18 +5265,23 @@ Maybe<bool> JSObject::DefineOwnPropertyIgnoreAttributes(
         // Special handling for AccessorInfo, which behaves like a data
         // property.
         if (accessors->IsAccessorInfo() && handling == DONT_FORCE_FIELD) {
-          PropertyDetails details = it->property_details();
+          PropertyAttributes current_attributes = it->property_attributes();
           // Ensure the context isn't changed after calling into accessors.
           AssertNoContextChange ncc(it->isolate());
 
+          // Update the attributes before calling the setter. The setter may
+          // later change the shape of the property.
+          if (current_attributes != attributes) {
+            it->TransitionToAccessorPair(accessors, attributes);
+          }
+
           Maybe<bool> result =
               JSObject::SetPropertyWithAccessor(it, value, should_throw);
-          if (result.IsNothing() || !result.FromJust()) return result;
 
-          if (details.attributes() == attributes) return Just(true);
+          if (current_attributes == attributes || result.IsNothing()) {
+            return result;
+          }
 
-          // Reconfigure the accessor if attributes mismatch.
-          it->TransitionToAccessorPair(accessors, attributes);
         } else {
           it->ReconfigureDataProperty(value, attributes);
         }
@@ -5299,10 +5301,9 @@ Maybe<bool> JSObject::DefineOwnPropertyIgnoreAttributes(
                                             should_throw);
 
       case LookupIterator::DATA: {
-        PropertyDetails details = it->property_details();
         Handle<Object> old_value = it->factory()->the_hole_value();
         // Regular property update if the attributes match.
-        if (details.attributes() == attributes) {
+        if (it->property_attributes() == attributes) {
           return SetDataProperty(it, value);
         }
 
@@ -5458,7 +5459,7 @@ Maybe<PropertyAttributes> JSReceiver::GetPropertyAttributes(
         return Just(ABSENT);
       case LookupIterator::ACCESSOR:
       case LookupIterator::DATA:
-        return Just(it->property_details().attributes());
+        return Just(it->property_attributes());
     }
   }
   return Just(ABSENT);
@@ -7201,7 +7202,7 @@ Maybe<bool> JSProxy::AddPrivateProperty(Isolate* isolate, Handle<JSProxy> proxy,
 
   if (it.IsFound()) {
     DCHECK_EQ(LookupIterator::DATA, it.state());
-    DCHECK_EQ(DONT_ENUM, it.property_details().attributes());
+    DCHECK_EQ(DONT_ENUM, it.property_attributes());
     it.WriteDataValue(value);
     return Just(true);
   }

src/snapshot/serialize.cc

@@ -268,8 +268,13 @@ ExternalReferenceTable::ExternalReferenceTable(Isolate* isolate) {
   static const AccessorRefTable accessors[] = {
 #define ACCESSOR_INFO_DECLARATION(name)                                     \
   { FUNCTION_ADDR(&Accessors::name##Getter), "Accessors::" #name "Getter" } \
-  , {FUNCTION_ADDR(&Accessors::name##Setter), "Accessors::" #name "Setter"},
+  ,
       ACCESSOR_INFO_LIST(ACCESSOR_INFO_DECLARATION)
+#undef ACCESSOR_INFO_DECLARATION
+#define ACCESSOR_SETTER_DECLARATION(name)                  \
+  { FUNCTION_ADDR(&Accessors::name), "Accessors::" #name } \
+  ,
+          ACCESSOR_SETTER_LIST(ACCESSOR_SETTER_DECLARATION)
 #undef ACCESSOR_INFO_DECLARATION
   };
 

test/mjsunit/regress/regress-crbug-580584.js

+// Copyright 2016 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.
+
+function f() { return arguments }
+
+// Reconfiguring function.name should update both the attributes and the value.
+Object.defineProperty(f, "name", {
+  writable: true, configurable: true, value: 10});
+assertEquals({value: 10, writable: true, enumerable: false, configurable: true},
+             Object.getOwnPropertyDescriptor(f, "name"));
+
+var args = f();
+
+// Setting a value for arguments[Symbol.iterator] should not affect the
+// attributes.
+args[Symbol.iterator] = 10;
+assertEquals({value: 10, writable: true, configurable: true, enumerable: false},
+             Object.getOwnPropertyDescriptor(args, Symbol.iterator));