With this fix, we only create the enum cache for own property descriptors...

With this fix, we only create the enum cache for own property descriptors (originally we cached all descriptors in the map).  The problem was that the size of all descriptors could be trimmed during GC triggered by allocating the storage for the cache, so we could have ended up with a wrong storage size.

This is really Toon's fix, I have only created a small repro case.

BUG=
R=verwaest@chromium.org

Review URL: https://codereview.chromium.org/212673011

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

src/handles.cc

@@ -627,20 +627,21 @@ Handle<FixedArray> GetEnumPropertyKeys(Handle<JSObject> object,
                                        bool cache_result) {
   Isolate* isolate = object->GetIsolate();
   if (object->HasFastProperties()) {
-    if (object->map()->instance_descriptors()->HasEnumCache()) {
-      int own_property_count = object->map()->EnumLength();
-      // If we have an enum cache, but the enum length of the given map is set
-      // to kInvalidEnumCache, this means that the map itself has never used the
-      // present enum cache. The first step to using the cache is to set the
-      // enum length of the map by counting the number of own descriptors that
-      // are not DONT_ENUM or SYMBOLIC.
-      if (own_property_count == kInvalidEnumCacheSentinel) {
-        own_property_count = object->map()->NumberOfDescribedProperties(
-            OWN_DESCRIPTORS, DONT_SHOW);
-
-        if (cache_result) object->map()->SetEnumLength(own_property_count);
-      }
+    int own_property_count = object->map()->EnumLength();
+    // If the enum length of the given map is set to kInvalidEnumCache, this
+    // means that the map itself has never used the present enum cache. The
+    // first step to using the cache is to set the enum length of the map by
+    // counting the number of own descriptors that are not DONT_ENUM or
+    // SYMBOLIC.
+    if (own_property_count == kInvalidEnumCacheSentinel) {
+      own_property_count = object->map()->NumberOfDescribedProperties(
+          OWN_DESCRIPTORS, DONT_SHOW);
+    } else {
+      ASSERT(own_property_count == object->map()->NumberOfDescribedProperties(
+          OWN_DESCRIPTORS, DONT_SHOW));
+    }
 
+    if (object->map()->instance_descriptors()->HasEnumCache()) {
       DescriptorArray* desc = object->map()->instance_descriptors();
       Handle<FixedArray> keys(desc->GetEnumCache(), isolate);
 
@@ -649,6 +650,7 @@ Handle<FixedArray> GetEnumPropertyKeys(Handle<JSObject> object,
       // enum cache was generated for a previous (smaller) version of the
       // Descriptor Array. In that case we regenerate the enum cache.
       if (own_property_count <= keys->length()) {
+        if (cache_result) object->map()->SetEnumLength(own_property_count);
         isolate->counters()->enum_cache_hits()->Increment();
         return ReduceFixedArrayTo(keys, own_property_count);
       }
@@ -663,23 +665,22 @@ Handle<FixedArray> GetEnumPropertyKeys(Handle<JSObject> object,
     }
 
     isolate->counters()->enum_cache_misses()->Increment();
-    int num_enum = map->NumberOfDescribedProperties(ALL_DESCRIPTORS, DONT_SHOW);
 
-    Handle<FixedArray> storage = isolate->factory()->NewFixedArray(num_enum);
-    Handle<FixedArray> indices = isolate->factory()->NewFixedArray(num_enum);
+    Handle<FixedArray> storage = isolate->factory()->NewFixedArray(
+        own_property_count);
+    Handle<FixedArray> indices = isolate->factory()->NewFixedArray(
+        own_property_count);
 
     Handle<DescriptorArray> descs =
         Handle<DescriptorArray>(object->map()->instance_descriptors(), isolate);
 
-    int real_size = map->NumberOfOwnDescriptors();
-    int enum_size = 0;
+    int size = map->NumberOfOwnDescriptors();
     int index = 0;
 
-    for (int i = 0; i < descs->number_of_descriptors(); i++) {
+    for (int i = 0; i < size; i++) {
       PropertyDetails details = descs->GetDetails(i);
       Object* key = descs->GetKey(i);
       if (!(details.IsDontEnum() || key->IsSymbol())) {
-        if (i < real_size) ++enum_size;
         storage->set(index, key);
         if (!indices.is_null()) {
           if (details.type() != FIELD) {
@@ -706,10 +707,9 @@ Handle<FixedArray> GetEnumPropertyKeys(Handle<JSObject> object,
                        indices.is_null() ? Object::cast(Smi::FromInt(0))
                                          : Object::cast(*indices));
     if (cache_result) {
-      object->map()->SetEnumLength(enum_size);
+      object->map()->SetEnumLength(own_property_count);
     }
-
-    return ReduceFixedArrayTo(storage, enum_size);
+    return storage;
   } else {
     Handle<NameDictionary> dictionary(object->property_dictionary());
     int length = dictionary->NumberOfEnumElements();

test/mjsunit/regress/regress-enum-prop-keys-cache-size.js

+// Copyright 2014 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax --stress-compaction
+
+%SetAllocationTimeout(100000, 100000);
+
+var x = {};
+x.a = 1;
+x.b = 2;
+x = {};
+
+var y = {};
+y.a = 1;
+
+%SetAllocationTimeout(100000, 0);
+
+for (var z in y) { }