Cleaned up setting of accessors.

This CL is an intermediate step only, in the end we need to have a single
DefineOrRedefineAccessorProperty call for a single Object.defineProperty
call. Currently we can end up making two such calls, making the necessary access
checks extremely ugly and hard (impossible?) to get right for complete spec
conformance.

The bulk of the change is quite mechanical:

 * Prepare an AccessorPair *before* we add it to our data structures,
   eliminating the previous voodoo-like threading of a placeholder.

 * The previous item makes it possible to activate our check that we do not
   share AccessorPairs by accident.

 * Split a monster method into 2 quite unrelated methods.

 * Use templated To method in a few places.

Review URL: https://chromiumcodereview.appspot.com/9428026

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@10788 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

src/heap.cc

@@ -5081,9 +5081,7 @@ void Heap::Verify() {
 
   lo_space_->Verify();
 
-  // TODO(svenpanne) We should enable this when our fast->slow->fast-mode dance
-  // for setting accessor properties is fixed.
-  // VerifyNoAccessorPairSharing();
+  VerifyNoAccessorPairSharing();
 }
 
 

src/objects.cc

@@ -4362,15 +4362,14 @@ void JSObject::LookupCallback(String* name, LookupResult* result) {
 }
 
 
-// Search for a getter or setter in an elements dictionary and update its
-// attributes.  Returns either undefined if the element is non-deletable, or the
-// getter/setter pair if there is an existing one, or the hole value if the
-// element does not exist or is a normal non-getter/setter data element.
-static Object* UpdateGetterSetterInDictionary(
+// Try to update an accessor in an elements dictionary. Return true if the
+// update succeeded, and false otherwise.
+static bool UpdateGetterSetterInDictionary(
     SeededNumberDictionary* dictionary,
     uint32_t index,
-    PropertyAttributes attributes,
-    Heap* heap) {
+    bool is_getter,
+    Object* fun,
+    PropertyAttributes attributes) {
   int entry = dictionary->FindEntry(index);
   if (entry != SeededNumberDictionary::kNotFound) {
     Object* result = dictionary->ValueAt(entry);
@@ -4382,108 +4381,116 @@ static Object* UpdateGetterSetterInDictionary(
         dictionary->DetailsAtPut(entry,
                                  PropertyDetails(attributes, CALLBACKS, index));
       }
-      return result;
+      AccessorPair::cast(result)->set(is_getter, fun);
+      return true;
     }
   }
-  return heap->the_hole_value();
+  return false;
 }
 
 
-MaybeObject* JSObject::DefineGetterSetter(String* name,
-                                          PropertyAttributes attributes) {
-  Heap* heap = GetHeap();
-  // Make sure that the top context does not change when doing callbacks or
-  // interceptor calls.
-  AssertNoContextChange ncc;
-
-  // Try to flatten before operating on the string.
-  name->TryFlatten();
-
-  if (!CanSetCallback(name)) {
-    return heap->undefined_value();
-  }
-
-  uint32_t index = 0;
-  bool is_element = name->AsArrayIndex(&index);
-
-  if (is_element) {
-    switch (GetElementsKind()) {
-      case FAST_SMI_ONLY_ELEMENTS:
-      case FAST_ELEMENTS:
-      case FAST_DOUBLE_ELEMENTS:
-        break;
-      case EXTERNAL_PIXEL_ELEMENTS:
-      case EXTERNAL_BYTE_ELEMENTS:
-      case EXTERNAL_UNSIGNED_BYTE_ELEMENTS:
-      case EXTERNAL_SHORT_ELEMENTS:
-      case EXTERNAL_UNSIGNED_SHORT_ELEMENTS:
-      case EXTERNAL_INT_ELEMENTS:
-      case EXTERNAL_UNSIGNED_INT_ELEMENTS:
-      case EXTERNAL_FLOAT_ELEMENTS:
-      case EXTERNAL_DOUBLE_ELEMENTS:
-        // Ignore getters and setters on pixel and external array
-        // elements.
-        return heap->undefined_value();
-      case DICTIONARY_ELEMENTS: {
-        Object* probe = UpdateGetterSetterInDictionary(element_dictionary(),
-                                                       index,
-                                                       attributes,
-                                                       heap);
-        if (!probe->IsTheHole()) return probe;
-        // Otherwise allow to override it.
-        break;
+MaybeObject* JSObject::DefineElementAccessor(uint32_t index,
+                                             bool is_getter,
+                                             Object* fun,
+                                             PropertyAttributes attributes) {
+  switch (GetElementsKind()) {
+    case FAST_SMI_ONLY_ELEMENTS:
+    case FAST_ELEMENTS:
+    case FAST_DOUBLE_ELEMENTS:
+      break;
+    case EXTERNAL_PIXEL_ELEMENTS:
+    case EXTERNAL_BYTE_ELEMENTS:
+    case EXTERNAL_UNSIGNED_BYTE_ELEMENTS:
+    case EXTERNAL_SHORT_ELEMENTS:
+    case EXTERNAL_UNSIGNED_SHORT_ELEMENTS:
+    case EXTERNAL_INT_ELEMENTS:
+    case EXTERNAL_UNSIGNED_INT_ELEMENTS:
+    case EXTERNAL_FLOAT_ELEMENTS:
+    case EXTERNAL_DOUBLE_ELEMENTS:
+      // Ignore getters and setters on pixel and external array elements.
+      return GetHeap()->undefined_value();
+    case DICTIONARY_ELEMENTS:
+      if (UpdateGetterSetterInDictionary(element_dictionary(),
+                                         index,
+                                         is_getter,
+                                         fun,
+                                         attributes)) {
+        return GetHeap()->undefined_value();
       }
-      case NON_STRICT_ARGUMENTS_ELEMENTS: {
-        // Ascertain whether we have read-only properties or an existing
-        // getter/setter pair in an arguments elements dictionary backing
-        // store.
-        FixedArray* parameter_map = FixedArray::cast(elements());
-        uint32_t length = parameter_map->length();
-        Object* probe =
-            index < (length - 2) ? parameter_map->get(index + 2) : NULL;
-        if (probe == NULL || probe->IsTheHole()) {
-          FixedArray* arguments = FixedArray::cast(parameter_map->get(1));
-          if (arguments->IsDictionary()) {
-            SeededNumberDictionary* dictionary =
-                SeededNumberDictionary::cast(arguments);
-            probe = UpdateGetterSetterInDictionary(dictionary,
-                                                   index,
-                                                   attributes,
-                                                   heap);
-            if (!probe->IsTheHole()) return probe;
+      break;
+    case NON_STRICT_ARGUMENTS_ELEMENTS: {
+      // Ascertain whether we have read-only properties or an existing
+      // getter/setter pair in an arguments elements dictionary backing
+      // store.
+      FixedArray* parameter_map = FixedArray::cast(elements());
+      uint32_t length = parameter_map->length();
+      Object* probe =
+          index < (length - 2) ? parameter_map->get(index + 2) : NULL;
+      if (probe == NULL || probe->IsTheHole()) {
+        FixedArray* arguments = FixedArray::cast(parameter_map->get(1));
+        if (arguments->IsDictionary()) {
+          SeededNumberDictionary* dictionary =
+              SeededNumberDictionary::cast(arguments);
+          if (UpdateGetterSetterInDictionary(dictionary,
+                                             index,
+                                             is_getter,
+                                             fun,
+                                             attributes)) {
+            return GetHeap()->undefined_value();
           }
         }
-        break;
       }
+      break;
     }
-  } else {
-    // Lookup the name.
-    LookupResult result(heap->isolate());
-    LocalLookupRealNamedProperty(name, &result);
-    if (result.IsFound()) {
-      // TODO(mstarzinger): We should check for result.IsDontDelete() here once
-      // we only call into the runtime once to set both getter and setter.
-      if (result.type() == CALLBACKS) {
-        Object* obj = result.GetCallbackObject();
-        // Need to preserve old getters/setters.
-        if (obj->IsAccessorPair()) {
-          // Use set to update attributes.
-          return SetPropertyCallback(name, obj, attributes);
+  }
+
+  AccessorPair* accessors;
+  { MaybeObject* maybe_accessors = GetHeap()->AllocateAccessorPair();
+    if (!maybe_accessors->To(&accessors)) return maybe_accessors;
+  }
+  accessors->set(is_getter, fun);
+
+  { MaybeObject* maybe_ok = SetElementCallback(index, accessors, attributes);
+    if (maybe_ok->IsFailure()) return maybe_ok;
+  }
+  return GetHeap()->undefined_value();
+}
+
+
+MaybeObject* JSObject::DefinePropertyAccessor(String* name,
+                                              bool is_getter,
+                                              Object* fun,
+                                              PropertyAttributes attributes) {
+  // Lookup the name.
+  LookupResult result(GetHeap()->isolate());
+  LocalLookupRealNamedProperty(name, &result);
+  if (result.IsFound()) {
+    // TODO(mstarzinger): We should check for result.IsDontDelete() here once
+    // we only call into the runtime once to set both getter and setter.
+    if (result.type() == CALLBACKS) {
+      Object* obj = result.GetCallbackObject();
+      // Need to preserve old getters/setters.
+      if (obj->IsAccessorPair()) {
+        AccessorPair::cast(obj)->set(is_getter, fun);
+        // Use set to update attributes.
+        { MaybeObject* maybe_ok = SetPropertyCallback(name, obj, attributes);
+          if (maybe_ok->IsFailure()) return maybe_ok;
         }
+        return GetHeap()->undefined_value();
       }
     }
   }
 
   AccessorPair* accessors;
-  { MaybeObject* maybe_accessors = heap->AllocateAccessorPair();
-    if (!maybe_accessors->To<AccessorPair>(&accessors)) return maybe_accessors;
+  { MaybeObject* maybe_accessors = GetHeap()->AllocateAccessorPair();
+    if (!maybe_accessors->To(&accessors)) return maybe_accessors;
   }
+  accessors->set(is_getter, fun);
 
-  if (is_element) {
-    return SetElementCallback(index, accessors, attributes);
-  } else {
-    return SetPropertyCallback(name, accessors, attributes);
+  { MaybeObject* maybe_ok = SetPropertyCallback(name, accessors, attributes);
+    if (maybe_ok->IsFailure()) return maybe_ok;
   }
+  return GetHeap()->undefined_value();
 }
 
 
@@ -4517,19 +4524,15 @@ MaybeObject* JSObject::SetElementCallback(uint32_t index,
   PropertyDetails details = PropertyDetails(attributes, CALLBACKS);
 
   // Normalize elements to make this operation simple.
-  SeededNumberDictionary* dictionary = NULL;
-  { Object* result;
-    MaybeObject* maybe = NormalizeElements();
-    if (!maybe->ToObject(&result)) return maybe;
-    dictionary = SeededNumberDictionary::cast(result);
+  SeededNumberDictionary* dictionary;
+  { MaybeObject* maybe_dictionary = NormalizeElements();
+    if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
   }
   ASSERT(HasDictionaryElements() || HasDictionaryArgumentsElements());
 
   // Update the dictionary with the new CALLBACKS property.
-  { Object* result;
-    MaybeObject* maybe = dictionary->Set(index, structure, details);
-    if (!maybe->ToObject(&result)) return maybe;
-    dictionary = SeededNumberDictionary::cast(result);
+  { MaybeObject* maybe_dictionary = dictionary->Set(index, structure, details);
+    if (!maybe_dictionary->To(&dictionary)) return maybe_dictionary;
   }
 
   dictionary->set_requires_slow_elements();
@@ -4541,8 +4544,7 @@ MaybeObject* JSObject::SetElementCallback(uint32_t index,
     // switch to a direct backing store without the parameter map.  This
     // would allow GC of the context.
     FixedArray* parameter_map = FixedArray::cast(elements());
-    uint32_t length = parameter_map->length();
-    if (index < length - 2) {
+    if (index < static_cast<uint32_t>(parameter_map->length()) - 2) {
       parameter_map->set(index + 2, GetHeap()->the_hole_value());
     }
     parameter_map->set(1, dictionary);
@@ -4550,7 +4552,7 @@ MaybeObject* JSObject::SetElementCallback(uint32_t index,
     set_elements(dictionary);
   }
 
-  return structure;
+  return GetHeap()->undefined_value();
 }
 
 
@@ -4564,19 +4566,18 @@ MaybeObject* JSObject::SetPropertyCallback(String* name,
           < DescriptorArray::kMaxNumberOfDescriptors);
 
   // Normalize object to make this operation simple.
-  Object* ok;
   { MaybeObject* maybe_ok = NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
-    if (!maybe_ok->ToObject(&ok)) return maybe_ok;
+    if (maybe_ok->IsFailure()) return maybe_ok;
   }
 
   // For the global object allocate a new map to invalidate the global inline
   // caches which have a global property cell reference directly in the code.
   if (IsGlobalObject()) {
-    Object* new_map;
+    Map* new_map;
     { MaybeObject* maybe_new_map = map()->CopyDropDescriptors();
-      if (!maybe_new_map->ToObject(&new_map)) return maybe_new_map;
+      if (!maybe_new_map->To(&new_map)) return maybe_new_map;
     }
-    set_map(Map::cast(new_map));
+    set_map(new_map);
     // When running crankshaft, changing the map is not enough. We
     // need to deoptimize all functions that rely on this global
     // object.
@@ -4584,17 +4585,15 @@ MaybeObject* JSObject::SetPropertyCallback(String* name,
   }
 
   // Update the dictionary with the new CALLBACKS property.
-  Object* result;
-  { MaybeObject* maybe_result = SetNormalizedProperty(name, structure, details);
-    if (!maybe_result->ToObject(&result)) return maybe_result;
+  { MaybeObject* maybe_ok = SetNormalizedProperty(name, structure, details);
+    if (maybe_ok->IsFailure()) return maybe_ok;
   }
 
   if (convert_back_to_fast) {
-    { MaybeObject* maybe_ok = TransformToFastProperties(0);
-      if (!maybe_ok->ToObject(&ok)) return maybe_ok;
-    }
+    MaybeObject* maybe_ok = TransformToFastProperties(0);
+    if (maybe_ok->IsFailure()) return maybe_ok;
   }
-  return result;
+  return GetHeap()->undefined_value();
 }
 
 MaybeObject* JSObject::DefineAccessor(String* name,
@@ -4618,17 +4617,19 @@ MaybeObject* JSObject::DefineAccessor(String* name,
                                                  fun, attributes);
   }
 
-  Object* accessors;
-  { MaybeObject* maybe_accessors = DefineGetterSetter(name, attributes);
-    if (!maybe_accessors->To<Object>(&accessors)) return maybe_accessors;
-  }
-  if (accessors->IsUndefined()) return accessors;
-  if (is_getter) {
-    AccessorPair::cast(accessors)->set_getter(fun);
-  } else {
-    AccessorPair::cast(accessors)->set_setter(fun);
-  }
-  return this;
+  // Make sure that the top context does not change when doing callbacks or
+  // interceptor calls.
+  AssertNoContextChange ncc;
+
+  // Try to flatten before operating on the string.
+  name->TryFlatten();
+
+  if (!CanSetCallback(name)) return isolate->heap()->undefined_value();
+
+  uint32_t index = 0;
+  return name->AsArrayIndex(&index) ?
+      DefineElementAccessor(index, is_getter, fun, attributes) :
+      DefinePropertyAccessor(name, is_getter, fun, attributes);
 }
 
 
@@ -4691,10 +4692,9 @@ MaybeObject* JSObject::DefineAccessor(AccessorInfo* info) {
         break;
     }
 
-    Object* ok;
     { MaybeObject* maybe_ok =
           SetElementCallback(index, info, info->property_attributes());
-      if (!maybe_ok->ToObject(&ok)) return maybe_ok;
+      if (maybe_ok->IsFailure()) return maybe_ok;
     }
   } else {
     // Lookup the name.
@@ -4705,10 +4705,9 @@ MaybeObject* JSObject::DefineAccessor(AccessorInfo* info) {
     if (result.IsProperty() && (result.IsReadOnly() || result.IsDontDelete())) {
       return isolate->heap()->undefined_value();
     }
-    Object* ok;
     { MaybeObject* maybe_ok =
           SetPropertyCallback(name, info, info->property_attributes());
-      if (!maybe_ok->ToObject(&ok)) return maybe_ok;
+      if (maybe_ok->IsFailure()) return maybe_ok;
     }
   }
 
@@ -12672,6 +12671,11 @@ MaybeObject* StringDictionary::TransformPropertiesToFastFor(
                           details.index());
         descriptors->Set(next_descriptor++, &d, witness);
       } else if (type == CALLBACKS) {
+        if (value->IsAccessorPair()) {
+          MaybeObject* maybe_copy =
+              AccessorPair::cast(value)->CopyWithoutTransitions();
+          if (!maybe_copy->To(&value)) return maybe_copy;
+        }
         CallbacksDescriptor d(String::cast(key),
                               value,
                               details.attributes(),

src/objects.h

@@ -2139,10 +2139,16 @@ class JSObject: public JSReceiver {
       String* name,
       Object* structure,
       PropertyAttributes attributes);
-  MUST_USE_RESULT MaybeObject* DefineGetterSetter(
+  MUST_USE_RESULT MaybeObject* DefineElementAccessor(
+      uint32_t index,
+      bool is_getter,
+      Object* fun,
+      PropertyAttributes attributes);
+  MUST_USE_RESULT MaybeObject* DefinePropertyAccessor(
       String* name,
+      bool is_getter,
+      Object* fun,
       PropertyAttributes attributes);
-
   void LookupInDescriptor(String* name, LookupResult* result);
 
   // Returns the hidden properties backing store object, currently
@@ -7837,6 +7843,15 @@ class AccessorPair: public Struct {
 
   MUST_USE_RESULT MaybeObject* CopyWithoutTransitions();
 
+  // TODO(svenpanne) Evil temporary helper, will vanish soon...
+  void set(bool modify_getter, Object* value) {
+    if (modify_getter) {
+      set_getter(value);
+    } else {
+      set_setter(value);
+    }
+  }
+
 #ifdef OBJECT_PRINT
   void AccessorPairPrint(FILE* out = stdout);
 #endif