Fix race condition between GCDaemon and DeleteLocalReference

There is a race condition between the GCDaemon visiting
thread local roots starting from the thread's indirect ref table (IRT)
and another thread calling JNI::DeleteLocalReference, which is clearing
one of the indirect references.

To cope with the race condition the DeleteLocalReference used to transit
from suspended to running state by creating a ScopedObjectReference(env).
But this transition was removed with the following patch:
https://android.googlesource.com/platform/art/+/ef28b14268ed0f9db0c7bbd571aa514354a360bd%5E!/#F0

If so the GCDaemon must be careful to work with IRT entries.

This new patch:
1. calls the visitor only if the reference is not null. This if-null
   behavior of ART GC is consistent with what Dalvik GC does.
2. But this might be not enough for some future sophisticated GC algorithms.
   For example, if GC moves an object, then the IRT entry must be changed
   with CAS only if it has not been cleared. So, for the safety reasons
   the patch put backs the ScopedObjectReference soa(env) to
   DeleteLocalReference.

Only one of those two changes would be enough.

mathieuc note: I decided to delete the root null check but kept the
ScopedObjectAccess in DeleteLocalRef and added missing annotations as
well as more ScopedObjectAccess in jni internals.

Bug: 14626564

Change-Id: I90d4b8494f61404579ecdd2918d1482093d99387
Signed-off-by: Yevgeny Rouban <yevgeny.y.rouban@intel.com>
Signed-off-by: Yang Chang <yang.chang@intel.com>

5 files changed, 22 insertions, 12 deletions

diff --git a/runtime/entrypoints/portable/portable_jni_entrypoints.cc b/runtime/entrypoints/portable/portable_jni_entrypoints.cc
index 17ad4d0..3e7b30a 100644
--- a/runtime/entrypoints/portable/portable_jni_entrypoints.cc
+++ b/runtime/entrypoints/portable/portable_jni_entrypoints.cc
@@ -37,7 +37,8 @@ extern "C" uint32_t art_portable_jni_method_start_synchronized(jobject to_lock,
   return art_portable_jni_method_start(self);
 }
 
-static void PopLocalReferences(uint32_t saved_local_ref_cookie, Thread* self) {
+static void PopLocalReferences(uint32_t saved_local_ref_cookie, Thread* self)
+    SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   JNIEnvExt* env = self->GetJniEnv();
   env->locals.SetSegmentState(env->local_ref_cookie);
   env->local_ref_cookie = saved_local_ref_cookie;
diff --git a/runtime/entrypoints/quick/quick_jni_entrypoints.cc b/runtime/entrypoints/quick/quick_jni_entrypoints.cc
index 9c9cca8..5d36b4c 100644
--- a/runtime/entrypoints/quick/quick_jni_entrypoints.cc
+++ b/runtime/entrypoints/quick/quick_jni_entrypoints.cc
@@ -61,7 +61,8 @@ static void GoToRunnable(Thread* self) NO_THREAD_SAFETY_ANALYSIS {
   }
 }
 
-static void PopLocalReferences(uint32_t saved_local_ref_cookie, Thread* self) {
+static void PopLocalReferences(uint32_t saved_local_ref_cookie, Thread* self)
+    SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   JNIEnvExt* env = self->GetJniEnv();
   env->locals.SetSegmentState(env->local_ref_cookie);
   env->local_ref_cookie = saved_local_ref_cookie;
diff --git a/runtime/jni_internal.cc b/runtime/jni_internal.cc
index 80969bf..6f3317d 100644
--- a/runtime/jni_internal.cc
+++ b/runtime/jni_internal.cc
@@ -722,7 +722,9 @@ class JNI {
   }
 
   static jint PushLocalFrame(JNIEnv* env, jint capacity) {
-    if (EnsureLocalCapacity(env, capacity, "PushLocalFrame") != JNI_OK) {
+    // TODO: SOA may not be necessary but I do it to please lock annotations.
+    ScopedObjectAccess soa(env);
+    if (EnsureLocalCapacity(soa, capacity, "PushLocalFrame") != JNI_OK) {
       return JNI_ERR;
     }
     static_cast<JNIEnvExt*>(env)->PushFrame(capacity);
@@ -737,7 +739,9 @@ class JNI {
   }
 
   static jint EnsureLocalCapacity(JNIEnv* env, jint desired_capacity) {
-    return EnsureLocalCapacity(env, desired_capacity, "EnsureLocalCapacity");
+    // TODO: SOA may not be necessary but I do it to please lock annotations.
+    ScopedObjectAccess soa(env);
+    return EnsureLocalCapacity(soa, desired_capacity, "EnsureLocalCapacity");
   }
 
   static jobject NewGlobalRef(JNIEnv* env, jobject obj) {
@@ -795,6 +799,7 @@ class JNI {
     if (obj == nullptr) {
       return;
     }
+    ScopedObjectAccess soa(env);
     IndirectReferenceTable& locals = reinterpret_cast<JNIEnvExt*>(env)->locals;
 
     uint32_t cookie = reinterpret_cast<JNIEnvExt*>(env)->local_ref_cookie;
@@ -2457,18 +2462,17 @@ class JNI {
   }
 
  private:
-  static jint EnsureLocalCapacity(JNIEnv* env, jint desired_capacity,
-                                  const char* caller) {
+  static jint EnsureLocalCapacity(ScopedObjectAccess& soa, jint desired_capacity,
+                                  const char* caller) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
     // TODO: we should try to expand the table if necessary.
     if (desired_capacity < 0 || desired_capacity > static_cast<jint>(kLocalsMax)) {
       LOG(ERROR) << "Invalid capacity given to " << caller << ": " << desired_capacity;
       return JNI_ERR;
     }
     // TODO: this isn't quite right, since "capacity" includes holes.
-    size_t capacity = static_cast<JNIEnvExt*>(env)->locals.Capacity();
+    const size_t capacity = soa.Env()->locals.Capacity();
     bool okay = (static_cast<jint>(kLocalsMax - capacity) >= desired_capacity);
     if (!okay) {
-      ScopedObjectAccess soa(env);
       soa.Self()->ThrowOutOfMemoryError(caller);
     }
     return okay ? JNI_OK : JNI_ERR;
@@ -2892,13 +2896,14 @@ void JNIEnvExt::DumpReferenceTables(std::ostream& os) {
   monitors.Dump(os);
 }
 
-void JNIEnvExt::PushFrame(int /*capacity*/) {
+void JNIEnvExt::PushFrame(int capacity) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
+  UNUSED(capacity);  // cpplint gets confused with (int) and thinks its a cast.
   // TODO: take 'capacity' into account.
   stacked_local_ref_cookies.push_back(local_ref_cookie);
   local_ref_cookie = locals.GetSegmentState();
 }
 
-void JNIEnvExt::PopFrame() {
+void JNIEnvExt::PopFrame() SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) {
   locals.SetSegmentState(local_ref_cookie);
   local_ref_cookie = stacked_local_ref_cookies.back();
   stacked_local_ref_cookies.pop_back();
diff --git a/runtime/jni_internal.h b/runtime/jni_internal.h
index 5964947..37195eb 100644
--- a/runtime/jni_internal.h
+++ b/runtime/jni_internal.h
@@ -170,7 +170,7 @@ struct JNIEnvExt : public JNIEnv {
   uint32_t local_ref_cookie;
 
   // JNI local references.
-  IndirectReferenceTable locals;
+  IndirectReferenceTable locals GUARDED_BY(Locks::mutator_lock_);
 
   // Stack of cookies corresponding to PushLocalFrame/PopLocalFrame calls.
   // TODO: to avoid leaks (and bugs), we need to clear this vector on entry (or return)
diff --git a/runtime/runtime.cc b/runtime/runtime.cc
index 986c09d..95a6b39 100644
--- a/runtime/runtime.cc
+++ b/runtime/runtime.cc
@@ -395,7 +395,10 @@ bool Runtime::Start() {
 
   system_class_loader_ = CreateSystemClassLoader();
 
-  self->GetJniEnv()->locals.AssertEmpty();
+  {
+    ScopedObjectAccess soa(self);
+    self->GetJniEnv()->locals.AssertEmpty();
+  }
 
   VLOG(startup) << "Runtime::Start exiting";