Binder linkage no longer depends on JNI objrefs as persistent tokens
There are two areas that have changed to eliminate the assumption that
local jobject references are both canonical and persistent:
1. JavaBBinderHolder no longer holds onto and reuses it parent object
reference per se. Since the underlying JavaBBinder object holds a
real global ref, this was redundant anyway. Now, for purposes of its
transient need to perform JNI operations, it simply uses the current
jobject ref(s) passed during method invocation, and no longer attempts
to hold these refs beyond the scope of a single invocation.
2. Binder obituaries no longer assume that a jobject reference to a
recipient will always compare == as a 32-bit value with any future
reference to the same object. The implementation now asks Dalvik
whether object references match.
Bug 2090115
Change-Id: If62edd554d0a9fbb2d2977b0cbf8ad7cc8e2e68d
diff --git a/core/jni/android_util_Binder.cpp b/core/jni/android_util_Binder.cpp
index 7a53874..bd5305d 100644
--- a/core/jni/android_util_Binder.cpp
+++ b/core/jni/android_util_Binder.cpp
@@ -31,6 +31,8 @@
#include <binder/IPCThreadState.h>
#include <utils/Log.h>
#include <utils/SystemClock.h>
+#include <utils/List.h>
+#include <utils/KeyedVector.h>
#include <cutils/logger.h>
#include <binder/Parcel.h>
#include <binder/ProcessState.h>
@@ -322,25 +324,15 @@
class JavaBBinderHolder : public RefBase
{
public:
- JavaBBinderHolder(JNIEnv* env, jobject object)
- : mObject(object)
- {
- LOGV("Creating JavaBBinderHolder for Object %p\n", object);
- }
- ~JavaBBinderHolder()
- {
- LOGV("Destroying JavaBBinderHolder for Object %p\n", mObject);
- }
-
- sp<JavaBBinder> get(JNIEnv* env)
+ sp<JavaBBinder> get(JNIEnv* env, jobject obj)
{
AutoMutex _l(mLock);
sp<JavaBBinder> b = mBinder.promote();
if (b == NULL) {
- b = new JavaBBinder(env, mObject);
+ b = new JavaBBinder(env, obj);
mBinder = b;
LOGV("Creating JavaBinder %p (refs %p) for Object %p, weakCount=%d\n",
- b.get(), b->getWeakRefs(), mObject, b->getWeakRefs()->getWeakCount());
+ b.get(), b->getWeakRefs(), obj, b->getWeakRefs()->getWeakCount());
}
return b;
@@ -354,20 +346,41 @@
private:
Mutex mLock;
- jobject mObject;
wp<JavaBBinder> mBinder;
};
// ----------------------------------------------------------------------------
+// Per-IBinder death recipient bookkeeping. This is how we reconcile local jobject
+// death recipient references passed in through JNI with the permanent corresponding
+// JavaDeathRecipient objects.
+
+class JavaDeathRecipient;
+
+class DeathRecipientList : public RefBase {
+ List< sp<JavaDeathRecipient> > mList;
+ Mutex mLock;
+
+public:
+ ~DeathRecipientList();
+
+ void add(const sp<JavaDeathRecipient>& recipient);
+ void remove(const sp<JavaDeathRecipient>& recipient);
+ sp<JavaDeathRecipient> find(jobject recipient);
+};
+
+// ----------------------------------------------------------------------------
+
class JavaDeathRecipient : public IBinder::DeathRecipient
{
public:
- JavaDeathRecipient(JNIEnv* env, jobject object)
- : mVM(jnienv_to_javavm(env)), mObject(env->NewGlobalRef(object)),
- mHoldsRef(true)
+ JavaDeathRecipient(JNIEnv* env, jobject object, sp<DeathRecipientList>& list)
+ : mVM(jnienv_to_javavm(env)), mObject(env->NewGlobalRef(object)), mList(list)
{
- incStrong(this);
+ // These objects manage their own lifetimes so are responsible for final bookkeeping.
+ // The list holds a strong reference to this object.
+ mList->add(this);
+
android_atomic_inc(&gNumDeathRefs);
incRefsCreated(env);
}
@@ -391,16 +404,12 @@
void clearReference()
{
- bool release = false;
- mLock.lock();
- if (mHoldsRef) {
- mHoldsRef = false;
- release = true;
- }
- mLock.unlock();
- if (release) {
- decStrong(this);
- }
+ mList->remove(this);
+ }
+
+ bool matches(jobject obj) {
+ JNIEnv* env = javavm_to_jnienv(mVM);
+ return env->IsSameObject(obj, mObject);
}
protected:
@@ -415,12 +424,57 @@
private:
JavaVM* const mVM;
jobject const mObject;
- Mutex mLock;
- bool mHoldsRef;
+ sp<DeathRecipientList> mList;
};
// ----------------------------------------------------------------------------
+DeathRecipientList::~DeathRecipientList() {
+ AutoMutex _l(mLock);
+
+ // Should never happen -- the JavaDeathRecipient objects that have added themselves
+ // 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);
+ }
+}
+
+void DeathRecipientList::add(const sp<JavaDeathRecipient>& recipient) {
+ AutoMutex _l(mLock);
+
+ mList.push_back(recipient);
+}
+
+void DeathRecipientList::remove(const sp<JavaDeathRecipient>& recipient) {
+ AutoMutex _l(mLock);
+
+ List< sp<JavaDeathRecipient> >::iterator iter;
+ for (iter = mList.begin(); iter != mList.end(); iter++) {
+ if (*iter == recipient) {
+ mList.erase(iter);
+ return;
+ }
+ }
+}
+
+sp<JavaDeathRecipient> DeathRecipientList::find(jobject recipient) {
+ AutoMutex _l(mLock);
+
+ List< sp<JavaDeathRecipient> >::iterator iter;
+ for (iter = mList.begin(); iter != mList.end(); iter++) {
+ if ((*iter)->matches(recipient)) {
+ return *iter;
+ }
+ }
+ return NULL;
+}
+
+static KeyedVector<IBinder*, sp<DeathRecipientList> > gDeathRecipientsByIBinder;
+static Mutex gDeathRecipientMapLock;
+
+// ----------------------------------------------------------------------------
+
namespace android {
static void proxy_cleanup(const void* id, void* obj, void* cleanupCookie)
@@ -490,7 +544,7 @@
if (env->IsInstanceOf(obj, gBinderOffsets.mClass)) {
JavaBBinderHolder* jbh = (JavaBBinderHolder*)
env->GetIntField(obj, gBinderOffsets.mObject);
- return jbh != NULL ? jbh->get(env) : NULL;
+ return jbh != NULL ? jbh->get(env, obj) : NULL;
}
if (env->IsInstanceOf(obj, gBinderProxyOffsets.mClass)) {
@@ -621,26 +675,26 @@
IPCThreadState::self()->flushCommands();
}
-static void android_os_Binder_init(JNIEnv* env, jobject clazz)
+static void android_os_Binder_init(JNIEnv* env, jobject obj)
{
- JavaBBinderHolder* jbh = new JavaBBinderHolder(env, clazz);
+ JavaBBinderHolder* jbh = new JavaBBinderHolder();
if (jbh == NULL) {
jniThrowException(env, "java/lang/OutOfMemoryError", NULL);
return;
}
- LOGV("Java Binder %p: acquiring first ref on holder %p", clazz, jbh);
- jbh->incStrong(clazz);
- env->SetIntField(clazz, gBinderOffsets.mObject, (int)jbh);
+ LOGV("Java Binder %p: acquiring first ref on holder %p", obj, jbh);
+ jbh->incStrong((void*)android_os_Binder_init);
+ env->SetIntField(obj, gBinderOffsets.mObject, (int)jbh);
}
-static void android_os_Binder_destroy(JNIEnv* env, jobject clazz)
+static void android_os_Binder_destroy(JNIEnv* env, jobject obj)
{
JavaBBinderHolder* jbh = (JavaBBinderHolder*)
- env->GetIntField(clazz, gBinderOffsets.mObject);
+ env->GetIntField(obj, gBinderOffsets.mObject);
if (jbh != NULL) {
- env->SetIntField(clazz, gBinderOffsets.mObject, 0);
- LOGV("Java Binder %p: removing ref on holder %p", clazz, jbh);
- jbh->decStrong(clazz);
+ env->SetIntField(obj, gBinderOffsets.mObject, 0);
+ LOGV("Java Binder %p: removing ref on holder %p", obj, jbh);
+ jbh->decStrong((void*)android_os_Binder_init);
} else {
// Encountering an uninitialized binder is harmless. All it means is that
// the Binder was only partially initialized when its finalizer ran and called
@@ -648,7 +702,7 @@
// For example, a Binder subclass constructor might have thrown an exception before
// it could delegate to its superclass's constructor. Consequently init() would
// not have been called and the holder pointer would remain NULL.
- LOGV("Java Binder %p: ignoring uninitialized binder", clazz);
+ LOGV("Java Binder %p: ignoring uninitialized binder", obj);
}
}
@@ -973,8 +1027,25 @@
LOGV("linkToDeath: binder=%p recipient=%p\n", target, recipient);
if (!target->localBinder()) {
- sp<JavaDeathRecipient> jdr = new JavaDeathRecipient(env, recipient);
- status_t err = target->linkToDeath(jdr, recipient, flags);
+ 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);
+ }
+
+ status_t err = target->linkToDeath(jdr, NULL, flags);
if (err != NO_ERROR) {
// Failure adding the death recipient, so clear its reference
// now.
@@ -1003,15 +1074,29 @@
LOGV("unlinkToDeath: binder=%p recipient=%p\n", target, recipient);
if (!target->localBinder()) {
- wp<IBinder::DeathRecipient> dr;
- status_t err = target->unlinkToDeath(NULL, recipient, flags, &dr);
- if (err == NO_ERROR && dr != NULL) {
- sp<IBinder::DeathRecipient> sdr = dr.promote();
- JavaDeathRecipient* jdr = static_cast<JavaDeathRecipient*>(sdr.get());
- if (jdr != NULL) {
- jdr->clearReference();
+ 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);
}
}
+ // If we found the matching recipient, proceed to unlink using that
+ if (origJDR != NULL) {
+ wp<IBinder::DeathRecipient> dr;
+ err = target->unlinkToDeath(origJDR, NULL, flags, &dr);
+ if (err == NO_ERROR && dr != NULL) {
+ sp<IBinder::DeathRecipient> sdr = dr.promote();
+ JavaDeathRecipient* jdr = static_cast<JavaDeathRecipient*>(sdr.get());
+ if (jdr != NULL) {
+ jdr->clearReference();
+ }
+ }
+ }
+
if (err == NO_ERROR || err == DEAD_OBJECT) {
res = JNI_TRUE;
} else {
@@ -1031,6 +1116,15 @@
env->SetIntField(obj, gBinderProxyOffsets.mObject, 0);
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);
+ }
+ }
}
// ----------------------------------------------------------------------------
diff --git a/include/binder/IBinder.h b/include/binder/IBinder.h
index 749a977..81b56c2 100644
--- a/include/binder/IBinder.h
+++ b/include/binder/IBinder.h
@@ -98,7 +98,7 @@
* Register the @a recipient for a notification if this binder
* goes away. If this binder object unexpectedly goes away
* (typically because its hosting process has been killed),
- * then DeathRecipient::binderDied() will be called with a referene
+ * then DeathRecipient::binderDied() will be called with a reference
* to this.
*
* The @a cookie is optional -- if non-NULL, it should be a