Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 86284c60 authored by Christopher Tate's avatar Christopher Tate
Browse files

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
parent 8f668414

core/jni/android_util_Binder.cpp

+37 −15
Original line number Diff line number Diff line
@@ -385,7 +385,8 @@ class JavaDeathRecipient : public IBinder::DeathRecipient 
{

public:

    JavaDeathRecipient(JNIEnv* env, jobject object, const sp<DeathRecipientList>& 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,9 +399,9 @@ public: 


    void binderDied(const wp<IBinder>& 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);
@@ -409,6 +410,13 @@ public: 
                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;

        }

    }



    void clearReference()
@@ -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);

        if (mObject != NULL) {

            env->DeleteGlobalRef(mObject);

        } else {

            env->DeleteWeakGlobalRef(mObjectWeak);

        }

    }



private:

    JavaVM* const mVM;

    jobject const   mObject;

    jobject mObject;

    jweak mObjectWeak; // will be a weak ref to the same VM-side DeathRecipient after binderDied()

    wp<DeathRecipientList> mList;

};
@@ -512,7 +534,7 @@ jobject javaObjectForIBinder(JNIEnv* env, const sp<IBinder>& val) 
    if (val->checkSubclass(&gBinderOffsets)) {

        // One of our own!

        jobject object = static_cast<JavaBBinder*>(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<IBinder>& 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);