[Android] Implement renderer side of Gin Java Bridge

[Android] Implement renderer side of Gin Java Bridge

This change adds GinJavaBridgeObject and GinJavaBridgeDispatcher classes.

GinJavaBridgeDispatcher is a per-RenderFrame manager of injected
objects. It interacts with the browser side.

GinJavaBridgeObject is a Gin-based wrapper injected into page's context
that exposes methods of the corresponding Java object and receives
method invocation requests from JavaScript.

BUG=355644
R=jochen@chromium.org, thakis@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272997

[mnaganov (inactive), 2014-04-28 14:46:31]

Hi Jochen,

Can you please take a look at this to check Gin and V8 usage? See
go/clank/engineering/legacy-webview/java-bridge for a design reference.

Please note that this change depends on
https://codereview.chromium.org/249473002/ which is currently under a security
review, but I'm not expecting much changes there.

[jochen (gone - plz use gerrit), 2014-04-29 13:50:15]

overall looks good

is it possible to add unit tests for the serialization strategy?

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
File content/renderer/java/gin_java_bridge_dispatcher.cc (right):

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
content/renderer/java/gin_java_bridge_dispatcher.cc:34: void
GinJavaBridgeDispatcher::DidClearWindowObject(int world_id) {
for world_id != 0, you should exit early.

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
File content/renderer/java/gin_java_bridge_dispatcher.h (right):

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
content/renderer/java/gin_java_bridge_dispatcher.h:42:
GinJavaBridgeDispatcher(RenderFrame* render_frame);
explicit

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
File content/renderer/java/gin_java_bridge_object.cc (right):

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
content/renderer/java/gin_java_bridge_object.cc:64: gin::CreateHandle(isolate,
object);
return false here if controller.IsEmpty()

[mnaganov (inactive), 2014-04-30 16:46:25]

> is it possible to add unit tests for the serialization strategy?

That's a good idea! I had to split the value converter code from
GinJavaBridgeObject, which is a win.

I'm having troubles adding a test for typed array conversions. Maybe, I don't
actually need them, as I have end-to-end tests in Java for typed arrays. But
just in case, the details are below.

The conversion code crashes inside bindings code on a call chain starting from
WebArrayBufferView::createFromV8Value. I actually already had this issue when I
was trying to extend tests for V8ValueConverter to verify typed arrays
conversions. At that time, I was supposing that this is because Blink isn't
initalized in the test, but here in my test, I have tried subclassing from
RenderViewTest and found that this conjecture isn't true, as RenderViewTest was
crashing because it was trying to initialize Blink twice. So I'm not really
sure, what I'm missing.

This is the sequence of calls that leads to crash:

blink::WebArrayBufferView::createFromV8Value(v8::Handle<v8::Value>)
 -> WebCore::V8ArrayBufferView::toNative(v8::Handle<v8::Object>)
 -> WebCore::V8TypedArray<WTF::Int8Array>::toNative(v8::Handle<v8::Object>)
 -> WebCore::V8ArrayBuffer::toNative(v8::Handle<v8::Object>)
 -> void WebCore::DOMDataStore::setWrapper<WebCore::V8ArrayBuffer,
WTF::ArrayBuffer>(WTF::ArrayBuffer*, v8::Handle<v8::Object>, v8::Isolate*,
WebCore::WrapperConfiguration const&)
 -> WebCore::DOMDataStore::current(v8::Isolate*)
 -> WebCore::DOMWrapperWorld::current(v8::Isolate*)
 -> v8::Local<v8::Context>::New(v8::Isolate*, v8::Context*)
  return Local<T>(reinterpret_cast<T*>(HandleScope::CreateHandle(
      reinterpret_cast<internal::Isolate*>(isolate), *p)));

  |p| turns out to be an invalid pointer

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
File content/renderer/java/gin_java_bridge_dispatcher.cc (right):

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
content/renderer/java/gin_java_bridge_dispatcher.cc:34: void
GinJavaBridgeDispatcher::DidClearWindowObject(int world_id) {
On 2014/04/29 13:50:16, jochen (OOO Wed-Thu) wrote:
> for world_id != 0, you should exit early.

Done.

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
File content/renderer/java/gin_java_bridge_dispatcher.h (right):

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
content/renderer/java/gin_java_bridge_dispatcher.h:42:
GinJavaBridgeDispatcher(RenderFrame* render_frame);
On 2014/04/29 13:50:16, jochen (OOO Wed-Thu) wrote:
> explicit

Done.

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
File content/renderer/java/gin_java_bridge_object.cc (right):

https://codereview.chromium.org/259033002/diff/1/content/renderer/java/gin_ja...
content/renderer/java/gin_java_bridge_object.cc:64: gin::CreateHandle(isolate,
object);
On 2014/04/29 13:50:16, jochen (OOO Wed-Thu) wrote:
> return false here if controller.IsEmpty()

Done here and in line 164.

[jochen (gone - plz use gerrit), 2014-05-05 10:55:48]

the reason your tests crash is because the webarraybufferview assumes that blink
is up and running, however, in your test you create the v8 instance yourself.

Either you have to create a blink instance, or you could use the gin array
buffers (in gin/array_buffer.h). You'll need to create a gin::IsolateHolder()
then in the unit test and use it's v8::Isolate instead of using
v8::Isolate::GetCurrent().

Instead of managing raw pointers to the java bridge objects, you should use a
v8::PersistentValueMap with weak persistent handles, see e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/v8/test/cctest/tes...
- creating multiple gin wrappers for a single object will lead to crashes

https://codereview.chromium.org/259033002/diff/20001/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/259033002/diff/20001/base/id_map.h#newcode36
base/id_map.h:36: typedef int32 KeyType;
nit. empty line after this line

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
File content/renderer/java/gin_java_bridge_dispatcher.cc (right):

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_dispatcher.cc:46: if (object) {
I'm not sure I understand this. InjectExisting will create a new wrapper, so you
don't get the properties.

Also, the CreateHandle() call will take ownership of the object - if you call
CreateHandle() multiple times, you'll end up with double-free and use-after-free
behavior

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
File content/renderer/java/gin_java_bridge_object.cc (right):

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_object.cc:62: gin::CreateHandle(isolate,
object);
CreateHandle will delete object if it fails to create controller... so if you
return in line 64, you'll have a double-free in line 36

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_object.cc:109:
dispatcher_->GetJavaMethods(object_id_, &method_names);
nit. no { }

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
File content/renderer/java/gin_java_bridge_object.h (right):

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_object.h:41: static GinJavaBridgeObject*
Inject(
nit. static members should come first (before object_id() in this case)

[mnaganov (inactive), 2014-05-06 10:54:15]

> the reason your tests crash is because the webarraybufferview assumes that
blink
> is up and running, however, in your test you create the v8 instance yourself.

> Either you have to create a blink instance, or you could use the gin array
> buffers (in gin/array_buffer.h). You'll need to create a gin::IsolateHolder()
> then in the unit test and use it's v8::Isolate instead of using
> v8::Isolate::GetCurrent().

Thanks, I'll try that! Let's sort out other issues you have pointed out first,
though.

> Instead of managing raw pointers to the java bridge objects, you should use a
> v8::PersistentValueMap with weak persistent handles, see e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/test/cctest/tes...
> - creating multiple gin wrappers for a single object will lead to crashes

In fact, I'm not "managing" raw pointers -- I understand that WrappableBase
controls the lifetime of itself based on the lifetime of the corresponding V8
wrapper object. I never delete GinJavaBridgeObject instances myself, except, by
misunderstanding, in the case when wrapper creation has been failed, thanks for
pointing this out!

GinJavaBridgeObjects report to GinJavaBridgeDispatcher whenever they get
deleted, so the map should not contain stale pointers.

My point is that as WrappableBase already holds a persistent weak handle to V8
wrapper object, and monitors wrapper being collected event, why should I create
another persistent handle?

https://codereview.chromium.org/259033002/diff/20001/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/259033002/diff/20001/base/id_map.h#newcode36
base/id_map.h:36: typedef int32 KeyType;
On 2014/05/05 10:55:49, jochen wrote:
> nit. empty line after this line

Done.

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
File content/renderer/java/gin_java_bridge_dispatcher.cc (right):

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_dispatcher.cc:46: if (object) {
On 2014/05/05 10:55:49, jochen wrote:
> I'm not sure I understand this. InjectExisting will create a new wrapper, so
you
> don't get the properties.
> 
> Also, the CreateHandle() call will take ownership of the object - if you call
> CreateHandle() multiple times, you'll end up with double-free and
use-after-free
> behavior

This is my understanding of how this works, please correct me if I'm wrong.

WrappableBase holds a persistent weak handle to V8 wrapper object, and it
destroys itself when it gets notified that the wrapper object has been
collected.

When one calls gin::CreateHandle, this comes down to
WrappableBase::GetWrapperImpl, which on the first call creates the V8 wrapper
object and creates a weak handle, and on subsequent calls simply creates a local
V8 handle from the persistent weak one WrappableBase holds.

Since gin::Handle is just a thin wrapper over V8's handle, and it doesn't take
ownership of the Wrappable subclass, calling CreateHandle multiple times on the
same Wrappable child simply creates new a V8 local handle each time. So I'm not
sure, how a double-free scenario can be achieved.

Now, returning to the situation that I handle in this method.
DidClearWindowObject is called on a page reload, which means, all references to
the V8 wrapper object inside V8 get dropped, and the wrapper can be collected,
if a round of GC happens after the reload. In that case, however,
GinJavaBridgeObject will be destroyed by WrappableBase, and the former will
remove itself from |objects_|. This case is handled by the 'else' clause.

But if GC didn't happen after the reload, the V8 wrapper is still alive (but
have no references inside V8), so we simply re-bind it under the same name to
the 'window' object. This situation actually happens when I'm running unit
tests.

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
File content/renderer/java/gin_java_bridge_object.cc (right):

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_object.cc:62: gin::CreateHandle(isolate,
object);
On 2014/05/05 10:55:49, jochen wrote:
> CreateHandle will delete object if it fails to create controller... so if you
> return in line 64, you'll have a double-free in line 36

Thanks for pointing this out! I missed that fact that WrappableBase deletes
itself in case of wrapper creation failure.

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_object.cc:109:
dispatcher_->GetJavaMethods(object_id_, &method_names);
On 2014/05/05 10:55:49, jochen wrote:
> nit. no { }

Done.

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
File content/renderer/java/gin_java_bridge_object.h (right):

https://codereview.chromium.org/259033002/diff/20001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_object.h:41: static GinJavaBridgeObject*
Inject(
On 2014/05/05 10:55:49, jochen wrote:
> nit. static members should come first (before object_id() in this case)

I think that is only for static data members. From the style guide:

"Within each section, the declarations generally should be in the following
order:

Typedefs and Enums
Constants (static const data members)
Constructors
Destructor
Methods, including static methods
Data Members (except static const data members)"

Thus, indeed, kWrapperInfo must come first, but static methods can go along with
instance methods.

[jochen (gone - plz use gerrit), 2014-05-06 11:54:20]

On 2014/05/06 10:54:15, Mikhail Naganov (Cr) wrote:
> This is my understanding of how this works, please correct me if I'm wrong.

i double-checked and indeed, you are correct!

What is supposed to happen when the webview navigates? Since the wrapper is
probably not garbage collected right away, you might end up exposing javascript
propoerties from one context in another.

Should the objects map maybe be cleared on navigations?

[mnaganov (inactive), 2014-05-06 12:43:05]

On 2014/05/06 11:54:20, jochen wrote:
> On 2014/05/06 10:54:15, Mikhail Naganov (Cr) wrote:
> > This is my understanding of how this works, please correct me if I'm wrong.
> 
> i double-checked and indeed, you are correct!
> 
> What is supposed to happen when the webview navigates? Since the wrapper is
> probably not garbage collected right away, you might end up exposing
javascript
> propoerties from one context in another.
> 
> Should the objects map maybe be cleared on navigations?

From security and memory leaks points of view, custom properties should be
cleaned up, I agree. But the existing implementation actually preserves them.
That's basically why I have wrote this in the first place. But now I'm thinking,
I should discuss this with relevant people, whether should we eliminate this
behaviour in the existing Java Bridge.

[jochen (gone - plz use gerrit), 2014-05-06 15:57:36]

On 2014/05/06 12:43:05, Mikhail Naganov (Cr) wrote:
> On 2014/05/06 11:54:20, jochen wrote:
> > On 2014/05/06 10:54:15, Mikhail Naganov (Cr) wrote:
> > > This is my understanding of how this works, please correct me if I'm
wrong.
> > 
> > i double-checked and indeed, you are correct!
> > 
> > What is supposed to happen when the webview navigates? Since the wrapper is
> > probably not garbage collected right away, you might end up exposing
> javascript
> > propoerties from one context in another.
> > 
> > Should the objects map maybe be cleared on navigations?
> 
> From security and memory leaks points of view, custom properties should be
> cleaned up, I agree. But the existing implementation actually preserves them.
> That's basically why I have wrote this in the first place. But now I'm
thinking,
> I should discuss this with relevant people, whether should we eliminate this
> behaviour in the existing Java Bridge.

If that's possible I think that would be vastly better behavior

[mnaganov (inactive), 2014-05-15 09:36:42]

OK, I have updated the implementation to discard V8 wrapper objects on page
reloads. PTAL.

[jochen (gone - plz use gerrit), 2014-05-15 09:54:45]

much better, thank you!

Do you still have the problems with the unit tests and WebArrayBufferView?

[mnaganov (inactive), 2014-05-15 13:11:04]

On 2014/05/15 09:54:45, jochen wrote:
> much better, thank you!
> 
> Do you still have the problems with the unit tests and WebArrayBufferView?

Yes, I actually have a couple of questions here.

First, I'm not quite sure I understand, what is the scenario for using
gin::ArrayBuffer and View. Their constructors take V8 ArrayBuffer and View, this
implies I must create the V8 side objects first. But then, gin::ArrayBuffer and
View can only help in accessing arrays' data, they don't seem to have 'ToV8'
converters. Does that mean, I still need to pass V8 ArrayBufferView and V8's
typed arrays to the V8ValueConverter, and the only gin::ArrayBuffer's and View's
task in my scenario is to externalize typed arrays' data as a side effect of
creating them?

Second, I'm also not sure why, but I'm getting linker errors on getting
gin::ArrayBufferView's constructor and destructor when doing a component build.
I clearly see that gin::ArrayBufferView class is marked with GIN_EXPORT, should
that be enough? Or perhaps this is just a glitch and I should try nuking my out?
I'm building with OS=Android.

And also, as I've mentioned before, I have tried the 'create blink instance' way
by using RenderViewTest as a base class for my test (and removing the current
code for creating a V8's Context from the test), and the test was crashing
because it was somehow trying to initialize Blink twice.

[jochen (gone - plz use gerrit), 2014-05-15 14:46:41]

On 2014/05/15 13:11:04, Mikhail Naganov (Cr) wrote:
> On 2014/05/15 09:54:45, jochen wrote:
> > much better, thank you!
> > 
> > Do you still have the problems with the unit tests and WebArrayBufferView?
> 
> Yes, I actually have a couple of questions here.
> 
> First, I'm not quite sure I understand, what is the scenario for using
> gin::ArrayBuffer and View. Their constructors take V8 ArrayBuffer and View,
this
> implies I must create the V8 side objects first. But then, gin::ArrayBuffer
and
> View can only help in accessing arrays' data, they don't seem to have 'ToV8'
> converters. Does that mean, I still need to pass V8 ArrayBufferView and V8's
> typed arrays to the V8ValueConverter, and the only gin::ArrayBuffer's and
View's
> task in my scenario is to externalize typed arrays' data as a side effect of
> creating them?

The idea is to use them like this:
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/bindings/js/c...

you'd first create a v8::ArrayBuffer, and then create a wrapper which allows you
to modify the contents directly. Since you had to create the v8 version first,
you have a handle anyway, and don't need a ToV8.

However, if having a ToV8 is more convenient, we should add one

> 
> Second, I'm also not sure why, but I'm getting linker errors on getting
> gin::ArrayBufferView's constructor and destructor when doing a component
build.
> I clearly see that gin::ArrayBufferView class is marked with GIN_EXPORT,
should
> that be enough? Or perhaps this is just a glitch and I should try nuking my
out?
> I'm building with OS=Android.

the class is not correctly marked as DISALLOW_COPY_AND_ASSIGN. And the isolate
constructor for ArrayBuffer is also not defined :-/
> 
> And also, as I've mentioned before, I have tried the 'create blink instance'
way
> by using RenderViewTest as a base class for my test (and removing the current
> code for creating a V8's Context from the test), and the test was crashing
> because it was somehow trying to initialize Blink twice.

[mnaganov (inactive), 2014-05-16 11:41:08]

On 2014/05/15 14:46:41, jochen wrote:
> On 2014/05/15 13:11:04, Mikhail Naganov (Cr) wrote:
> > On 2014/05/15 09:54:45, jochen wrote:
> > > much better, thank you!
> > > 
> > > Do you still have the problems with the unit tests and WebArrayBufferView?
> > 
> > Yes, I actually have a couple of questions here.
> > 
> > First, I'm not quite sure I understand, what is the scenario for using
> > gin::ArrayBuffer and View. Their constructors take V8 ArrayBuffer and View,
> this
> > implies I must create the V8 side objects first. But then, gin::ArrayBuffer
> and
> > View can only help in accessing arrays' data, they don't seem to have 'ToV8'
> > converters. Does that mean, I still need to pass V8 ArrayBufferView and V8's
> > typed arrays to the V8ValueConverter, and the only gin::ArrayBuffer's and
> View's
> > task in my scenario is to externalize typed arrays' data as a side effect of
> > creating them?
> 
> The idea is to use them like this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/mojo/bindings/js/c...
> 
> you'd first create a v8::ArrayBuffer, and then create a wrapper which allows
you
> to modify the contents directly. Since you had to create the v8 version first,
> you have a handle anyway, and don't need a ToV8.
> 
> However, if having a ToV8 is more convenient, we should add one
> 
> > 
> > Second, I'm also not sure why, but I'm getting linker errors on getting
> > gin::ArrayBufferView's constructor and destructor when doing a component
> build.
> > I clearly see that gin::ArrayBufferView class is marked with GIN_EXPORT,
> should
> > that be enough? Or perhaps this is just a glitch and I should try nuking my
> out?
> > I'm building with OS=Android.
> 
> the class is not correctly marked as DISALLOW_COPY_AND_ASSIGN. And the isolate
> constructor for ArrayBuffer is also not defined :-/
> > 
> > And also, as I've mentioned before, I have tried the 'create blink instance'
> way
> > by using RenderViewTest as a base class for my test (and removing the
current
> > code for creating a V8's Context from the test), and the test was crashing
> > because it was somehow trying to initialize Blink twice.

I dug a little deeper and have found that content_unittests use
UnitTestTestSuite which initializes Blink, so I actually don't need
IsolateHolder.

But then I started to investigate why WebArrayBufferView is crashing (see
https://codereview.chromium.org/259033002/#msg3) and have found that it expects
an instance of DOMWrapperWorld to exist, and there is none in the test, because
DOMWrapperWorld is usually created with a frame.

Do you know any easy public way to instantiate a DOMWrapperWorld?

[mnaganov (inactive), 2014-05-16 14:48:00]

Jochen, thanks to your advice, I have changed GinJavaBridgeValue converter to
use gin::ArrayBufferView and finally I was able to write a test for typed arrays
conversion.

PTAL!

[jochen (gone - plz use gerrit), 2014-05-19 13:26:55]

thank you, lgtm

https://codereview.chromium.org/259033002/diff/80001/content/renderer/java/gi...
File content/renderer/java/gin_java_bridge_value_converter.cc (right):

https://codereview.chromium.org/259033002/diff/80001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_value_converter.cc:94: };
nit. disallow copy & assign

[mnaganov (inactive), 2014-05-19 14:57:52]

Jochen, thank you very much! Your comments were super helpful!

brettw@: Brett, can you please look at the tiny change in base/id_map.h? You
seem to be familiar with it.

https://codereview.chromium.org/259033002/diff/80001/content/renderer/java/gi...
File content/renderer/java/gin_java_bridge_value_converter.cc (right):

https://codereview.chromium.org/259033002/diff/80001/content/renderer/java/gi...
content/renderer/java/gin_java_bridge_value_converter.cc:94: };
On 2014/05/19 13:26:55, jochen wrote:
> nit. disallow copy & assign

Done.

[mnaganov (inactive), 2014-05-22 12:09:19]

Nico, maybe you can looks at the base/id_map.h change, please?

[Nico, 2014-05-22 19:27:31]

id_map lgtm

[mnaganov (inactive), 2014-05-27 10:55:44]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-05-27 10:56:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mnaganov@chromium.org/259033002/110001

[commit-bot: I haz the power, 2014-05-27 12:52:13]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-27 12:57:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-27 12:57:50]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)

[mnaganov (inactive), 2014-05-27 18:10:13]

Committed patchset #7 manually as r272997 (presubmit successful).