Make setting of accessors even more atomic.

Now the whole getter/setter/attributes triple gets created/set together,
avoiding any hacks regarding previous values/attributes, making things a lot
simpler.

While doing this, an interesting problem surfaced, which has been there for a
long time: After adding/changing acessors in slow mode, we could potentially
fail going back to fast mode because of a failed memory allocation, signaling
the need for a GC. But we have already changed the object in slow mode, so we
are not idempotent and the retry would trigger a newly inserted assertion
(namely, that the code obeys access restrictions). This has been solved by
splitting the transformation to fast mode from the actual setting of the
accessors.

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

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

src/api.cc

@@ -3064,8 +3064,11 @@ bool Object::SetAccessor(Handle<String> name,
   i::Handle<i::AccessorInfo> info = MakeAccessorInfo(name,
                                                      getter, setter, data,
                                                      settings, attributes);
+  bool fast = Utils::OpenHandle(this)->HasFastProperties();
   i::Handle<i::Object> result = i::SetAccessor(Utils::OpenHandle(this), info);
-  return !result.is_null() && !result->IsUndefined();
+  if (result.is_null() || result->IsUndefined()) return false;
+  if (fast) i::JSObject::TransformToFastProperties(Utils::OpenHandle(this), 0);
+  return true;
 }
 
 

src/objects.cc

@@ -4324,21 +4324,20 @@ void JSObject::LookupCallback(String* name, LookupResult* result) {
 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;
     }
   }
@@ -4347,8 +4346,8 @@ static bool UpdateGetterSetterInDictionary(
 
 
 MaybeObject* JSObject::DefineElementAccessor(uint32_t index,
-                                             AccessorComponent component,
-                                             Object* fun,
+                                             Object* getter,
+                                             Object* setter,
                                              PropertyAttributes attributes) {
   switch (GetElementsKind()) {
     case FAST_SMI_ONLY_ELEMENTS:
@@ -4369,8 +4368,8 @@ MaybeObject* JSObject::DefineElementAccessor(uint32_t index,
     case DICTIONARY_ELEMENTS:
       if (UpdateGetterSetterInDictionary(element_dictionary(),
                                          index,
-                                         component,
-                                         fun,
+                                         getter,
+                                         setter,
                                          attributes)) {
         return GetHeap()->undefined_value();
       }
@@ -4390,8 +4389,8 @@ MaybeObject* JSObject::DefineElementAccessor(uint32_t index,
               SeededNumberDictionary::cast(arguments);
           if (UpdateGetterSetterInDictionary(dictionary,
                                              index,
-                                             component,
-                                             fun,
+                                             getter,
+                                             setter,
                                              attributes)) {
             return GetHeap()->undefined_value();
           }
@@ -4405,23 +4404,22 @@ MaybeObject* JSObject::DefineElementAccessor(uint32_t index,
   { 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()) {
@@ -4430,7 +4428,7 @@ MaybeObject* JSObject::DefinePropertyAccessor(String* name,
               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);
       }
@@ -4441,7 +4439,7 @@ MaybeObject* JSObject::DefinePropertyAccessor(String* name,
   { 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);
 }
@@ -4512,12 +4510,6 @@ MaybeObject* JSObject::SetElementCallback(uint32_t index,
 MaybeObject* JSObject::SetPropertyCallback(String* name,
                                            Object* structure,
                                            PropertyAttributes attributes) {
-  PropertyDetails details = PropertyDetails(attributes, CALLBACKS);
-
-  bool convert_back_to_fast = HasFastProperties() &&
-      (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;
@@ -4538,22 +4530,29 @@ MaybeObject* JSObject::SetPropertyCallback(String* name,
   }
 
   // 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() &&
@@ -4566,8 +4565,8 @@ MaybeObject* JSObject::DefineAccessor(String* name,
     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
@@ -4581,8 +4580,8 @@ MaybeObject* JSObject::DefineAccessor(String* name,
 
   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);
 }
 
 
@@ -4696,7 +4695,7 @@ Object* JSObject::LookupAccessor(String* name, AccessorComponent component) {
           Object* element = dictionary->ValueAt(entry);
           if (dictionary->DetailsAt(entry).type() == CALLBACKS &&
               element->IsAccessorPair()) {
-            return AccessorPair::cast(element)->SafeGet(component);
+            return AccessorPair::cast(element)->GetComponent(component);
           }
         }
       }
@@ -4712,7 +4711,7 @@ Object* JSObject::LookupAccessor(String* name, AccessorComponent component) {
         if (result.type() == CALLBACKS) {
           Object* obj = result.GetCallbackObject();
           if (obj->IsAccessorPair()) {
-            return AccessorPair::cast(obj)->SafeGet(component);
+            return AccessorPair::cast(obj)->GetComponent(component);
           }
         }
       }
@@ -5949,8 +5948,8 @@ MaybeObject* AccessorPair::CopyWithoutTransitions() {
 }
 
 
-Object* AccessorPair::SafeGet(AccessorComponent component) {
-    Object* accessor = get(component);
+Object* AccessorPair::GetComponent(AccessorComponent component) {
+    Object* accessor = (component == ACCESSOR_GETTER) ? getter() : setter();
     return accessor->IsTheHole() ? GetHeap()->undefined_value() : accessor;
 }
 

src/objects.h

@@ -1627,9 +1627,14 @@ class JSObject: public JSReceiver {
       String* name,
       bool continue_search);
 
+  static void DefineAccessor(Handle<JSObject> object,
+                             Handle<String> name,
+                             Handle<Object> getter,
+                             Handle<Object> setter,
+                             PropertyAttributes attributes);
   MUST_USE_RESULT MaybeObject* DefineAccessor(String* name,
-                                              AccessorComponent component,
-                                              Object* fun,
+                                              Object* getter,
+                                              Object* setter,
                                               PropertyAttributes attributes);
   Object* LookupAccessor(String* name, AccessorComponent component);
 
@@ -2178,13 +2183,13 @@ class JSObject: public JSReceiver {
       PropertyAttributes attributes);
   MUST_USE_RESULT MaybeObject* DefineElementAccessor(
       uint32_t index,
-      AccessorComponent component,
-      Object* fun,
+      Object* getter,
+      Object* setter,
       PropertyAttributes attributes);
   MUST_USE_RESULT MaybeObject* DefinePropertyAccessor(
       String* name,
-      AccessorComponent component,
-      Object* fun,
+      Object* getter,
+      Object* setter,
       PropertyAttributes attributes);
   void LookupInDescriptor(String* name, LookupResult* result);
 
@@ -8045,23 +8050,15 @@ class AccessorPair: public Struct {
 
   MUST_USE_RESULT MaybeObject* CopyWithoutTransitions();
 
-  Object* get(AccessorComponent component) {
-    ASSERT(component == ACCESSOR_GETTER || component == ACCESSOR_SETTER);
-    return (component == ACCESSOR_GETTER) ? getter() : setter();
-  }
+  // Note: Returns undefined instead in case of a hole.
+  Object* GetComponent(AccessorComponent component);
 
-  void set(AccessorComponent component, Object* value) {
-    ASSERT(component == ACCESSOR_GETTER || component == ACCESSOR_SETTER);
-    if (component == ACCESSOR_GETTER) {
-      set_getter(value);
-    } else {
-      set_setter(value);
-    }
+  // Set both components, skipping arguments which are a JavaScript null.
+  void SetComponents(Object* getter, Object* setter) {
+    if (!getter->IsNull()) set_getter(getter);
+    if (!setter->IsNull()) set_setter(setter);
   }
 
-  // Same as get, but returns undefined instead of the hole.
-  Object* SafeGet(AccessorComponent component);
-
   bool ContainsAccessor() {
     return IsJSAccessor(getter()) || IsJSAccessor(setter());
   }

src/runtime.cc

@@ -1065,10 +1065,10 @@ static MaybeObject* GetOwnProperty(Isolate* isolate,
                 AccessorPair::cast(dictionary->ValueAt(entry));
             elms->set(IS_ACCESSOR_INDEX, heap->true_value());
             if (CheckElementAccess(*obj, index, v8::ACCESS_GET)) {
-              elms->set(GETTER_INDEX, accessors->SafeGet(ACCESSOR_GETTER));
+              elms->set(GETTER_INDEX, accessors->GetComponent(ACCESSOR_GETTER));
             }
             if (CheckElementAccess(*obj, index, v8::ACCESS_SET)) {
-              elms->set(SETTER_INDEX, accessors->SafeGet(ACCESSOR_SETTER));
+              elms->set(SETTER_INDEX, accessors->GetComponent(ACCESSOR_SETTER));
             }
             break;
           }
@@ -1115,10 +1115,10 @@ static MaybeObject* GetOwnProperty(Isolate* isolate,
 
     AccessorPair* accessors = AccessorPair::cast(result.GetCallbackObject());
     if (CheckAccess(*obj, *name, &result, v8::ACCESS_GET)) {
-      elms->set(GETTER_INDEX, accessors->SafeGet(ACCESSOR_GETTER));
+      elms->set(GETTER_INDEX, accessors->GetComponent(ACCESSOR_GETTER));
     }
     if (CheckAccess(*obj, *name, &result, v8::ACCESS_SET)) {
-      elms->set(SETTER_INDEX, accessors->SafeGet(ACCESSOR_SETTER));
+      elms->set(SETTER_INDEX, accessors->GetComponent(ACCESSOR_SETTER));
     }
   } else {
     elms->set(IS_ACCESSOR_INDEX, heap->false_value());
@@ -4350,26 +4350,9 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DefineOrRedefineAccessorProperty) {
   RUNTIME_ASSERT((unchecked & ~(READ_ONLY | DONT_ENUM | DONT_DELETE)) == 0);
   PropertyAttributes attr = static_cast<PropertyAttributes>(unchecked);
 
-  // TODO(svenpanne) Define getter/setter/attributes in a single step.
-  if (getter->IsNull() && setter->IsNull()) {
-    JSArray* array;
-    { MaybeObject* maybe_array = GetOwnProperty(isolate, obj, name);
-      if (!maybe_array->To(&array)) return maybe_array;
-    }
-    Object* current = FixedArray::cast(array->elements())->get(GETTER_INDEX);
-    getter = Handle<Object>(current, isolate);
-  }
-  if (!getter->IsNull()) {
-    MaybeObject* ok =
-        obj->DefineAccessor(*name, ACCESSOR_GETTER, *getter, attr);
-    if (ok->IsFailure()) return ok;
-  }
-  if (!setter->IsNull()) {
-    MaybeObject* ok =
-        obj->DefineAccessor(*name, ACCESSOR_SETTER, *setter, attr);
-    if (ok->IsFailure()) return ok;
-  }
-
+  bool fast = obj->HasFastProperties();
+  JSObject::DefineAccessor(obj, name, getter, setter, attr);
+  if (fast) JSObject::TransformToFastProperties(obj, 0);
   return isolate->heap()->undefined_value();
 }
 
@@ -10231,8 +10214,8 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_DebugGetPropertyDetails) {
       if (hasJavaScriptAccessors) {
         AccessorPair* accessors = AccessorPair::cast(*result_callback_obj);
         details->set(2, isolate->heap()->ToBoolean(caught_exception));
-        details->set(3, accessors->SafeGet(ACCESSOR_GETTER));
-        details->set(4, accessors->SafeGet(ACCESSOR_SETTER));
+        details->set(3, accessors->GetComponent(ACCESSOR_GETTER));
+        details->set(4, accessors->GetComponent(ACCESSOR_SETTER));
       }
 
       return *isolate->factory()->NewJSArrayWithElements(details);