[Android] Fix leak in 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 570 matching lines @@
   NPObject* object = NPVARIANT_TO_OBJECT(variant);
   bool is_java_object = &JavaNPObject::kNPClass == object->_class;
 
   jvalue result;
   switch (target_type.type) {
     case JavaType::TypeObject:
       if (is_java_object) {
         // LIVECONNECT_COMPLIANCE: Existing behavior is to pass all Java
         // objects. Spec requires passing only Java objects which are
         // assignment-compatibile.
-        result.l = AttachCurrentThread()->NewLocalRef(
-            JavaBoundObject::GetJavaObject(object));
+        result.l = JavaBoundObject::GetJavaObject(object);

[joth, 2013/01/07 19:32:41]

this may now be NULL. Any idea what that might break?

[benm (inactive), 2013/01/08 14:43:05]

I think this is for the case that during a JS -> java method call, you pass a JS
object that's really a Java object as a parameter. So now that parameter could
be passed as null.

Hrm, maybe this CL isn't right then. If we had the following JavaScript:

window.jsbridge.foo(window.jsbridge.getBar());

with jsbridge defined in java:

class jsbridge {
  public void foo(Object o) {
    //assume o is not null...
  }

   public Object getBar() {
     return new Object();
   }
}

Then we'll only have a weakref native side to the getBar() object and so it
could get GC'd before we receive it into the NewLocalRef here in
java_bound_object.cc for the call to foo(), right? And in the case that that
happens, the foo function will now start crashing (or not, depending on GC).

Does that make sense?

[joth, 2013/01/09 00:28:22]

Yes. I chatted with sgurun and looks like classic didn't cope with that case
ether.
However in
https://android.googlesource.com/platform/frameworks/base/+/master/core/java/...
you'll see there's  both mJavaScriptObjects (serving same purpose as your field
of same name) but also a 'set' of objects bound to the current page (including
those that have been removed from the JS API). Adding this set would be the
logical place to stash a hard ref to any object we received here (as unlike the
primary API objects, these should not persist across navigations).
we pass a jweak that refers to the Set<Object> which the implementation can just
call add() on here as needed. Hmmmm that might be sweet. Call it the "bound java
object retaining set" or something.
JavaBridgeDispatcherHostManager is a WebContents observer, so it knows when page
navigations occur and could automatically  clear() the set as needed.
Suggest opening a new crbug to track that....

[benm (inactive), 2013/01/09 11:17:00]

Happy to add this in a follow up change but just want to make sure I understand
fully...

1. Add a Set<Object> in CVC that keeps tack of removed JS interfaces and is
cleared on page navigation (actually that feels like it should be part of this
patch as I think as it stands this change will potentially break the case that
you remove a JS interface but expect to still be able to call methods on it
until the page is navigated)
2. Pass a weak ref to that set to native (somehow, via addJavascriptInterface
perhaps?) so that in java_bound_object.cc we can add any object returned from
java to that set, thus keeping it alive and fixing the issue described in my
comment above. I guess we'd do that add() in JavaBoundObject::CallJNIMethod()
when the return type is a JavaType::Object? (line 211 in this file?)
3. I can imagine this set might become quite large for apps that make heavy use
of the java bridge, potentially unnecessarily keeping objects returned from java
alive until the next page navigation. Should we be more proactive about clearing
it up? Can we be?

I think I'll try to add 1. in this patch, and if I've understood right file a
bug and do 2/3 in the follow up patch.

[benm (inactive), 2013/01/09 14:46:13]

For 3, I guess we could remove from the set in JavaNPObject::Deallocate. (line
86)

       } else {
         // LIVECONNECT_COMPLIANCE: Existing behavior is to pass null. Spec
         // requires converting if the target type is
         // netscape.javascript.JSObject, otherwise raising a JavaScript
         // exception.
         result.l = NULL;
       }
       break;
     case JavaType::TypeString:
       // LIVECONNECT_COMPLIANCE: Existing behavior is to convert to
@@ skipping 132 matching lines @@
       &JavaNPObject::kNPClass));
   // The NPObject takes ownership of the JavaBoundObject.
   reinterpret_cast<JavaNPObject*>(np_object)->bound_object =
       new JavaBoundObject(object, safe_annotation_clazz);
   return np_object;
 }
 
 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
   // lazily when a method is first invoked.
 }
 
 JavaBoundObject::~JavaBoundObject() {
 }
 
 jobject JavaBoundObject::GetJavaObject(NPObject* object) {
   DCHECK_EQ(&JavaNPObject::kNPClass, object->_class);
   JavaBoundObject* jbo = reinterpret_cast<JavaNPObject*>(object)->bound_object;
-  return jbo->java_object_.obj();
+  JNIEnv* env = AttachCurrentThread();
+  return env->NewLocalRef(jbo->java_object_.get(env).obj());
 }
 
 bool JavaBoundObject::HasMethod(const std::string& name) const {
   EnsureMethodsAreSetUp();
   return methods_.find(name) != methods_.end();
 }
 
 bool JavaBoundObject::Invoke(const std::string& name, const NPVariant* args,
                              size_t arg_count, NPVariant* result) {
   EnsureMethodsAreSetUp();
@@ skipping 19 matching lines @@
   }
 
   // Coerce
   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
-  bool ok = CallJNIMethod(java_object_.obj(), method->return_type(),
-                          method->id(), &parameters[0], result,
-                          safe_annotation_clazz_);
+  ScopedJavaLocalRef<jobject> obj = java_object_.get(AttachCurrentThread());
+
+  bool ok = false;
+  if (!obj.is_null()) {
+    // Call
+    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().
   JNIEnv* env = AttachCurrentThread();
   for (size_t i = 0; i < arg_count; ++i) {
     ReleaseJavaValueIfRequired(env, &parameters[i], method->parameter_type(i));
   }
 
   return ok;
 }
 
 void JavaBoundObject::EnsureMethodsAreSetUp() const {
   if (are_methods_set_up_)
     return;
   are_methods_set_up_ = true;
 
   JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> obj = java_object_.get(env);
+
+  DCHECK(!obj.is_null());

[joth, 2013/01/07 19:32:41]

this could happen though. If the java side ophen's its WebView and associated
tree of java objects, they will get GC'ed but the native side won't get told
until some time after. (CleanupReference is async in nature)

Ah... the assumption is EnsureMethodsAreSetUp() is done synchronously in a setup
flow, where it would be impossible for for a java-side GC to have already
released these guys? Hmmm guess so but definitely add a comment here explaining
it, to discourage anyone from copying this DCHECK(!null) pattern anywhere else.

[benm (inactive), 2013/01/08 14:43:05]

No, I think your first comment is correct. EnsureMethodsAreSetup is performed
lazily so I think that we could end up in the situation you describe - a null
obj here. I think in that case we just want to return without adding anything
into methods_.

+
   ScopedJavaLocalRef<jclass> clazz(env, static_cast<jclass>(
-      env->CallObjectMethod(java_object_.obj(),  GetMethodIDFromClassName(
+      env->CallObjectMethod(obj.obj(),  GetMethodIDFromClassName(
           env,
           kJavaLangObject,
           kGetClass,
           kReturningJavaLangClass))));
 
   ScopedJavaLocalRef<jobjectArray> methods(env, static_cast<jobjectArray>(
       env->CallObjectMethod(clazz.obj(), GetMethodIDFromClassName(
           env,
           kJavaLangClass,
           kGetMethods,
@@ skipping 20 matching lines @@
       if (!safe)
         continue;
     }
 
     JavaMethod* method = new JavaMethod(java_method);
     methods_.insert(std::make_pair(method->name(), method));
   }
 }
 
 }  // namespace content