heap: Fix data race when clearing hash table

This CL also adds tests to catch this data race and re-enables the
incremental marking test that was disabled because of it (CL 2087957).

Bug: 986235, 1058622
Change-Id: Icfb06fb08aaede2b20ef89ebcf8406d314577128
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2088514Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747198}

third_party/blink/renderer/platform/heap/concurrent_marking_test.cc

@@ -74,6 +74,25 @@ void RemoveFromCollection() {
   driver.FinishGC();
 }
 
+template <typename C>
+void ClearCollection() {
+  constexpr int kIterations = 10;
+  IncrementalMarkingTestDriver driver(ThreadState::Current());
+  Persistent<CollectionWrapper<C>> persistent =
+      MakeGarbageCollected<CollectionWrapper<C>>();
+  C* collection = persistent->GetCollection();
+  driver.Start();
+  for (int i = 0; i < kIterations; ++i) {
+    driver.SingleConcurrentStep();
+    for (int j = 0; j < kIterations; ++j) {
+      collection->insert(MakeGarbageCollected<IntegerObject>(i));
+    }
+    collection->clear();
+  }
+  driver.FinishSteps();
+  driver.FinishGC();
+}
+
 template <typename C>
 void SwapCollections() {
   constexpr int kIterations = 10;
@@ -113,6 +132,10 @@ TEST_F(ConcurrentMarkingTest, RemoveFromHashMap) {
   RemoveFromCollection<HeapHashMapAdapter<Member<IntegerObject>>>();
 }
 
+TEST_F(ConcurrentMarkingTest, ClearHashMap) {
+  ClearCollection<HeapHashMapAdapter<Member<IntegerObject>>>();
+}
+
 TEST_F(ConcurrentMarkingTest, SwapHashMaps) {
   SwapCollections<HeapHashMapAdapter<Member<IntegerObject>>>();
 }
@@ -127,6 +150,10 @@ TEST_F(ConcurrentMarkingTest, RemoveFromHashSet) {
   RemoveFromCollection<HeapHashSet<Member<IntegerObject>>>();
 }
 
+TEST_F(ConcurrentMarkingTest, ClearHashSet) {
+  ClearCollection<HeapHashSet<Member<IntegerObject>>>();
+}
+
 TEST_F(ConcurrentMarkingTest, SwapHashSets) {
   SwapCollections<HeapHashSet<Member<IntegerObject>>>();
 }
@@ -148,6 +175,10 @@ TEST_F(ConcurrentMarkingTest, RemoveFromLinkedHashSet) {
   RemoveFromCollection<HeapLinkedHashSetAdapter<Member<IntegerObject>>>();
 }
 
+TEST_F(ConcurrentMarkingTest, ClearLinkedHashSet) {
+  ClearCollection<HeapLinkedHashSetAdapter<Member<IntegerObject>>>();
+}
+
 TEST_F(ConcurrentMarkingTest, SwapLinkedHashSets) {
   SwapCollections<HeapLinkedHashSetAdapter<Member<IntegerObject>>>();
 }
@@ -170,6 +201,10 @@ TEST_F(ConcurrentMarkingTest, RemoveFromListHashSet) {
   RemoveFromCollection<HeapListHashSetAdapter<Member<IntegerObject>>>();
 }
 
+TEST_F(ConcurrentMarkingTest, ClearListHashSet) {
+  ClearCollection<HeapListHashSetAdapter<Member<IntegerObject>>>();
+}
+
 TEST_F(ConcurrentMarkingTest, SwapListHashSets) {
   SwapCollections<HeapListHashSetAdapter<Member<IntegerObject>>>();
 }
@@ -184,6 +219,10 @@ TEST_F(ConcurrentMarkingTest, RemoveFromHashCountedSet) {
   RemoveFromCollection<HeapHashCountedSet<Member<IntegerObject>>>();
 }
 
+TEST_F(ConcurrentMarkingTest, ClearHashCountedSet) {
+  ClearCollection<HeapHashCountedSet<Member<IntegerObject>>>();
+}
+
 TEST_F(ConcurrentMarkingTest, SwapHashCountedSets) {
   SwapCollections<HeapHashCountedSet<Member<IntegerObject>>>();
 }
@@ -210,6 +249,10 @@ TEST_F(ConcurrentMarkingTest, RemoveFromVector) {
   RemoveFromCollection<HeapVectorAdapter<Member<IntegerObject>>>();
 }
 
+TEST_F(ConcurrentMarkingTest, ClearVector) {
+  ClearCollection<HeapVectorAdapter<Member<IntegerObject>>>();
+}
+
 TEST_F(ConcurrentMarkingTest, SwapVectors) {
   SwapCollections<HeapVectorAdapter<Member<IntegerObject>>>();
 }
@@ -239,6 +282,10 @@ TEST_F(ConcurrentMarkingTest, RemoveFromDeque) {
   RemoveFromCollection<HeapDequeAdapter<Member<IntegerObject>>>();
 }
 
+TEST_F(ConcurrentMarkingTest, ClearDeque) {
+  ClearCollection<HeapDequeAdapter<Member<IntegerObject>>>();
+}
+
 TEST_F(ConcurrentMarkingTest, SwapDeques) {
   SwapCollections<HeapDequeAdapter<Member<IntegerObject>>>();
 }

third_party/blink/renderer/platform/heap/incremental_marking_test.cc

@@ -1414,15 +1414,7 @@ TEST_F(IncrementalMarkingTest, DropBackingStore) {
   driver.FinishGC();
 }
 
-// TODO(crbug.com/1058622): Disabled for TSAN flakes.
-#if defined(THREAD_SANITIZER)
-#define MAYBE_NoBackingFreeDuringIncrementalMarking \
-  DISABLED_NoBackingFreeDuringIncrementalMarking
-#else
-#define MAYBE_NoBackingFreeDuringIncrementalMarking \
-  NoBackingFreeDuringIncrementalMarking
-#endif
-TEST_F(IncrementalMarkingTest, MAYBE_NoBackingFreeDuringIncrementalMarking) {
+TEST_F(IncrementalMarkingTest, NoBackingFreeDuringIncrementalMarking) {
   // Regression test: https://crbug.com/870306
   // Only reproduces in ASAN configurations.
   using WeakStore = HeapHashCountedSet<WeakMember<Object>>;

third_party/blink/renderer/platform/wtf/hash_table.h

@@ -1916,7 +1916,7 @@ void HashTable<Key,
   EnterAccessForbiddenScope();
   DeleteAllBucketsAndDeallocate(table_, table_size_);
   LeaveAccessForbiddenScope();
-  table_ = nullptr;
+  AsAtomicPtr(&table_)->store(nullptr, std::memory_order_relaxed);
   table_size_ = 0;
   key_count_ = 0;
 }