Bubble up the transitions associated with PreventExtensionsWithTransition

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index b4c8a528f048a29053d4adb8d93013d0d51873aa..426fff7b3e9f61c05ffd51f177d2a935a9062b2a 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -2361,6 +2361,32 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
   DCHECK(store_mode != FORCE_FIELD || modify_index >= 0);
   Isolate* isolate = old_map->GetIsolate();
 
+  Handle<Map> real_old_map = old_map;
+  Handle<Object> prev_object = handle(old_map->GetBackPointer(), isolate);
+  PropertyAttributes extra_attributes = NONE;
+  if (!prev_object->IsUndefined()) {

[Igor Sheludko, 2015/09/16 13:09:15]

How about adding "!old_map->is_extensible() &&" to avoid unnecessary transition
array queries?

+    Handle<Map> prev_map = Handle<Map>::cast(prev_object);
+    Map* transition = TransitionArray::SearchSpecial(
+        *prev_map, *isolate->factory()->nonextensible_symbol());
+    if (transition == *old_map) {
+      old_map = prev_map;
+    } else {
+      transition = TransitionArray::SearchSpecial(
+          *prev_map, *isolate->factory()->sealed_symbol());
+      if (transition == *old_map) {
+        extra_attributes = SEALED;
+        old_map = prev_map;
+      } else {
+        Map* transition = TransitionArray::SearchSpecial(
+            *prev_map, *isolate->factory()->frozen_symbol());
+        if (transition == *old_map) {
+          extra_attributes = FROZEN;
+          old_map = prev_map;
+        }
+      }
+    }
+  }
+
   Handle<DescriptorArray> old_descriptors(
       old_map->instance_descriptors(), isolate);
   int old_nof = old_map->NumberOfOwnDescriptors();
@@ -2396,16 +2422,16 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
                  .Equals(new_representation));
       DCHECK(
           old_descriptors->GetFieldType(modify_index)->NowIs(new_field_type));
-      return old_map;
+      return real_old_map;
     }
   }
 
   // Check the state of the root map.
   Handle<Map> root_map(old_map->FindRootMap(), isolate);
   if (!old_map->EquivalentToForTransition(*root_map)) {
-    return CopyGeneralizeAllRepresentations(old_map, modify_index, store_mode,
-                                            new_kind, new_attributes,
-                                            "GenAll_NotEquivalent");
+    return CopyGeneralizeAllRepresentations(
+        real_old_map, modify_index, store_mode, new_kind, new_attributes,
+        "GenAll_NotEquivalent");
   }
 
   ElementsKind from_kind = root_map->elements_kind();
@@ -2415,26 +2441,26 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
       to_kind != SLOW_SLOPPY_ARGUMENTS_ELEMENTS &&
       !(IsTransitionableFastElementsKind(from_kind) &&
         IsMoreGeneralElementsKindTransition(from_kind, to_kind))) {
-    return CopyGeneralizeAllRepresentations(old_map, modify_index, store_mode,
-                                            new_kind, new_attributes,
-                                            "GenAll_InvalidElementsTransition");
+    return CopyGeneralizeAllRepresentations(
+        real_old_map, modify_index, store_mode, new_kind, new_attributes,
+        "GenAll_InvalidElementsTransition");
   }
   int root_nof = root_map->NumberOfOwnDescriptors();
   if (modify_index >= 0 && modify_index < root_nof) {
     PropertyDetails old_details = old_descriptors->GetDetails(modify_index);
     if (old_details.kind() != new_kind ||
         old_details.attributes() != new_attributes) {
-      return CopyGeneralizeAllRepresentations(old_map, modify_index, store_mode,
-                                              new_kind, new_attributes,
-                                              "GenAll_RootModification1");
+      return CopyGeneralizeAllRepresentations(
+          real_old_map, modify_index, store_mode, new_kind, new_attributes,
+          "GenAll_RootModification1");
     }
     if ((old_details.type() != DATA && store_mode == FORCE_FIELD) ||
         (old_details.type() == DATA &&
          (!new_field_type->NowIs(old_descriptors->GetFieldType(modify_index)) ||
           !new_representation.fits_into(old_details.representation())))) {
-      return CopyGeneralizeAllRepresentations(old_map, modify_index, store_mode,
-                                              new_kind, new_attributes,
-                                              "GenAll_RootModification2");
+      return CopyGeneralizeAllRepresentations(
+          real_old_map, modify_index, store_mode, new_kind, new_attributes,
+          "GenAll_RootModification2");
     }
   }
 
@@ -2488,9 +2514,9 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
     if (next_kind == kAccessor &&
         !EqualImmutableValues(old_descriptors->GetValue(i),
                               tmp_descriptors->GetValue(i))) {
-      return CopyGeneralizeAllRepresentations(old_map, modify_index, store_mode,
-                                              new_kind, new_attributes,
-                                              "GenAll_Incompatible");
+      return CopyGeneralizeAllRepresentations(
+          real_old_map, modify_index, store_mode, new_kind, new_attributes,
+          "GenAll_Incompatible");
     }
     if (next_location == kField && tmp_details.location() == kDescriptor) break;
 
@@ -2547,10 +2573,23 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
                  target_descriptors->GetFieldType(modify_index)));
     }
 #endif
+
     if (*target_map != *old_map) {
+      // Add any preventExtensions/seal/freeze transition back onto the end.
+      if (!old_map.is_identical_to(real_old_map)) {
+        MaybeHandle<Map> transition_map = TransitionForPreventExtensions(
+            target_map, extra_attributes,
+            AsTransitionMarker(isolate, extra_attributes));
+        if (!transition_map.ToHandle(&target_map)) {
+          return CopyGeneralizeAllRepresentations(
+              real_old_map, modify_index, store_mode, new_kind, new_attributes,
+              "GenAll_CantHaveMoreTransitions");
+        }

[Igor Sheludko, 2015/09/16 13:09:15]

You should add
  if (*target_map != *real_old_map) {
    real_old_map->NotifyLeafMapLayoutChange();
  }
here.

+      }
       old_map->NotifyLeafMapLayoutChange();
+      return target_map;
     }
-    return target_map;
+    return real_old_map;
   }
 
   // Find the last compatible target map in the transition tree.
@@ -2581,9 +2620,9 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
     if (next_kind == kAccessor &&
         !EqualImmutableValues(old_descriptors->GetValue(i),
                               tmp_descriptors->GetValue(i))) {
-      return CopyGeneralizeAllRepresentations(old_map, modify_index, store_mode,
-                                              new_kind, new_attributes,
-                                              "GenAll_Incompatible");
+      return CopyGeneralizeAllRepresentations(
+          real_old_map, modify_index, store_mode, new_kind, new_attributes,
+          "GenAll_Incompatible");
     }
     DCHECK(!tmp_map->is_deprecated());
     target_map = tmp_map;
@@ -2808,9 +2847,29 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
   // could be inserted regardless of whether transitions array is full or not.
   if (!transition_target_deprecated &&
       !TransitionArray::CanHaveMoreTransitions(split_map)) {
-    return CopyGeneralizeAllRepresentations(old_map, modify_index, store_mode,
-                                            new_kind, new_attributes,
-                                            "GenAll_CantHaveMoreTransitions");
+    return CopyGeneralizeAllRepresentations(
+        real_old_map, modify_index, store_mode, new_kind, new_attributes,
+        "GenAll_CantHaveMoreTransitions");
+  }
+
+  // Add missing transitions.
+  Handle<Map> new_map = split_map;
+  for (int i = split_nof; i < old_nof; ++i) {
+    new_map = CopyInstallDescriptors(new_map, i, new_descriptors,
+                                     new_layout_descriptor);
+  }
+  new_map->set_owns_descriptors(true);
+
+  // Add any preventExtensions/seal/freeze transition back onto the end.
+  if (!old_map.is_identical_to(real_old_map)) {
+    MaybeHandle<Map> transition_map = TransitionForPreventExtensions(

[Igor Sheludko, 2015/09/16 13:09:15]

It does not make sense to search for transition here because they do not exist.
At this point we do CopyInstallDescriptors at lease once which creates new map
without transitions. So I think you should just call
Map::CopyForPreventExtensions() here instead which will also call necessary
"real_old_map->NotifyLeafMapLayoutChange()".

+        new_map, extra_attributes,
+        AsTransitionMarker(isolate, extra_attributes));
+    if (!transition_map.ToHandle(&new_map)) {
+      return CopyGeneralizeAllRepresentations(
+          real_old_map, modify_index, store_mode, new_kind, new_attributes,
+          "GenAll_CantHaveMoreTransitions");
+    }
   }
 
   old_map->NotifyLeafMapLayoutChange();
@@ -2837,13 +2896,6 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
         *old_field_type, *new_field_type);
   }
 
-  // Add missing transitions.
-  Handle<Map> new_map = split_map;
-  for (int i = split_nof; i < old_nof; ++i) {
-    new_map = CopyInstallDescriptors(new_map, i, new_descriptors,
-                                     new_layout_descriptor);
-  }
-  new_map->set_owns_descriptors(true);
   return new_map;
 }
 
@@ -5559,30 +5611,13 @@ MaybeHandle<Object> JSObject::PreventExtensionsWithTransition(
     isolate->UpdateArrayProtectorOnNormalizeElements(object);
   }
 
-  Handle<Symbol> transition_marker;
-  if (attrs == NONE) {
-    transition_marker = isolate->factory()->nonextensible_symbol();
-  } else if (attrs == SEALED) {
-    transition_marker = isolate->factory()->sealed_symbol();
-  } else {
-    DCHECK(attrs == FROZEN);
-    transition_marker = isolate->factory()->frozen_symbol();
-  }
+  Handle<Symbol> transition_marker = Map::AsTransitionMarker(isolate, attrs);
 
   Handle<Map> old_map(object->map(), isolate);
-  Map* transition =
-      TransitionArray::SearchSpecial(*old_map, *transition_marker);
-  if (transition != NULL) {
-    Handle<Map> transition_map(transition, isolate);
-    DCHECK(transition_map->has_dictionary_elements());
-    DCHECK(!transition_map->is_extensible());
-    JSObject::MigrateToMap(object, transition_map);
-  } else if (TransitionArray::CanHaveMoreTransitions(old_map)) {
-    // Create a new descriptor array with the appropriate property attributes
-    Handle<Map> new_map = Map::CopyForPreventExtensions(
-        old_map, attrs, transition_marker, "CopyForPreventExtensions");
-    JSObject::MigrateToMap(object, new_map);
-  } else {
+  MaybeHandle<Map> transition_map =
+      Map::TransitionForPreventExtensions(old_map, attrs, transition_marker);
+  Handle<Map> new_map;
+  if (!transition_map.ToHandle(&new_map)) {
     DCHECK(old_map->is_dictionary_map() || !old_map->is_prototype_map());
     // Slow path: need to normalize properties for safety
     NormalizeProperties(object, CLEAR_INOBJECT_PROPERTIES, 0,
@@ -5590,8 +5625,7 @@ MaybeHandle<Object> JSObject::PreventExtensionsWithTransition(
 
     // Create a new map, since other objects with this map may be extensible.
     // TODO(adamk): Extend the NormalizedMapCache to handle non-extensible maps.
-    Handle<Map> new_map =
-        Map::Copy(handle(object->map()), "SlowCopyForPreventExtensions");
+    new_map = Map::Copy(handle(object->map()), "SlowCopyForPreventExtensions");
     new_map->set_is_extensible(false);
     new_map->set_elements_kind(DICTIONARY_ELEMENTS);
     JSObject::MigrateToMap(object, new_map);
@@ -5603,6 +5637,8 @@ MaybeHandle<Object> JSObject::PreventExtensionsWithTransition(
         ApplyAttributesToDictionary(object->property_dictionary(), attrs);
       }
     }
+  } else {
+    JSObject::MigrateToMap(object, new_map);
   }
 
   DCHECK(object->map()->has_dictionary_elements());
@@ -6906,6 +6942,40 @@ Handle<Map> Map::CopyForPreventExtensions(Handle<Map> map,
 }
 
 
+MaybeHandle<Map> Map::TransitionForPreventExtensions(
+    Handle<Map> old_map, PropertyAttributes attrs,
+    Handle<Symbol> transition_marker) {
+  Isolate* isolate = old_map->GetIsolate();
+  Map* transition =
+      TransitionArray::SearchSpecial(*old_map, *transition_marker);
+  if (transition != NULL) {
+    Handle<Map> transition_map(transition, isolate);
+    DCHECK(transition_map->has_dictionary_elements());
+    DCHECK(!transition_map->is_extensible());
+    return transition_map;
+  } else if (TransitionArray::CanHaveMoreTransitions(old_map)) {
+    // Create a new descriptor array with the appropriate property attributes
+    Handle<Map> new_map = Map::CopyForPreventExtensions(
+        old_map, attrs, transition_marker, "CopyForPreventExtensions");
+    return new_map;
+  }
+  return MaybeHandle<Map>();
+}
+
+
+Handle<Symbol> Map::AsTransitionMarker(Isolate* isolate,
+                                       PropertyAttributes attrs) {
+  if (attrs == NONE) {
+    return isolate->factory()->nonextensible_symbol();
+  } else if (attrs == SEALED) {
+    return isolate->factory()->sealed_symbol();
+  } else {
+    DCHECK(attrs == FROZEN);
+    return isolate->factory()->frozen_symbol();
+  }
+}
+
+
 Handle<Map> Map::FixProxy(Handle<Map> map, InstanceType type, int size) {
   DCHECK(type == JS_OBJECT_TYPE || type == JS_FUNCTION_TYPE);
   DCHECK(map->IsJSProxyMap());