[field type tracking] Fix handling of cleared WeakCells

Whenever a generalization is computed, the inputs must be checked for being cleared, and if they are, the generalization must be Type::Any.

Hopefully this fixes Chromium issue 527994 as well.

BUG=v8:4325,chromium:527994
LOG=n

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

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

diff --git a/src/accessors.cc b/src/accessors.cc
index 7d5d27a..441ff4d 100644 (file)
--- a/src/accessors.cc
+++ b/src/accessors.cc
@@ -1010,7 +1010,6 @@ MUST_USE_RESULT static MaybeHandle<Object> ReplaceAccessorWithDataProperty(
   CHECK_EQ(LookupIterator::ACCESSOR, it.state());
   DCHECK(it.HolderIsReceiverOrHiddenPrototype());
   it.ReconfigureDataProperty(value, it.property_details().attributes());
-  it.WriteDataValue(value);
 
   if (is_observed && !old_value->SameValue(*value)) {
     return JSObject::EnqueueChangeRecord(object, "update", name, old_value);

diff --git a/src/lookup.cc b/src/lookup.cc
index 69b733e..809c35e 100644 (file)
--- a/src/lookup.cc
+++ b/src/lookup.cc
@@ -178,6 +178,13 @@ void LookupIterator::ReconfigureDataProperty(Handle<Object> value,
   }
 
   ReloadPropertyInformation();
+  WriteDataValue(value);
+
+#if VERIFY_HEAP
+  if (FLAG_verify_heap) {
+    holder->JSObjectVerify();
+  }
+#endif
 }
 
 
@@ -290,6 +297,12 @@ void LookupIterator::TransitionToAccessorProperty(
   }
 
   TransitionToAccessorPair(pair, attributes);
+
+#if VERIFY_HEAP
+  if (FLAG_verify_heap) {
+    receiver->JSObjectVerify();
+  }
+#endif
 }
 
 

diff --git a/src/objects.cc b/src/objects.cc
index 6b057c1..d5cbcda 100644 (file)
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -2726,10 +2726,23 @@ void Map::UpdateFieldType(int descriptor, Handle<Name> name,
 }
 
 
+bool FieldTypeIsCleared(Representation rep, Handle<HeapType> type) {
+  return type->Is(HeapType::None()) && rep.IsHeapObject();
+}
+
+
 // static
-Handle<HeapType> Map::GeneralizeFieldType(Handle<HeapType> type1,
+Handle<HeapType> Map::GeneralizeFieldType(Representation rep1,
+                                          Handle<HeapType> type1,
+                                          Representation rep2,
                                           Handle<HeapType> type2,
                                           Isolate* isolate) {
+  // Cleared field types need special treatment. They represent lost knowledge,
+  // so we must be conservative, so their generalization with any other type
+  // is "Any".
+  if (FieldTypeIsCleared(rep1, type1) || FieldTypeIsCleared(rep2, type2)) {
+    return HeapType::Any(isolate);
+  }
   if (type1->NowIs(type2)) return type2;
   if (type2->NowIs(type1)) return type1;
   return HeapType::Any(isolate);
@@ -2750,10 +2763,13 @@ void Map::GeneralizeFieldType(Handle<Map> map, int modify_index,
                                   isolate);
 
   if (old_representation.Equals(new_representation) &&
+      !FieldTypeIsCleared(new_representation, new_field_type) &&
+      // Checking old_field_type for being cleared is not necessary because
+      // the NowIs check below would fail anyway in that case.
       new_field_type->NowIs(old_field_type)) {
-    DCHECK(Map::GeneralizeFieldType(old_field_type,
-                                    new_field_type,
-                                    isolate)->NowIs(old_field_type));
+    DCHECK(Map::GeneralizeFieldType(old_representation, old_field_type,
+                                    new_representation, new_field_type, isolate)
+               ->NowIs(old_field_type));
     return;
   }
 
@@ -2762,17 +2778,10 @@ void Map::GeneralizeFieldType(Handle<Map> map, int modify_index,
   Handle<DescriptorArray> descriptors(
       field_owner->instance_descriptors(), isolate);
   DCHECK_EQ(*old_field_type, descriptors->GetFieldType(modify_index));
-  bool old_field_type_was_cleared =
-      old_field_type->Is(HeapType::None()) && old_representation.IsHeapObject();
 
-  // Determine the generalized new field type. Conservatively assume type Any
-  // for cleared field types because the cleared type could have been a
-  // deprecated map and there still could be live instances with a non-
-  // deprecated version of the map.
   new_field_type =
-      old_field_type_was_cleared
-          ? HeapType::Any(isolate)
-          : Map::GeneralizeFieldType(old_field_type, new_field_type, isolate);
+      Map::GeneralizeFieldType(old_representation, old_field_type,
+                               new_representation, new_field_type, isolate);
 
   PropertyDetails details = descriptors->GetDetails(modify_index);
   Handle<Name> name(descriptors->GetKey(modify_index));
@@ -2996,8 +3005,10 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
             Handle<HeapType> old_field_type =
                 GetFieldType(isolate, old_descriptors, i,
                              old_details.location(), tmp_representation);
-            next_field_type =
-                GeneralizeFieldType(next_field_type, old_field_type, isolate);
+            Representation old_representation = old_details.representation();
+            next_field_type = GeneralizeFieldType(
+                old_representation, old_field_type, new_representation,
+                next_field_type, isolate);
           }
         } else {
           Handle<HeapType> old_field_type =
@@ -3161,21 +3172,24 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
 
         Handle<HeapType> next_field_type;
         if (modify_index == i) {
-          next_field_type =
-              GeneralizeFieldType(target_field_type, new_field_type, isolate);
+          next_field_type = GeneralizeFieldType(
+              target_details.representation(), target_field_type,
+              new_representation, new_field_type, isolate);
           if (!property_kind_reconfiguration) {
             Handle<HeapType> old_field_type =
                 GetFieldType(isolate, old_descriptors, i,
                              old_details.location(), next_representation);
-            next_field_type =
-                GeneralizeFieldType(next_field_type, old_field_type, isolate);
+            next_field_type = GeneralizeFieldType(
+                old_details.representation(), old_field_type,
+                next_representation, next_field_type, isolate);
           }
         } else {
           Handle<HeapType> old_field_type =
               GetFieldType(isolate, old_descriptors, i, old_details.location(),
                            next_representation);
-          next_field_type =
-              GeneralizeFieldType(target_field_type, old_field_type, isolate);
+          next_field_type = GeneralizeFieldType(
+              old_details.representation(), old_field_type, next_representation,
+              target_field_type, isolate);
         }
         Handle<Object> wrapped_type(WrapType(next_field_type));
         DataDescriptor d(target_key, current_offset, wrapped_type,
@@ -3236,8 +3250,9 @@ Handle<Map> Map::ReconfigureProperty(Handle<Map> old_map, int modify_index,
             Handle<HeapType> old_field_type =
                 GetFieldType(isolate, old_descriptors, i,
                              old_details.location(), next_representation);
-            next_field_type =
-                GeneralizeFieldType(next_field_type, old_field_type, isolate);
+            next_field_type = GeneralizeFieldType(
+                old_details.representation(), old_field_type,
+                next_representation, next_field_type, isolate);
           }
         } else {
           Handle<HeapType> old_field_type =
@@ -3798,6 +3813,11 @@ MaybeHandle<Object> Object::SetDataProperty(LookupIterator* it,
                         Object);
   }
 
+#if VERIFY_HEAP
+  if (FLAG_verify_heap) {
+    receiver->JSObjectVerify();
+  }
+#endif
   return value;
 }
 
@@ -3920,6 +3940,11 @@ MaybeHandle<Object> Object::AddDataProperty(LookupIterator* it,
                                        it->factory()->the_hole_value()),
                           Object);
     }
+#if VERIFY_HEAP
+    if (FLAG_verify_heap) {
+      receiver->JSObjectVerify();
+    }
+#endif
   }
 
   return value;
@@ -4572,6 +4597,11 @@ void JSObject::MigrateInstance(Handle<JSObject> object) {
   if (FLAG_trace_migration) {
     object->PrintInstanceMigration(stdout, *original_map, *map);
   }
+#if VERIFY_HEAP
+  if (FLAG_verify_heap) {
+    object->JSObjectVerify();
+  }
+#endif
 }
 
 
@@ -4588,6 +4618,11 @@ bool JSObject::TryMigrateInstance(Handle<JSObject> object) {
   if (FLAG_trace_migration) {
     object->PrintInstanceMigration(stdout, *original_map, object->map());
   }
+#if VERIFY_HEAP
+  if (FLAG_verify_heap) {
+    object->JSObjectVerify();
+  }
+#endif
   return true;
 }
 
@@ -4696,7 +4731,6 @@ MaybeHandle<Object> JSObject::DefineOwnPropertyIgnoreAttributes(
           it->TransitionToAccessorPair(new_data, attributes);
         } else {
           it->ReconfigureDataProperty(value, attributes);
-          it->WriteDataValue(value);
         }
 
         if (is_observed) {
@@ -4732,7 +4766,6 @@ MaybeHandle<Object> JSObject::DefineOwnPropertyIgnoreAttributes(
         if (is_observed) old_value = it->GetDataValue();
 
         it->ReconfigureDataProperty(value, attributes);
-        it->WriteDataValue(value);
 
         if (is_observed) {
           if (old_value->SameValue(*value)) {

diff --git a/src/objects.h b/src/objects.h
index a24d029..f859318 100644 (file)
--- a/src/objects.h
+++ b/src/objects.h
@@ -5469,9 +5469,8 @@ class Map: public HeapObject {
   // TODO(ishell): moveit!
   static Handle<Map> GeneralizeAllFieldRepresentations(Handle<Map> map);
   MUST_USE_RESULT static Handle<HeapType> GeneralizeFieldType(
-      Handle<HeapType> type1,
-      Handle<HeapType> type2,
-      Isolate* isolate);
+      Representation rep1, Handle<HeapType> type1, Representation rep2,
+      Handle<HeapType> type2, Isolate* isolate);
   static void GeneralizeFieldType(Handle<Map> map, int modify_index,
                                   Representation new_representation,
                                   Handle<HeapType> new_field_type);

diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status
index b045803..2a616b0 100644 (file)
--- a/test/mjsunit/mjsunit.status
+++ b/test/mjsunit/mjsunit.status
@@ -154,9 +154,6 @@
   # issue 4078:
   'allocation-site-info': [PASS, NO_VARIANTS],
 
-  # issue 4325:
-  'regress/regress-3960': [PASS, NO_VARIANTS],
-
   ##############################################################################
   # Too slow in debug mode with --stress-opt mode.
   'compiler/regress-stacktrace-methods': [PASS, ['mode == debug', SKIP]],

diff --git a/test/mjsunit/regress/regress-4325.js b/test/mjsunit/regress/regress-4325.js
new file mode 100644 (file)
index 0000000..e88bdd3
--- /dev/null
+++ b/test/mjsunit/regress/regress-4325.js
@@ -0,0 +1,48 @@
+// Copyright 2015 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 --expose-gc
+
+function Inner() {
+    this.p1 = 0;
+      this.p2 = 3;
+}
+
+function Outer() {
+    this.p3 = 0;
+}
+
+var i1 = new Inner();
+var i2 = new Inner();
+var o1 = new Outer();
+o1.inner = i1;
+// o1.map now thinks "inner" has type Inner.map1.
+// Deprecate Inner.map1:
+i1.p1 = 0.5;
+// Let Inner.map1 die by migrating i2 to Inner.map2:
+print(i2.p1);
+gc();
+// o1.map's descriptor for "inner" is now a cleared WeakCell;
+// o1.inner's actual map is Inner.map2.
+// Prepare Inner.map3, deprecating Inner.map2.
+i2.p2 = 0.5;
+// Deprecate o1's map.
+var o2 = new Outer();
+o2.p3 = 0.5;
+o2.inner = i2;
+// o2.map (Outer.map2) now says that o2.inner's type is Inner.map3.
+// Migrate o1 to Outer.map2.
+print(o1.p3);
+// o1.map now thinks that o1.inner has map Inner.map3 just like o2.inner,
+// but in fact o1.inner.map is still Inner.map2!
+
+function loader(o) {
+    return o.inner.p2;
+}
+loader(o2);
+loader(o2);
+%OptimizeFunctionOnNextCall(loader);
+assertEquals(0.5, loader(o2));
+assertEquals(3, loader(o1));
+gc();  // Crashes with --verify-heap.