[Android] Fix method invocation and wrappers cleanup handling in Java Bridge

While investigating flakiness of JavaBridgeBasicsTest#testRemovalNotReflectedUntilReload
I realized that we don't handle invocation of injected objects' methods
properly -- InvokeMethod was bound to a GinJavaBridgeObject instance and was
making two wrong assumptions:
  - that the method can only be called for the wrapper where the method has been
    obtained from -- not true, as it is possible to use `call()` method of
    `Function` to pass a different `this` object;
  - that it is safe to store an unretained pointer to GinJavaBridgeObject --
    it's not, as it is possible to retain a function representing method
    separately from the injected object, which can be collected (if it's not a
    named object).

The issue that was causing the test flakiness originated from the fact that
DidClearWindowObject can be called several times during the page lifecycle.
GinJavaBridgeDispatcher injects new wrappers each time, this isn't a problem
on its own, but leads to premature issuing of wrapper deletion notificiations
when the wrappers of the old generation got cleaned up.

BUG=468679

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

Cr-Commit-Position: refs/heads/master@{#321567}

content/content_renderer.gypi

@@ -219,6 +219,8 @@
       'renderer/java/gin_java_bridge_object.h',
       'renderer/java/gin_java_bridge_value_converter.cc',
       'renderer/java/gin_java_bridge_value_converter.h',
+      'renderer/java/gin_java_function_invocation_helper.cc',
+      'renderer/java/gin_java_function_invocation_helper.h',
       'renderer/manifest/manifest_manager.cc',
       'renderer/manifest/manifest_manager.h',
       'renderer/manifest/manifest_parser.cc',
@@ -783,10 +785,8 @@
         'renderer/java/gin_java_bridge_object.h',
         'renderer/java/gin_java_bridge_value_converter.cc',
         'renderer/java/gin_java_bridge_value_converter.h',
-        'renderer/java/java_bridge_channel.cc',
-        'renderer/java/java_bridge_channel.h',
-        'renderer/java/java_bridge_dispatcher.cc',
-        'renderer/java/java_bridge_dispatcher.h',
+        'renderer/java/gin_java_function_invocation_helper.cc',
+        'renderer/java/gin_java_function_invocation_helper.h',
       ],
     }],
     # TODO(jrg): remove the OS=="android" section?

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

@@ -6,7 +6,6 @@ package org.chromium.content.browser;
 
 import static org.chromium.base.test.util.ScalableTimeout.scaleTimeout;
 
-import android.test.FlakyTest;
 import android.test.suitebuilder.annotation.SmallTest;
 
 import junit.framework.Assert;
@@ -180,12 +179,8 @@ public class JavaBridgeBasicsTest extends JavaBridgeTestBase {
         assertEquals("object", executeJavaScriptAndGetStringResult("typeof testObject"));
     }
 
-    /**
-     * @SmallTest
-     * @Feature({"AndroidWebView", "Android-JavaBridge"})
-     * http://crbug.com/468679
-     */
-    @FlakyTest
+    @SmallTest
+    @Feature({"AndroidWebView", "Android-JavaBridge"})
     public void testRemovalNotReflectedUntilReload() throws Throwable {
         injectObjectAndReload(new Object() {
             public void method() {
@@ -259,6 +254,29 @@ public class JavaBridgeBasicsTest extends JavaBridgeTestBase {
         assertRaisesException("testObject.method()");
     }
 
+    @SmallTest
+    @Feature({"AndroidWebView", "Android-JavaBridge"})
+    public void testCallingAsConstructorRaisesException() throws Throwable {
+        assertRaisesException("new testController.setStringValue('foo')");
+    }
+
+    @SmallTest
+    @Feature({"AndroidWebView", "Android-JavaBridge"})
+    public void testCallingOnNonInjectedObjectRaisesException() throws Throwable {
+        assertRaisesException("testController.setStringValue.call({}, 'foo')");
+    }
+
+    @SmallTest
+    @Feature({"AndroidWebView", "Android-JavaBridge"})
+    public void testCallingOnInstanceOfOtherClassRaisesException() throws Throwable {
+        injectObjectAndReload(new Object(), "testObject");
+        assertEquals("object", executeJavaScriptAndGetStringResult("typeof testObject"));
+        assertEquals("object", executeJavaScriptAndGetStringResult("typeof testController"));
+        assertEquals("function",
+                executeJavaScriptAndGetStringResult("typeof testController.setStringValue"));
+        assertRaisesException("testController.setStringValue.call(testObject, 'foo')");
+    }
+
     // Note that this requires that we can pass a JavaScript string to Java.
     @SmallTest
     @Feature({"AndroidWebView", "Android-JavaBridge"})
@@ -1005,4 +1023,38 @@ public class JavaBridgeBasicsTest extends JavaBridgeTestBase {
         injectObjectAndReload(new Test(42), "testObject");
         assertEquals("42", executeJavaScriptAndGetStringResult("testObject.getValue()"));
     }
+
+    @SmallTest
+    @Feature({"AndroidWebView", "Android-JavaBridge"})
+    public void testMethodCalledOnAnotherInstance() throws Throwable {
+        class TestObject {
+            private int mIndex;
+            TestObject(int index) {
+                mIndex = index;
+            }
+            public void method() {
+                mTestController.setIntValue(mIndex);
+            }
+        }
+        final TestObject testObject1 = new TestObject(1);
+        final TestObject testObject2 = new TestObject(2);
+        runTestOnUiThread(new Runnable() {
+            @Override
+            public void run() {
+                getContentViewCore().addPossiblyUnsafeJavascriptInterface(
+                        testObject1, "testObject1", null);
+                getContentViewCore().addPossiblyUnsafeJavascriptInterface(
+                        testObject2, "testObject2", null);
+                getContentViewCore().getWebContents().getNavigationController().reload(true);
+            }
+        });
+        executeJavaScript("testObject1.method()");
+        assertEquals(1, mTestController.waitForIntValue());
+        executeJavaScript("testObject2.method()");
+        assertEquals(2, mTestController.waitForIntValue());
+        executeJavaScript("testObject1.method.call(testObject2)");
+        assertEquals(2, mTestController.waitForIntValue());
+        executeJavaScript("testObject2.method.call(testObject1)");
+        assertEquals(1, mTestController.waitForIntValue());
+    }
 }

content/renderer/BUILD.gn

@@ -98,6 +98,8 @@ source_set("renderer") {
       "java/gin_java_bridge_object.h",
       "java/gin_java_bridge_value_converter.cc",
       "java/gin_java_bridge_value_converter.h",
+      "java/gin_java_function_invocation_helper.cc",
+      "java/gin_java_function_invocation_helper.h",
     ]
   }
 

content/renderer/java/gin_java_bridge_dispatcher.cc

@@ -123,9 +123,11 @@ GinJavaBridgeObject* GinJavaBridgeDispatcher::GetObject(ObjectID object_id) {
   return result;
 }
 
-void GinJavaBridgeDispatcher::OnGinJavaBridgeObjectDeleted(ObjectID object_id) {
-  if (!objects_.Lookup(object_id))
-    return;
+void GinJavaBridgeDispatcher::OnGinJavaBridgeObjectDeleted(
+    GinJavaBridgeObject* object) {
+  int object_id = object->object_id();
+  // Ignore cleaning up of old object wrappers.
+  if (objects_.Lookup(object_id) != object) return;
   objects_.Remove(object_id);
   render_frame()->Send(
       new GinJavaBridgeHostMsg_ObjectWrapperDeleted(routing_id(), object_id));

content/renderer/java/gin_java_bridge_dispatcher.h

@@ -54,7 +54,7 @@ class GinJavaBridgeDispatcher
                                            const base::ListValue& arguments,
                                            GinJavaBridgeError* error);
   GinJavaBridgeObject* GetObject(ObjectID object_id);
-  void OnGinJavaBridgeObjectDeleted(ObjectID object_id);
+  void OnGinJavaBridgeObjectDeleted(GinJavaBridgeObject* object);
 
  private:
   void OnAddNamedObject(const std::string& name,

content/renderer/java/gin_java_bridge_object.cc

@@ -4,25 +4,13 @@
 
 #include "content/renderer/java/gin_java_bridge_object.h"
 
-#include "base/strings/utf_string_conversions.h"
-#include "content/common/android/gin_java_bridge_errors.h"
-#include "content/common/android/gin_java_bridge_value.h"
-#include "content/public/child/v8_value_converter.h"
-#include "content/renderer/java/gin_java_bridge_value_converter.h"
+#include "content/renderer/java/gin_java_function_invocation_helper.h"
 #include "gin/function_template.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebKit.h"
 
 namespace content {
 
-namespace {
-
-const char kMethodInvocationErrorMessage[] =
-    "Java bridge method invocation error";
-
-}  // namespace
-
-
 // static
 GinJavaBridgeObject* GinJavaBridgeObject::InjectNamed(
     blink::WebFrame* frame,
@@ -66,13 +54,12 @@ GinJavaBridgeObject::GinJavaBridgeObject(
     : gin::NamedPropertyInterceptor(isolate, this),
       dispatcher_(dispatcher),
       object_id_(object_id),
-      converter_(new GinJavaBridgeValueConverter()),
       template_cache_(isolate) {
 }
 
 GinJavaBridgeObject::~GinJavaBridgeObject() {
   if (dispatcher_)
-    dispatcher_->OnGinJavaBridgeObjectDeleted(object_id_);
+    dispatcher_->OnGinJavaBridgeObjectDeleted(this);
 }
 
 gin::ObjectTemplateBuilder GinJavaBridgeObject::GetObjectTemplateBuilder(
@@ -113,72 +100,13 @@ v8::Local<v8::FunctionTemplate> GinJavaBridgeObject::GetFunctionTemplate(
   if (!function_template.IsEmpty())
     return function_template;
   function_template = gin::CreateFunctionTemplate(
-      isolate, base::Bind(&GinJavaBridgeObject::InvokeMethod,
-                          base::Unretained(this), name));
+      isolate, base::Bind(&GinJavaFunctionInvocationHelper::Invoke,
+                          base::Owned(new GinJavaFunctionInvocationHelper(
+                              name, dispatcher_))));
   template_cache_.Set(name, function_template);
   return function_template;
 }
 
-v8::Handle<v8::Value> GinJavaBridgeObject::InvokeMethod(
-    const std::string& name,
-    gin::Arguments* args) {
-  if (!dispatcher_) {
-    args->isolate()->ThrowException(v8::Exception::Error(gin::StringToV8(
-        args->isolate(), kMethodInvocationErrorMessage)));
-    return v8::Undefined(args->isolate());
-  }
-
-  base::ListValue arguments;
-  {
-    v8::HandleScope handle_scope(args->isolate());
-    v8::Handle<v8::Context> context = args->isolate()->GetCurrentContext();
-    v8::Handle<v8::Value> val;
-    while (args->GetNext(&val)) {
-      scoped_ptr<base::Value> arg(converter_->FromV8Value(val, context));
-      if (arg.get()) {
-        arguments.Append(arg.release());
-      } else {
-        arguments.Append(base::Value::CreateNullValue());
-      }
-    }
-  }
-
-  GinJavaBridgeError error;
-  scoped_ptr<base::Value> result = dispatcher_->InvokeJavaMethod(
-      object_id_, name, arguments, &error);
-  if (!result.get()) {
-    args->isolate()->ThrowException(v8::Exception::Error(gin::StringToV8(
-        args->isolate(), GinJavaBridgeErrorToString(error))));
-    return v8::Undefined(args->isolate());
-  }
-  if (!result->IsType(base::Value::TYPE_BINARY)) {
-    return converter_->ToV8Value(result.get(),
-                                 args->isolate()->GetCurrentContext());
-  }
-
-  scoped_ptr<const GinJavaBridgeValue> gin_value =
-      GinJavaBridgeValue::FromValue(result.get());
-  if (gin_value->IsType(GinJavaBridgeValue::TYPE_OBJECT_ID)) {
-    GinJavaBridgeObject* result = NULL;
-    GinJavaBridgeDispatcher::ObjectID object_id;
-    if (gin_value->GetAsObjectID(&object_id)) {
-      result = dispatcher_->GetObject(object_id);
-    }
-    if (result) {
-      gin::Handle<GinJavaBridgeObject> controller =
-          gin::CreateHandle(args->isolate(), result);
-      if (controller.IsEmpty())
-        return v8::Undefined(args->isolate());
-      return controller.ToV8();
-    }
-  } else if (gin_value->IsType(GinJavaBridgeValue::TYPE_NONFINITE)) {
-    float float_value;
-    gin_value->GetAsNonFinite(&float_value);
-    return v8::Number::New(args->isolate(), float_value);
-  }
-  return v8::Undefined(args->isolate());
-}
-
 gin::WrapperInfo GinJavaBridgeObject::kWrapperInfo = {gin::kEmbedderNativeGin};
 
 }  // namespace content

content/renderer/java/gin_java_bridge_object.h

@@ -7,7 +7,6 @@
 
 #include <map>
 
-#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "content/renderer/java/gin_java_bridge_dispatcher.h"
 #include "gin/handle.h"
@@ -22,8 +21,6 @@ class WebFrame;
 
 namespace content {
 
-class GinJavaBridgeValueConverter;
-
 class GinJavaBridgeObject : public gin::Wrappable<GinJavaBridgeObject>,
                             public gin::NamedPropertyInterceptor {
  public:
@@ -58,12 +55,9 @@ class GinJavaBridgeObject : public gin::Wrappable<GinJavaBridgeObject>,
 
   v8::Local<v8::FunctionTemplate> GetFunctionTemplate(v8::Isolate* isolate,
                                                       const std::string& name);
-  v8::Handle<v8::Value> InvokeMethod(const std::string& name,
-                                     gin::Arguments* args);
 
   base::WeakPtr<GinJavaBridgeDispatcher> dispatcher_;
   GinJavaBridgeDispatcher::ObjectID object_id_;
-  scoped_ptr<GinJavaBridgeValueConverter> converter_;
   std::map<std::string, bool> known_methods_;
   v8::StdPersistentValueMap<std::string, v8::FunctionTemplate> template_cache_;
 

content/renderer/java/gin_java_function_invocation_helper.cc

+// Copyright 2015 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/renderer/java/gin_java_function_invocation_helper.h"
+
+#include "content/common/android/gin_java_bridge_errors.h"
+#include "content/common/android/gin_java_bridge_value.h"
+#include "content/public/child/v8_value_converter.h"
+#include "content/renderer/java/gin_java_bridge_object.h"
+#include "content/renderer/java/gin_java_bridge_value_converter.h"
+
+namespace content {
+
+namespace {
+
+const char kMethodInvocationAsConstructorDisallowed[] =
+    "Java bridge method can't be invoked as a constructor";
+const char kMethodInvocationOnNonInjectedObjectDisallowed[] =
+    "Java bridge method can't be invoked on a non-injected object";
+const char kMethodInvocationErrorMessage[] =
+    "Java bridge method invocation error";
+
+}  // namespace
+
+GinJavaFunctionInvocationHelper::GinJavaFunctionInvocationHelper(
+    const std::string& method_name,
+    const base::WeakPtr<GinJavaBridgeDispatcher>& dispatcher)
+    : method_name_(method_name),
+      dispatcher_(dispatcher),
+      converter_(new GinJavaBridgeValueConverter()) {
+}
+
+GinJavaFunctionInvocationHelper::~GinJavaFunctionInvocationHelper() {
+}
+
+v8::Handle<v8::Value> GinJavaFunctionInvocationHelper::Invoke(
+    gin::Arguments* args) {
+  if (!dispatcher_) {
+    args->isolate()->ThrowException(v8::Exception::Error(gin::StringToV8(
+        args->isolate(), kMethodInvocationErrorMessage)));
+    return v8::Undefined(args->isolate());
+  }
+
+  if (args->IsConstructCall()) {
+    args->isolate()->ThrowException(v8::Exception::Error(gin::StringToV8(
+        args->isolate(), kMethodInvocationAsConstructorDisallowed)));
+    return v8::Undefined(args->isolate());
+  }
+
+  content::GinJavaBridgeObject* object = NULL;
+  if (!args->GetHolder(&object) || !object) {
+    args->isolate()->ThrowException(v8::Exception::Error(gin::StringToV8(
+        args->isolate(), kMethodInvocationOnNonInjectedObjectDisallowed)));
+    return v8::Undefined(args->isolate());
+  }
+
+  base::ListValue arguments;
+  {
+    v8::HandleScope handle_scope(args->isolate());
+    v8::Handle<v8::Context> context = args->isolate()->GetCurrentContext();
+    v8::Handle<v8::Value> val;
+    while (args->GetNext(&val)) {
+      scoped_ptr<base::Value> arg(converter_->FromV8Value(val, context));
+      if (arg.get()) {
+        arguments.Append(arg.release());
+      } else {
+        arguments.Append(base::Value::CreateNullValue());
+      }
+    }
+  }
+
+  GinJavaBridgeError error;
+  scoped_ptr<base::Value> result = dispatcher_->InvokeJavaMethod(
+      object->object_id(), method_name_, arguments, &error);
+  if (!result.get()) {
+    args->isolate()->ThrowException(v8::Exception::Error(gin::StringToV8(
+        args->isolate(), GinJavaBridgeErrorToString(error))));
+    return v8::Undefined(args->isolate());
+  }
+  if (!result->IsType(base::Value::TYPE_BINARY)) {
+    return converter_->ToV8Value(result.get(),
+                                 args->isolate()->GetCurrentContext());
+  }
+
+  scoped_ptr<const GinJavaBridgeValue> gin_value =
+      GinJavaBridgeValue::FromValue(result.get());
+  if (gin_value->IsType(GinJavaBridgeValue::TYPE_OBJECT_ID)) {
+    GinJavaBridgeObject* result = NULL;
+    GinJavaBridgeDispatcher::ObjectID object_id;
+    if (gin_value->GetAsObjectID(&object_id)) {
+      result = dispatcher_->GetObject(object_id);
+    }
+    if (result) {
+      gin::Handle<GinJavaBridgeObject> controller =
+          gin::CreateHandle(args->isolate(), result);
+      if (controller.IsEmpty())
+        return v8::Undefined(args->isolate());
+      return controller.ToV8();
+    }
+  } else if (gin_value->IsType(GinJavaBridgeValue::TYPE_NONFINITE)) {
+    float float_value;
+    gin_value->GetAsNonFinite(&float_value);
+    return v8::Number::New(args->isolate(), float_value);
+  }
+  return v8::Undefined(args->isolate());
+}
+
+}  // namespace content

content/renderer/java/gin_java_function_invocation_helper.h

+// Copyright 2015 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.
+
+#ifndef CONTENT_RENDERER_JAVA_GIN_JAVA_FUNCTION_INVOCATION_HELPER_H_
+#define CONTENT_RENDERER_JAVA_GIN_JAVA_FUNCTION_INVOCATION_HELPER_H_
+
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
+#include "content/renderer/java/gin_java_bridge_dispatcher.h"
+#include "gin/arguments.h"
+#include "gin/handle.h"
+
+namespace content {
+
+class GinJavaBridgeValueConverter;
+
+class GinJavaFunctionInvocationHelper {
+ public:
+  GinJavaFunctionInvocationHelper(
+      const std::string& method_name,
+      const base::WeakPtr<GinJavaBridgeDispatcher>& dispatcher);
+  ~GinJavaFunctionInvocationHelper();
+
+  v8::Handle<v8::Value> Invoke(gin::Arguments* args);
+
+ private:
+  std::string method_name_;
+  base::WeakPtr<GinJavaBridgeDispatcher> dispatcher_;
+  scoped_ptr<GinJavaBridgeValueConverter> converter_;
+
+  DISALLOW_COPY_AND_ASSIGN(GinJavaFunctionInvocationHelper);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_RENDERER_JAVA_GIN_JAVA_FUNCTION_INVOCATION_HELPER_H_