Add an extra level of indirection to make sure that native IHwBinder

objects, while alive, maintain a strong reference to their Java
counterpart.

Bug: 33042939
Test: hidl_test_java
Change-Id: I69179772b3218705e9ef46cbb10b21b6473280d6

diff --git a/core/jni/android_os_HwBinder.cpp b/core/jni/android_os_HwBinder.cpp
index 19f779f..cdd3c09 100644 (file)
--- a/core/jni/android_os_HwBinder.cpp
+++ b/core/jni/android_os_HwBinder.cpp
@@ -58,6 +58,29 @@ static struct fields_t {
     jmethodID onTransactID;
 } gFields;
 
+struct JHwBinderHolder : public RefBase {
+    JHwBinderHolder() {}
+
+    sp<JHwBinder> get(JNIEnv *env, jobject obj) {
+        Mutex::Autolock autoLock(mLock);
+
+        sp<JHwBinder> binder = mBinder.promote();
+
+        if (binder == NULL) {
+            binder = new JHwBinder(env, obj);
+            mBinder = binder;
+        }
+
+        return binder;
+    }
+
+private:
+    Mutex mLock;
+    wp<JHwBinder> mBinder;
+
+    DISALLOW_COPY_AND_ASSIGN(JHwBinderHolder);
+};
+
 // static
 void JHwBinder::InitClass(JNIEnv *env) {
     ScopedLocalRef<jclass> clazz(
@@ -75,10 +98,10 @@ void JHwBinder::InitClass(JNIEnv *env) {
 }
 
 // static
-sp<JHwBinder> JHwBinder::SetNativeContext(
-        JNIEnv *env, jobject thiz, const sp<JHwBinder> &context) {
-    sp<JHwBinder> old =
-        (JHwBinder *)env->GetLongField(thiz, gFields.contextID);
+sp<JHwBinderHolder> JHwBinder::SetNativeContext(
+        JNIEnv *env, jobject thiz, const sp<JHwBinderHolder> &context) {
+    sp<JHwBinderHolder> old =
+        (JHwBinderHolder *)env->GetLongField(thiz, gFields.contextID);
 
     if (context != NULL) {
         context->incStrong(NULL /* id */);
@@ -94,27 +117,27 @@ sp<JHwBinder> JHwBinder::SetNativeContext(
 }
 
 // static
-sp<JHwBinder> JHwBinder::GetNativeContext(
+sp<JHwBinder> JHwBinder::GetNativeBinder(
         JNIEnv *env, jobject thiz) {
-    return (JHwBinder *)env->GetLongField(thiz, gFields.contextID);
+    JHwBinderHolder *holder =
+        reinterpret_cast<JHwBinderHolder *>(
+                env->GetLongField(thiz, gFields.contextID));
+
+    return holder->get(env, thiz);
 }
 
 JHwBinder::JHwBinder(JNIEnv *env, jobject thiz) {
     jclass clazz = env->GetObjectClass(thiz);
     CHECK(clazz != NULL);
 
-    mClass = (jclass)env->NewGlobalRef(clazz);
-    mObject = env->NewWeakGlobalRef(thiz);
+    mObject = env->NewGlobalRef(thiz);
 }
 
 JHwBinder::~JHwBinder() {
     JNIEnv *env = AndroidRuntime::getJNIEnv();
 
-    env->DeleteWeakGlobalRef(mObject);
+    env->DeleteGlobalRef(mObject);
     mObject = NULL;
-
-    env->DeleteGlobalRef(mClass);
-    mClass = NULL;
 }
 
 status_t JHwBinder::onTransact(
@@ -203,10 +226,10 @@ status_t JHwBinder::onTransact(
 using namespace android;
 
 static void releaseNativeContext(void *nativeContext) {
-    sp<JHwBinder> binder = (JHwBinder *)nativeContext;
+    sp<JHwBinderHolder> context = static_cast<JHwBinderHolder *>(nativeContext);
 
-    if (binder != NULL) {
-        binder->decStrong(NULL /* id */);
+    if (context != NULL) {
+        context->decStrong(NULL /* id */);
     }
 }
 
@@ -217,8 +240,7 @@ static jlong JHwBinder_native_init(JNIEnv *env) {
 }
 
 static void JHwBinder_native_setup(JNIEnv *env, jobject thiz) {
-    sp<JHwBinder> context = new JHwBinder(env, thiz);
-
+    sp<JHwBinderHolder> context = new JHwBinderHolder;
     JHwBinder::SetNativeContext(env, thiz, context);
 }
 
@@ -246,7 +268,7 @@ static void JHwBinder_native_registerService(
         return;  // XXX exception already pending?
     }
 
-    sp<hardware::IBinder> binder = JHwBinder::GetNativeContext(env, thiz);
+    sp<hardware::IBinder> binder = JHwBinder::GetNativeBinder(env, thiz);
 
     /* TODO(b/33440494) this is not right */
     sp<hidl::base::V1_0::IBase> base = new hidl::base::V1_0::BpHwBase(binder);

diff --git a/core/jni/android_os_HwBinder.h b/core/jni/android_os_HwBinder.h
index fa8fe01..5352f1e 100644 (file)
--- a/core/jni/android_os_HwBinder.h
+++ b/core/jni/android_os_HwBinder.h
@@ -24,13 +24,15 @@
 
 namespace android {
 
+struct JHwBinderHolder;
+
 struct JHwBinder : public hardware::BHwBinder {
     static void InitClass(JNIEnv *env);
 
-    static sp<JHwBinder> SetNativeContext(
-            JNIEnv *env, jobject thiz, const sp<JHwBinder> &context);
+    static sp<JHwBinderHolder> SetNativeContext(
+            JNIEnv *env, jobject thiz, const sp<JHwBinderHolder> &context);
 
-    static sp<JHwBinder> GetNativeContext(JNIEnv *env, jobject thiz);
+    static sp<JHwBinder> GetNativeBinder(JNIEnv *env, jobject thiz);
 
     JHwBinder(JNIEnv *env, jobject thiz);
 
@@ -45,7 +47,6 @@ protected:
             TransactCallback callback);
 
 private:
-    jclass mClass;
     jobject mObject;
 
     DISALLOW_COPY_AND_ASSIGN(JHwBinder);

diff --git a/core/jni/android_os_HwParcel.cpp b/core/jni/android_os_HwParcel.cpp
index b21ea82..6ea809a 100644 (file)
--- a/core/jni/android_os_HwParcel.cpp
+++ b/core/jni/android_os_HwParcel.cpp
@@ -169,7 +169,6 @@ JHwParcel::JHwParcel(JNIEnv *env, jobject thiz)
     jclass clazz = env->GetObjectClass(thiz);
     CHECK(clazz != NULL);
 
-    mClass = (jclass)env->NewGlobalRef(clazz);
     mObject = env->NewWeakGlobalRef(thiz);
 }
 
@@ -182,9 +181,6 @@ JHwParcel::~JHwParcel() {
 
     env->DeleteWeakGlobalRef(mObject);
     mObject = NULL;
-
-    env->DeleteGlobalRef(mClass);
-    mClass = NULL;
 }
 
 hardware::Parcel *JHwParcel::getParcel() {
@@ -542,7 +538,7 @@ static void JHwParcel_native_writeStrongBinder(
                 env, FindClassOrDie(env, PACKAGE_PATH "/HwRemoteBinder"));
 
         if (env->IsInstanceOf(binderObj, hwBinderKlass.get())) {
-            binder = JHwBinder::GetNativeContext(env, binderObj);
+            binder = JHwBinder::GetNativeBinder(env, binderObj);
         } else if (env->IsInstanceOf(binderObj, hwRemoteBinderKlass.get())) {
             binder = JHwRemoteBinder::GetNativeContext(
                     env, binderObj)->getBinder();

diff --git a/core/jni/android_os_HwParcel.h b/core/jni/android_os_HwParcel.h
index f81de9b..f6e6100 100644 (file)
--- a/core/jni/android_os_HwParcel.h
+++ b/core/jni/android_os_HwParcel.h
@@ -53,7 +53,6 @@ protected:
     virtual ~JHwParcel();
 
 private:
-    jclass mClass;
     jobject mObject;
 
     hardware::Parcel *mParcel;

diff --git a/core/jni/android_os_HwRemoteBinder.cpp b/core/jni/android_os_HwRemoteBinder.cpp
index f2f8e52..9c2ee9c 100644 (file)
--- a/core/jni/android_os_HwRemoteBinder.cpp
+++ b/core/jni/android_os_HwRemoteBinder.cpp
@@ -272,7 +272,6 @@ JHwRemoteBinder::JHwRemoteBinder(
     jclass clazz = env->GetObjectClass(thiz);
     CHECK(clazz != NULL);
 
-    mClass = (jclass)env->NewGlobalRef(clazz);
     mObject = env->NewWeakGlobalRef(thiz);
 }
 
@@ -281,9 +280,6 @@ JHwRemoteBinder::~JHwRemoteBinder() {
 
     env->DeleteWeakGlobalRef(mObject);
     mObject = NULL;
-
-    env->DeleteGlobalRef(mClass);
-    mClass = NULL;
 }
 
 sp<hardware::IBinder> JHwRemoteBinder::getBinder() const {

diff --git a/core/jni/android_os_HwRemoteBinder.h b/core/jni/android_os_HwRemoteBinder.h
index 77a0278..63aad0a 100644 (file)
--- a/core/jni/android_os_HwRemoteBinder.h
+++ b/core/jni/android_os_HwRemoteBinder.h
@@ -68,7 +68,6 @@ protected:
     virtual ~JHwRemoteBinder();
 
 private:
-    jclass mClass;
     jobject mObject;
 
     sp<hardware::IBinder> mBinder;