Explicitly release JNI local references in the Java Bridge

content/browser/renderer_host/java/java_bound_object.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/java/java_bound_object.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/memory/singleton.h"
 #include "base/string_number_conversions.h"
@@ skipping 379 matching lines @@
     case JavaType::TypeVoid:
       // Conversion to void must never happen.
     case JavaType::TypeArray:
     case JavaType::TypeObject:
       // Not handled.
       NOTREACHED();
   }
   return NULL;
 }
 
-// Note that this only handles primitive types and strings.
+// Sets the specified element of the supplied array to the value of the
+// supplied jvalue. Requires that the type of the array matches that of the
+// jvalue. Handles only primitive types and strings. Note that in the case of a
+// string, the array takes a new reference to the string object.
 void SetArrayElement(jobject array,
                      const JavaType& type,
                      jsize index,
                      const jvalue& value) {
   JNIEnv* env = AttachCurrentThread();
   switch (type.type) {
     case JavaType::TypeBoolean:
       env->SetBooleanArrayRegion(static_cast<jbooleanArray>(array), index, 1,
                                  &value.z);
       break;
@@ skipping 36 matching lines @@
       // Not handled.
       NOTREACHED();
   }
   base::android::CheckException(env);
 }
 
 jvalue CoerceJavaScriptValueToJavaValue(const NPVariant& variant,
                                         const JavaType& target_type,
                                         bool coerce_to_string);
 
+// Returns a new local reference to a Java array.
 jobject CoerceJavaScriptObjectToArray(const NPVariant& variant,
                                       const JavaType& target_type) {
   DCHECK_EQ(JavaType::TypeArray, target_type.type);
   NPObject* object = NPVARIANT_TO_OBJECT(variant);
   DCHECK_NE(&JavaNPObject::kNPClass, object->_class);
 
   const JavaType& target_inner_type = *target_type.inner_type.get();
   // LIVECONNECT_COMPLIANCE: Existing behavior is to return null for
   // multi-dimensional arrays. Spec requires handling multi-demensional arrays.
   if (target_inner_type.type == JavaType::TypeArray) {
@@ skipping 24 matching lines @@
   } else if (NPVARIANT_IS_DOUBLE(length_variant)
              && NPVARIANT_TO_DOUBLE(length_variant) >= 0.0
              && NPVARIANT_TO_DOUBLE(length_variant) <= kint32max) {
     length = static_cast<jsize>(NPVARIANT_TO_DOUBLE(length_variant));
   }
   WebBindings::releaseVariantValue(&length_variant);
   if (length == -1) {
     return NULL;
   }
 
-  // Create the Java array. Note that we don't explicitly release the local
-  // ref to the result or any of its elements.
+  // Create the Java array.
   // TODO(steveblock): Handle failure to create the array.
   jobject result = CreateJavaArray(target_inner_type, length);
   NPVariant value_variant;
   for (jsize i = 0; i < length; ++i) {
     // It seems that getProperty() will set the variant to type void on failure,
     // but this doesn't seem to be documented, so do it explicitly here for
     // safety.
     VOID_TO_NPVARIANT(value_variant);
     // If this fails, for example due to a missing element, we simply treat the
     // value as JavaScript undefined.
     WebBindings::getProperty(0, object, WebBindings::getIntIdentifier(i),
                              &value_variant);
-    SetArrayElement(result, target_inner_type, i,
-                    CoerceJavaScriptValueToJavaValue(value_variant,
-                                                     target_inner_type,
-                                                     false));
+    jvalue element = CoerceJavaScriptValueToJavaValue(value_variant,
+                                                      target_inner_type,
+                                                      false);
+    SetArrayElement(result, target_inner_type, i, element);
+    // CoerceJavaScriptValueToJavaValue() creates new local references to
+    // strings, objects and arrays. Of these, only strings can occur here.
+    // SetArrayElement() causes the array to take its own reference to the
+    // string, so we can now release the local reference.
+    DCHECK_NE(JavaType::TypeObject, target_inner_type.type);
+    DCHECK_NE(JavaType::TypeArray, target_inner_type.type);
+    if (target_inner_type.type == JavaType::TypeString) {
+      AttachCurrentThread()->DeleteLocalRef(element.l);

[joth, 2012/03/22 12:42:12]

nit: Grab the JNIEnv* outside the loop.

[Steve Block, 2012/03/22 13:03:23]

Done.

+    }
     WebBindings::releaseVariantValue(&value_variant);
   }
 
   return result;
 }
 
 jvalue CoerceJavaScriptObjectToJavaValue(const NPVariant& variant,
                                          const JavaType& target_type,
                                          bool coerce_to_string) {
   // This covers both JavaScript objects (including arrays) and Java objects.
@@ skipping 108 matching lines @@
       NOTREACHED();
       break;
   }
   return result;
 }
 
 // coerce_to_string means that we should try to coerce all JavaScript values to
 // strings when required, rather than simply converting to NULL. This is used
 // to maintain current behaviour, which differs slightly depending upon whether
 // or not the coercion in question is for an array element.
+//
+// Note that the jvalue returned by this method may contain a new local
+// reference to an object (string, object or array). This must be released by
+// the caller.
 jvalue CoerceJavaScriptValueToJavaValue(const NPVariant& variant,
                                         const JavaType& target_type,
                                         bool coerce_to_string) {
   // Note that in all these conversions, the relevant field of the jvalue must
   // always be explicitly set, as jvalue does not initialize its fields.
 
-  // Some of these methods create new Java Strings. Note that we don't
-  // explicitly release the local ref to these new objects, as there's no simple
-  // way to do so.
   switch (variant.type) {
     case NPVariantType_Int32:
     case NPVariantType_Double:
       return CoerceJavaScriptNumberToJavaValue(variant, target_type,
                                                coerce_to_string);
     case NPVariantType_Bool:
       return CoerceJavaScriptBooleanToJavaValue(variant, target_type,
                                                 coerce_to_string);
     case NPVariantType_String:
       return CoerceJavaScriptStringToJavaValue(variant, target_type);
@@ skipping 72 matching lines @@
   std::vector<jvalue> parameters(arg_count);
   for (size_t i = 0; i < arg_count; ++i) {
     parameters[i] = CoerceJavaScriptValueToJavaValue(args[i],
                                                      method->parameter_type(i),
                                                      true);
   }
 
   // Call
   *result = CallJNIMethod(java_object_.obj(), method->return_type(),
                           method->id(), &parameters[0]);
+
+  // Now that we're done with the jvalue, release any local references created
+  // by CoerceJavaScriptValueToJavaValue().
+  for (size_t i = 0; i < arg_count; ++i) {
+    if (method->parameter_type(i).type == JavaType::TypeString ||
+        method->parameter_type(i).type == JavaType::TypeObject ||
+        method->parameter_type(i).type == JavaType::TypeArray) {
+      AttachCurrentThread()->DeleteLocalRef(parameters[i].l);
+    }

[joth, 2012/03/22 12:42:12]

to make it a bit more self documenting, how about we extract a function

  void ReleaseJavaValueOfType(JNIEnv* env, jvalue* value, const JavaType& type)
{
    if (type == String | Object | Array) {
      env->DeleteLocalRef(v->l);
      v->l = NULL;
    }
  }

can call it here and in CoerceJavaScriptObjectToArray

[Steve Block, 2012/03/22 13:03:23]

Done.

+  }
+
   return true;
 }
 
 void JavaBoundObject::EnsureMethodsAreSetUp() const {
   if (!methods_.empty()) {
     return;
   }
 
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jclass> clazz(env, static_cast<jclass>(
@@ skipping 11 matching lines @@
   size_t num_methods = env->GetArrayLength(methods.obj());
   DCHECK(num_methods) << "Java objects always have public methods";
   for (size_t i = 0; i < num_methods; ++i) {
     ScopedJavaLocalRef<jobject> java_method(
         env,
         env->GetObjectArrayElement(methods.obj(), i));
     JavaMethod* method = new JavaMethod(java_method);
     methods_.insert(std::make_pair(method->name(), method));
   }
 }