Fix binder proxy death notice tracking

There was an issue with stale recipient tracking when BinderProxy weak
references had been purged and a new proxy object allocated for a
still-live underlying IBinder.  The death recipient bookkeeping has
now been reworked so that it's fundmentally tied to the BinderProxy
instances, not maintained as global state, to prevent this sort of
confusion entirely.

Bug 3499939

Change-Id: I75c5216b6d53b90868ac969e32c9725201e51be3

2 files changed, 35 insertions, 52 deletions

diff --git a/core/java/android/os/Binder.java b/core/java/android/os/Binder.java
index a402c91..7dc36f9 100644
--- a/core/java/android/os/Binder.java
+++ b/core/java/android/os/Binder.java
@@ -392,4 +392,5 @@ final class BinderProxy implements IBinder {
     
     final private WeakReference mSelf;
     private int mObject;
+    private int mOrgue;
 }
diff --git a/core/jni/android_util_Binder.cpp b/core/jni/android_util_Binder.cpp
index 15362eb..5deed1e 100644
--- a/core/jni/android_util_Binder.cpp
+++ b/core/jni/android_util_Binder.cpp
@@ -105,6 +105,7 @@ static struct binderproxy_offsets_t
     // Object state.
     jfieldID mObject;
     jfieldID mSelf;
+    jfieldID mOrgue;
 
 } gBinderProxyOffsets;
 
@@ -374,12 +375,12 @@ public:
 class JavaDeathRecipient : public IBinder::DeathRecipient
 {
 public:
-    JavaDeathRecipient(JNIEnv* env, jobject object, sp<DeathRecipientList>& list)
+    JavaDeathRecipient(JNIEnv* env, jobject object, const sp<DeathRecipientList>& list)
         : mVM(jnienv_to_javavm(env)), mObject(env->NewGlobalRef(object)), mList(list)
     {
         // These objects manage their own lifetimes so are responsible for final bookkeeping.
         // The list holds a strong reference to this object.
-        mList->add(this);
+        list->add(this);
 
         android_atomic_inc(&gNumDeathRefs);
         incRefsCreated(env);
@@ -404,7 +405,10 @@ public:
 
     void clearReference()
     {
-        mList->remove(this);
+        sp<DeathRecipientList> list = mList.promote();
+        if (list != NULL) {
+            list->remove(this);
+        }
     }
 
     bool matches(jobject obj) {
@@ -424,7 +428,7 @@ protected:
 private:
     JavaVM* const   mVM;
     jobject const   mObject;
-    sp<DeathRecipientList> mList;
+    wp<DeathRecipientList> mList;
 };
 
 // ----------------------------------------------------------------------------
@@ -436,7 +440,7 @@ DeathRecipientList::~DeathRecipientList() {
     // to the list are holding references on the list object.  Only when they are torn
     // down can the list header be destroyed.
     if (mList.size() > 0) {
-        LOGE("Retiring binder %p with extant death recipients\n", this);
+        LOGE("Retiring DRL %p with extant death recipients\n", this);
     }
 }
 
@@ -470,9 +474,6 @@ sp<JavaDeathRecipient> DeathRecipientList::find(jobject recipient) {
     return NULL;
 }
 
-static KeyedVector<IBinder*, sp<DeathRecipientList> > gDeathRecipientsByIBinder;
-static Mutex gDeathRecipientMapLock;
-
 // ----------------------------------------------------------------------------
 
 namespace android {
@@ -517,7 +518,7 @@ jobject javaObjectForIBinder(JNIEnv* env, const sp<IBinder>& val)
 
     object = env->NewObject(gBinderProxyOffsets.mClass, gBinderProxyOffsets.mConstructor);
     if (object != NULL) {
-        LOGV("objectForBinder %p: created new %p!\n", val.get(), object);
+        LOGV("objectForBinder %p: created new proxy %p !\n", val.get(), object);
         // The proxy holds a reference to the native object.
         env->SetIntField(object, gBinderProxyOffsets.mObject, (int)val.get());
         val->incStrong(object);
@@ -529,6 +530,11 @@ jobject javaObjectForIBinder(JNIEnv* env, const sp<IBinder>& val)
         val->attachObject(&gBinderProxyOffsets, refObject,
                 jnienv_to_javavm(env), proxy_cleanup);
 
+        // Also remember the death recipients registered on this proxy
+        sp<DeathRecipientList> drl = new DeathRecipientList;
+        drl->incStrong((void*)javaObjectForIBinder);
+        env->SetIntField(object, gBinderProxyOffsets.mOrgue, reinterpret_cast<jint>(drl.get()));
+
         // Note that a new object reference has been created.
         android_atomic_inc(&gNumProxyRefs);
         incRefsCreated(env);
@@ -1027,24 +1033,9 @@ static void android_os_BinderProxy_linkToDeath(JNIEnv* env, jobject obj,
     LOGV("linkToDeath: binder=%p recipient=%p\n", target, recipient);
 
     if (!target->localBinder()) {
-        sp<JavaDeathRecipient> jdr;
-
-        {
-            sp<DeathRecipientList> list;
-            AutoMutex _maplocker(gDeathRecipientMapLock);
-
-            ssize_t listIndex = gDeathRecipientsByIBinder.indexOfKey(target);
-            if (listIndex < 0) {
-                // Set up the death notice bookkeeping for this binder lazily
-                list = new DeathRecipientList;
-                gDeathRecipientsByIBinder.add(target, list);
-            } else {
-                list = gDeathRecipientsByIBinder.valueAt(listIndex);
-            }
-
-            jdr = new JavaDeathRecipient(env, recipient, list);
-        }
-
+        DeathRecipientList* list = (DeathRecipientList*)
+                env->GetIntField(obj, gBinderProxyOffsets.mOrgue);
+        sp<JavaDeathRecipient> jdr = new JavaDeathRecipient(env, recipient, list);
         status_t err = target->linkToDeath(jdr, NULL, flags);
         if (err != NO_ERROR) {
             // Failure adding the death recipient, so clear its reference
@@ -1075,20 +1066,11 @@ static jboolean android_os_BinderProxy_unlinkToDeath(JNIEnv* env, jobject obj,
 
     if (!target->localBinder()) {
         status_t err = NAME_NOT_FOUND;
-        sp<JavaDeathRecipient> origJDR;
-        {
-            AutoMutex _maplocker(gDeathRecipientMapLock);
-            ssize_t listIndex = gDeathRecipientsByIBinder.indexOfKey(target);
-            if (listIndex >= 0) {
-                sp<DeathRecipientList> list = gDeathRecipientsByIBinder.valueAt(listIndex);
-                origJDR = list->find(recipient);
-            } else {
-                // If there is no DeathRecipientList for this binder, it means the binder
-                // is dead and in the process of being cleaned up.
-                err = DEAD_OBJECT;
-            }
-        }
-        // If we found the matching recipient, proceed to unlink using that
+
+        // If we find the matching recipient, proceed to unlink using that
+        DeathRecipientList* list = (DeathRecipientList*)
+                env->GetIntField(obj, gBinderProxyOffsets.mOrgue);
+        sp<JavaDeathRecipient> origJDR = list->find(recipient);
         if (origJDR != NULL) {
             wp<IBinder::DeathRecipient> dr;
             err = target->unlinkToDeath(origJDR, NULL, flags, &dr);
@@ -1115,20 +1097,17 @@ static jboolean android_os_BinderProxy_unlinkToDeath(JNIEnv* env, jobject obj,
 static void android_os_BinderProxy_destroy(JNIEnv* env, jobject obj)
 {
     IBinder* b = (IBinder*)
-        env->GetIntField(obj, gBinderProxyOffsets.mObject);
-    LOGV("Destroying BinderProxy %p: binder=%p\n", obj, b);
+            env->GetIntField(obj, gBinderProxyOffsets.mObject);
+    DeathRecipientList* drl = (DeathRecipientList*)
+            env->GetIntField(obj, gBinderProxyOffsets.mOrgue);
+
+    LOGV("Destroying BinderProxy %p: binder=%p drl=%p\n", obj, b, drl);
     env->SetIntField(obj, gBinderProxyOffsets.mObject, 0);
+    env->SetIntField(obj, gBinderProxyOffsets.mOrgue, 0);
+    drl->decStrong((void*)javaObjectForIBinder);
     b->decStrong(obj);
-    IPCThreadState::self()->flushCommands();
 
-    // tear down the death recipient bookkeeping
-    {
-        AutoMutex _maplocker(gDeathRecipientMapLock);
-        ssize_t listIndex = gDeathRecipientsByIBinder.indexOfKey(b);
-        if (listIndex >= 0) {
-            gDeathRecipientsByIBinder.removeItemsAt((size_t)listIndex);
-        }
-    }
+    IPCThreadState::self()->flushCommands();
 }
 
 // ----------------------------------------------------------------------------
@@ -1178,6 +1157,9 @@ static int int_register_android_os_BinderProxy(JNIEnv* env)
     gBinderProxyOffsets.mSelf
         = env->GetFieldID(clazz, "mSelf", "Ljava/lang/ref/WeakReference;");
     assert(gBinderProxyOffsets.mSelf);
+    gBinderProxyOffsets.mOrgue
+        = env->GetFieldID(clazz, "mOrgue", "I");
+    assert(gBinderProxyOffsets.mOrgue);
 
     return AndroidRuntime::registerNativeMethods(
         env, kBinderProxyPathName,