Revert "Revert of Fix memory leak caused by field type in descriptor array."

This reverts commit b57be748b175695be2507cedf3423b59b9d6cd20 and
disables the test/mjsunit/debug-clearbreakpointgroup.js because
BreakLocationIterator::ClearBreakPoint is already broken for unrelated reasons (see v8:3924).

BUG=v8:3877
LOG=N
TEST=cctest/test-heap/Regress3877

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

Cr-Commit-Position: refs/heads/master@{#26893}

diff --git a/src/objects-inl.h b/src/objects-inl.h
index 84513b7136bb74a41bd7d406c1dd16c2f2608dc0..0a3c6753e737553e5cb1b81eab89ef4712ef5e07 100644 (file)
--- a/src/objects-inl.h
+++ b/src/objects-inl.h
@@ -3136,7 +3136,12 @@ int DescriptorArray::GetFieldIndex(int descriptor_number) {
 
 HeapType* DescriptorArray::GetFieldType(int descriptor_number) {
   DCHECK(GetDetails(descriptor_number).location() == kField);
-  return HeapType::cast(GetValue(descriptor_number));
+  Object* value = GetValue(descriptor_number);
+  if (value->IsWeakCell()) {
+    if (WeakCell::cast(value)->cleared()) return HeapType::Any();
+    value = WeakCell::cast(value)->value();
+  }
+  return HeapType::cast(value);
 }
 
 

diff --git a/src/objects.cc b/src/objects.cc
index a001f70be4515cdcdeae80d01f6510b20e208d62..6fd95030c538d2d5618eb680b1c0a8752b535e75 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -1704,6 +1704,12 @@ String* JSReceiver::constructor_name() {
 }
 
 
+static Handle<Object> WrapType(Handle<HeapType> type) {
+  if (type->IsClass()) return Map::WeakCellForMap(type->AsClass()->Map());
+  return type;
+}
+
+
 MaybeHandle<Map> Map::CopyWithField(Handle<Map> map,
                                     Handle<Name> name,
                                     Handle<HeapType> type,
@@ -1729,7 +1735,10 @@ MaybeHandle<Map> Map::CopyWithField(Handle<Map> map,
     type = HeapType::Any(isolate);
   }
 
-  DataDescriptor new_field_desc(name, index, type, attributes, representation);
+  Handle<Object> wrapped_type(WrapType(type));
+
+  DataDescriptor new_field_desc(name, index, wrapped_type, attributes,
+                                representation);
   Handle<Map> new_map = Map::CopyAddDescriptor(map, &new_field_desc, flag);
   int unused_property_fields = new_map->unused_property_fields() - 1;
   if (unused_property_fields < 0) {
@@ -2305,15 +2314,16 @@ Map* Map::FindFieldOwner(int descriptor) {
 
 void Map::UpdateFieldType(int descriptor, Handle<Name> name,
                           Representation new_representation,
-                          Handle<HeapType> new_type) {
+                          Handle<Object> new_wrapped_type) {
+  DCHECK(new_wrapped_type->IsSmi() || new_wrapped_type->IsWeakCell());
   DisallowHeapAllocation no_allocation;
   PropertyDetails details = instance_descriptors()->GetDetails(descriptor);
   if (details.type() != DATA) return;
   if (HasTransitionArray()) {
     TransitionArray* transitions = this->transitions();
     for (int i = 0; i < transitions->number_of_transitions(); ++i) {
-      transitions->GetTarget(i)
-          ->UpdateFieldType(descriptor, name, new_representation, new_type);
+      transitions->GetTarget(i)->UpdateFieldType(
+          descriptor, name, new_representation, new_wrapped_type);
     }
   }
   // It is allowed to change representation here only from None to something.
@@ -2321,9 +2331,9 @@ void Map::UpdateFieldType(int descriptor, Handle<Name> name,
          details.representation().IsNone());
 
   // Skip if already updated the shared descriptor.
-  if (instance_descriptors()->GetFieldType(descriptor) == *new_type) return;
+  if (instance_descriptors()->GetValue(descriptor) == *new_wrapped_type) return;
   DataDescriptor d(name, instance_descriptors()->GetFieldIndex(descriptor),
-                   new_type, details.attributes(), new_representation);
+                   new_wrapped_type, details.attributes(), new_representation);
   instance_descriptors()->Replace(descriptor, &d);
 }
 
@@ -2371,8 +2381,10 @@ void Map::GeneralizeFieldType(Handle<Map> map, int modify_index,
 
   PropertyDetails details = descriptors->GetDetails(modify_index);
   Handle<Name> name(descriptors->GetKey(modify_index));
+
+  Handle<Object> wrapped_type(WrapType(new_field_type));
   field_owner->UpdateFieldType(modify_index, name, new_representation,
-                               new_field_type);
+                               wrapped_type);
   field_owner->dependent_code()->DeoptimizeDependentCodeGroup(
       isolate, DependentCode::kFieldTypeGroup);
 
@@ -2764,7 +2776,8 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
           next_field_type =
               GeneralizeFieldType(target_field_type, old_field_type, isolate);
         }
-        DataDescriptor d(target_key, current_offset, next_field_type,
+        Handle<Object> wrapped_type(WrapType(next_field_type));
+        DataDescriptor d(target_key, current_offset, wrapped_type,
                          next_attributes, next_representation);
         current_offset += d.GetDetails().field_width_in_words();
         new_descriptors->Set(i, &d);
@@ -2832,8 +2845,10 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
           next_field_type = old_field_type;
         }
 
-        DataDescriptor d(old_key, current_offset, next_field_type,
-                         next_attributes, next_representation);
+        Handle<Object> wrapped_type(WrapType(next_field_type));
+
+        DataDescriptor d(old_key, current_offset, wrapped_type, next_attributes,
+                         next_representation);
         current_offset += d.GetDetails().field_width_in_words();
         new_descriptors->Set(i, &d);
       } else {
@@ -2964,33 +2979,41 @@ MaybeHandle<Map> Map::TryUpdate(Handle<Map> old_map) {
     if (!old_details.representation().fits_into(new_details.representation())) {
       return MaybeHandle<Map>();
     }
-    Object* new_value = new_descriptors->GetValue(i);
-    Object* old_value = old_descriptors->GetValue(i);
     switch (new_details.type()) {
       case DATA: {
-        PropertyType old_type = old_details.type();
-        if (old_type == DATA) {
-          if (!HeapType::cast(old_value)->NowIs(HeapType::cast(new_value))) {
+        HeapType* new_type = new_descriptors->GetFieldType(i);
+        PropertyType old_property_type = old_details.type();
+        if (old_property_type == DATA) {
+          HeapType* old_type = old_descriptors->GetFieldType(i);
+          if (!old_type->NowIs(new_type)) {
             return MaybeHandle<Map>();
           }
         } else {
-          DCHECK(old_type == DATA_CONSTANT);
-          if (!HeapType::cast(new_value)->NowContains(old_value)) {
+          DCHECK(old_property_type == DATA_CONSTANT);
+          Object* old_value = old_descriptors->GetValue(i);
+          if (!new_type->NowContains(old_value)) {
             return MaybeHandle<Map>();
           }
         }
         break;
       }
-      case ACCESSOR:
-        DCHECK(HeapType::Any()->Is(HeapType::cast(new_value)));
+      case ACCESSOR: {
+#ifdef DEBUG
+        HeapType* new_type = new_descriptors->GetFieldType(i);
+        DCHECK(HeapType::Any()->Is(new_type));
+#endif
         break;
+      }
 
       case DATA_CONSTANT:
-      case ACCESSOR_CONSTANT:
+      case ACCESSOR_CONSTANT: {
+        Object* old_value = old_descriptors->GetValue(i);
+        Object* new_value = new_descriptors->GetValue(i);
         if (old_details.location() == kField || old_value != new_value) {
           return MaybeHandle<Map>();
         }
         break;
+      }
     }
   }
   if (new_map->NumberOfOwnDescriptors() != old_nof) return MaybeHandle<Map>();

diff --git a/src/objects.h b/src/objects.h
index 1a240f949ba084a5253bbad5418acb2cf2de98b2..b8b6874cfc0ceb0bc9a6118a6783177ae2bb2561 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -6447,9 +6447,12 @@ class Map: public HeapObject {
 
   Map* FindLastMatchMap(int verbatim, int length, DescriptorArray* descriptors);
 
+  // Update field type of the given descriptor to new representation and new
+  // type. The type must be prepared for storing in descriptor array:
+  // it must be either a simple type or a map wrapped in a weak cell.
   void UpdateFieldType(int descriptor_number, Handle<Name> name,
                        Representation new_representation,
-                       Handle<HeapType> new_type);
+                       Handle<Object> new_wrapped_type);
 
   void PrintReconfiguration(FILE* file, int modify_index, PropertyKind kind,
                             PropertyAttributes attributes);

diff --git a/src/property.h b/src/property.h
index 5f8e6da40722f200d8f5081e6f48cd8b6f8f2622..c6556b3fc1432c9eaf1a5a175a54f09671fc04b2 100644 (file)
--- a/src/property.h
+++ b/src/property.h
@@ -79,10 +79,14 @@ class DataDescriptor FINAL : public Descriptor {
                  PropertyAttributes attributes, Representation representation)
       : Descriptor(key, HeapType::Any(key->GetIsolate()), attributes, DATA,
                    representation, field_index) {}
-  DataDescriptor(Handle<Name> key, int field_index, Handle<HeapType> field_type,
+  // The field type is either a simple type or a map wrapped in a weak cell.
+  DataDescriptor(Handle<Name> key, int field_index,
+                 Handle<Object> wrapped_field_type,
                  PropertyAttributes attributes, Representation representation)
-      : Descriptor(key, field_type, attributes, DATA, representation,
-                   field_index) {}
+      : Descriptor(key, wrapped_field_type, attributes, DATA, representation,
+                   field_index) {
+    DCHECK(wrapped_field_type->IsSmi() || wrapped_field_type->IsWeakCell());
+  }
 };
 
 

diff --git a/test/cctest/test-heap.cc b/test/cctest/test-heap.cc
index b5e091fc42d95e4ee8cc5ef4d8dc6c293cceb1da..2d8d7b9d787a2a28023017b20e08b89d7cce6ee8 100644 (file)
--- a/test/cctest/test-heap.cc
+++ b/test/cctest/test-heap.cc
@@ -5083,6 +5083,40 @@ TEST(NumberStringCacheSize) {
 }
 
 
+TEST(Regress3877) {
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+  Heap* heap = isolate->heap();
+  Factory* factory = isolate->factory();
+  HandleScope scope(isolate);
+  CompileRun("function cls() { this.x = 10; }");
+  Handle<WeakCell> weak_prototype;
+  {
+    HandleScope inner_scope(isolate);
+    v8::Local<v8::Value> result = CompileRun("cls.prototype");
+    Handle<JSObject> proto =
+        v8::Utils::OpenHandle(*v8::Handle<v8::Object>::Cast(result));
+    weak_prototype = inner_scope.CloseAndEscape(factory->NewWeakCell(proto));
+  }
+  CHECK(!weak_prototype->cleared());
+  CompileRun(
+      "var a = { };"
+      "a.x = new cls();"
+      "cls.prototype = null;");
+  for (int i = 0; i < 4; i++) {
+    heap->CollectAllGarbage(Heap::kNoGCFlags);
+  }
+  // The map of a.x keeps prototype alive
+  CHECK(!weak_prototype->cleared());
+  // Change the map of a.x and make the previous map garbage collectable.
+  CompileRun("a.x.__proto__ = {};");
+  for (int i = 0; i < 4; i++) {
+    heap->CollectAllGarbage(Heap::kNoGCFlags);
+  }
+  CHECK(weak_prototype->cleared());
+}
+
+
 #ifdef DEBUG
 TEST(PathTracer) {
   CcTest::InitializeVM();

diff --git a/test/cctest/test-migrations.cc b/test/cctest/test-migrations.cc
index 9eb367639a647da1592873dc6ea569cf4c831948..c7aaf29d6643e420254aeefa6d4df871ed6ca99e 100644 (file)
--- a/test/cctest/test-migrations.cc
+++ b/test/cctest/test-migrations.cc
@@ -233,12 +233,14 @@ class Expectations {
                       representations_[descriptor])) {
       return false;
     }
-    Object* expected_value = *values_[descriptor];
     Object* value = descriptors->GetValue(descriptor);
+    Object* expected_value = *values_[descriptor];
     switch (type) {
       case DATA:
-      case ACCESSOR:
-        return HeapType::cast(expected_value)->Equals(HeapType::cast(value));
+      case ACCESSOR: {
+        HeapType* type = descriptors->GetFieldType(descriptor);
+        return HeapType::cast(expected_value)->Equals(type);
+      }
 
       case DATA_CONSTANT:
         return value == expected_value;

diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status
index 5daeee43b72c95410d2f1e2ef2a64616a2def8ea..357c94e2cf58b520b3ad3ff9e1637014272da87f 100644 (file)
--- a/test/mjsunit/mjsunit.status
+++ b/test/mjsunit/mjsunit.status
@@ -244,6 +244,8 @@
 
   # Issue 3723.
   'regress/regress-3717': [SKIP],
+  # Issue 3924.
+  'mjsunit/debug-clearbreakpointgroup': [SKIP],
 }],  # 'gc_stress == True'
 
 ##############################################################################