Improve JNI indirect ref diagnostics

Improved CheckJNI problem detection and failure reporting.  Also,
the indirect reference table data structure now responds to invalid
requests with a magic bogus object instead of NULL, so that failures
are immediate and obvious.

The extended checks in the indirect ref implementation are still on,
mostly so that bad code fails in an obvious way even when CheckJNI
is not enabled.

This change also includes a hack to allow weak globals to work.
Returning non-NULL for invalid refs turned a rarely-fails-weirdly
issue into an always-fails-loudly, which is good, but we need to
paper over it until the WGR fix is in place.  This will need to be
removed later.

Also, reduced some log levels.

Bug 4184721

Change-Id: Iaec75c71dbc85934366be2e717329b635d0fa49e
diff --git a/vm/CheckJni.c b/vm/CheckJni.c
index 5478eb1..17c76c8 100644
--- a/vm/CheckJni.c
+++ b/vm/CheckJni.c
@@ -35,7 +35,10 @@
 
 #include <zlib.h>
 
-static void abortMaybe(void);       // fwd
+#define kUnknownFuncName "      -"      /* can pass to showLocation() */
+
+static void showLocation(const char* func);
+static void abortMaybe(void);
 
 
 /*
@@ -60,11 +63,19 @@
  *
  * At this point, pResult->l has already been converted to an object pointer.
  */
-static void checkCallResultCommon(const u4* args, JValue* pResult,
+static void checkCallResultCommon(const u4* args, const JValue* pResult,
     const Method* method, Thread* self)
 {
     assert(pResult->l != NULL);
-    Object* resultObj = (Object*) pResult->l;
+    const Object* resultObj = (const Object*) pResult->l;
+
+    if (resultObj == kInvalidIndirectRefObject) {
+        LOGW("JNI WARNING: invalid reference returned from native code\n");
+        showLocation(kUnknownFuncName);
+        abortMaybe();
+        return;
+    }
+
     ClassObject* objClazz = resultObj->clazz;
 
     /*
@@ -117,8 +128,8 @@
 /*
  * Determine if we need to check the return type coming out of the call.
  *
- * (We don't do this at the top of checkCallResultCommon() because this is on
- * the critical path for native method calls.)
+ * (We don't simply do this at the top of checkCallResultCommon() because
+ * this is on the critical path for native method calls.)
  */
 static inline bool callNeedsCheck(const u4* args, JValue* pResult,
     const Method* method, Thread* self)
@@ -227,12 +238,14 @@
 
 #define CHECK_FIELD_TYPE(_env, _obj, _fieldid, _prim, _isstatic)            \
     checkFieldType(_env, _obj, _fieldid, _prim, _isstatic, __FUNCTION__)
+#define CHECK_STATIC_FIELD_ID(_env, _clazz, _fieldid)                       \
+    checkStaticFieldID(_env, _clazz, _fieldid, __FUNCTION__)
 #define CHECK_INST_FIELD_ID(_env, _obj, _fieldid)                           \
     checkInstanceFieldID(_env, _obj, _fieldid, __FUNCTION__)
 #define CHECK_CLASS(_env, _clazz)                                           \
-    checkClass(_env, _clazz, __FUNCTION__)
+    checkInstance(_env, _clazz, gDvm.classJavaLangClass, "jclass", __FUNCTION__)
 #define CHECK_STRING(_env, _str)                                            \
-    checkString(_env, _str, __FUNCTION__)
+    checkInstance(_env, _str, gDvm.classJavaLangString, "jstring", __FUNCTION__)
 #define CHECK_UTF_STRING(_env, _str)                                        \
     checkUtfString(_env, _str, #_str, __FUNCTION__)
 #define CHECK_NULLABLE_UTF_STRING(_env, _str)                               \
@@ -281,10 +294,11 @@
  *
  * "func" looks like "Check_DeleteLocalRef"; we drop the "Check_".
  */
-static void showLocation(const Method* meth, const char* func)
+static void showLocation(const char* func)
 {
+    const Method* meth = dvmGetCurrentJNIMethod();
     char* desc = dexProtoCopyMethodDescriptor(&meth->prototype);
-    LOGW("             in %s.%s %s (%s)",
+    LOGW("             in %s.%s:%s (%s)",
         meth->clazz->descriptor, meth->name, desc, func + 6);
     free(desc);
 }
@@ -393,7 +407,7 @@
     }
 
     if (printWarn)
-        showLocation(dvmGetCurrentJNIMethod(), func);
+        showLocation(func);
     if (printException) {
         LOGW("Pending exception is:");
         dvmLogExceptionStackTrace();
@@ -423,7 +437,7 @@
 
 /*
  * Verify that the field is of the appropriate type.  If the field has an
- * object type, "obj" is the object we're trying to assign into it.
+ * object type, "jobj" is the object we're trying to assign into it.
  *
  * Works for both static and instance fields.
  */
@@ -434,14 +448,26 @@
     bool printWarn = false;
 
     if (fieldID == NULL) {
-        LOGE("JNI ERROR: null field ID");
+        LOGW("JNI WARNING: null field ID");
+        showLocation(func);
         abortMaybe();
     }
 
-    if (field->signature[0] == 'L' || field->signature[0] == '[') {
+    if ((field->signature[0] == 'L' || field->signature[0] == '[') &&
+        jobj != NULL)
+    {
         JNI_ENTER();
         Object* obj = dvmDecodeIndirectRef(env, jobj);
-        if (obj != NULL) {
+        /*
+         * If jobj is a weak global ref whose referent has been cleared,
+         * obj will be NULL.  Otherwise, obj should always be non-NULL
+         * and valid.
+         */
+        if (obj != NULL && !dvmIsValidObject(obj)) {
+            LOGW("JNI WARNING: field operation on invalid %s ref (%p)\n",
+                dvmIndirectRefTypeName(jobj), jobj);
+            printWarn = true;
+        } else {
             ClassObject* fieldClass =
                 dvmFindLoadedClass(field->signature);
             ClassObject* objClass = obj->clazz;
@@ -450,14 +476,14 @@
             assert(objClass != NULL);
 
             if (!dvmInstanceof(objClass, fieldClass)) {
-                LOGW("JNI WARNING: field '%s' with type '%s' set with wrong type (%s)",
+                LOGW("JNI WARNING: set field '%s' expected type %s, got %s",
                     field->name, field->signature, objClass->descriptor);
                 printWarn = true;
             }
         }
         JNI_EXIT();
     } else if (dexGetPrimitiveTypeFromDescriptorChar(field->signature[0]) != prim) {
-        LOGW("JNI WARNING: field '%s' with type '%s' set with wrong type (%s)",
+        LOGW("JNI WARNING: set field '%s' expected type %s, got %s",
             field->name, field->signature, primitiveTypeToName(prim));
         printWarn = true;
     } else if (isStatic && !dvmIsStaticField(field)) {
@@ -471,40 +497,7 @@
     }
 
     if (printWarn) {
-        showLocation(dvmGetCurrentJNIMethod(), func);
-        abortMaybe();
-    }
-}
-
-/*
- * Verify that "jobj" is a valid object, and that it's an object that JNI
- * is allowed to know about.  We allow NULL references.
- *
- * Must be in "running" mode before calling here.
- */
-static void checkObject0(JNIEnv* env, jobject jobj, const char* func)
-{
-    UNUSED_PARAMETER(env);
-    bool printWarn = false;
-
-    if (jobj == NULL)
-        return;
-
-    if (dvmGetJNIRefType(env, jobj) == JNIInvalidRefType) {
-        LOGW("JNI WARNING: %p is not a valid JNI reference", 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)",
-                jobj, obj, func);
-            printWarn = true;
-        }
-    }
-
-    if (printWarn) {
-        showLocation(dvmGetCurrentJNIMethod(), func);
+        showLocation(func);
         abortMaybe();
     }
 }
@@ -517,85 +510,77 @@
  */
 static void checkObject(JNIEnv* env, jobject jobj, const char* func)
 {
+    bool printWarn = false;
+
+    if (jobj == NULL)
+        return;
+
     JNI_ENTER();
-    checkObject0(env, jobj, func);
+
+    if (dvmGetJNIRefType(env, jobj) == JNIInvalidRefType) {
+        LOGW("JNI WARNING: %p is not a valid JNI reference (type=%s)",
+            jobj, dvmIndirectRefTypeName(jobj));
+        printWarn = true;
+    } else {
+        Object* obj = dvmDecodeIndirectRef(env, jobj);
+
+        /*
+         * The decoded object will be NULL if this is a weak global ref
+         * with a cleared referent.
+         */
+        if (obj == kInvalidIndirectRefObject ||
+            (obj != NULL && !dvmIsValidObject(obj)))
+        {
+            LOGW("JNI WARNING: native code passing in bad object %p %p",
+                jobj, obj);
+            printWarn = true;
+        }
+    }
+
+    if (printWarn) {
+        showLocation(func);
+        abortMaybe();
+    }
     JNI_EXIT();
 }
 
 /*
- * Verify that "clazz" actually points to a class object.  (Also performs
- * checkObject.)
- *
- * We probably don't need to identify where we're being called from,
- * because the VM is most likely about to crash and leave a core dump
- * if something is wrong.
+ * Verify that "jobj" is a valid non-NULL object reference, and points to
+ * an instance of expectedClass.
  *
  * Because we're looking at an object on the GC heap, we have to switch
  * to "running" mode before doing the checks.
  */
-static void checkClass(JNIEnv* env, jclass jclazz, const char* func)
+static void checkInstance(JNIEnv* env, jobject jobj,
+    ClassObject* expectedClass, const char* argName, const char* func)
 {
+    if (jobj == NULL) {
+        LOGW("JNI WARNING: received null %s", argName);
+        showLocation(func);
+        abortMaybe();
+        return;
+    }
+
     JNI_ENTER();
     bool printWarn = false;
 
-    Object* obj = dvmDecodeIndirectRef(env, jclazz);
+    Object* obj = dvmDecodeIndirectRef(env, jobj);
 
-    if (obj == NULL) {
-        LOGW("JNI WARNING: received null jclass");
+    if (!dvmIsValidObject(obj)) {
+        LOGW("JNI WARNING: %s is invalid %s ref (%p)", argName,
+            dvmIndirectRefTypeName(jobj), jobj);
         printWarn = true;
-    } else if (!dvmIsValidObject(obj)) {
-        LOGW("JNI WARNING: jclass points to invalid object %p", obj);
-        printWarn = true;
-    } else if (obj->clazz != gDvm.classJavaLangClass) {
-        LOGW("JNI WARNING: jclass arg is not a Class reference "
-             "(%p is instance of %s)",
-            jclazz, obj->clazz->descriptor);
+    } else if (obj->clazz != expectedClass) {
+        LOGW("JNI WARNING: %s arg has wrong type (expected %s, got %s)",
+            argName, expectedClass->descriptor, obj->clazz->descriptor);
         printWarn = true;
     }
     JNI_EXIT();
 
-    if (printWarn)
+    if (printWarn) {
+        showLocation(func);
         abortMaybe();
-    else
-        checkObject(env, jclazz, func);
-}
-
-/*
- * Verify that "str" is non-NULL and points to a String object.
- *
- * Since we're dealing with objects, switch to "running" mode.
- */
-static void checkString(JNIEnv* env, jstring jstr, const char* func)
-{
-    JNI_ENTER();
-    bool printWarn = false;
-
-    Object* obj = dvmDecodeIndirectRef(env, jstr);
-
-    if (obj == NULL) {
-        LOGW("JNI WARNING: received null jstring (%s)", func);
-        printWarn = true;
-    } else if (obj->clazz != gDvm.classJavaLangString) {
-        /*
-         * TODO: we probably should test dvmIsValidObject first, because
-         * this will crash if "obj" is non-null but pointing to an invalid
-         * memory region.  However, the "is valid" test is a little slow,
-         * we're doing it again over in checkObject().
-         */
-        if (dvmIsValidObject(obj))
-            LOGW("JNI WARNING: jstring %p points to object of type %s (%s)",
-                jstr, obj->clazz->descriptor, func);
-        else
-            LOGW("JNI WARNING: jstring %p is not a valid object (%s)", jstr,
-                func);
-        printWarn = true;
     }
-    JNI_EXIT();
-
-    if (printWarn)
-        abortMaybe();
-    else
-        checkObject(env, jstr, func);
 }
 
 /*
@@ -676,7 +661,7 @@
         errorKind, utf8);
     LOGW("             string: '%s'", origBytes);
 fail:
-    showLocation(dvmGetCurrentJNIMethod(), func);
+    showLocation(func);
     abortMaybe();
 }
 
@@ -710,29 +695,34 @@
  */
 static void checkArray(JNIEnv* env, jarray jarr, const char* func)
 {
+    if (jarr == NULL) {
+        LOGW("JNI WARNING: received null array");
+        showLocation(func);
+        abortMaybe();
+        return;
+    }
+
     JNI_ENTER();
     bool printWarn = false;
 
     Object* obj = dvmDecodeIndirectRef(env, jarr);
 
-    if (obj == NULL) {
-        LOGW("JNI WARNING: received null array (%s)", func);
+    if (!dvmIsValidObject(obj)) {
+        LOGW("JNI WARNING: jarray is invalid %s ref (%p)",
+            dvmIndirectRefTypeName(jarr), jarr);
         printWarn = true;
     } else if (obj->clazz->descriptor[0] != '[') {
-        if (dvmIsValidObject(obj))
-            LOGW("JNI WARNING: jarray %p points to non-array object (%s)",
-                jarr, obj->clazz->descriptor);
-        else
-            LOGW("JNI WARNING: jarray %p is not a valid object", jarr);
+        LOGW("JNI WARNING: jarray arg has wrong type (expected array, got %s)",
+            obj->clazz->descriptor);
         printWarn = true;
     }
 
     JNI_EXIT();
 
-    if (printWarn)
+    if (printWarn) {
+        showLocation(func);
         abortMaybe();
-    else
-        checkObject(env, jarr, func);
+    }
 }
 
 /*
@@ -796,15 +786,18 @@
         LOGW("             calling %s.%s %s",
             meth->clazz->descriptor, meth->name, desc);
         free(desc);
-        showLocation(dvmGetCurrentJNIMethod(), func);
+        showLocation(func);
         abortMaybe();
     }
 }
 
 /*
  * Verify that this static field ID is valid for this class.
+ *
+ * Assumes "jclazz" has already been validated.
  */
-static void checkStaticFieldID(JNIEnv* env, jclass jclazz, jfieldID fieldID)
+static void checkStaticFieldID(JNIEnv* env, jclass jclazz, jfieldID fieldID,
+    const char* func)
 {
     JNI_ENTER();
     ClassObject* clazz = (ClassObject*) dvmDecodeIndirectRef(env, jclazz);
@@ -817,6 +810,7 @@
         LOGW("JNI WARNING: static fieldID %p not valid for class %s",
             fieldID, clazz->descriptor);
         LOGW("             base=%p count=%d", base, fieldCount);
+        showLocation(func);
         abortMaybe();
     }
     JNI_EXIT();
@@ -824,18 +818,14 @@
 
 /*
  * Verify that this instance field ID is valid for this object.
+ *
+ * Assumes "jobj" has already been validated.
  */
 static void checkInstanceFieldID(JNIEnv* env, jobject jobj, jfieldID fieldID,
     const char* func)
 {
     JNI_ENTER();
 
-    if (jobj == NULL) {
-        LOGW("JNI WARNING: invalid null object (%s)", func);
-        abortMaybe();
-        goto bail;
-    }
-
     Object* obj = dvmDecodeIndirectRef(env, jobj);
     ClassObject* clazz = obj->clazz;
 
@@ -855,6 +845,7 @@
 
     LOGW("JNI WARNING: inst fieldID %p not valid for class %s",
         fieldID, obj->clazz->descriptor);
+    showLocation(func);
     abortMaybe();
 
 bail:
@@ -879,6 +870,7 @@
     if (!dvmInstanceof(obj->clazz, meth->clazz)) {
         LOGW("JNI WARNING: can't call %s.%s on instance of %s",
             meth->clazz->descriptor, meth->name, obj->clazz->descriptor);
+        showLocation(func);
         abortMaybe();
     }
 
@@ -888,7 +880,7 @@
 /*
  * Verify that "methodID" is appropriate for "clazz".
  *
- * A mismatch isn't dangerous, because the method defines the class.  In
+ * A mismatch isn't dangerous, because the jmethodID defines the class.  In
  * fact, jclazz is unused in the implementation.  It's best if we don't
  * allow bad code in the system though.
  *
@@ -905,7 +897,8 @@
     if (!dvmInstanceof(clazz, meth->clazz)) {
         LOGW("JNI WARNING: can't call static %s.%s on class %s",
             meth->clazz->descriptor, meth->name, clazz->descriptor);
-        // no abort
+        showLocation(func);
+        // no abort?
     }
 
     JNI_EXIT();
@@ -1307,6 +1300,7 @@
 {
     CHECK_ENTER(env, kFlag_Default);
     CHECK_OBJECT(env, obj);
+    /* TODO: verify that "obj" is an instance of Throwable */
     jint result;
     result = BASE_ENV(env)->Throw(env, obj);
     CHECK_EXIT(env);
@@ -1576,7 +1570,7 @@
         CHECK_ENTER(env, kFlag_Default);                                    \
         CHECK_CLASS(env, clazz);                                            \
         _ctype result;                                                      \
-        checkStaticFieldID(env, clazz, fieldID);                            \
+        CHECK_STATIC_FIELD_ID(env, clazz, fieldID);                         \
         result = BASE_ENV(env)->GetStatic##_jname##Field(env, clazz,        \
             fieldID);                                                       \
         CHECK_EXIT(env);                                                    \
@@ -1598,7 +1592,7 @@
     {                                                                       \
         CHECK_ENTER(env, kFlag_Default);                                    \
         CHECK_CLASS(env, clazz);                                            \
-        checkStaticFieldID(env, clazz, fieldID);                            \
+        CHECK_STATIC_FIELD_ID(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,        \
diff --git a/vm/IndirectRefTable.c b/vm/IndirectRefTable.c
index 81cfeed..819b15d 100644
--- a/vm/IndirectRefTable.c
+++ b/vm/IndirectRefTable.c
@@ -173,7 +173,7 @@
                 pRef->allocEntries, newSize, pRef->maxEntries);
             return false;
         }
-        LOGI("Growing ireftab %p from %d to %d (max=%d)\n",
+        LOGV("Growing ireftab %p from %d to %d (max=%d)\n",
             pRef, pRef->allocEntries, newSize, pRef->maxEntries);
 
         /* update entries; adjust "nextEntry" in case memory moved */
@@ -230,19 +230,19 @@
     int idx = dvmIndirectRefToIndex(iref);
 
     if (iref == NULL) {
-        LOGI("--- lookup on NULL iref\n");
+        LOGD("Attempt to look up NULL iref\n");
         return false;
     }
     if (idx >= topIndex) {
         /* bad -- stale reference? */
-        LOGI("Attempt to access invalid index %d (top=%d)\n",
+        LOGD("Attempt to access invalid index %d (top=%d)\n",
             idx, topIndex);
         return false;
     }
 
     Object* obj = pRef->table[idx];
     if (obj == NULL) {
-        LOGI("Attempt to read from hole, iref=%p\n", iref);
+        LOGD("Attempt to read from hole, iref=%p\n", iref);
         return false;
     }
     if (!checkEntry(pRef, iref, idx))
@@ -285,7 +285,7 @@
     }
     if (idx >= topIndex) {
         /* bad -- stale reference? */
-        LOGI("Attempt to remove invalid index %d (bottom=%d top=%d)\n",
+        LOGD("Attempt to remove invalid index %d (bottom=%d top=%d)\n",
             idx, bottomIndex, topIndex);
         return false;
     }
@@ -345,6 +345,20 @@
 }
 
 /*
+ * Return a type name, useful for debugging.
+ */
+const char* dvmIndirectRefTypeName(IndirectRef iref)
+{
+    switch (dvmGetIndirectRefType(iref)) {
+    case kIndirectKindInvalid:      return "invalid";
+    case kIndirectKindLocal:        return "local";
+    case kIndirectKindGlobal:       return "global";
+    case kIndirectKindWeakGlobal:   return "weak global";
+    default:                        return "UNKNOWN";
+    }
+}
+
+/*
  * Dump the contents of a IndirectRefTable to the log.
  */
 void dvmDumpIndirectRefTable(const IndirectRefTable* pRef, const char* descr)
diff --git a/vm/IndirectRefTable.h b/vm/IndirectRefTable.h
index c33a509..7248a7b 100644
--- a/vm/IndirectRefTable.h
+++ b/vm/IndirectRefTable.h
@@ -240,6 +240,11 @@
 }
 
 /*
+ * Return a string constant describing the indirect ref type.
+ */
+const char* dvmIndirectRefTypeName(IndirectRef iref);
+
+/*
  * Initialize an IndirectRefTable.
  *
  * If "initialCount" != "maxCount", the table will expand as required.
@@ -347,16 +352,19 @@
 /* extra debugging checks */
 bool dvmGetFromIndirectRefTableCheck(IndirectRefTable* pRef, IndirectRef iref);
 
+/* magic failure value; must not pass dvmIsValidObject() */
+#define kInvalidIndirectRefObject ((Object*)0xdead4321)
+
 /*
  * Given an IndirectRef in the table, return the Object it refers to.
  *
- * Returns NULL if iref is invalid.
+ * Returns kInvalidIndirectRefObject if iref is invalid.
  */
 INLINE Object* dvmGetFromIndirectRefTable(IndirectRefTable* pRef,
     IndirectRef iref)
 {
     if (!dvmGetFromIndirectRefTableCheck(pRef, iref))
-        return NULL;
+        return kInvalidIndirectRefObject;
 
     int idx = dvmIndirectRefToIndex(iref);
     return pRef->table[idx];
diff --git a/vm/Jni.c b/vm/Jni.c
index dab94bb..6f96132 100644
--- a/vm/Jni.c
+++ b/vm/Jni.c
@@ -448,7 +448,9 @@
  * Convert an indirect reference to an Object reference.  The indirect
  * reference may be local, global, or weak-global.
  *
- * If "jobj" is NULL or an invalid indirect reference, this returns NULL.
+ * If "jobj" is NULL, or is a weak global reference whose reference has
+ * been cleared, this returns NULL.  If jobj is an invalid indirect
+ * reference, kInvalidIndirectRefObject is returned.
  */
 Object* dvmDecodeIndirectRef(JNIEnv* env, jobject jobj)
 {
@@ -480,13 +482,25 @@
             dvmLockMutex(&gDvm.jniWeakGlobalRefLock);
             result = dvmGetFromIndirectRefTable(pRefTable, jobj);
             dvmUnlockMutex(&gDvm.jniWeakGlobalRefLock);
+            /*
+             * TODO: this is a temporary workaround for broken weak global
+             * refs (bug 4260055).  We treat any invalid reference as if it
+             * were a weak global with a cleared referent.  This means that
+             * actual invalid references won't be detected, and if an empty
+             * slot gets re-used we will return the new reference instead.
+             * This must be removed when weak global refs get fixed.
+             */
+            if (result == kInvalidIndirectRefObject) {
+                LOGW("Warning: used weak global ref hack\n");
+                result = NULL;
+            }
         }
         break;
     case kIndirectKindInvalid:
     default:
         LOGW("Invalid indirect reference %p in decodeIndirectRef\n", jobj);
         dvmAbort();
-        result = NULL;
+        result = kInvalidIndirectRefObject;
         break;
     }
 
@@ -815,10 +829,6 @@
  *  - is present in the JNI global refs table
  *
  * Used by -Xcheck:jni and GetObjectRefType.
- *
- * NOTE: in the current VM, global and local references are identical.  If
- * something is both global and local, we can't tell them apart, and always
- * return "local".
  */
 jobjectRefType dvmGetJNIRefType(JNIEnv* env, jobject jobj)
 {
@@ -827,10 +837,11 @@
      * jobjectRefType, so this is easy.  We have to decode it to determine
      * if it's a valid reference and not merely valid-looking.
      */
+    assert(jobj != NULL);
+
     Object* obj = dvmDecodeIndirectRef(env, jobj);
 
-    if (obj == NULL) {
-        /* invalid ref, or jobj was NULL */
+    if (obj == kInvalidIndirectRefObject) {
         return JNIInvalidRefType;
     } else {
         return (jobjectRefType) dvmGetIndirectRefType(jobj);
@@ -1205,6 +1216,9 @@
 /*
  * If necessary, convert the value in pResult from a local/global reference
  * to an object pointer.
+ *
+ * If the returned reference is invalid, kInvalidIndirectRefObject will
+ * be returned in pResult.
  */
 static inline void convertReferenceResult(JNIEnv* env, JValue* pResult,
     const Method* method, Thread* self)
@@ -1505,11 +1519,10 @@
 static jclass GetSuperclass(JNIEnv* env, jclass jclazz)
 {
     JNI_ENTER();
-    jclass jsuper = NULL;
 
     ClassObject* clazz = (ClassObject*) dvmDecodeIndirectRef(env, jclazz);
-    if (clazz != NULL)
-        jsuper = addLocalReference(env, (Object*)clazz->super);
+    jclass jsuper = addLocalReference(env, (Object*)clazz->super);
+
     JNI_EXIT();
     return jsuper;
 }
@@ -2688,14 +2701,14 @@
     JNI_ENTER();
 
     jobjectArray newArray = NULL;
-    ClassObject* elemClassObj =
-        (ClassObject*) dvmDecodeIndirectRef(env, jelementClass);
 
-    if (elemClassObj == NULL) {
-        dvmThrowNullPointerException("JNI NewObjectArray");
+    if (jelementClass == NULL) {
+        dvmThrowNullPointerException("JNI NewObjectArray element class");
         goto bail;
     }
 
+    ClassObject* elemClassObj =
+        (ClassObject*) dvmDecodeIndirectRef(env, jelementClass);
     ArrayObject* newObj =
         dvmAllocObjectArray(elemClassObj, length, ALLOC_DEFAULT);
     if (newObj == NULL) {
diff --git a/vm/test/TestIndirectRefTable.c b/vm/test/TestIndirectRefTable.c
index 25f1dd1..155ba32 100644
--- a/vm/test/TestIndirectRefTable.c
+++ b/vm/test/TestIndirectRefTable.c
@@ -95,7 +95,7 @@
     }
 
     /* get invalid entry (off the end of the list) */
-    if (dvmGetFromIndirectRefTable(&irt, iref0) != NULL) {
+    if (dvmGetFromIndirectRefTable(&irt, iref0) != kInvalidIndirectRefObject) {
         LOGE("stale entry get succeeded unexpectedly\n");
         goto bail;
     }
@@ -153,7 +153,7 @@
     }
 
     /* get invalid entry (from hole) */
-    if (dvmGetFromIndirectRefTable(&irt, iref1) != NULL) {
+    if (dvmGetFromIndirectRefTable(&irt, iref1) != kInvalidIndirectRefObject) {
         LOGE("hole get succeeded unexpectedly\n");
         goto bail;
     }