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..b7acb33857d547d4e42dada4a46511483ae47f5c 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,17 @@ jvalue CoerceJavaScriptValueToJavaValue(const NPVariant& variant,
                                         const JavaType& target_type,
                                         bool coerce_to_string);
 
+void ReleaseJavaValueIfRequired(JNIEnv* env, jvalue* value,

[joth, 2012/03/22 13:20:48]

nit: one param per line.
(if you want, you could group value & type together as they are very closely
coupled)

+                                const JavaType& type) {
+  if (type.type == JavaType::TypeString ||
+      type.type == JavaType::TypeObject ||
+      type.type == JavaType::TypeArray) {
+    env->DeleteLocalRef(value->l);
+    value->l = NULL;
+  }
+}
+
+// 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,11 +512,11 @@ 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;
+  JNIEnv* env = AttachCurrentThread();
   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
@@ -512,10 +526,17 @@ 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);
+    ReleaseJavaValueIfRequired(env, &element, target_inner_type);
     WebBindings::releaseVariantValue(&value_variant);
   }
 
@@ -644,15 +665,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 +767,14 @@ 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().
+  JNIEnv* env = AttachCurrentThread();
+  for (size_t i = 0; i < arg_count; ++i) {
+    ReleaseJavaValueIfRequired(env, &parameters[i], method->parameter_type(i));
+  }
+
   return true;
 }