Explicitly release JNI local references in the Java Bridge

content/browser/renderer_host/java/java_bound_object.cc

Index: content/browser/renderer_host/java/java_bound_object.cc
diff --git a/content/browser/renderer_host/java/java_bound_object.cc b/content/browser/renderer_host/java/java_bound_object.cc
index a9f7920d1c036e2f2547724de8337f9e50a378ec..b5f7c70b978bc7b47847726ee1af19a3abd0e6b9 100644
--- a/content/browser/renderer_host/java/java_bound_object.cc
+++ b/content/browser/renderer_host/java/java_bound_object.cc
@@ -397,7 +397,10 @@ jobject CreateJavaArray(const JavaType& type, jsize length) {
   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,
@@ -454,6 +457,7 @@ 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);
@@ -498,8 +502,7 @@ jobject CoerceJavaScriptObjectToArray(const NPVariant& variant,
     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;
@@ -512,10 +515,19 @@ jobject CoerceJavaScriptObjectToArray(const NPVariant& variant,
     // 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);
   }
 
@@ -644,15 +656,16 @@ jvalue CoerceJavaScriptNullOrUndefinedToJavaValue(const NPVariant& variant,
 // 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:
@@ -745,6 +758,17 @@ bool JavaBoundObject::Invoke(const std::string& name, const NPVariant* args,
   // 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;
 }