Confusion on changing data property callback attributes

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index 956e46f73b196265bff0b341e6924ccecde882b9..902b715df0217593f34c4d559232b63c4012da21 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -4318,9 +4318,6 @@ MaybeHandle<Object> JSObject::SetPropertyForResult(
 // callback setter removed.  The two lines looking up the LookupResult
 // result are also added.  If one of the functions is changed, the other
 // should be.
-// Note that this method cannot be used to set the prototype of a function
-// because ConvertDescriptorToField() which is called in "case CALLBACKS:"
-// doesn't handle function prototypes correctly.
 MaybeHandle<Object> JSObject::SetLocalPropertyIgnoreAttributes(
     Handle<JSObject> object,
     Handle<Name> name,
@@ -4329,7 +4326,8 @@ MaybeHandle<Object> JSObject::SetLocalPropertyIgnoreAttributes(
     ValueType value_type,
     StoreMode mode,
     ExtensibilityCheck extensibility_check,
-    StoreFromKeyed store_from_keyed) {
+    StoreFromKeyed store_from_keyed,
+    ExecutableAccessorInfoHandling handling) {
   Isolate* isolate = object->GetIsolate();
 
   // Make sure that the top context does not change when doing callbacks or
@@ -4384,6 +4382,8 @@ MaybeHandle<Object> JSObject::SetLocalPropertyIgnoreAttributes(
     old_attributes = lookup.GetAttributes();
   }
 
+  bool dont_enqueue_records = false;
+
   // Check of IsReadOnly removed from here in clone.
   if (lookup.IsTransition()) {
     Handle<Object> result;
@@ -4408,8 +4408,47 @@ MaybeHandle<Object> JSObject::SetLocalPropertyIgnoreAttributes(
         }
         break;
       case CALLBACKS:
-        ConvertAndSetLocalProperty(&lookup, name, value, attributes);
+      {
+        Handle<Object> callback(lookup.GetCallbackObject(), isolate);
+        if (callback->IsExecutableAccessorInfo() &&
+            handling == DONT_FORCE_FIELD) {
+          Handle<Object> result;
+          ASSIGN_RETURN_ON_EXCEPTION(
+              isolate, result,
+              JSObject::SetPropertyWithCallback(object,
+                                                callback,
+                                                name,
+                                                value,
+                                                handle(lookup.holder()),
+                                                STRICT),
+              Object);
+
+          if (attributes != lookup.GetAttributes()) {
+            Handle<ExecutableAccessorInfo> new_data =
+                Accessors::CloneAccessor(
+                    isolate, Handle<ExecutableAccessorInfo>::cast(callback));
+            new_data->set_property_attributes(attributes);
+            if (attributes & READ_ONLY) {
+              // This way we don't have to introduce a lookup to the setter,
+              // simply make it unavailable to reflect the attributes.
+              new_data->clear_setter();
+            }
+
+            SetPropertyCallback(object, name, new_data, attributes);
+          } else if (is_observed) {

[ulan, 2014/05/23 10:14:48]

Shouldn't we check is_observed independent of (attributes !=
lookup.GetAttributes()) ?

[mvstanton, 2014/05/27 13:31:24]

Good catch. And I renamed the variable from dont_enqueue_records to
executed_set_prototype when is less awkward. Basically setting the prototype is
the one exception for this "observed" infrastructure.

+            // If we are setting the prototype of a function and are observed,
+            // don't send change records because the prototype handles that
+            // itself.
+            dont_enqueue_records = object->IsJSFunction() &&
+                String::Equals(isolate->factory()->prototype_string(),
+                               Handle<String>::cast(name)) &&
+                Handle<JSFunction>::cast(object)->should_have_prototype();
+          }
+        } else {
+          ConvertAndSetLocalProperty(&lookup, name, value, attributes);
+        }
         break;
+      }
       case NONEXISTENT:
       case HANDLER:
       case INTERCEPTOR:
@@ -4417,7 +4456,7 @@ MaybeHandle<Object> JSObject::SetLocalPropertyIgnoreAttributes(
     }
   }
 
-  if (is_observed) {
+  if (is_observed && !dont_enqueue_records) {
     if (lookup.IsTransition()) {
       EnqueueChangeRecord(object, "add", name, old_value);
     } else if (old_value->IsTheHole()) {