[heap] Expand verification of layout changes to non-JSObject objects.

On map change of an object this patch checks that
- either GC was notified about this change,
- or the change leaves the slot set of the object the same.

BUG=chromium:694255

Review-Url: https://codereview.chromium.org/2886223002
Cr-Commit-Position: refs/heads/master@{#45427}

src/heap/heap.cc

@@ -4293,14 +4293,47 @@ void Heap::NotifyObjectLayoutChange(HeapObject* object,
 }
 
 #ifdef VERIFY_HEAP
+// Helper class for collecting slot addresses.
+class SlotCollectingVisitor final : public ObjectVisitor {
+ public:
+  void VisitPointers(HeapObject* host, Object** start, Object** end) override {
+    for (Object** p = start; p < end; p++) {
+      slots_.push_back(p);
+    }
+  }
+
+  int number_of_slots() { return static_cast<int>(slots_.size()); }
+
+  Object** slot(int i) { return slots_[i]; }
+
+ private:
+  std::vector<Object**> slots_;
+};
+
 void Heap::VerifyObjectLayoutChange(HeapObject* object, Map* new_map) {
   // Check that Heap::NotifyObjectLayout was called for object transitions
   // that are not safe for concurrent marking.
   // If you see this check triggering for a freshly allocated object,
   // use object->set_map_after_allocation() to initialize its map.
   if (pending_layout_change_object_ == nullptr) {
-    DCHECK(!object->IsJSObject() ||
-           !object->map()->TransitionRequiresSynchronizationWithGC(new_map));
+    if (object->IsJSObject()) {
+      DCHECK(!object->map()->TransitionRequiresSynchronizationWithGC(new_map));
+    } else {
+      // Check that the set of slots before and after the transition match.
+      SlotCollectingVisitor old_visitor;
+      object->IterateFast(&old_visitor);
+      MapWord old_map_word = object->map_word();
+      // Temporarily set the new map to iterate new slots.
+      object->set_map_word(MapWord::FromMap(new_map));
+      SlotCollectingVisitor new_visitor;
+      object->IterateFast(&new_visitor);
+      // Restore the old map.
+      object->set_map_word(old_map_word);
+      DCHECK_EQ(new_visitor.number_of_slots(), old_visitor.number_of_slots());
+      for (int i = 0; i < new_visitor.number_of_slots(); i++) {
+        DCHECK_EQ(new_visitor.slot(i), old_visitor.slot(i));
+      }
+    }
   } else {
     DCHECK_EQ(pending_layout_change_object_, object);
     pending_layout_change_object_ = nullptr;

src/heap/heap.h

@@ -1248,11 +1248,9 @@ class Heap {
 
   // The runtime uses this function to notify potentially unsafe object layout
   // changes that require special synchronization with the concurrent marker.
-  // A layout change is unsafe if
-  // - it removes a tagged in-object field.
-  // - it replaces a tagged in-objects field with an untagged in-object field.
   void NotifyObjectLayoutChange(HeapObject* object,
                                 const DisallowHeapAllocation&);
+
 #ifdef VERIFY_HEAP
   // This function checks that either
   // - the map transition is safe,

src/heap/incremental-marking.cc

@@ -873,8 +873,11 @@ void IncrementalMarking::VisitObject(Map* map, HeapObject* obj, int size) {
   // 1. The object is a fixed array with the progress bar.
   // 2. The object is a JSObject that was colored black before
   //    unsafe layout change.
+  // 3. The object is a string that was colored black before
+  //    unsafe layout change.
   if (!ObjectMarking::GreyToBlack<kAtomicity>(obj, marking_state(obj))) {
-    DCHECK(IsFixedArrayWithProgressBar(obj) || obj->IsJSObject());
+    DCHECK(IsFixedArrayWithProgressBar(obj) || obj->IsJSObject() ||
+           obj->IsString());
   }
   DCHECK(ObjectMarking::IsBlack<kAtomicity>(obj, marking_state(obj)));
   WhiteToGreyAndPush(map);

src/objects.cc

@@ -2476,6 +2476,7 @@ Handle<String> String::SlowFlatten(Handle<ConsString> cons,
 
 
 bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
+  DisallowHeapAllocation no_allocation;
   // Externalizing twice leaks the external resource, so it's
   // prohibited by the API.
   DCHECK(!this->IsExternalString());
@@ -2498,7 +2499,9 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
   bool is_one_byte = this->IsOneByteRepresentation();
   bool is_internalized = this->IsInternalizedString();
   bool has_pointers = StringShape(this).IsIndirect();
-
+  if (has_pointers) {
+    heap->NotifyObjectLayoutChange(this, no_allocation);
+  }
   // Morph the string to an external string by replacing the map and
   // reinitializing the fields.  This won't work if the space the existing
   // string occupies is too small for a regular  external string.
@@ -2544,6 +2547,7 @@ bool String::MakeExternal(v8::String::ExternalStringResource* resource) {
 
 
 bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
+  DisallowHeapAllocation no_allocation;
   // Externalizing twice leaks the external resource, so it's
   // prohibited by the API.
   DCHECK(!this->IsExternalString());
@@ -2571,6 +2575,10 @@ bool String::MakeExternal(v8::String::ExternalOneByteStringResource* resource) {
   bool is_internalized = this->IsInternalizedString();
   bool has_pointers = StringShape(this).IsIndirect();
 
+  if (has_pointers) {
+    heap->NotifyObjectLayoutChange(this, no_allocation);
+  }
+
   // Morph the string to an external string by replacing the map and
   // reinitializing the fields.  This won't work if the space the existing
   // string occupies is too small for a regular  external string.
@@ -17457,6 +17465,7 @@ void MakeStringThin(String* string, String* internalized, Isolate* isolate) {
 
   if (!string->IsInternalizedString()) {
     DisallowHeapAllocation no_gc;
+    isolate->heap()->NotifyObjectLayoutChange(string, no_gc);
     bool one_byte = internalized->IsOneByteRepresentation();
     Handle<Map> map = one_byte ? isolate->factory()->thin_one_byte_string_map()
                                : isolate->factory()->thin_string_map();