[Android] Check for Java object types covariance in Java Bridge

ART does check covariance of types passed via JNI anyway, causing the VM to
crash if they doesn't. We'd better check this in Java Bridge and throw a
JavaScript exception, as LiveConnect spec suggests.

BUG=392489

Review URL: https://codereview.chromium.org/393733002

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

build/android/findbugs_filter/findbugs_known_bugs.txt

-H C EC: Using pointer equality to compare a JavaBridgeCoercionTest$CustomType with a JavaBridgeCoercionTest$CustomType2 in org.chromium.content.browser.JavaBridgeCoercionTest.testPassJavaObject()  At JavaBridgeCoercionTest.java
 M C CSM: Shouldn't use synchronized method, please narrow down the synchronization scope.  At JavaBridgeArrayCoercionTest.java
 M C CSM: Shouldn't use synchronized method, please narrow down the synchronization scope.  At JavaBridgeArrayTest.java
 M C CSM: Shouldn't use synchronized method, please narrow down the synchronization scope.  At JavaBridgeBasicsTest.java

content/browser/android/java/gin_java_method_invocation_helper.cc

@@ -147,19 +147,29 @@ void GinJavaMethodInvocationHelper::Invoke() {
     return;
   }
 
+  GinJavaBridgeError coercion_error = kGinJavaBridgeNoError;
   std::vector<jvalue> parameters(method->num_parameters());
   for (size_t i = 0; i < method->num_parameters(); ++i) {
     const base::Value* argument;
     arguments_->Get(i, &argument);
-    parameters[i] = CoerceJavaScriptValueToJavaValue(
-        env, argument, method->parameter_type(i), true, object_refs_);
+    parameters[i] = CoerceJavaScriptValueToJavaValue(env,
+                                                     argument,
+                                                     method->parameter_type(i),
+                                                     true,
+                                                     object_refs_,
+                                                     &coercion_error);
   }
-  if (method->is_static()) {
-    InvokeMethod(
-        NULL, cls.obj(), method->return_type(), method->id(), &parameters[0]);
+
+  if (coercion_error == kGinJavaBridgeNoError) {
+    if (method->is_static()) {
+      InvokeMethod(
+          NULL, cls.obj(), method->return_type(), method->id(), &parameters[0]);
+    } else {
+      InvokeMethod(
+          obj.obj(), NULL, method->return_type(), method->id(), &parameters[0]);
+    }
   } else {
-    InvokeMethod(
-        obj.obj(), NULL, method->return_type(), method->id(), &parameters[0]);
+    SetInvocationError(coercion_error);
   }
 
   // Now that we're done with the jvalue, release any local references created

content/browser/android/java/gin_java_script_to_java_types_coercion.h

@@ -11,6 +11,7 @@
 #include "base/values.h"
 #include "content/browser/android/java/gin_java_bound_object.h"
 #include "content/browser/android/java/java_type.h"
+#include "content/common/android/gin_java_bridge_errors.h"
 
 namespace content {
 
@@ -22,7 +23,8 @@ jvalue CoerceJavaScriptValueToJavaValue(
     const base::Value* value,
     const JavaType& target_type,
     bool coerce_to_string,
-    const ObjectRefs& object_refs);
+    const ObjectRefs& object_refs,
+    GinJavaBridgeError* error);
 
 void ReleaseJavaValueIfRequired(JNIEnv* env,
                                 jvalue* value,

content/browser/android/java/java_method.cc

@@ -8,7 +8,6 @@
 #include "base/android/jni_string.h"
 #include "base/lazy_instance.h"
 #include "base/memory/singleton.h"
-#include "base/strings/string_util.h"  // For ReplaceSubstringsAfterOffset
 #include "content/browser/android/java/jni_helper.h"
 
 using base::android::AttachCurrentThread;
@@ -50,43 +49,11 @@ struct ModifierClassTraits :
 base::LazyInstance<ScopedJavaGlobalRef<jclass>, ModifierClassTraits>
     g_java_lang_reflect_modifier_class = LAZY_INSTANCE_INITIALIZER;
 
-std::string BinaryNameToJNIName(const std::string& binary_name,
-                                JavaType* type) {
+std::string BinaryNameToJNISignature(const std::string& binary_name,
+                                     JavaType* type) {
   DCHECK(type);
   *type = JavaType::CreateFromBinaryName(binary_name);
-  switch (type->type) {
-    case JavaType::TypeBoolean:
-      return "Z";
-    case JavaType::TypeByte:
-      return "B";
-    case JavaType::TypeChar:
-      return "C";
-    case JavaType::TypeShort:
-      return "S";
-    case JavaType::TypeInt:
-      return "I";
-    case JavaType::TypeLong:
-      return "J";
-    case JavaType::TypeFloat:
-      return "F";
-    case JavaType::TypeDouble:
-      return "D";
-    case JavaType::TypeVoid:
-      return "V";
-    case JavaType::TypeArray: {
-      // For array types, the binary name uses the JNI name encodings.
-      std::string jni_name = binary_name;
-      ReplaceSubstringsAfterOffset(&jni_name, 0, ".", "/");
-      return jni_name;
-    }
-    case JavaType::TypeString:
-    case JavaType::TypeObject:
-      std::string jni_name = "L" + binary_name + ";";
-      ReplaceSubstringsAfterOffset(&jni_name, 0, ".", "/");
-      return jni_name;
-  }
-  NOTREACHED();
-  return std::string();
+  return type->JNISignature();
 }
 
 }  // namespace
@@ -191,7 +158,7 @@ void JavaMethod::EnsureTypesAndIDAreSetUp() const {
             kGetName,
             kReturningJavaLangString))));
     std::string name_utf8 = ConvertJavaStringToUTF8(name);
-    signature += BinaryNameToJNIName(name_utf8, &parameter_types_[i]);
+    signature += BinaryNameToJNISignature(name_utf8, &parameter_types_[i]);
   }
   signature += ")";
 
@@ -208,8 +175,8 @@ void JavaMethod::EnsureTypesAndIDAreSetUp() const {
           kJavaLangClass,
           kGetName,
           kReturningJavaLangString))));
-  signature += BinaryNameToJNIName(ConvertJavaStringToUTF8(name),
-                                   &return_type_);
+  signature += BinaryNameToJNISignature(ConvertJavaStringToUTF8(name),
+                                        &return_type_);
 
   // Determine whether the method is static.
   jint modifiers = env->CallIntMethod(

content/browser/android/java/java_type.cc

@@ -5,53 +5,62 @@
 #include "content/browser/android/java/java_type.h"
 
 #include "base/logging.h"
+#include "base/strings/string_util.h"  // For ReplaceSubstringsAfterOffset
 
 namespace content {
 namespace {
 
-JavaType JavaTypeFromJNIName(const std::string& jni_name) {
-  JavaType result;
-  DCHECK(!jni_name.empty());
-  switch (jni_name[0]) {
+// Array component type names are similar to JNI type names, except for using
+// dots as namespace separators in class names.
+scoped_ptr<JavaType> CreateFromArrayComponentTypeName(
+    const std::string& type_name) {
+  scoped_ptr<JavaType> result(new JavaType());
+  DCHECK(!type_name.empty());
+  switch (type_name[0]) {
     case 'Z':
-      result.type = JavaType::TypeBoolean;
+      result->type = JavaType::TypeBoolean;
       break;
     case 'B':
-      result.type = JavaType::TypeByte;
+      result->type = JavaType::TypeByte;
       break;
     case 'C':
-      result.type = JavaType::TypeChar;
+      result->type = JavaType::TypeChar;
       break;
     case 'S':
-      result.type = JavaType::TypeShort;
+      result->type = JavaType::TypeShort;
       break;
     case 'I':
-      result.type = JavaType::TypeInt;
+      result->type = JavaType::TypeInt;
       break;
     case 'J':
-      result.type = JavaType::TypeLong;
+      result->type = JavaType::TypeLong;
       break;
     case 'F':
-      result.type = JavaType::TypeFloat;
+      result->type = JavaType::TypeFloat;
       break;
     case 'D':
-      result.type = JavaType::TypeDouble;
+      result->type = JavaType::TypeDouble;
       break;
     case '[':
-      result.type = JavaType::TypeArray;
-      // LIVECONNECT_COMPLIANCE: We don't support multi-dimensional arrays, so
-      // there's no need to populate the inner types.
+      result->type = JavaType::TypeArray;
+      result->inner_type =
+          CreateFromArrayComponentTypeName(type_name.substr(1));
       break;
     case 'L':
-      result.type = jni_name == "Ljava.lang.String;" ?
-                    JavaType::TypeString :
-                    JavaType::TypeObject;
+      if (type_name == "Ljava.lang.String;") {
+        result->type = JavaType::TypeString;
+        result->class_jni_name = "java/lang/String";
+      } else {
+        result->type = JavaType::TypeObject;
+        result->class_jni_name = type_name.substr(1, type_name.length() - 2);
+        ReplaceSubstringsAfterOffset(&result->class_jni_name, 0, ".", "/");
+      }
       break;
     default:
       // Includes void (V).
       NOTREACHED();
   }
-  return result;
+  return result.Pass();
 }
 
 }  // namespace
@@ -67,6 +76,8 @@ JavaType::~JavaType() {
 }
 
 JavaType& JavaType::operator=(const JavaType& other) {
+  if (this == &other)
+    return *this;
   type = other.type;
   if (other.inner_type) {
     DCHECK_EQ(JavaType::TypeArray, type);
@@ -74,9 +85,11 @@ JavaType& JavaType::operator=(const JavaType& other) {
   } else {
     inner_type.reset();
   }
+  class_jni_name = other.class_jni_name;
   return *this;
 }
 
+// static
 JavaType JavaType::CreateFromBinaryName(const std::string& binary_name) {
   JavaType result;
   DCHECK(!binary_name.empty());
@@ -100,15 +113,53 @@ JavaType JavaType::CreateFromBinaryName(const std::string& binary_name) {
     result.type = JavaType::TypeVoid;
   } else if (binary_name[0] == '[') {
     result.type = JavaType::TypeArray;
-    // The inner type of an array is represented in JNI format.
-    result.inner_type.reset(new JavaType(JavaTypeFromJNIName(
-        binary_name.substr(1))));
+    result.inner_type = CreateFromArrayComponentTypeName(binary_name.substr(1));
   } else if (binary_name == "java.lang.String") {
     result.type = JavaType::TypeString;
+    result.class_jni_name = "java/lang/String";
   } else {
     result.type = JavaType::TypeObject;
+    result.class_jni_name = binary_name;
+    ReplaceSubstringsAfterOffset(&result.class_jni_name, 0, ".", "/");
   }
   return result;
 }
 
+std::string JavaType::JNIName() const {
+  switch (type) {
+    case JavaType::TypeBoolean:
+      return "Z";
+    case JavaType::TypeByte:
+      return "B";
+    case JavaType::TypeChar:
+      return "C";
+    case JavaType::TypeShort:
+      return "S";
+    case JavaType::TypeInt:
+      return "I";
+    case JavaType::TypeLong:
+      return "J";
+    case JavaType::TypeFloat:
+      return "F";
+    case JavaType::TypeDouble:
+      return "D";
+    case JavaType::TypeVoid:
+      return "V";
+    case JavaType::TypeArray:
+      return "[" + inner_type->JNISignature();
+    case JavaType::TypeString:
+    case JavaType::TypeObject:
+      return class_jni_name;
+  }
+  NOTREACHED();
+  return std::string();
+}
+
+std::string JavaType::JNISignature() const {
+  if (type == JavaType::TypeString || type == JavaType::TypeObject)
+    return "L" + JNIName() + ";";
+  else
+    return JNIName();
+}
+
 }  // namespace content

content/browser/android/java/java_type.h

@@ -8,12 +8,13 @@
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
+#include "content/common/content_export.h"
 
 namespace content {
 
 // The type of a Java value. A light-weight enum-like structure intended for
 // use by value and in STL containers.
-struct JavaType {
+struct CONTENT_EXPORT JavaType {
   JavaType();
   JavaType(const JavaType& other);
   ~JavaType();
@@ -23,6 +24,11 @@ struct JavaType {
   // 'binary name'.
   static JavaType CreateFromBinaryName(const std::string& binary_name);
 
+  // JNIName is used with FindClass.
+  std::string JNIName() const;
+  // JNISignature is used for creating method signatures.
+  std::string JNISignature() const;
+
   enum Type {
     TypeBoolean,
     TypeByte,
@@ -43,6 +49,7 @@ struct JavaType {
 
   Type type;
   scoped_ptr<JavaType> inner_type;  // Used for TypeArray only.
+  std::string class_jni_name;  // Used for TypeString and TypeObject only.
 };
 
 }  // namespace content

content/browser/android/java/java_type_unittest.cc

+// Copyright 2014 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/android/java/java_type.h"
+
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace content {
+
+class JavaTypeTest : public testing::Test {
+};
+
+TEST_F(JavaTypeTest, ScalarTypes) {
+  struct {
+    const char* binary_type;
+    JavaType::Type java_type;
+    const char* jni_name;
+    const char* jni_signature;
+  } scalar_types[] = {
+    {"boolean", JavaType::TypeBoolean, "Z", "Z"},
+    {"byte", JavaType::TypeByte, "B", "B"},
+    {"char", JavaType::TypeChar, "C", "C"},
+    {"short", JavaType::TypeShort, "S", "S"},
+    {"int", JavaType::TypeInt, "I", "I"},
+    {"long", JavaType::TypeLong, "J", "J"},
+    {"float", JavaType::TypeFloat, "F", "F"},
+    {"double", JavaType::TypeDouble, "D", "D"},
+    {"void", JavaType::TypeVoid, "V", "V"},
+    {"java.lang.String", JavaType::TypeString, "java/lang/String",
+     "Ljava/lang/String;"},
+    {"java.lang.Object", JavaType::TypeObject, "java/lang/Object",
+     "Ljava/lang/Object;"},
+    {"my.nested.Type$Foo", JavaType::TypeObject, "my/nested/Type$Foo",
+     "Lmy/nested/Type$Foo;"}};
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(scalar_types); ++i) {
+    JavaType jt =
+        JavaType::CreateFromBinaryName(scalar_types[i].binary_type);
+    EXPECT_EQ(scalar_types[i].java_type, jt.type);
+    EXPECT_FALSE(jt.inner_type);
+    EXPECT_EQ(scalar_types[i].jni_name, jt.JNIName());
+    EXPECT_EQ(scalar_types[i].jni_signature, jt.JNISignature());
+  }
+}
+
+TEST_F(JavaTypeTest, ArrayTypes) {
+  JavaType array_of_boolean = JavaType::CreateFromBinaryName("[Z");
+  EXPECT_EQ(JavaType::TypeArray, array_of_boolean.type);
+  EXPECT_TRUE(array_of_boolean.inner_type);
+  EXPECT_EQ(JavaType::TypeBoolean, array_of_boolean.inner_type->type);
+  EXPECT_FALSE(array_of_boolean.inner_type->inner_type);
+  EXPECT_EQ("[Z", array_of_boolean.JNIName());
+  EXPECT_EQ("[Z", array_of_boolean.JNISignature());
+
+  JavaType array_of_boolean_2d = JavaType::CreateFromBinaryName("[[Z");
+  EXPECT_EQ(JavaType::TypeArray, array_of_boolean_2d.type);
+  EXPECT_TRUE(array_of_boolean_2d.inner_type);
+  EXPECT_EQ(JavaType::TypeArray, array_of_boolean_2d.inner_type->type);
+  EXPECT_TRUE(array_of_boolean_2d.inner_type->inner_type);
+  EXPECT_EQ(JavaType::TypeBoolean,
+            array_of_boolean_2d.inner_type->inner_type->type);
+  EXPECT_FALSE(array_of_boolean_2d.inner_type->inner_type->inner_type);
+  EXPECT_EQ("[[Z", array_of_boolean_2d.JNIName());
+  EXPECT_EQ("[[Z", array_of_boolean_2d.JNISignature());
+
+  JavaType array_of_string =
+      JavaType::CreateFromBinaryName("[Ljava.lang.String;");
+  EXPECT_EQ(JavaType::TypeArray, array_of_string.type);
+  EXPECT_TRUE(array_of_string.inner_type);
+  EXPECT_EQ(JavaType::TypeString, array_of_string.inner_type->type);
+  EXPECT_FALSE(array_of_string.inner_type->inner_type);
+  EXPECT_EQ("[Ljava/lang/String;", array_of_string.JNIName());
+  EXPECT_EQ("[Ljava/lang/String;", array_of_string.JNISignature());
+
+  JavaType array_of_object =
+      JavaType::CreateFromBinaryName("[Ljava.lang.Object;");
+  EXPECT_EQ(JavaType::TypeArray, array_of_object.type);
+  EXPECT_TRUE(array_of_object.inner_type);
+  EXPECT_EQ(JavaType::TypeObject, array_of_object.inner_type->type);
+  EXPECT_FALSE(array_of_object.inner_type->inner_type);
+  EXPECT_EQ("[Ljava/lang/Object;", array_of_object.JNIName());
+  EXPECT_EQ("[Ljava/lang/Object;", array_of_object.JNISignature());
+}
+
+}  // namespace content

content/common/android/gin_java_bridge_errors.cc

@@ -22,6 +22,9 @@ const char* GinJavaBridgeErrorToString(GinJavaBridgeError error) {
       return "Access to java.lang.Object.getClass is blocked";
     case kGinJavaBridgeJavaExceptionRaised:
       return "Java exception was raised during method invocation";
+    case kGinJavaBridgeNonAssignableTypes:
+      return "The type of the object passed to the method is incompatible "
+          "with the type of method's argument";
   }
   NOTREACHED();
   return "Unknown error";

content/common/android/gin_java_bridge_errors.h

@@ -16,6 +16,7 @@ enum GinJavaBridgeError {
   kGinJavaBridgeMethodNotFound,
   kGinJavaBridgeAccessToObjectGetClassIsBlocked,
   kGinJavaBridgeJavaExceptionRaised,
+  kGinJavaBridgeNonAssignableTypes,
 };
 
 CONTENT_EXPORT const char* GinJavaBridgeErrorToString(GinJavaBridgeError error);

content/content_tests.gypi

@@ -890,6 +890,7 @@
         ['OS == "android"', {
           'sources': [
             'browser/android/java/gin_java_method_invocation_helper_unittest.cc',
+            'browser/android/java/java_type_unittest.cc',
             'browser/android/java/jni_helper_unittest.cc',
             'renderer/java/gin_java_bridge_value_converter_unittest.cc',
           ],

content/public/android/javatests/src/org/chromium/content/browser/JavaBridgeCoercionTest.java

@@ -153,11 +153,39 @@ public class JavaBridgeCoercionTest extends JavaBridgeTestBase {
 
     private TestObject mTestObject;
 
+    private class TestController extends Controller {
+        private boolean mBooleanValue;
+
+        public synchronized void setBooleanValue(boolean x) {
+            mBooleanValue = x;
+            notifyResultIsReady();
+        }
+        public synchronized boolean waitForBooleanValue() {
+            waitForResult();
+            return mBooleanValue;
+        }
+    }
+
+    TestController mTestController;
+
+    // Note that this requires that we can pass a JavaScript boolean to Java.
+    private void assertRaisesException(String script) throws Throwable {
+        executeJavaScript("try {" +
+                          script + ";" +
+                          "  testController.setBooleanValue(false);" +
+                          "} catch (exception) {" +
+                          "  testController.setBooleanValue(true);" +
+                          "}");
+        assertTrue(mTestController.waitForBooleanValue());
+    }
+
     @Override
     protected void setUp() throws Exception {
         super.setUp();
         mTestObject = new TestObject();
         setUpContentView(mTestObject, "testObject");
+        mTestController = new TestController();
+        setUpContentView(mTestController, "testController");
     }
 
     // Test passing a 32-bit integer JavaScript number to a method of an
@@ -539,14 +567,11 @@ public class JavaBridgeCoercionTest extends JavaBridgeTestBase {
         executeJavaScript("testObject.setObjectValue(testObject.getCustomTypeInstance());");
         assertTrue(mTestObject.getCustomTypeInstance() == mTestObject.waitForObjectValue());
 
-        executeJavaScript("testObject.setCustomTypeValue(testObject.getObjectInstance());");
-        assertTrue(mTestObject.getObjectInstance() == mTestObject.waitForCustomTypeValue());
+        assertRaisesException("testObject.setCustomTypeValue(testObject.getObjectInstance());");
         executeJavaScript("testObject.setCustomTypeValue(testObject.getCustomTypeInstance());");
         assertTrue(mTestObject.getCustomTypeInstance() == mTestObject.waitForCustomTypeValue());
-        // LIVECONNECT_COMPLIANCE: Should raise a JavaScript exception, as the types are unrelated.
-        executeJavaScript("testObject.setCustomTypeValue(testObject.getCustomType2Instance());");
-        assertTrue(mTestObject.getCustomType2Instance() ==
-                (Object)mTestObject.waitForCustomTypeValue());
+        assertRaisesException(
+                "testObject.setCustomTypeValue(testObject.getCustomType2Instance());");
 
         // LIVECONNECT_COMPLIANCE: Should call toString() on object.
         executeJavaScript("testObject.setStringValue(testObject.getObjectInstance());");