Always return failure when we didn't manage to add transitions.

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 1521 matching lines @@
   // Normalize the object if the name is an actual string (not the
   // hidden symbols) and is not a real identifier.
   // Normalize the object if it will have too many fast properties.
   Isolate* isolate = GetHeap()->isolate();
   StringInputBuffer buffer(name);
   if ((!IsIdentifier(isolate->unicode_cache(), &buffer)
        && name != isolate->heap()->hidden_symbol()) ||
       (map()->unused_property_fields() == 0 &&
        TooManyFastProperties(properties()->length(), store_mode))) {
     Object* obj;
-    { MaybeObject* maybe_obj =
-          NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
-      if (!maybe_obj->ToObject(&obj)) return maybe_obj;
-    }
+    MaybeObject* maybe_obj =
+        NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
+    if (!maybe_obj->ToObject(&obj)) return maybe_obj;
+
     return AddSlowProperty(name, value, attributes);
   }
 
   DescriptorArray* old_descriptors = map()->instance_descriptors();
   // Compute the new index for new field.
   int index = map()->NextFreePropertyIndex();
 
   // Allocate new instance descriptors with (name, index) added
   FieldDescriptor new_field(name, index, attributes, 0);
+
   DescriptorArray* new_descriptors;
-  { MaybeObject* maybe_new_descriptors = old_descriptors->CopyAdd(&new_field);
-    if (!maybe_new_descriptors->To(&new_descriptors)) {
-      return maybe_new_descriptors;
-    }
+  MaybeObject* maybe_new_descriptors = old_descriptors->CopyAdd(&new_field);
+  if (!maybe_new_descriptors->To(&new_descriptors)) {
+    return maybe_new_descriptors;
   }
 
   // Only allow map transition if the object isn't the global object.
-  bool allow_map_transition =
-      (isolate->context()->global_context()->object_function()->map() != map());
+  bool allow_map_transition = isolate->global_object_map() != map();
 
   ASSERT(index < map()->inobject_properties() ||
          (index - map()->inobject_properties()) < properties()->length() ||
          map()->unused_property_fields() == 0);
 
   // Allocate a new map for the object.
   Map* new_map;
-  { MaybeObject* maybe_r = map()->CopyReplaceDescriptors(new_descriptors);
-    if (!maybe_r->To(&new_map)) return maybe_r;
-  }
+  MaybeObject* maybe_r = map()->CopyReplaceDescriptors(new_descriptors);
+  if (!maybe_r->To(&new_map)) return maybe_r;
 
   TransitionArray* new_transitions = NULL;
   if (allow_map_transition) {
-    MaybeObject* maybe_new_transitions = map()->AddTransition(name, new_map);
-    if (!maybe_new_transitions->To(&new_transitions)) {
-      return maybe_new_transitions;
-    }
+    MaybeObject* maybe_transitions = map()->AddTransition(name, new_map);
+    if (!maybe_transitions->To(&new_transitions)) return maybe_transitions;
   }
 
   if (map()->unused_property_fields() == 0) {
     // Make room for the new value
     FixedArray* values;
-    { MaybeObject* maybe_values =
-          properties()->CopySize(properties()->length() + kFieldsAdded);
-      if (!maybe_values->To(&values)) return maybe_values;
-    }
+    MaybeObject* maybe_values =
+        properties()->CopySize(properties()->length() + kFieldsAdded);
+    if (!maybe_values->To(&values)) return maybe_values;
+
     set_properties(values);
     new_map->set_unused_property_fields(kFieldsAdded - 1);
   } else {
     new_map->set_unused_property_fields(map()->unused_property_fields() - 1);
   }
 
   // Apply all changes at once, so they are atomic.
   if (allow_map_transition) {
     MaybeObject* transition_added = map()->set_transitions(new_transitions);
     if (transition_added->IsFailure()) return transition_added;
   }
 
   new_map->SetBackPointer(map());
   set_map(new_map);
   return FastPropertyAtPut(index, value);
 }
 
 
 MaybeObject* JSObject::AddConstantFunctionProperty(
     String* name,
     JSFunction* function,
     PropertyAttributes attributes) {
   // Allocate new instance descriptors with (name, function) added
   ConstantFunctionDescriptor d(name, function, attributes, 0);
+
   DescriptorArray* new_descriptors;
-  { MaybeObject* maybe_new_descriptors =
-        map()->instance_descriptors()->CopyAdd(&d);
-    if (!maybe_new_descriptors->To(&new_descriptors)) {
-      return maybe_new_descriptors;
-    }
+  MaybeObject* maybe_new_descriptors =
+      map()->instance_descriptors()->CopyAdd(&d);
+  if (!maybe_new_descriptors->To(&new_descriptors)) {
+    return maybe_new_descriptors;
   }
 
   // Allocate a new map for the object.
   Map* new_map;
-  { MaybeObject* maybe_new_map = map()->CopyReplaceDescriptors(new_descriptors);
-    if (!maybe_new_map->To(&new_map)) return maybe_new_map;
-  }
+  MaybeObject* maybe_new_map = map()->CopyReplaceDescriptors(new_descriptors);
+  if (!maybe_new_map->To(&new_map)) return maybe_new_map;
 
   Map* old_map = map();
-  set_map(new_map);
 
-  // If the old map is the global object map (from new Object()),
-  // then transitions are not added to it, so we are done.
   Heap* heap = GetHeap();
-  if (old_map == heap->isolate()->context()->global_context()->
-      object_function()->map()) {
+  // Do not add transitions to objects with the global object map, nor to global

[Jakob Kummerow, 2012/07/12 14:38:19]

for clarity:
"Do not add transitions to empty object maps, nor to the global object."
See my other comment for "global object map" vs. "empty object map"; and there's
only one global object.

+  // objects.
+  if (old_map == heap->isolate()->global_object_map() || IsGlobalObject()) {
+    set_map(new_map);
     return function;
   }
 
-  // Do not add constant transitions to global objects
-  if (IsGlobalObject()) {
+  // Don't add transitions to special properties, with non-trival attributes.

[Jakob Kummerow, 2012/07/12 14:38:19]

nit: "trivial", and the comma is unnecessary.

+  // TODO(verwaest): Once we support attribute changes, these transitions should
+  // be kept as well.
+  if (attributes != NONE) {
+    set_map(new_map);
     return function;
   }
 
   // Add a constant transition to the old map, so future assignments to this
   // property on other objects of the same type will create a normal field, not
-  // a constant function. Don't do this for special properties, with non-trival
-  // attributes.
-  if (attributes != NONE) {
-    return function;
-  }
+  // a constant function.
+  TransitionArray* transitions;
+  MaybeObject* maybe_transitions = old_map->AddTransition(name, new_map);
+  if (!maybe_transitions->To(&transitions)) return maybe_transitions;
 
-  TransitionArray* new_transitions;
-  { MaybeObject* maybe_new_transitions =
-        old_map->AddTransition(name, new_map);
-    if (!maybe_new_transitions->To(&new_transitions)) {
-      // We have accomplished the main goal, so return success.
-      return function;
-    }
-  }
+  MaybeObject* transition_added = old_map->set_transitions(transitions);
+  if (transition_added->IsFailure()) return transition_added;
 
-  { MaybeObject* transition_added = old_map->set_transitions(new_transitions);
-    // We have accomplished the main goal, so return success.
-    if (transition_added->IsFailure()) {
-      return function;
-    }
-  }
-
+  set_map(new_map);
   new_map->SetBackPointer(old_map);
   return function;
 }
 
 
 // Add property in slow mode
 MaybeObject* JSObject::AddSlowProperty(String* name,
                                        Object* value,
                                        PropertyAttributes attributes) {
   ASSERT(!HasFastProperties());
@@ skipping 114 matching lines @@
   PropertyDetails new_details(attributes, NORMAL, new_enumeration_index);
   return SetNormalizedProperty(name, value, new_details);
 }
 
 
 MaybeObject* JSObject::ConvertDescriptorToFieldAndMapTransition(
     String* name,
     Object* new_value,
     PropertyAttributes attributes) {
   Map* old_map = map();
+  FixedArray* old_properties = properties();
   Object* result;
 
-  { MaybeObject* maybe_result =
-        ConvertDescriptorToField(name, new_value, attributes);
-    if (!maybe_result->To(&result)) return maybe_result;
-  }
+  MaybeObject* maybe_result =
+      ConvertDescriptorToField(name, new_value, attributes);
+  if (!maybe_result->To(&result)) return maybe_result;
 
   // If we get to this point we have succeeded - do not return failure

[Jakob Kummerow, 2012/07/12 14:38:19]

Outdated comment. Just remove it.

   // after this point.  Later stuff is optional.
-  if (!HasFastProperties()) {
-    return result;
+  if (!HasFastProperties()) return result;
+
+  // This method should only be used to convert existing transitions. Objects
+  // with the map of "new Object()" cannot have transitions in the first place.
+  ASSERT(map() != GetIsolate()->global_object_map());
+
+  TransitionArray* new_transitions;
+  MaybeObject* maybe_new_transitions = old_map->AddTransition(name, map());
+  if (!maybe_new_transitions->To(&new_transitions)) {
+    // Undo changes and return failure.
+    set_map(old_map);
+    set_properties(old_properties);
+    return maybe_new_transitions;
   }
 
-  // Do not add transitions to the map of "new Object()".
-  if (map() == GetIsolate()->context()->global_context()->
-      object_function()->map()) {
-    return result;
+  MaybeObject* transition_added = old_map->set_transitions(new_transitions);
+  if (transition_added->IsFailure()) {
+    // Undo changes and return failure.
+    set_map(old_map);
+    set_properties(old_properties);
+    return transition_added;
   }
 
-  TransitionArray* new_transitions;
-  { MaybeObject* maybe_new_transitions = old_map->AddTransition(name, map());
-    if (!maybe_new_transitions->To(&new_transitions)) {
-      // We have accomplished the main goal, so return success.
-      return result;
-    }
-  }
-
-  { MaybeObject* transition_added = old_map->set_transitions(new_transitions);
-    // Return success if failure since we accomplished the main goal. Otherwise
-    // also set backpointer.
-    if (!transition_added->IsFailure()) {
-      map()->SetBackPointer(old_map);
-    }
-  }
-
+  map()->SetBackPointer(old_map);
   return result;
 }
 
 
 MaybeObject* JSObject::ConvertDescriptorToField(String* name,
                                                 Object* new_value,
                                                 PropertyAttributes attributes) {
   if (map()->unused_property_fields() == 0 &&
       TooManyFastProperties(properties()->length(), MAY_BE_STORE_FROM_KEYED)) {
     Object* obj;
-    { MaybeObject* maybe_obj =
-          NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
-      if (!maybe_obj->ToObject(&obj)) return maybe_obj;
-    }
+    MaybeObject* maybe_obj = NormalizeProperties(CLEAR_INOBJECT_PROPERTIES, 0);
+    if (!maybe_obj->ToObject(&obj)) return maybe_obj;
     return ReplaceSlowProperty(name, new_value, attributes);
   }
 
   int index = map()->NextFreePropertyIndex();
   FieldDescriptor new_field(name, index, attributes, 0);
+
   // Make a new DescriptorArray replacing an entry with FieldDescriptor.
   DescriptorArray* new_descriptors;
-  { MaybeObject* maybe_descriptors =
-        map()->instance_descriptors()->CopyInsert(&new_field);
-    if (!maybe_descriptors->To(&new_descriptors)) return maybe_descriptors;
-  }
+  MaybeObject* maybe_descriptors =
+      map()->instance_descriptors()->CopyInsert(&new_field);
+  if (!maybe_descriptors->To(&new_descriptors)) return maybe_descriptors;
 
   // Make a new map for the object.
   Map* new_map;
-  { MaybeObject* maybe_new_map = map()->CopyReplaceDescriptors(new_descriptors);
-    if (!maybe_new_map->To(&new_map)) return maybe_new_map;
-  }
+  MaybeObject* maybe_new_map = map()->CopyReplaceDescriptors(new_descriptors);
+  if (!maybe_new_map->To(&new_map)) return maybe_new_map;
 
   // Make new properties array if necessary.
-  FixedArray* new_properties = 0;  // Will always be NULL or a valid pointer.
+  FixedArray* new_properties = NULL;  // Will always be NULL or a valid pointer.

[Jakob Kummerow, 2012/07/12 14:38:19]

You can remove the initialization and the comment (we don't have them in other
places either).

   int new_unused_property_fields = map()->unused_property_fields() - 1;
   if (map()->unused_property_fields() == 0) {
     new_unused_property_fields = kFieldsAdded - 1;
-    Object* new_properties_object;
-    { MaybeObject* maybe_new_properties_object =
-          properties()->CopySize(properties()->length() + kFieldsAdded);
-      if (!maybe_new_properties_object->ToObject(&new_properties_object)) {
-        return maybe_new_properties_object;
-      }
-    }
-    new_properties = FixedArray::cast(new_properties_object);
+    MaybeObject* maybe_new_properties =
+        properties()->CopySize(properties()->length() + kFieldsAdded);
+    if (!maybe_new_properties->To(&new_properties)) return maybe_new_properties;
   }
 
   // Update pointers to commit changes.
   // Object points to the new map.
   new_map->set_unused_property_fields(new_unused_property_fields);
   set_map(new_map);
-  if (new_properties) {
-    set_properties(FixedArray::cast(new_properties));
+  if (new_properties != NULL) {
+    set_properties(new_properties);
   }
   return FastPropertyAtPut(index, new_value);
 }
 
 
 
 MaybeObject* JSObject::SetPropertyWithInterceptor(
     String* name,
     Object* value,
     PropertyAttributes attributes,
@@ skipping 441 matching lines @@
 
 
 MaybeObject* JSObject::GetElementsTransitionMapSlow(ElementsKind to_kind) {
   Map* start_map = map();
   ElementsKind from_kind = start_map->elements_kind();
 
   if (from_kind == to_kind) {
     return start_map;
   }
 
-  Context* global_context = GetIsolate()->context()->global_context();
   bool allow_store_transition =
       // Only remember the map transition if the object's map is NOT equal to
       // the global object_function's map and there is not an already existing
       // non-matching element transition.
-      (global_context->object_function()->map() != map()) &&
+      (GetIsolate()->global_object_map() != map()) &&
       !start_map->IsUndefined() && !start_map->is_shared() &&
       IsFastElementsKind(from_kind);
 
   // Only store fast element maps in ascending generality.
   if (IsFastElementsKind(to_kind)) {
     allow_store_transition &=
         IsTransitionableFastElementsKind(from_kind) &&
         IsMoreGeneralElementsKindTransition(from_kind, to_kind);
   }
 
@@ skipping 996 matching lines @@
     }
   }
 
   Heap* current_heap = GetHeap();
 
   // Copy the next enumeration index from instance descriptor.
   int index = map_of_this->instance_descriptors()->NextEnumerationIndex();
   dictionary->SetNextEnumerationIndex(index);
 
   Map* new_map;
-  { MaybeObject* maybe_map =
-        current_heap->isolate()->context()->global_context()->
-        normalized_map_cache()->Get(this, mode);
-    if (!maybe_map->To(&new_map)) return maybe_map;
-  }
+  MaybeObject* maybe_map =
+      current_heap->isolate()->context()->global_context()->
+      normalized_map_cache()->Get(this, mode);
+  if (!maybe_map->To(&new_map)) return maybe_map;
 
   // We have now successfully allocated all the necessary objects.
   // Changes can now be made with the guarantee that all of them take effect.
 
   // Resize the object in the heap if necessary.
   int new_instance_size = new_map->instance_size();
   int instance_size_delta = map_of_this->instance_size() - new_instance_size;
   ASSERT(instance_size_delta >= 0);
   current_heap->CreateFillerObjectAt(this->address() + new_instance_size,
                                      instance_size_delta);
@@ skipping 9924 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