[heap] Fix data race in runtime functions that use std::sort.

BUG=chromium:694255

Change-Id: I52237650b2e80428d21acfa2c4993a07d224b8c5
Reviewed-on: https://chromium-review.googlesource.com/542819
Commit-Queue: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#46098}

src/base/atomic-utils.h

@@ -381,6 +381,42 @@ class AsAtomicWord {
   }
 };
 
+// This class is intended to be used as a wrapper for elements of an array
+// that is passed in to STL functions such as std::sort. It ensures that
+// elements accesses are atomic.
+// Usage example:
+//   Object** given_array;
+//   AtomicElement<Object*>* wrapped =
+//       reinterpret_cast<AtomicElement<Object*>(given_array);
+//   std::sort(wrapped, wrapped + given_length, cmp);
+// where the cmp function uses the value() accessor to compare the elements.
+template <typename T>
+class AtomicElement {
+ public:
+  AtomicElement(const AtomicElement<T>& other) {
+    AsAtomicWord::Relaxed_Store(&value_,
+                                AsAtomicWord::Relaxed_Load(&other.value_));
+  }
+
+  void operator=(const AtomicElement<T>& other) {
+    AsAtomicWord::Relaxed_Store(&value_,
+                                AsAtomicWord::Relaxed_Load(&other.value_));
+  }
+
+  T value() const { return AsAtomicWord::Relaxed_Load(&value_); }
+
+  bool operator<(const AtomicElement<T>& other) const {
+    return value() < other.value();
+  }
+
+  bool operator==(const AtomicElement<T>& other) const {
+    return value() == other.value();
+  }
+
+ private:
+  T value_;
+};
+
 }  // namespace base
 }  // namespace v8
 

src/elements.cc

@@ -451,7 +451,10 @@ static void SortIndices(
     Handle<FixedArray> indices, uint32_t sort_size,
     WriteBarrierMode write_barrier_mode = UPDATE_WRITE_BARRIER) {
   struct {
-    bool operator()(Object* a, Object* b) {
+    bool operator()(const base::AtomicElement<Object*>& elementA,
+                    const base::AtomicElement<Object*>& elementB) {
+      const Object* a = elementA.value();
+      const Object* b = elementB.value();
       if (a->IsSmi() || !a->IsUndefined(HeapObject::cast(a)->GetIsolate())) {
         if (!b->IsSmi() && b->IsUndefined(HeapObject::cast(b)->GetIsolate())) {
           return true;
@@ -461,8 +464,11 @@ static void SortIndices(
       return !b->IsSmi() && b->IsUndefined(HeapObject::cast(b)->GetIsolate());
     }
   } cmp;
-  Object** start =
-      reinterpret_cast<Object**>(indices->GetFirstElementAddress());
+  // Use AtomicElement wrapper to ensure that std::sort uses atomic load and
+  // store operations that are safe for concurrent marking.
+  base::AtomicElement<Object*>* start =
+      reinterpret_cast<base::AtomicElement<Object*>*>(
+          indices->GetFirstElementAddress());
   std::sort(start, start + sort_size, cmp);
   if (write_barrier_mode != SKIP_WRITE_BARRIER) {
     FIXED_ARRAY_ELEMENTS_WRITE_BARRIER(indices->GetIsolate()->heap(), *indices,

src/objects.cc

@@ -17720,9 +17720,10 @@ int Dictionary<Derived, Shape>::NumberOfEnumerableProperties() {
 template <typename Dictionary>
 struct EnumIndexComparator {
   explicit EnumIndexComparator(Dictionary* dict) : dict(dict) {}
-  bool operator() (Smi* a, Smi* b) {
-    PropertyDetails da(dict->DetailsAt(a->value()));
-    PropertyDetails db(dict->DetailsAt(b->value()));
+  bool operator()(const base::AtomicElement<Smi*>& a,
+                  const base::AtomicElement<Smi*>& b) {
+    PropertyDetails da(dict->DetailsAt(a.value()->value()));
+    PropertyDetails db(dict->DetailsAt(b.value()->value()));
     return da.dictionary_index() < db.dictionary_index();
   }
   Dictionary* dict;
@@ -17766,7 +17767,11 @@ void Dictionary<Derived, Shape>::CopyEnumKeysTo(
   Dictionary<Derived, Shape>* raw_dictionary = *dictionary;
   FixedArray* raw_storage = *storage;
   EnumIndexComparator<Derived> cmp(static_cast<Derived*>(*dictionary));
-  Smi** start = reinterpret_cast<Smi**>(storage->GetFirstElementAddress());
+  // Use AtomicElement wrapper to ensure that std::sort uses atomic load and
+  // store operations that are safe for concurrent marking.
+  base::AtomicElement<Smi*>* start =
+      reinterpret_cast<base::AtomicElement<Smi*>*>(
+          storage->GetFirstElementAddress());
   std::sort(start, start + length, cmp);
   for (int i = 0; i < length; i++) {
     int index = Smi::cast(raw_storage->get(i))->value();
@@ -17795,7 +17800,11 @@ Handle<FixedArray> Dictionary<Derived, Shape>::IterationIndices(
     DCHECK_EQ(array_size, length);
 
     EnumIndexComparator<Derived> cmp(static_cast<Derived*>(raw_dict));
-    Smi** start = reinterpret_cast<Smi**>(array->GetFirstElementAddress());
+    // Use AtomicElement wrapper to ensure that std::sort uses atomic load and
+    // store operations that are safe for concurrent marking.
+    base::AtomicElement<Smi*>* start =
+        reinterpret_cast<base::AtomicElement<Smi*>*>(
+            array->GetFirstElementAddress());
     std::sort(start, start + array_size, cmp);
   }
   array->Shrink(array_size);
@@ -17836,7 +17845,11 @@ void Dictionary<Derived, Shape>::CollectKeysTo(
     }
 
     EnumIndexComparator<Derived> cmp(static_cast<Derived*>(raw_dict));
-    Smi** start = reinterpret_cast<Smi**>(array->GetFirstElementAddress());
+    // Use AtomicElement wrapper to ensure that std::sort uses atomic load and
+    // store operations that are safe for concurrent marking.
+    base::AtomicElement<Smi*>* start =
+        reinterpret_cast<base::AtomicElement<Smi*>*>(
+            array->GetFirstElementAddress());
     std::sort(start, start + array_size, cmp);
   }