JavaBridge should use Annotation

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 f82588dee3c457ecbee4d1d1f0104216eda70995..8a96864245cd1af815713c15989a584697a10475 100644
--- a/content/browser/renderer_host/java/java_bound_object.cc
+++ b/content/browser/renderer_host/java/java_bound_object.cc
@@ -35,17 +35,19 @@ namespace {
 const char kJavaLangClass[] = "java/lang/Class";
 const char kJavaLangObject[] = "java/lang/Object";
 const char kJavaLangReflectMethod[] = "java/lang/reflect/Method";
+// TODO(dtrainor): Parameterize this so that WebView and Chrome for Android can
+// use different annotations.

[Steve Block, 2012/08/06 12:12:30]

What's the motivation for this?

[joth, 2012/08/06 15:59:31]

WebView would need to use one defined in the SDK, but chrome app can't, as that
wouldn't be present all platforms targeted by the app.

+const char kJavaScriptInterfaceAnnotation[] =
+    "org/chromium/content/common/JavaScriptInterface";

[joth, 2012/08/04 20:38:28]

could be in content/browser, as it's only relevant in browser process?

[David Trainor- moved to gerrit, 2012/08/06 23:42:33]

Originally I thought we were just going to move the current one to WebView and
in the SDK, so I thought common kind of made sense.  But if we're going to have
a different one for Chrome and for WebView yeah I'll move it into
content/browser :).

 const char kGetClass[] = "getClass";
-const char kGetDeclaredMethods[] = "getDeclaredMethods";
 const char kGetMethods[] = "getMethods";
-const char kGetModifiers[] = "getModifiers";
-const char kReturningInteger[] = "()I";
+const char kIsAnnotationPresent[] = "isAnnotationPresent";
 const char kReturningJavaLangClass[] = "()Ljava/lang/Class;";
 const char kReturningJavaLangReflectMethodArray[] =
     "()[Ljava/lang/reflect/Method;";
+const char kTakesJavaLangClassReturningBoolean[] = "(Ljava/lang/Class;)Z";
 
-// This constant represents the value at java.lang.reflect.Modifier.PUBLIC.
-const int kJavaPublicModifier = 1;
+jclass g_JavaScriptInterface_clazz = NULL;

[joth, 2012/08/04 20:38:28]

nit: in the context of this file 'JavaScriptInterface' is pretty ambiguous. Also
naming for non-generated code should be hacker_case.
Maybe g_safe_annotation_clazz 

[David Trainor- moved to gerrit, 2012/08/06 23:42:33]

Done.

 
 // Our special NPObject type.  We extend an NPObject with a pointer to a
 // JavaBoundObject.  We also add static methods for each of the NPObject
@@ -128,7 +130,7 @@ bool JavaNPObject::GetProperty(NPObject* np_object,
 // return value is simply converted to the corresponding NPAPI type.
 bool CallJNIMethod(jobject object, const JavaType& return_type, jmethodID id,
                    jvalue* parameters, NPVariant* result,
-                   bool allow_inherited_methods) {
+                   bool require_safe_annotation) {
   JNIEnv* env = AttachCurrentThread();
   switch (return_type.type) {
     case JavaType::TypeBoolean:
@@ -210,7 +212,7 @@ bool CallJNIMethod(jobject object, const JavaType& return_type, jmethodID id,
         break;
       }
       OBJECT_TO_NPVARIANT(JavaBoundObject::Create(scoped_java_object,
-                                                  allow_inherited_methods),
+                                                  require_safe_annotation),
                           *result);
       break;
     }
@@ -726,9 +728,16 @@ jvalue CoerceJavaScriptValueToJavaValue(const NPVariant& variant,
 
 }  // namespace
 
+bool RegisterJavaBoundObject(JNIEnv* env) {
+  g_JavaScriptInterface_clazz = reinterpret_cast<jclass>(env->NewGlobalRef(
+      base::android::GetUnscopedClass(env, kJavaScriptInterfaceAnnotation)));
+  DCHECK(g_JavaScriptInterface_clazz);
+
+  return true;
+}

[Steve Block, 2012/08/06 12:12:30]

Do we usually use browser_jni_registrar.cc to trigger the  the JNI lookup for
these kind of internals? We don't need to do it at startup, so why not do it
lazily when first required? Also, the name of the method suggests that we're
registering JavaBoundObject for use from Java, which isn't the case. Is this a
standard pattern?

[joth, 2012/08/06 15:59:31]

It needs to be done in a thread that originated in Java, the easiest way to
ensure it (consistent with our other JNI using code) is to setup at startup.
(see email thread on Friday on this)

It's not usual to have a RegisterFoo method that only does hand-coded lookups
like this, but I'm sure that some other methods do add some manual registartions
along side the script generated ones. I suggested naming it this way, as from
the caller's point of view (i.e. the content registrar) they don't really care
what registration is happening inside this file, the important thing is that
RegisterFoo lives in foo.cc, and that it must be called. 

[David Trainor- moved to gerrit, 2012/08/06 23:42:33]

If we want to change the name, I can pull this out to a new file, but since it's
basically contained to java_bound_object and solely for it's use, this seemed to
make sense.

 
 NPObject* JavaBoundObject::Create(const JavaRef<jobject>& object,
-                                  bool allow_inherited_methods) {
+                                  bool require_safe_annotation) {
   // The first argument (a plugin's instance handle) is passed through to the
   // allocate function directly, and we don't use it, so it's ok to be 0.
   // The object is created with a ref count of one.
@@ -736,15 +745,15 @@ NPObject* JavaBoundObject::Create(const JavaRef<jobject>& object,
       &JavaNPObject::kNPClass));
   // The NPObject takes ownership of the JavaBoundObject.
   reinterpret_cast<JavaNPObject*>(np_object)->bound_object =
-      new JavaBoundObject(object, allow_inherited_methods);
+      new JavaBoundObject(object, require_safe_annotation);
   return np_object;
 }
 
 JavaBoundObject::JavaBoundObject(const JavaRef<jobject>& object,
-                                 bool allow_inherited_methods)
+                                 bool require_safe_annotation)
     : java_object_(object),
       are_methods_set_up_(false),
-      allow_inherited_methods_(allow_inherited_methods) {
+      require_safe_annotation_(require_safe_annotation) {
   // We don't do anything with our Java object when first created. We do it all
   // lazily when a method is first invoked.
 }
@@ -798,7 +807,7 @@ bool JavaBoundObject::Invoke(const std::string& name, const NPVariant* args,
   // Call
   bool ok = CallJNIMethod(java_object_.obj(), method->return_type(),
                           method->id(), &parameters[0], result,
-                          allow_inherited_methods_);
+                          require_safe_annotation_);
 
   // Now that we're done with the jvalue, release any local references created
   // by CoerceJavaScriptValueToJavaValue().
@@ -823,19 +832,15 @@ void JavaBoundObject::EnsureMethodsAreSetUp() const {
           kGetClass,
           kReturningJavaLangClass))));
 
-  const char* get_method = allow_inherited_methods_ ?
-      kGetMethods : kGetDeclaredMethods;
-
   ScopedJavaLocalRef<jobjectArray> methods(env, static_cast<jobjectArray>(
       env->CallObjectMethod(clazz.obj(), GetMethodIDFromClassName(
           env,
           kJavaLangClass,
-          get_method,
+          kGetMethods,
           kReturningJavaLangReflectMethodArray))));
 
   size_t num_methods = env->GetArrayLength(methods.obj());
-  if (num_methods <= 0)
-    return;
+  DCHECK(num_methods) << "Java objects always have public methods";

[joth, 2012/08/04 20:38:28]

nit: I think chromium style is to put purely static message as a comment
instead, to keep bin size down. (sending runtime valies into the check is ok)

[David Trainor- moved to gerrit, 2012/08/06 23:42:33]

Done.

 
   for (size_t i = 0; i < num_methods; ++i) {
     ScopedJavaLocalRef<jobject> java_method(
@@ -843,14 +848,16 @@ void JavaBoundObject::EnsureMethodsAreSetUp() const {
         env->GetObjectArrayElement(methods.obj(), i));
 
     bool is_method_allowed = true;
-    if (!allow_inherited_methods_) {
-      jint modifiers = env->CallIntMethod(java_method.obj(),
-                                          GetMethodIDFromClassName(
-                                              env,
-                                              kJavaLangReflectMethod,
-                                              kGetModifiers,
-                                              kReturningInteger));
-      is_method_allowed &= (modifiers & kJavaPublicModifier);
+    if (require_safe_annotation_) {
+      jboolean safe = env->CallBooleanMethod(java_method.obj(),
+        GetMethodIDFromClassName(
+            env,
+            kJavaLangReflectMethod,
+            kIsAnnotationPresent,
+            kTakesJavaLangClassReturningBoolean),
+        g_JavaScriptInterface_clazz);

[Steve Block, 2012/08/06 12:12:30]

wrong indentation

[David Trainor- moved to gerrit, 2012/08/06 23:42:33]

Ah moved these in by 2.  Not sure if anything else is incorrect with this.  The
last parameter isn't a member of the GetMethodIDFromClassName so not sure if it
should stay at the same indent level as the parameter above.

+
+      is_method_allowed &= safe;

[joth, 2012/08/04 20:38:28]

&= is bitwise operator, and it's not obvious if jboolean is actually type bool,
so could in theory do the wrong thing.
I'd be tempted to say s_method_allowed = s_method_allowed && safe, or maybe
just:
if (!safe)
  continue;
(and remove is_method_allowed)

[David Trainor- moved to gerrit, 2012/08/06 23:42:33]

Ah good catch.  Done.

     }
 
     if (is_method_allowed) {