[Android] Fix leak in Java Bridge.

First draft at moving the strong reference native side into a
weak reference and maintaining the strong reference on the
java side.

Android only change, android bots green.
NOTRY=true

Review URL: https://chromiumcodereview.appspot.com/11802002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@176079 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/renderer_host/java/java_bound_object.cc

@@ -589,7 +589,7 @@ jvalue CoerceJavaScriptObjectToJavaValue(const NPVariant& variant,
         // objects. Spec requires passing only Java objects which are
         // assignment-compatibile.
         result.l = AttachCurrentThread()->NewLocalRef(
-            JavaBoundObject::GetJavaObject(object));
+            JavaBoundObject::GetJavaObject(object).obj());
       } else {
         // LIVECONNECT_COMPLIANCE: Existing behavior is to pass null. Spec
         // requires converting if the target type is
@@ -742,7 +742,7 @@ NPObject* JavaBoundObject::Create(
 JavaBoundObject::JavaBoundObject(
     const JavaRef<jobject>& object,
     base::android::JavaRef<jclass>& safe_annotation_clazz)
-    : java_object_(object),
+    : java_object_(AttachCurrentThread(), object.obj()),
       are_methods_set_up_(false),
       safe_annotation_clazz_(safe_annotation_clazz) {
   // We don't do anything with our Java object when first created. We do it all
@@ -752,10 +752,10 @@ JavaBoundObject::JavaBoundObject(
 JavaBoundObject::~JavaBoundObject() {
 }
 
-jobject JavaBoundObject::GetJavaObject(NPObject* object) {
+ScopedJavaLocalRef<jobject> JavaBoundObject::GetJavaObject(NPObject* object) {
   DCHECK_EQ(&JavaNPObject::kNPClass, object->_class);
   JavaBoundObject* jbo = reinterpret_cast<JavaNPObject*>(object)->bound_object;
-  return jbo->java_object_.obj();
+  return jbo->java_object_.get(AttachCurrentThread());
 }
 
 bool JavaBoundObject::HasMethod(const std::string& name) const {
@@ -795,10 +795,15 @@ bool JavaBoundObject::Invoke(const std::string& name, const NPVariant* args,
                                                      true);
   }
 
+  ScopedJavaLocalRef<jobject> obj = java_object_.get(AttachCurrentThread());
+
+  bool ok = false;
+  if (!obj.is_null()) {
     // Call
-  bool ok = CallJNIMethod(java_object_.obj(), method->return_type(),
+    ok = CallJNIMethod(obj.obj(), method->return_type(),
                        method->id(), &parameters[0], result,
                        safe_annotation_clazz_);
+  }
 
   // Now that we're done with the jvalue, release any local references created
   // by CoerceJavaScriptValueToJavaValue().
@@ -816,8 +821,14 @@ void JavaBoundObject::EnsureMethodsAreSetUp() const {
   are_methods_set_up_ = true;
 
   JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> obj = java_object_.get(env);
+
+  if (obj.is_null()) {
+    return;
+  }
+
   ScopedJavaLocalRef<jclass> clazz(env, static_cast<jclass>(
-      env->CallObjectMethod(java_object_.obj(),  GetMethodIDFromClassName(
+      env->CallObjectMethod(obj.obj(),  GetMethodIDFromClassName(
           env,
           kJavaLangObject,
           kGetClass,

content/browser/renderer_host/java/java_bound_object.h

@@ -9,6 +9,7 @@
 #include <map>
 #include <string>
 
+#include "base/android/jni_helper.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/memory/linked_ptr.h"
 #include "content/browser/renderer_host/java/java_method.h"
@@ -37,10 +38,11 @@ class JavaBoundObject {
 
   virtual ~JavaBoundObject();
 
-  // Gets a global ref to the underlying JavaObject from a JavaBoundObject
-  // wrapped as an NPObject. Ownership of the global ref is retained by the
-  // JavaBoundObject: the caller must NOT release it.
-  static jobject GetJavaObject(NPObject* object);
+  // Gets a local ref to the underlying JavaObject from a JavaBoundObject
+  // wrapped as an NPObject. May return null if the underlying object has
+  // been garbage collected.
+  static base::android::ScopedJavaLocalRef<jobject> GetJavaObject(
+      NPObject* object);
 
   // Methods to implement the NPObject callbacks.
   bool HasMethod(const std::string& name) const;
@@ -54,10 +56,9 @@ class JavaBoundObject {
 
   void EnsureMethodsAreSetUp() const;
 
-  // The global ref to the underlying Java object that this JavaBoundObject
+  // The weak ref to the underlying Java object that this JavaBoundObject
   // instance represents.
-  base::android::ScopedJavaGlobalRef<jobject> java_object_;
-
+  JavaObjectWeakGlobalRef java_object_;
   // Map of public methods, from method name to Method instance. Multiple
   // entries will be present for overloaded methods. Note that we can't use
   // scoped_ptr in STL containers as we can't copy it.

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java

@@ -49,6 +49,10 @@ import org.chromium.content.common.TraceEvent;
 import org.chromium.ui.gfx.NativeWindow;
 
 import java.lang.annotation.Annotation;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import java.util.HashSet;
 
 /**
  * Provides a Java-side 'wrapper' around a WebContent (native) instance.
@@ -75,6 +79,27 @@ public class ContentViewCore implements MotionEventDelegate, NavigationClient {
     // Personality of the ContentView.
     private final int mPersonality;
 
+    // If the embedder adds a JavaScript interface object that contains an indirect reference to
+    // the ContentViewCore, then storing a strong ref to the interface object on the native
+    // side would prevent garbage collection of the ContentViewCore (as that strong ref would
+    // create a new GC root).
+    // For that reason, we store only a weak reference to the interface object on the
+    // native side. However we still need a strong reference on the Java side to
+    // prevent garbage collection if the embedder doesn't maintain their own ref to the
+    // interface object - the Java side ref won't create a new GC root.
+    // This map stores those refernces. We put into the map on addJavaScriptInterface()
+    // and remove from it in removeJavaScriptInterface().
+    private Map<String, Object> mJavaScriptInterfaces = new HashMap<String, Object>();
+
+    // Additionally, we keep track of all Java bound JS objects that are in use on the
+    // current page to ensure that they are not garbage collected until the page is
+    // navigated. This includes interface objects that have been removed
+    // via the removeJavaScriptInterface API and transient objects returned from methods
+    // on the interface object.
+    // TODO(benm): Implement the transient object retention - likely by moving the
+    // management of this set into the native Java bridge. (crbug/169228) (crbug/169228)
+    private Set<Object> mRetainedJavaScriptObjects = new HashSet<Object>();
+
     /**
      * Interface that consumers of {@link ContentViewCore} must implement to allow the proper
      * dispatching of view methods through the containing view.
@@ -494,6 +519,10 @@ public class ContentViewCore implements MotionEventDelegate, NavigationClient {
             public void didStartLoading(String url) {
                 hidePopupDialog();
                 resetGestureDetectors();
+                // TODO(benm): This isn't quite the right place to do this. Management
+                // of this set should be moving into the native java bridge in crbug/169228
+                // and until that's ready this will do.
+                mRetainedJavaScriptObjects.clear();
             }
         };
     }
@@ -625,6 +654,8 @@ public class ContentViewCore implements MotionEventDelegate, NavigationClient {
         }
         mNativeContentViewCore = 0;
         mContentSettings = null;
+        mJavaScriptInterfaces.clear();
+        mRetainedJavaScriptObjects.clear();
     }
 
     /**
@@ -2131,13 +2162,6 @@ public class ContentViewCore implements MotionEventDelegate, NavigationClient {
         return mZoomManager.getZoomControlsViewForTest();
     }
 
-    // TODO(joth): Remove in next patch.
-    @Deprecated
-    public void addJavascriptInterface(Object object, String name, boolean requireAnnotation) {
-        addPossiblyUnsafeJavascriptInterface(object, name,
-                requireAnnotation ? JavascriptInterface.class : null);
-    }
-
     /**
      * This will mimic {@link #addPossiblyUnsafeJavascriptInterface(Object, String, Class)}
      * and automatically pass in {@link JavascriptInterface} as the required annotation.
@@ -2195,6 +2219,7 @@ public class ContentViewCore implements MotionEventDelegate, NavigationClient {
     public void addPossiblyUnsafeJavascriptInterface(Object object, String name,
             Class<? extends Annotation> requiredAnnotation) {
         if (mNativeContentViewCore != 0 && object != null) {
+            mJavaScriptInterfaces.put(name, object);
             nativeAddJavascriptInterface(mNativeContentViewCore, object, name, requiredAnnotation);
         }
     }
@@ -2205,6 +2230,10 @@ public class ContentViewCore implements MotionEventDelegate, NavigationClient {
      * @param name The name of the interface to remove.
      */
     public void removeJavascriptInterface(String name) {
+        // TODO(benm): Move the management of this retained object set
+        // into the native java bridge. (crbug/169228)
+        mRetainedJavaScriptObjects.add(mJavaScriptInterfaces.get(name));
+        mJavaScriptInterfaces.remove(name);
         if (mNativeContentViewCore != 0) {
             nativeRemoveJavascriptInterface(mNativeContentViewCore, name);
         }