Post a Message from Java to JS

Post a message from Java to JS using message channels.

BUG=393291
Committed: https://crrev.com/92afec533e58387d2364f49227a053993e1afd8f
Cr-Commit-Position: refs/heads/master@{#313817}

[sgurun-gerrit only, 2015-01-26 23:18:30]

sgurun@chromium.org changed reviewers:
+ jochen@chromium.org, mnaganov@chromium.org

[mnaganov (inactive), 2015-01-27 13:30:18]

Great job, Selim! But I have some suggestions.

https://codereview.chromium.org/869133005/diff/40001/android_webview/common/a...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/869133005/diff/40001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:20: // and can only be
serialized/deserialized in renderer.
You may mention that we couldn't have Blink or V8 on the browser side due to
their reliance on static variables.

https://codereview.chromium.org/869133005/diff/40001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:23: // use. Since WebView is
single-process this is not terribly expensive, but
I would argue with this statement. Using IPC takes its toll even for WebView due
to locking that is uses. Try conducting an experiment that sends, say, 10000
short messages and check how much time will you spend on that.

https://codereview.chromium.org/869133005/diff/40001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:29:
IPC_MESSAGE_ROUTED3(AwMessagePortMsg_InMessage,
The terms "In" and "Out" doesn't clearly mark the origin or the recipient of the
message. There may be people around that place the Web platform in the center of
the universe, not WebView :)

How about naming the directions something like "WebToApp" and "AppToWeb" or
"WebToNative" and "NativeToWeb"?

https://codereview.chromium.org/869133005/diff/40001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(right):

https://codereview.chromium.org/869133005/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:112:
throw new IllegalStateException(" Cannot post to unknown port " + senderId);
nit: extra leading space in the message

https://codereview.chromium.org/869133005/diff/40001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/MessagePort.java
(right):

https://codereview.chromium.org/869133005/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:41:
mClosed = true;
I guess, it is an error to attempt to close the port twice? Should we add an
assert or throw here?

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:148:
@Feature({"AndroidWebView", "Android-PostMessage"})
Perhaps, you can just annotate the test with "android.test.UiThreadTest",
because it is effectively a single-threaded test, and it runs on the ui thread
most of the time.

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:166:
new MessagePort[]{channel.port2()});
Perhaps, you can simply insert a call to "fail" after the method, so the test
will fail if there were no exception?

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:275:
final String hello = "HELLO";
Would it be better to use separate messages for request and reply, so we can be
sure that we haven't accidentally read back our original message instead of the
reply?

[sgurun-gerrit only, 2015-01-29 03:06:09]

jochen please review content/

https://codereview.chromium.org/869133005/diff/40001/android_webview/common/a...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/869133005/diff/40001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:20: // and can only be
serialized/deserialized in renderer.
On 2015/01/27 13:30:17, mnaganov (cr) wrote:
> You may mention that we couldn't have Blink or V8 on the browser side due to
> their reliance on static variables.

Done.

https://codereview.chromium.org/869133005/diff/40001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:23: // use. Since WebView is
single-process this is not terribly expensive, but
On 2015/01/27 13:30:17, mnaganov (cr) wrote:
> I would argue with this statement. Using IPC takes its toll even for WebView
due
> to locking that is uses. Try conducting an experiment that sends, say, 10000
> short messages and check how much time will you spend on that.

Acknowledged. I am planning to add some perf tests in a next CL. Either way, I
don't feel like there is much to do at this moment. One possibility is
investigating a custom serializer/deserializer that does not depend on Blink,
but I am leaving it to later.

https://codereview.chromium.org/869133005/diff/40001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:29:
IPC_MESSAGE_ROUTED3(AwMessagePortMsg_InMessage,
On 2015/01/27 13:30:17, mnaganov (cr) wrote:
> The terms "In" and "Out" doesn't clearly mark the origin or the recipient of
the
> message. There may be people around that place the Web platform in the center
of
> the universe, not WebView :)
> 
> How about naming the directions something like "WebToApp" and "AppToWeb" or
> "WebToNative" and "NativeToWeb"?

Done.

https://codereview.chromium.org/869133005/diff/40001/android_webview/java/src...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(right):

https://codereview.chromium.org/869133005/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:112:
throw new IllegalStateException(" Cannot post to unknown port " + senderId);
On 2015/01/27 13:30:17, mnaganov (cr) wrote:
> nit: extra leading space in the message

Done.

https://codereview.chromium.org/869133005/diff/40001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/MessagePort.java
(right):

https://codereview.chromium.org/869133005/diff/40001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:41:
mClosed = true;
On 2015/01/27 13:30:17, mnaganov (cr) wrote:
> I guess, it is an error to attempt to close the port twice? Should we add an
> assert or throw here?

Done.

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:148:
@Feature({"AndroidWebView", "Android-PostMessage"})
On 2015/01/27 13:30:17, mnaganov (cr) wrote:
> Perhaps, you can just annotate the test with "android.test.UiThreadTest",
> because it is effectively a single-threaded test, and it runs on the ui thread
> most of the time.

In a UI thread test, how do I make sure the test does not exit before running
the asynchronous callback?

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:166:
new MessagePort[]{channel.port2()});
On 2015/01/27 13:30:17, mnaganov (cr) wrote:
> Perhaps, you can simply insert a call to "fail" after the method, so the test
> will fail if there were no exception?

Done.

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:275:
final String hello = "HELLO";
On 2015/01/27 13:30:18, mnaganov (cr) wrote:
> Would it be better to use separate messages for request and reply, so we can
be
> sure that we haven't accidentally read back our original message instead of
the
> reply?

Done. that would be a very sad bug :)

[jochen (gone - plz use gerrit), 2015-01-29 15:13:31]

content/ lgtm

https://codereview.chromium.org/869133005/diff/60001/content/public/browser/m...
File content/public/browser/message_port_provider.h (right):

https://codereview.chromium.org/869133005/diff/60001/content/public/browser/m...
content/public/browser/message_port_provider.h:48: const std::vector<int>&
sent_ports);
nit. the variable names of the header and impl don't match up

[mnaganov (inactive), 2015-01-29 16:56:20]

LGTM

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/869133005/diff/40001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:148:
@Feature({"AndroidWebView", "Android-PostMessage"})
On 2015/01/29 03:06:09, sgurun wrote:
> On 2015/01/27 13:30:17, mnaganov (cr) wrote:
> > Perhaps, you can just annotate the test with "android.test.UiThreadTest",
> > because it is effectively a single-threaded test, and it runs on the ui
thread
> > most of the time.
> 
> In a UI thread test, how do I make sure the test does not exit before running
> the asynchronous callback?

Ah, sorry, I missed the fact that all the code is inside "onReceiveValue"
callback.

https://codereview.chromium.org/869133005/diff/60001/android_webview/browser/...
File android_webview/browser/aw_message_port_message_filter.h (right):

https://codereview.chromium.org/869133005/diff/60001/android_webview/browser/...
android_webview/browser/aw_message_port_message_filter.h:27: const
base::string16& message,
nit: Indentation looks wrong

https://codereview.chromium.org/869133005/diff/60001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/MessagePort.java
(right):

https://codereview.chromium.org/869133005/diff/60001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:72: for
(int i = 0; i < msgPorts.length; i++)
nit: In Java you need to put curly brackets even around one line blocks.

https://codereview.chromium.org/869133005/diff/60001/android_webview/native/a...
File android_webview/native/aw_message_port_service_impl.cc (right):

https://codereview.chromium.org/869133005/diff/60001/android_webview/native/a...
android_webview/native/aw_message_port_service_impl.cc:141: for
(std::vector<int>::const_iterator iter = sent_ports.begin();
nit: These days you can write it simpler: 

for (const auto& iter : sent_ports)
    ports_.erase(*iter);

[sgurun-gerrit only, 2015-01-29 19:15:39]

sgurun@chromium.org changed reviewers:
+ tsepez@chromium.org

[sgurun-gerrit only, 2015-01-29 19:15:40]

thanks all done. at the webview side, I had to disable
testReceiveMessageInBackgroundThread
since the test is flaky. The test blocks the UI thread and it is likely that a
sync IPC is blocked at the renderer side waiting for UI thread (see the anlaysis
on top of the test.

Will look into it later.
tsepez please review android_webview/common

https://codereview.chromium.org/869133005/diff/60001/android_webview/browser/...
File android_webview/browser/aw_message_port_message_filter.h (right):

https://codereview.chromium.org/869133005/diff/60001/android_webview/browser/...
android_webview/browser/aw_message_port_message_filter.h:27: const
base::string16& message,
On 2015/01/29 16:56:20, mnaganov (cr) wrote:
> nit: Indentation looks wrong

Done.

https://codereview.chromium.org/869133005/diff/60001/android_webview/java/src...
File android_webview/java/src/org/chromium/android_webview/MessagePort.java
(right):

https://codereview.chromium.org/869133005/diff/60001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/MessagePort.java:72: for
(int i = 0; i < msgPorts.length; i++)
On 2015/01/29 16:56:20, mnaganov (cr) wrote:
> nit: In Java you need to put curly brackets even around one line blocks.

Done.

https://codereview.chromium.org/869133005/diff/60001/android_webview/native/a...
File android_webview/native/aw_message_port_service_impl.cc (right):

https://codereview.chromium.org/869133005/diff/60001/android_webview/native/a...
android_webview/native/aw_message_port_service_impl.cc:141: for
(std::vector<int>::const_iterator iter = sent_ports.begin();
On 2015/01/29 16:56:20, mnaganov (cr) wrote:
> nit: These days you can write it simpler: 
> 
> for (const auto& iter : sent_ports)
>     ports_.erase(*iter);
                   ^
                  no deref?
loved it.

https://codereview.chromium.org/869133005/diff/60001/content/public/browser/m...
File content/public/browser/message_port_provider.h (right):

https://codereview.chromium.org/869133005/diff/60001/content/public/browser/m...
content/public/browser/message_port_provider.h:48: const std::vector<int>&
sent_ports);
On 2015/01/29 15:13:31, jochen (slow) wrote:
> nit. the variable names of the header and impl don't match up

Done.

[Tom Sepez, 2015-01-29 19:38:53]

So, I think the flow you're describing is

java app --> browser --> renderer for decode --> browser --> possibly other
renderer.

Getting decoding out of the browser is great, but how do we know that the
untrusted renderer that did the decoding did so accuratele?  What's the
implication of deliberate tampering at that stage?  Thanks.

[sgurun-gerrit only, 2015-01-29 20:04:55]

On 2015/01/29 19:38:53, Tom Sepez wrote:
> So, I think the flow you're describing is
> 
> java app --> browser --> renderer for decode --> browser --> possibly other
> renderer.
> 
> Getting decoding out of the browser is great, but how do we know that the
> untrusted renderer that did the decoding did so accuratele?  What's the
> implication of deliberate tampering at that stage?  Thanks.

In android webview, it would probably not make a much difference since it is
single process, and all renderers run in the some address space as with browser.
We are actually investigating how/if this conversion could be done at the
browser side.

[sgurun-gerrit only, 2015-01-29 20:06:48]

On 2015/01/29 20:04:55, sgurun wrote:
> On 2015/01/29 19:38:53, Tom Sepez wrote:
> > So, I think the flow you're describing is
> > 
> > java app --> browser --> renderer for decode --> browser --> possibly other
> > renderer.
> > 
> > Getting decoding out of the browser is great, but how do we know that the
> > untrusted renderer that did the decoding did so accuratele?  What's the
> > implication of deliberate tampering at that stage?  Thanks.
> 
> In android webview, it would probably not make a much difference since it is
> single process, and all renderers run in the some address space as with
browser.
> We are actually investigating how/if this conversion could be done at the
> browser side.

I am also adding a note that to explain this in great detail, since it is
something to be aware of, if in distant future webview switches to a
multiprocess model (this is a very big fish though)

[sgurun-gerrit only, 2015-01-29 20:18:00]

On 2015/01/29 20:06:48, sgurun wrote:
> On 2015/01/29 20:04:55, sgurun wrote:
> > On 2015/01/29 19:38:53, Tom Sepez wrote:
> > > So, I think the flow you're describing is
> > > 
> > > java app --> browser --> renderer for decode --> browser --> possibly
other
> > > renderer.
> > > 
> > > Getting decoding out of the browser is great, but how do we know that the
> > > untrusted renderer that did the decoding did so accuratele?  What's the
> > > implication of deliberate tampering at that stage?  Thanks.
> > 
> > In android webview, it would probably not make a much difference since it is
> > single process, and all renderers run in the some address space as with
> browser.
> > We are actually investigating how/if this conversion could be done at the
> > browser side.
> 
> I am also adding a note that to explain this in great detail, since it is
> something to be aware of, if in distant future webview switches to a
> multiprocess model (this is a very big fish though)

tspeze: updated the description in aw_message_port_messages, PTAL, thanks!

[sgurun-gerrit only, 2015-01-29 20:19:02]

On 2015/01/29 20:18:00, sgurun wrote:
> On 2015/01/29 20:06:48, sgurun wrote:
> > On 2015/01/29 20:04:55, sgurun wrote:
> > > On 2015/01/29 19:38:53, Tom Sepez wrote:
> > > > So, I think the flow you're describing is
> > > > 
> > > > java app --> browser --> renderer for decode --> browser --> possibly
> other
> > > > renderer.
> > > > 
> > > > Getting decoding out of the browser is great, but how do we know that
the
> > > > untrusted renderer that did the decoding did so accuratele?  What's the
> > > > implication of deliberate tampering at that stage?  Thanks.
> > > 
> > > In android webview, it would probably not make a much difference since it
is
> > > single process, and all renderers run in the some address space as with
> > browser.
> > > We are actually investigating how/if this conversion could be done at the
> > > browser side.
> > 
> > I am also adding a note that to explain this in great detail, since it is
> > something to be aware of, if in distant future webview switches to a
> > multiprocess model (this is a very big fish though)
> 
      > tspeze: updated the description in aw_message_port_messages, PTAL,
thanks!
        ^^^^^^
sorry, tsepez

[Tom Sepez, 2015-01-29 22:01:55]

LGTM. Thanks for the comment.

[sgurun-gerrit only, 2015-01-29 22:08:29]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2015-01-29 22:09:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/869133005/110001

[commit-bot: I haz the power, 2015-01-29 23:08:31]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-01-29 23:09:25]

Patchset 7 (id:??) landed as
https://crrev.com/92afec533e58387d2364f49227a053993e1afd8f
Cr-Commit-Position: refs/heads/master@{#313817}