Make setting of accessors even more atomic.

src/objects.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 4306 matching lines @@
   }
   result->NotFound();
 }
 
 
 // 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,
-    AccessorComponent component,
-    Object* fun,
+    Object* getter,
+    Object* setter,
     PropertyAttributes attributes) {
   int entry = dictionary->FindEntry(index);
   if (entry != SeededNumberDictionary::kNotFound) {
     Object* result = dictionary->ValueAt(entry);
     PropertyDetails details = dictionary->DetailsAt(entry);
-    // TODO(mstarzinger): We should check for details.IsDontDelete() here once
-    // we only call into the runtime once to set both getter and setter.
     if (details.type() == CALLBACKS && result->IsAccessorPair()) {
+      ASSERT(!details.IsDontDelete());
       if (details.attributes() != attributes) {
         dictionary->DetailsAtPut(entry,
                                  PropertyDetails(attributes, CALLBACKS, index));
       }
-      AccessorPair::cast(result)->set(component, fun);
+      AccessorPair::cast(result)->SetComponents(getter, setter);
       return true;
     }
   }
   return false;
 }
 
 
 MaybeObject* JSObject::DefineElementAccessor(uint32_t index,
-                                             AccessorComponent component,
-                                             Object* fun,
+                                             Object* getter,
+                                             Object* setter,
                                              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,
-                                         component,
-                                         fun,
+                                         getter,
+                                         setter,
                                          attributes)) {
         return GetHeap()->undefined_value();
       }
       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,
-                                             component,
-                                             fun,
+                                             getter,
+                                             setter,
                                              attributes)) {
             return GetHeap()->undefined_value();
           }
         }
       }
       break;
     }
   }
 
   AccessorPair* accessors;
   { MaybeObject* maybe_accessors = GetHeap()->AllocateAccessorPair();
     if (!maybe_accessors->To(&accessors)) return maybe_accessors;
   }
-  accessors->set(component, fun);
+  accessors->SetComponents(getter, setter);
 
   return SetElementCallback(index, accessors, attributes);
 }
 
 
 MaybeObject* JSObject::DefinePropertyAccessor(String* name,
-                                              AccessorComponent component,
-                                              Object* fun,
+                                              Object* getter,
+                                              Object* setter,
                                               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) {
+      ASSERT(!result.IsDontDelete());
       Object* obj = result.GetCallbackObject();
       // Need to preserve old getters/setters.
       if (obj->IsAccessorPair()) {
         AccessorPair* copy;
         { MaybeObject* maybe_copy =
               AccessorPair::cast(obj)->CopyWithoutTransitions();
           if (!maybe_copy->To(&copy)) return maybe_copy;
         }
-        copy->set(component, fun);
+        copy->SetComponents(getter, setter);
         // Use set to update attributes.
         return SetPropertyCallback(name, copy, attributes);
       }
     }
   }
 
   AccessorPair* accessors;
   { MaybeObject* maybe_accessors = GetHeap()->AllocateAccessorPair();
     if (!maybe_accessors->To(&accessors)) return maybe_accessors;
   }
-  accessors->set(component, fun);
+  accessors->SetComponents(getter, setter);
 
   return SetPropertyCallback(name, accessors, attributes);
 }
 
 
 bool JSObject::CanSetCallback(String* name) {
   ASSERT(!IsAccessCheckNeeded() ||
          GetIsolate()->MayNamedAccess(this, name, v8::ACCESS_SET));
 
   // Check if there is an API defined callback object which prohibits
@@ skipping 50 matching lines @@
     set_elements(dictionary);
   }
 
   return GetHeap()->undefined_value();
 }
 
 
 MaybeObject* JSObject::SetPropertyCallback(String* name,
                                            Object* structure,
                                            PropertyAttributes attributes) {
-  PropertyDetails details = PropertyDetails(attributes, CALLBACKS);
-
-  bool convert_back_to_fast = HasFastProperties() &&

[Michael Starzinger, 2012/03/21 12:33:08]

Here we only convert the object back to fast properties when it had fast
properties before. With the current change we will always convert back to fast
properties after defining an accessor. I am not sure this what we want.

[Sven Panne, 2012/03/21 13:14:18]

This was an accidental change in semantics, I don't think we always want to
convert to fast mode, e.g. in the case of objects used as hash maps. I'll add
the right logic to the callers. Done.

-      (map()->instance_descriptors()->number_of_descriptors()
-          < DescriptorArray::kMaxNumberOfDescriptors);
-
   // Normalize object to make this operation simple.
   { MaybeObject* maybe_ok = NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
     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()) {
     Map* new_map;
     { MaybeObject* maybe_new_map = map()->CopyDropDescriptors();
       if (!maybe_new_map->To(&new_map)) return maybe_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.
     Deoptimizer::DeoptimizeGlobalObject(this);
   }
 
   // Update the dictionary with the new CALLBACKS property.
+  PropertyDetails details = PropertyDetails(attributes, CALLBACKS);
   { 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->IsFailure()) return maybe_ok;
-  }
   return GetHeap()->undefined_value();
 }
 
+
+void JSObject::DefineAccessor(Handle<JSObject> object,
+                              Handle<String> name,
+                              Handle<Object> getter,
+                              Handle<Object> setter,
+                              PropertyAttributes attributes) {
+  CALL_HEAP_FUNCTION_VOID(
+      object->GetIsolate(),
+      object->DefineAccessor(*name, *getter, *setter, attributes));
+}
+
 MaybeObject* JSObject::DefineAccessor(String* name,
-                                      AccessorComponent component,
-                                      Object* fun,
+                                      Object* getter,
+                                      Object* setter,
                                       PropertyAttributes attributes) {
-  ASSERT(fun->IsSpecFunction() || fun->IsUndefined());
   Isolate* isolate = GetIsolate();
   // Check access rights if needed.
   if (IsAccessCheckNeeded() &&
       !isolate->MayNamedAccess(this, name, v8::ACCESS_SET)) {
     isolate->ReportFailedAccessCheck(this, v8::ACCESS_SET);
     return isolate->heap()->undefined_value();
   }
 
   if (IsJSGlobalProxy()) {
     Object* proto = GetPrototype();
     if (proto->IsNull()) return this;
     ASSERT(proto->IsJSGlobalObject());
-    return JSObject::cast(proto)->DefineAccessor(name, component,
-                                                 fun, attributes);
+    return JSObject::cast(proto)->DefineAccessor(
+        name, getter, setter, attributes);
   }
 
   // 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, component, fun, attributes) :
-      DefinePropertyAccessor(name, component, fun, attributes);
+      DefineElementAccessor(index, getter, setter, attributes) :
+      DefinePropertyAccessor(name, getter, setter, attributes);
 }
 
 
 MaybeObject* JSObject::DefineAccessor(AccessorInfo* info) {
   Isolate* isolate = GetIsolate();
   String* name = String::cast(info->name());
   // Check access rights if needed.
   if (IsAccessCheckNeeded() &&
       !isolate->MayNamedAccess(this, name, v8::ACCESS_SET)) {
     isolate->ReportFailedAccessCheck(this, v8::ACCESS_SET);
@@ skipping 1347 matching lines @@
   { MaybeObject* maybe_copy = heap->AllocateAccessorPair();
     if (!maybe_copy->To(&copy)) return maybe_copy;
   }
   copy->set_getter(getter()->IsMap() ? heap->the_hole_value() : getter());
   copy->set_setter(setter()->IsMap() ? heap->the_hole_value() : setter());
   return copy;
 }
 
 
 Object* AccessorPair::SafeGet(AccessorComponent component) {
-    Object* accessor = get(component);
+    Object* accessor = (component == ACCESSOR_GETTER) ? getter() : setter();
     return accessor->IsTheHole() ? GetHeap()->undefined_value() : accessor;
 }
 
 
 MaybeObject* DeoptimizationInputData::Allocate(int deopt_entry_count,
                                                PretenureFlag pretenure) {
   ASSERT(deopt_entry_count > 0);
   return HEAP->AllocateFixedArray(LengthFor(deopt_entry_count),
                                   pretenure);
 }
@@ skipping 6976 matching lines @@
   set_year(Smi::FromInt(year), SKIP_WRITE_BARRIER);
   set_month(Smi::FromInt(month), SKIP_WRITE_BARRIER);
   set_day(Smi::FromInt(day), SKIP_WRITE_BARRIER);
   set_weekday(Smi::FromInt(weekday), SKIP_WRITE_BARRIER);
   set_hour(Smi::FromInt(hour), SKIP_WRITE_BARRIER);
   set_min(Smi::FromInt(min), SKIP_WRITE_BARRIER);
   set_sec(Smi::FromInt(sec), SKIP_WRITE_BARRIER);
 }
 
 } }  // namespace v8::internal