commit 9e452d1d097bc0f29a24e13ced5477fa3c9463f9
author Mathieu Chartier <mathieuc@google.com> Wed Sep 18 16:35:15 2013 -0700
committer Mathieu Chartier <mathieuc@google.com> Wed Sep 18 17:09:42 2013 -0700
tree 3386b2ba7a3187bb0cea4cc1a990eb7c4e7578d8
parent 3c60d13761e245b74f862987b3eae7bd5fdda5f3

Fix system weak sweeping race.

There was a race related to sweeping the intern table with mutators
unpaused. The race occurred when an unused intern was seen as not
referenced by the GC but another thread attempted to create intern
this same string before we swept the system weaks. This caused the
thread to get a pointer to the stale string which was shortly going
to be reclaimed.

The fix moves sweeping the system weaks inside of the pause. This is
a temporary solution since it adds < 1ms of pause time.

Bug: 10626133

Change-Id: Ibf669ae5237ddb2ab44a9efd72e207bd06b53147

runtime/gc/collector/mark_sweep.cc

diff --git a/runtime/gc/collector/mark_sweep.cc b/runtime/gc/collector/mark_sweep.cc
index 92bfc4e..fa1e4e8 100644
--- a/runtime/gc/collector/mark_sweep.cc
+++ b/runtime/gc/collector/mark_sweep.cc
@@ -185,6 +185,7 @@
 }
 
 void MarkSweep::ProcessReferences(Thread* self) {
+  base::TimingLogger::ScopedSplit split("ProcessReferences", &timings_);
   WriterMutexLock mu(self, *Locks::heap_bitmap_lock_);
   ProcessReferences(&soft_reference_list_, clear_soft_references_, &weak_reference_list_,
                     &finalizer_reference_list_, &phantom_reference_list_);
@@ -206,6 +207,10 @@
   }
 
   ProcessReferences(self);
+  {
+    WriterMutexLock mu(self, *Locks::heap_bitmap_lock_);
+    SweepSystemWeaks();
+  }
 
   // Only need to do this if we have the card mark verification on, and only during concurrent GC.
   if (GetHeap()->verify_missing_card_marks_) {
@@ -273,8 +278,9 @@
   Thread* self = Thread::Current();
 
   if (!IsConcurrent()) {
-    base::TimingLogger::ScopedSplit split("ProcessReferences", &timings_);
     ProcessReferences(self);
+    WriterMutexLock mu(self, *Locks::heap_bitmap_lock_);
+    SweepSystemWeaks();
   } else {
     base::TimingLogger::ScopedSplit split("UnMarkAllocStack", &timings_);
     accounting::ObjectStack* allocation_stack = GetHeap()->allocation_stack_.get();
@@ -951,9 +957,7 @@
 }
 
 bool MarkSweep::IsMarkedCallback(const Object* object, void* arg) {
-  return
-      reinterpret_cast<MarkSweep*>(arg)->IsMarked(object) ||
-      !reinterpret_cast<MarkSweep*>(arg)->GetHeap()->GetLiveBitmap()->Test(object);
+  return reinterpret_cast<MarkSweep*>(arg)->IsMarked(object);
 }
 
 void MarkSweep::RecursiveMarkDirtyObjects(bool paused, byte minimum_age) {
@@ -1136,10 +1140,6 @@
 void MarkSweep::SweepArray(accounting::ObjectStack* allocations, bool swap_bitmaps) {
   space::DlMallocSpace* space = heap_->GetAllocSpace();
 
-  // If we don't swap bitmaps then newly allocated Weaks go into the live bitmap but not mark
-  // bitmap, resulting in occasional frees of Weaks which are still in use.
-  SweepSystemWeaksArray(allocations);
-
   timings_.StartSplit("SweepArray");
   // Newly allocated objects MUST be in the alloc space and those are the only objects which we are
   // going to free.
@@ -1220,10 +1220,6 @@
   DCHECK(mark_stack_->IsEmpty());
   base::TimingLogger::ScopedSplit("Sweep", &timings_);
 
-  // If we don't swap bitmaps then newly allocated Weaks go into the live bitmap but not mark
-  // bitmap, resulting in occasional frees of Weaks which are still in use.
-  SweepSystemWeaks();
-
   const bool partial = (GetGcType() == kGcTypePartial);
   SweepCallbackContext scc;
   scc.mark_sweep = this;