commit c4621985bdfc2b27494087e5dee65a6d0cc5a632
author Mathieu Chartier <mathieuc@google.com> Mon Sep 16 19:43:47 2013 -0700
committer Mathieu Chartier <mathieuc@google.com> Tue Sep 17 10:03:00 2013 -0700
tree d8165f5b8d337e73942ee29d66fb40585dc7caf1
parent 5666afd6854b5634ae741dc8a3a633fc47d52168

Fix race in root marking.

There was a race which caused the class linker / intern table to not
become dirty after adding a root. We now guard the is dirty flag by
the corresponding locks to prevent this from occuring. This was
causing roots to be occasionally missed.

Also fixes the bug where we occasionally scan more cards than
needed.

Bug: 10626133

Change-Id: I0f6e72d92035ff463954d66988ef610ea0df61be

runtime/class_linker.cc

diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 37b62ad..0773a8d 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -192,7 +192,8 @@
       class_roots_(NULL),
       array_iftable_(NULL),
       init_done_(false),
-      is_dirty_(false),
+      dex_caches_dirty_(false),
+      class_table_dirty_(false),
       intern_table_(intern_table),
       portable_resolution_trampoline_(NULL),
       quick_resolution_trampoline_(NULL) {
@@ -1088,20 +1089,30 @@
 // Keep in sync with InitCallback. Anything we visit, we need to
 // reinit references to when reinitializing a ClassLinker from a
 // mapped image.
-void ClassLinker::VisitRoots(RootVisitor* visitor, void* arg, bool clean_dirty) {
+void ClassLinker::VisitRoots(RootVisitor* visitor, void* arg, bool only_dirty, bool clean_dirty) {
   visitor(class_roots_, arg);
   Thread* self = Thread::Current();
   {
     ReaderMutexLock mu(self, dex_lock_);
-    for (mirror::DexCache* dex_cache : dex_caches_) {
-      visitor(dex_cache, arg);
+    if (!only_dirty || dex_caches_dirty_) {
+      for (mirror::DexCache* dex_cache : dex_caches_) {
+        visitor(dex_cache, arg);
+      }
+      if (clean_dirty) {
+        dex_caches_dirty_ = false;
+      }
     }
   }
 
   {
     ReaderMutexLock mu(self, *Locks::classlinker_classes_lock_);
-    for (const std::pair<size_t, mirror::Class*>& it : class_table_) {
-      visitor(it.second, arg);
+    if (!only_dirty || class_table_dirty_) {
+      for (const std::pair<size_t, mirror::Class*>& it : class_table_) {
+        visitor(it.second, arg);
+      }
+      if (clean_dirty) {
+        class_table_dirty_ = false;
+      }
     }
 
     // We deliberately ignore the class roots in the image since we
@@ -1109,9 +1120,6 @@
   }
 
   visitor(array_iftable_, arg);
-  if (clean_dirty) {
-    is_dirty_ = false;
-  }
 }
 
 void ClassLinker::VisitClasses(ClassVisitor* visitor, void* arg) {
@@ -1928,7 +1936,7 @@
   CHECK(dex_cache->GetLocation()->Equals(dex_file.GetLocation()));
   dex_caches_.push_back(dex_cache.get());
   dex_cache->SetDexFile(&dex_file);
-  Dirty();
+  dex_caches_dirty_ = true;
 }
 
 void ClassLinker::RegisterDexFile(const DexFile& dex_file) {
@@ -2203,7 +2211,7 @@
   }
   Runtime::Current()->GetHeap()->VerifyObject(klass);
   class_table_.insert(std::make_pair(hash, klass));
-  Dirty();
+  class_table_dirty_ = true;
   return NULL;
 }
 
@@ -2316,7 +2324,7 @@
       }
     }
   }
-  Dirty();
+  class_table_dirty_ = true;
   dex_cache_image_class_lookup_required_ = false;
   self->EndAssertNoThreadSuspension(old_no_suspend_cause);
 }