From 86284c60b52c40f027427aa32dbe8dc0899b63d5 Mon Sep 17 00:00:00 2001 From: Christopher Tate Date: Wed, 17 Aug 2011 15:19:29 -0700 Subject: [PATCH] Stop leaking DeathRecipient and BinderProxy instances The native-side death recipient object holds a global reference to the DVM-side DeathRecipient instance. This is necessary so that the DeathRecipient isn't GC'd early in the case when the caller of linkToDeath() doesn't retain their own reference to the recipient instance. However, that global reference was never being released in association with the delivery of the actual binderDied() event. In that case, if the death recipient did not explicitly call unlinkToDeath() themselves, the hard global reference was maintained and a hard-reference cycle was perpetuated, crossing the native <-> DVM boundary. This was visible as a gradual leakage of DVM-side DeathRecipient and BinderProxy instances. The one problematic constraint is that it needs to be valid to call unlinkToDeath() cleanly *after* binderDied() is delivered to the given recipient; that requires that we have the hard reference linkage described above. The fix is to replace the hard global reference with a weak reference to the DVM-side DeathRecipient after we deliver binderDied() to that recipient. In the case where the caller has retained their own reference to the instance (i.e. they might still call unlinkToDeath() on it later), the weak reference stays live and everything works cleanly (and is reaped explicitly by unlinkToDeath() as usual). In the case where the caller has *not* retained the instance to call unlinkToDeath() themselves, demoting to a weak DeathRecipient reference allows the DeathRecipient to be GC'd, which in turn frees the DVM-side BinderProxy it's tied to, and so on around the chain to free all of the associated allocations. Fixes bug 5174537 Change-Id: Ia4ad76e667140cc2a1b74508bbba652ab31ab876 --- core/jni/android_util_Binder.cpp | 52 +++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 15 deletions(-) diff --git a/core/jni/android_util_Binder.cpp b/core/jni/android_util_Binder.cpp index b432d658adc41..b8f2d6f2e0681 100644 --- a/core/jni/android_util_Binder.cpp +++ b/core/jni/android_util_Binder.cpp @@ -385,7 +385,8 @@ class JavaDeathRecipient : public IBinder::DeathRecipient { public: JavaDeathRecipient(JNIEnv* env, jobject object, const sp& list) - : mVM(jnienv_to_javavm(env)), mObject(env->NewGlobalRef(object)), mList(list) + : mVM(jnienv_to_javavm(env)), mObject(env->NewGlobalRef(object)), + mObjectWeak(NULL), mList(list) { // These objects manage their own lifetimes so are responsible for final bookkeeping. // The list holds a strong reference to this object. @@ -398,16 +399,23 @@ public: void binderDied(const wp& who) { - JNIEnv* env = javavm_to_jnienv(mVM); - LOGDEATH("Receiving binderDied() on JavaDeathRecipient %p\n", this); + if (mObject != NULL) { + JNIEnv* env = javavm_to_jnienv(mVM); - env->CallStaticVoidMethod(gBinderProxyOffsets.mClass, - gBinderProxyOffsets.mSendDeathNotice, mObject); - jthrowable excep = env->ExceptionOccurred(); - if (excep) { - report_exception(env, excep, - "*** Uncaught exception returned from death notification!"); + env->CallStaticVoidMethod(gBinderProxyOffsets.mClass, + gBinderProxyOffsets.mSendDeathNotice, mObject); + jthrowable excep = env->ExceptionOccurred(); + if (excep) { + report_exception(env, excep, + "*** Uncaught exception returned from death notification!"); + } + + // Demote from strong ref to weak after binderDied() has been delivered, + // to allow the DeathRecipient and BinderProxy to be GC'd if no longer needed. + mObjectWeak = env->NewWeakGlobalRef(mObject); + env->DeleteGlobalRef(mObject); + mObject = NULL; } } @@ -423,8 +431,17 @@ public: } bool matches(jobject obj) { + bool result; JNIEnv* env = javavm_to_jnienv(mVM); - return env->IsSameObject(obj, mObject); + + if (mObject != NULL) { + result = env->IsSameObject(obj, mObject); + } else { + jobject me = env->NewLocalRef(mObjectWeak); + result = env->IsSameObject(obj, me); + env->DeleteLocalRef(me); + } + return result; } protected: @@ -433,12 +450,17 @@ protected: //LOGI("Removing death ref: recipient=%p\n", mObject); android_atomic_dec(&gNumDeathRefs); JNIEnv* env = javavm_to_jnienv(mVM); - env->DeleteGlobalRef(mObject); + if (mObject != NULL) { + env->DeleteGlobalRef(mObject); + } else { + env->DeleteWeakGlobalRef(mObjectWeak); + } } private: - JavaVM* const mVM; - jobject const mObject; + JavaVM* const mVM; + jobject mObject; + jweak mObjectWeak; // will be a weak ref to the same VM-side DeathRecipient after binderDied() wp mList; }; @@ -512,7 +534,7 @@ jobject javaObjectForIBinder(JNIEnv* env, const sp& val) if (val->checkSubclass(&gBinderOffsets)) { // One of our own! jobject object = static_cast(val.get())->object(); - //printf("objectForBinder %p: it's our own %p!\n", val.get(), object); + LOGDEATH("objectForBinder %p: it's our own %p!\n", val.get(), object); return object; } @@ -528,7 +550,7 @@ jobject javaObjectForIBinder(JNIEnv* env, const sp& val) LOGV("objectForBinder %p: found existing %p!\n", val.get(), res); return res; } - LOGV("Proxy object %p of IBinder %p no longer in working set!!!", object, val.get()); + LOGDEATH("Proxy object %p of IBinder %p no longer in working set!!!", object, val.get()); android_atomic_dec(&gNumProxyRefs); val->detachObject(&gBinderProxyOffsets); env->DeleteGlobalRef(object);