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>

1 files changed, 13 insertions, 8 deletions

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();