Fix some JNI indirect reference stuff.

Convert where needed, don't convert where not needed, and make sure
we're using the right one.

With this, my ad-hoc tests pass, and the boot proceeds until we hit a
failure that looks like it might be due to logic outside the VM.

4 files changed, 86 insertions, 37 deletions

diff --git a/vm/CheckJni.c b/vm/CheckJni.c
index a8bb1138a..9ed741922 100644
--- a/vm/CheckJni.c
+++ b/vm/CheckJni.c
@@ -57,12 +57,14 @@ static void abortMaybe(void);       // fwd
  * match.  This will allow some false-positives when a class is redefined
  * by a class loader, but that's rare enough that it doesn't seem worth
  * testing for.
+ *
+ * At this point, pResult->l has already been converted to an object pointer.
  */
 static void checkCallCommon(const u4* args, JValue* pResult,
     const Method* method, Thread* self)
 {
     assert(pResult->l != NULL);
-    Object* resultObj = dvmDecodeIndirectRef(self->jniEnv, pResult->l);
+    Object* resultObj = (Object*) pResult->l;
     ClassObject* objClazz = resultObj->clazz;
 
     /*
@@ -181,7 +183,7 @@ void dvmCheckCallJNIMethod_staticNoRef(const u4* args, JValue* pResult,
 #define BASE_ENV(_env)  (((JNIEnvExt*)_env)->baseFuncTable)
 #define BASE_VM(_vm)    (((JavaVMExt*)_vm)->baseFuncTable)
 
-#define kRedundantDirectBufferTest true
+#define kRedundantDirectBufferTest false
 
 /*
  * Flags passed into checkThread().
@@ -413,14 +415,13 @@ static void checkFieldType(JNIEnv* env, jobject jobj, jfieldID fieldID,
     Field* field = (Field*) fieldID;
     bool printWarn = false;
 
-    Object* obj = dvmDecodeIndirectRef(env, jobj);
-
     if (fieldID == NULL) {
         LOGE("JNI ERROR: null field ID\n");
         abortMaybe();
     }
 
     if (field->signature[0] == 'L' || field->signature[0] == '[') {
+        Object* obj = dvmDecodeIndirectRef(env, jobj);
         if (obj != NULL) {
             ClassObject* fieldClass =
                 dvmFindLoadedClass(field->signature);
@@ -469,16 +470,19 @@ static void checkObject(JNIEnv* env, jobject jobj, const char* func)
     if (jobj == NULL)
         return;
 
-    Object* obj = dvmDecodeIndirectRef(env, jobj);
-
     JNI_ENTER();
-    if (obj == NULL || !dvmIsValidObject(obj)) {
-        LOGW("JNI WARNING: native code passing in bad object %p %p (%s)\n",
-            jobj, obj, func);
-        printWarn = true;
-    } else if (dvmGetJNIRefType(env, obj) == JNIInvalidRefType) {
+
+    if (dvmGetJNIRefType(env, jobj) == JNIInvalidRefType) {
         LOGW("JNI WARNING: %p is not a valid JNI reference\n", jobj);
         printWarn = true;
+    } else {
+        Object* obj = dvmDecodeIndirectRef(env, jobj);
+
+        if (obj == NULL || !dvmIsValidObject(obj)) {
+            LOGW("JNI WARNING: native code passing in bad object %p %p (%s)\n",
+                jobj, obj, func);
+            printWarn = true;
+        }
     }
 
     if (printWarn) {
@@ -1296,11 +1300,22 @@ static jobject Check_NewGlobalRef(JNIEnv* env, jobject obj)
     return result;
 }
 
-static void Check_DeleteGlobalRef(JNIEnv* env, jobject localRef)
+static void Check_DeleteGlobalRef(JNIEnv* env, jobject globalRef)
 {
     CHECK_ENTER(env, kFlag_Default | kFlag_ExcepOkay);
-    CHECK_OBJECT(env, localRef);
-    BASE_ENV(env)->DeleteGlobalRef(env, localRef);
+    CHECK_OBJECT(env, globalRef);
+#ifdef USE_INDIRECT_REF
+    if (globalRef != NULL &&
+        dvmGetJNIRefType(env, globalRef) != JNIGlobalRefType)
+    {
+        LOGW("JNI WARNING: DeleteGlobalRef on non-global %p (type=%d)\n",
+            globalRef, dvmGetJNIRefType(env, globalRef));
+        abortMaybe();
+    } else
+#endif
+    {
+        BASE_ENV(env)->DeleteGlobalRef(env, globalRef);
+    }
     CHECK_EXIT(env);
 }
 
@@ -1314,11 +1329,22 @@ static jobject Check_NewLocalRef(JNIEnv* env, jobject ref)
     return result;
 }
 
-static void Check_DeleteLocalRef(JNIEnv* env, jobject globalRef)
+static void Check_DeleteLocalRef(JNIEnv* env, jobject localRef)
 {
     CHECK_ENTER(env, kFlag_Default | kFlag_ExcepOkay);
-    CHECK_OBJECT(env, globalRef);
-    BASE_ENV(env)->DeleteLocalRef(env, globalRef);
+    CHECK_OBJECT(env, localRef);
+#ifdef USE_INDIRECT_REF
+    if (localRef != NULL &&
+        dvmGetJNIRefType(env, localRef) != JNILocalRefType)
+    {
+        LOGW("JNI WARNING: DeleteLocalRef on non-local %p (type=%d)\n",
+            localRef, dvmGetJNIRefType(env, localRef));
+        abortMaybe();
+    } else
+#endif
+    {
+        BASE_ENV(env)->DeleteLocalRef(env, localRef);
+    }
     CHECK_EXIT(env);
 }
 
@@ -1491,6 +1517,7 @@ GET_STATIC_TYPE_FIELD(jdouble, Double);
         CHECK_ENTER(env, kFlag_Default);                                    \
         CHECK_CLASS(env, clazz);                                            \
         checkStaticFieldID(env, clazz, fieldID);                            \
+        /* "value" arg only used when type == ref */                        \
         CHECK_FIELD_TYPE(env, (jobject)(u4)value, fieldID, _ftype, true);   \
         BASE_ENV(env)->SetStatic##_jname##Field(env, clazz, fieldID,        \
             value);                                                         \
@@ -1535,6 +1562,7 @@ GET_TYPE_FIELD(jdouble, Double);
         CHECK_ENTER(env, kFlag_Default);                                    \
         CHECK_OBJECT(env, obj);                                             \
         CHECK_INST_FIELD_ID(env, obj, fieldID);                             \
+        /* "value" arg only used when type == ref */                        \
         CHECK_FIELD_TYPE(env, (jobject)(u4) value, fieldID, _ftype, false); \
         BASE_ENV(env)->Set##_jname##Field(env, obj, fieldID, value);        \
         CHECK_EXIT(env);                                                    \
@@ -2179,6 +2207,7 @@ static void* Check_GetDirectBufferAddress(JNIEnv* env, jobject buf)
             goto bail;
         }
 
+        // TODO: this should not be using internal structures
         Method* toLong = ((Object*)platformAddr)->clazz->vtable[
                 gDvm.voffOrgApacheHarmonyLuniPlatformPlatformAddress_toLong];
         checkResult = (void*)(u4)(*env)->CallLongMethod(env, platformAddr,
diff --git a/vm/IndirectRefTable.c b/vm/IndirectRefTable.c
index 6e3ee2594..f7c764749 100644
--- a/vm/IndirectRefTable.c
+++ b/vm/IndirectRefTable.c
@@ -119,7 +119,7 @@ IndirectRef dvmAddToIndirectRefTable(IndirectRefTable* pRef, u4 cookie,
     if (topIndex == pRef->allocEntries) {
         /* reached end of allocated space; did we hit buffer max? */
         if (topIndex == pRef->maxEntries) {
-            LOGW("ReferenceTable overflow (max=%d)\n", pRef->maxEntries);
+            LOGW("IndirectRefTable overflow (max=%d)\n", pRef->maxEntries);
             return NULL;
         }
 
@@ -133,11 +133,12 @@ IndirectRef dvmAddToIndirectRefTable(IndirectRefTable* pRef, u4 cookie,
 
         newTable = (Object**) realloc(pRef->table, newSize * sizeof(Object*));
         if (newTable == NULL) {
-            LOGE("Unable to expand iref table (from %d to %d entries)\n",
-                pRef->allocEntries, newSize);
+            LOGE("Unable to expand iref table (from %d to %d, max=%d)\n",
+                pRef->allocEntries, newSize, pRef->maxEntries);
             return false;
         }
-        LOGI("Growing %p from %d to %d\n", pRef, pRef->allocEntries, newSize);
+        LOGI("Growing ireftab %p from %d to %d (max=%d)\n",
+            pRef, pRef->allocEntries, newSize, pRef->maxEntries);
 
         /* update entries; adjust "nextEntry" in case memory moved */
         pRef->table = newTable;
diff --git a/vm/Jni.c b/vm/Jni.c
index f93a3329f..a720648b4 100644
--- a/vm/Jni.c
+++ b/vm/Jni.c
@@ -1107,9 +1107,17 @@ jobjectRefType dvmGetJNIRefType(JNIEnv* env, jobject jobj)
 #ifdef USE_INDIRECT_REF
     /*
      * IndirectRefKind is currently defined as an exact match of
-     * jobjectRefType, so this is easy.
+     * jobjectRefType, so this is easy.  We have to decode it to determine
+     * if it's a valid reference and not merely valid-looking.
      */
-    return (jobjectRefType) dvmIndirectRefToIndex(jobj);
+    Object* obj = dvmDecodeIndirectRef(env, jobj);
+
+    if (obj == NULL) {
+        /* invalid ref, or jobj was NULL */
+        return JNIInvalidRefType;
+    } else {
+        return (jobjectRefType) dvmGetIndirectRefType(jobj);
+    }
 #else
     ReferenceTable* pRefTable = getLocalRefTable(env);
     Thread* self = dvmThreadSelf();
@@ -1447,6 +1455,20 @@ static void checkStackSum(Thread* self)
  */
 
 /*
+ * If necessary, convert the value in pResult from a local/global reference
+ * to an object pointer.
+ */
+static inline void convertReferenceResult(JNIEnv* env, JValue* pResult,
+    const Method* method, Thread* self)
+{
+    if (method->shorty[0] == 'L' && !dvmCheckException(self) &&
+            pResult->l != NULL)
+    {
+        pResult->l = dvmDecodeIndirectRef(env, pResult->l);
+    }
+}
+
+/*
  * General form, handles all cases.
  */
 void dvmCallJNIMethod_general(const u4* args, JValue* pResult,
@@ -1523,6 +1545,8 @@ void dvmCallJNIMethod_general(const u4* args, JValue* pResult,
     CHECK_STACK_SUM(self);
 
     dvmChangeStatus(self, oldStatus);
+
+    convertReferenceResult(env, pResult, method, self);
 }
 
 /*
@@ -1578,6 +1602,8 @@ void dvmCallJNIMethod_virtualNoRef(const u4* args, JValue* pResult,
     CHECK_STACK_SUM(self);
 
     dvmChangeStatus(self, oldStatus);
+
+    convertReferenceResult(self->jniEnv, pResult, method, self);
 }
 
 /*
@@ -1606,6 +1632,8 @@ void dvmCallJNIMethod_staticNoRef(const u4* args, JValue* pResult,
     CHECK_STACK_SUM(self);
 
     dvmChangeStatus(self, oldStatus);
+
+    convertReferenceResult(self->jniEnv, pResult, method, self);
 }
 
 /*
@@ -2666,7 +2694,7 @@ static jstring NewString(JNIEnv* env, const jchar* unicodeChars, jsize len)
     }
 
     JNI_EXIT();
-    return jstr;
+    return retval;
 }
 
 /*
@@ -2917,11 +2945,7 @@ static void SetObjectArrayElement(JNIEnv* env, jobjectArray jarr,
 
     //LOGV("JNI: set element %d in array %p to %p\n", index, array, value);
 
-    Object* obj;
-    if (jobj == NULL)
-        obj = NULL;
-    else
-        obj = dvmDecodeIndirectRef(env, jobj);
+    Object* obj = dvmDecodeIndirectRef(env, jobj);
     ((Object**) arrayObj->contents)[index] = obj;
 
 bail:
@@ -3257,7 +3281,7 @@ static void ReleaseStringCritical(JNIEnv* env, jstring jstr,
 {
     JNI_ENTER();
     StringObject* strObj = (StringObject*) dvmDecodeIndirectRef(env, jstr);
-    ArrayObject* strChars = dvmStringCharArray(jstr);
+    ArrayObject* strChars = dvmStringCharArray(strObj);
     unpinPrimitiveArray(strChars);
     JNI_EXIT();
 }
@@ -3312,12 +3336,7 @@ static jboolean ExceptionCheck(JNIEnv* env)
 static jobjectRefType GetObjectRefType(JNIEnv* env, jobject jobj)
 {
     JNI_ENTER();
-    jobjectRefType type;
-
-    if (jobj == NULL)
-        type = JNIInvalidRefType;
-    else
-        type = dvmGetJNIRefType(env, jobj);
+    jobjectRefType type = dvmGetJNIRefType(env, jobj);
     JNI_EXIT();
     return type;
 }
diff --git a/vm/alloc/Alloc.h b/vm/alloc/Alloc.h
index f1156d3bc..facc753d1 100644
--- a/vm/alloc/Alloc.h
+++ b/vm/alloc/Alloc.h
@@ -133,7 +133,7 @@ INLINE int dvmValidateObject(Object* obj)
     }
 #ifdef WITH_EXTRA_OBJECT_VALIDATION
     if (!dvmIsValidObject(obj)) {
-        //dvmAbort();
+        dvmAbort();
         dvmThrowException("Ljava/lang/InternalError;",
             "VM detected invalid object ptr");
         return false;