[heap] Fix missing slots recording for DescriptorArray

This fixes a bug from 0400fc20,
which assumed that we can set the markbits and the marked
descriptors counter independently. This does not work because
the Scavenger skips slots recording for non-black promoted objects.

The fix is to mark the descriptor array black whenever we change
the marked descriptors counter.

Bug: v8:8617, v8:8618, v8:8486
Tbr: mlippautz@chromium.org
Change-Id: I80f3488061fa648b6c81963ba802ef045d92bcc6
Reviewed-on: https://chromium-review.googlesource.com/c/1387486
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58417}

src/heap/heap-write-barrier-inl.h

@@ -171,14 +171,14 @@ inline void MarkingBarrierForCode(Code host, RelocInfo* rinfo,
   Heap::MarkingBarrierForCodeSlow(host, rinfo, object);
 }
 
-inline void MarkingBarrierForDescriptorArray(Heap* heap,
+inline void MarkingBarrierForDescriptorArray(Heap* heap, HeapObject host,
                                              HeapObject descriptor_array,
                                              int number_of_own_descriptors) {
   heap_internals::MemoryChunk* chunk =
       heap_internals::MemoryChunk::FromHeapObject(descriptor_array);
   if (!chunk->IsMarking()) return;
 
-  Heap::MarkingBarrierForDescriptorArraySlow(heap, descriptor_array,
+  Heap::MarkingBarrierForDescriptorArraySlow(heap, host, descriptor_array,
                                              number_of_own_descriptors);
 }
 

src/heap/heap-write-barrier.h

@@ -58,7 +58,8 @@ void MarkingBarrier(HeapObject* object, MaybeObjectSlot slot,
 void MarkingBarrierForElements(Heap* heap, HeapObject object);
 void MarkingBarrierForCode(Code host, RelocInfo* rinfo, HeapObject object);
 
-void MarkingBarrierForDescriptorArray(Heap* heap, HeapObject descriptor_array,
+void MarkingBarrierForDescriptorArray(Heap* heap, HeapObject host,
+                                      HeapObject descriptor_array,
                                       int number_of_own_descriptors);
 
 Heap* GetHeapFromWritableObject(const HeapObject object);

src/heap/heap.cc

@@ -5658,7 +5658,7 @@ void Heap::MarkingBarrierForCodeSlow(Code host, RelocInfo* rinfo,
   heap->incremental_marking()->RecordWriteIntoCode(host, rinfo, object);
 }
 
-void Heap::MarkingBarrierForDescriptorArraySlow(Heap* heap,
+void Heap::MarkingBarrierForDescriptorArraySlow(Heap* heap, HeapObject host,
                                                 HeapObject raw_descriptor_array,
                                                 int number_of_own_descriptors) {
   DCHECK(heap->incremental_marking()->IsMarking());
@@ -5668,7 +5668,7 @@ void Heap::MarkingBarrierForDescriptorArraySlow(Heap* heap,
   if (NumberOfMarkedDescriptors::decode(heap->mark_compact_collector()->epoch(),
                                         raw_marked) <
       number_of_own_descriptors) {
-    heap->incremental_marking()->VisitDescriptors(descriptor_array,
+    heap->incremental_marking()->VisitDescriptors(host, descriptor_array,
                                                   number_of_own_descriptors);
   }
 }

src/heap/heap.h

@@ -356,7 +356,8 @@ class Heap {
                                                           RelocInfo* rinfo,
                                                           HeapObject value);
   V8_EXPORT_PRIVATE static void MarkingBarrierForDescriptorArraySlow(
-      Heap* heap, HeapObject descriptor_array, int number_of_own_descriptors);
+      Heap* heap, HeapObject host, HeapObject descriptor_array,
+      int number_of_own_descriptors);
   V8_EXPORT_PRIVATE static bool PageFlagsAreConsistent(HeapObject object);
 
   // Notifies the heap that is ok to start marking or other activities that

src/heap/incremental-marking.cc

@@ -752,9 +752,11 @@ int IncrementalMarking::VisitObject(Map map, HeapObject obj) {
     // 3. The object is a string that was colored black before
     //    unsafe layout change.
     // 4. The object is materizalized by the deoptimizer.
+    // 5. The object is a descriptor array marked black by
+    //    the descriptor array marking barrier.
     DCHECK(obj->IsHashTable() || obj->IsPropertyArray() ||
            obj->IsFixedArray() || obj->IsContext() || obj->IsJSObject() ||
-           obj->IsString());
+           obj->IsString() || obj->IsDescriptorArray());
   }
   DCHECK(marking_state()->IsBlack(obj));
   WhiteToGreyAndPush(map);
@@ -783,11 +785,16 @@ void IncrementalMarking::RevisitObject(HeapObject obj) {
   visitor.Visit(map, obj);
 }
 
-void IncrementalMarking::VisitDescriptors(DescriptorArray descriptor_array,
+void IncrementalMarking::VisitDescriptors(HeapObject host,
+                                          DescriptorArray descriptors,
                                           int number_of_own_descriptors) {
   IncrementalMarkingMarkingVisitor visitor(heap()->mark_compact_collector(),
                                            marking_state());
-  visitor.VisitDescriptors(descriptor_array, number_of_own_descriptors);
+  // This is necessary because the Scavenger records slots only for the
+  // promoted black objects and the marking visitor of DescriptorArray skips
+  // the descriptors marked by the visitor.VisitDescriptors() below.
+  visitor.MarkDescriptorArrayBlack(host, descriptors);
+  visitor.VisitDescriptors(descriptors, number_of_own_descriptors);
 }
 
 template <WorklistToProcess worklist_to_process>

src/heap/incremental-marking.h

@@ -203,7 +203,8 @@ class V8_EXPORT_PRIVATE IncrementalMarking {
   void RevisitObject(HeapObject obj);
   // Ensures that all descriptors int range [0, number_of_own_descripts)
   // are visited.
-  void VisitDescriptors(DescriptorArray array, int number_of_own_descriptors);
+  void VisitDescriptors(HeapObject host, DescriptorArray array,
+                        int number_of_own_descriptors);
 
   void RecordWriteSlow(HeapObject obj, HeapObjectSlot slot, Object* value);
   void RecordWriteIntoCode(Code host, RelocInfo* rinfo, HeapObject value);

src/heap/mark-compact-inl.h

@@ -328,17 +328,22 @@ void MarkingVisitor<fixed_array_mode, retaining_path_mode,
 
 template <FixedArrayVisitationMode fixed_array_mode,
           TraceRetainingPathMode retaining_path_mode, typename MarkingState>
-bool MarkingVisitor<fixed_array_mode, retaining_path_mode,
-                    MarkingState>::MarkObjectWithoutPush(HeapObject host,
-                                                         HeapObject object) {
-  if (marking_state()->WhiteToBlack(object)) {
+void MarkingVisitor<fixed_array_mode, retaining_path_mode, MarkingState>::
+    MarkDescriptorArrayBlack(HeapObject host, DescriptorArray descriptors) {
+  // Note that WhiteToBlack is not sufficient here because it fails if the
+  // descriptor array is grey. So we need to do two steps: WhiteToGrey and
+  // GreyToBlack. Alternatively, we could check WhiteToGrey || WhiteToBlack.
+  if (marking_state()->WhiteToGrey(descriptors)) {
     if (retaining_path_mode == TraceRetainingPathMode::kEnabled &&
         V8_UNLIKELY(FLAG_track_retaining_path)) {
-      heap_->AddRetainer(host, object);
+      heap_->AddRetainer(host, descriptors);
     }
-    return true;
   }
-  return false;
+  if (marking_state()->GreyToBlack(descriptors)) {
+    VisitPointers(descriptors, descriptors->GetFirstPointerSlot(),
+                  descriptors->GetDescriptorSlot(0));
+  }
+  DCHECK(marking_state()->IsBlack(descriptors));
 }
 
 template <FixedArrayVisitationMode fixed_array_mode,
@@ -409,11 +414,9 @@ void MarkingVisitor<fixed_array_mode, retaining_path_mode,
   // descriptor array is marked, its header is also visited. The slot holding
   // the descriptor array will be implicitly recorded when the pointer fields of
   // this map are visited.
+
   DescriptorArray descriptors = map->instance_descriptors();
-  if (MarkObjectWithoutPush(map, descriptors)) {
-    VisitPointers(descriptors, descriptors->GetFirstPointerSlot(),
-                  descriptors->GetDescriptorSlot(0));
-  }
+  MarkDescriptorArrayBlack(map, descriptors);
   int number_of_own_descriptors = map->NumberOfOwnDescriptors();
   if (number_of_own_descriptors) {
     VisitDescriptors(descriptors, number_of_own_descriptors);
@@ -429,16 +432,15 @@ void MarkingVisitor<fixed_array_mode, retaining_path_mode,
 template <FixedArrayVisitationMode fixed_array_mode,
           TraceRetainingPathMode retaining_path_mode, typename MarkingState>
 void MarkingVisitor<fixed_array_mode, retaining_path_mode, MarkingState>::
-    VisitDescriptors(DescriptorArray descriptor_array,
+    VisitDescriptors(DescriptorArray descriptors,
                      int number_of_own_descriptors) {
   int16_t new_marked = static_cast<int16_t>(number_of_own_descriptors);
-  int16_t old_marked = descriptor_array->UpdateNumberOfMarkedDescriptors(
+  int16_t old_marked = descriptors->UpdateNumberOfMarkedDescriptors(
       mark_compact_epoch_, new_marked);
   if (old_marked < new_marked) {
-    VisitPointers(
-        descriptor_array,
-        MaybeObjectSlot(descriptor_array->GetDescriptorSlot(old_marked)),
-        MaybeObjectSlot(descriptor_array->GetDescriptorSlot(new_marked)));
+    VisitPointers(descriptors,
+                  MaybeObjectSlot(descriptors->GetDescriptorSlot(old_marked)),
+                  MaybeObjectSlot(descriptors->GetDescriptorSlot(new_marked)));
   }
 }
 

src/heap/mark-compact.h

@@ -990,8 +990,12 @@ class MarkingVisitor final
   void VisitCustomWeakPointers(HeapObject host, ObjectSlot start,
                                ObjectSlot end) final {}
 
-  V8_INLINE void VisitDescriptors(DescriptorArray descriptor_array,
+  V8_INLINE void VisitDescriptors(DescriptorArray descriptors,
                                   int number_of_own_descriptors);
+  // Marks the descriptor array black without pushing it on the marking work
+  // list and visits its header.
+  V8_INLINE void MarkDescriptorArrayBlack(HeapObject host,
+                                          DescriptorArray descriptors);
 
  private:
   // Granularity in which FixedArrays are scanned if |fixed_array_mode|
@@ -1011,10 +1015,6 @@ class MarkingVisitor final
 
   V8_INLINE void MarkMapContents(Map map);
 
-  // Marks the object black without pushing it on the marking work list. Returns
-  // true if the object needed marking and false otherwise.
-  V8_INLINE bool MarkObjectWithoutPush(HeapObject host, HeapObject object);
-
   // Marks the object grey and pushes it on the marking work list.
   V8_INLINE void MarkObject(HeapObject host, HeapObject obj);
 

src/objects.cc

@@ -12827,7 +12827,7 @@ void Map::SetInstanceDescriptors(Isolate* isolate, DescriptorArray descriptors,
                                  int number_of_own_descriptors) {
   set_raw_instance_descriptors(descriptors);
   SetNumberOfOwnDescriptors(number_of_own_descriptors);
-  MarkingBarrierForDescriptorArray(isolate->heap(), descriptors,
+  MarkingBarrierForDescriptorArray(isolate->heap(), *this, descriptors,
                                    number_of_own_descriptors);
 }
 

src/objects/map-inl.h

@@ -646,7 +646,7 @@ void Map::AppendDescriptor(Isolate* isolate, Descriptor* desc) {
     // barrier.
     descriptors->Append(desc);
     SetNumberOfOwnDescriptors(number_of_own_descriptors + 1);
-    MarkingBarrierForDescriptorArray(isolate->heap(), descriptors,
+    MarkingBarrierForDescriptorArray(isolate->heap(), *this, descriptors,
                                      number_of_own_descriptors + 1);
   }
   // Properly mark the map if the {desc} is an "interesting symbol".