[heap] Correctly check for black allocated objects in concurrent marker.

The markbit check should be performed before using the map of the
object.

Change-Id: Ia19e48fd4660387d239e1e330368808727359c7f
Reviewed-on: https://chromium-review.googlesource.com/c/1301496
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Hannes Payer <hpayer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#57040}

src/heap/concurrent-marking.cc

@@ -105,6 +105,8 @@ class ConcurrentMarkingVisitor final
   }
 
   bool ShouldVisit(HeapObject* object) {
+    // TODO(ulan): It is sufficient to flip the second markbit without
+    // doing checking for the first bit.
     return marking_state_.GreyToBlack(object);
   }
 
@@ -406,6 +408,8 @@ class ConcurrentMarkingVisitor final
     }
   }
 
+  ConcurrentMarkingState* marking_state() { return &marking_state_; }
+
  private:
   // Helper class for collecting in-object slot addresses and values.
   class SlotSnapshottingVisitor final : public ObjectVisitor {
@@ -620,7 +624,21 @@ void ConcurrentMarking::Run(int task_id, TaskState* task_state) {
         Address addr = object->address();
         if (new_space_top <= addr && addr < new_space_limit) {
           on_hold_->Push(task_id, object);
-        } else {
+        } else if (!visitor.marking_state()->IsBlackAssumingNotWhite(object)) {
+          // We need to ensure that the object is fully initialized.
+          // The new_space_top check above takes care of the newly allocated
+          // objects in the young generation. Now we need to rule out the black
+          // allocated objects in the old generation.
+          //
+          // Black allocation performs the following operations:
+          // 1) Set marking bitmap range to black using relaxed stores.
+          // 2) Issue base::SeqCst_MemoryFence().
+          // 3) Allocate, initialize, and publish an object.
+          //
+          // Since we loaded the object from the worklist, it must have
+          // been published. Since IsBlackAssumingNotWhite was false,
+          // was grey after it was published, so it is not black allocated.
+          // Thus the following operation loads a valid map pointer.
           Map* map = object->synchronized_map();
           current_marked_bytes += visitor.Visit(map, object);
         }

src/heap/mark-compact.h

@@ -64,6 +64,10 @@ class MarkingStateBase {
     return Marking::IsBlackOrGrey<access_mode>(MarkBitFrom(obj));
   }
 
+  V8_INLINE bool IsBlackAssumingNotWhite(HeapObject* obj) {
+    return Marking::IsBlackAssumingNotWhite<access_mode>(MarkBitFrom(obj));
+  }
+
   V8_INLINE bool WhiteToGrey(HeapObject* obj);
   V8_INLINE bool WhiteToBlack(HeapObject* obj);
   V8_INLINE bool GreyToBlack(HeapObject* obj);

src/heap/marking.h

@@ -248,6 +248,13 @@ class Marking : public AllStatic {
     return mark_bit.Get<mode>();
   }
 
+  // IsBlackAssumingNotWhite assumes that the color known to be
+  // black or grey. It only checks the second bit.
+  template <AccessMode mode = AccessMode::NON_ATOMIC>
+  V8_INLINE static bool IsBlackAssumingNotWhite(MarkBit mark_bit) {
+    return mark_bit.Next().Get<mode>();
+  }
+
   template <AccessMode mode = AccessMode::NON_ATOMIC>
   V8_INLINE static void MarkWhite(MarkBit markbit) {
     STATIC_ASSERT(mode == AccessMode::NON_ATOMIC);