Enable posting a message from JS to Android webview.

Enable posting a message from JS to Android webview.

For future CL: queueing messages, sending messages from Java to JS through message channel, transferring ports in message channels, closing channels.

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

[sgurun-gerrit only, 2015-01-06 17:53:24]

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

[sgurun-gerrit only, 2015-01-06 17:57:06]

Ptal. I will add more tests. At the time it does only handle posting string
messages from JS to Java. Posting from Java to JS via message channels is for
the next CL.

https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
File content/browser/message_port_provider.cc (right):

https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
content/browser/message_port_provider.cc:22: const base::string16&
source_origin,
passing web_contents to IO thread is not safe. We need to fix that.

[mnaganov (inactive), 2015-01-07 11:43:55]

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/browser/...
android_webview/browser/aw_message_port_message_filter.h:19:
AwMessagePortMessageFilter(int routing_id);
explicit AwMessagePortMessageFilter(int route_id)

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: remove "(c)"

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:36:
private SparseArray<MessagePort> mMessagePorts = new SparseArray<MessagePort>();
This class is only used on UI thread, right? It's just not obvious due to
Java<->C++ and thread transitions in C++.

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:207:
channelContainer.waitForMessage();
BTW, do we allow blocking UI thread while waiting for a message from the JS
side? In your test, you have the instrumentation thread, so this always works. I
guess, it will not work for an app that uses the main thread for waiting, due to
those occasional transitions through UI thread.

Are we intentionally not supporting this? Java Bridge allows such
synchronization, and people do really use it. How we will be retiring JB then?

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
android_webview/native/aw_message_port_service_impl.cc:129: // TODO(sgurun)
seems like we are accessing ports_ from both UI and IO thread.
What do we use |ports_| for?

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
android_webview/native/aw_message_port_service_impl.h:20: // java class for an
explanation of use, ownership and lifetime.
I would recommend adding comments and asserts about threading assumptions, as
this object is intended to be used on IO thread.

https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
File android_webview/renderer/aw_message_port_client.h (right):

https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
android_webview/renderer/aw_message_port_client.h:24: // RenderFrame::Observer
RenderFrameObserver

https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
File content/browser/message_port_provider.cc (right):

https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
content/browser/message_port_provider.cc:20: void
MessagePortProvider::PostMessageToFrame(
Why do we need to do this on IO thread? Can we prepare all the data, except the
new routing ids on UI thread?

https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
content/browser/message_port_provider.cc:26:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
nit: use DCHECK_CURRENTLY_ON(BrowserThread::IO)

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

https://codereview.chromium.org/831523004/diff/60001/content/public/browser/m...
content/public/browser/message_port_provider.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
nit: remove (c)

[sgurun-gerrit only, 2015-01-10 02:36:29]

https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
File content/browser/message_port_provider.cc (right):

https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
content/browser/message_port_provider.cc:22: const base::string16&
source_origin,
On 2015/01/06 17:57:06, sgurun wrote:
> passing web_contents to IO thread is not safe. We need to fix that.

Done.

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/browser/...
android_webview/browser/aw_message_port_message_filter.h:19:
AwMessagePortMessageFilter(int routing_id);
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> explicit AwMessagePortMessageFilter(int route_id)

Done.

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/common/a...
android_webview/common/aw_message_port_messages.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> nit: remove "(c)"

Done.

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/java/src...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:36:
private SparseArray<MessagePort> mMessagePorts = new SparseArray<MessagePort>();
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> This class is only used on UI thread, right? It's just not obvious due to
> Java<->C++ and thread transitions in C++.

yes, good point. clarified on class documentation

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/javatest...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:207:
channelContainer.waitForMessage();
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> BTW, do we allow blocking UI thread while waiting for a message from the JS
> side? In your test, you have the instrumentation thread, so this always works.
I
> guess, it will not work for an app that uses the main thread for waiting, due
to
> those occasional transitions through UI thread.
> 
> Are we intentionally not supporting this? Java Bridge allows such
> synchronization, and people do really use it. How we will be retiring JB then?

Acknowledged. will do on monday

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
android_webview/native/aw_message_port_service_impl.cc:129: // TODO(sgurun)
seems like we are accessing ports_ from both UI and IO thread.
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> What do we use |ports_| for?

This will be used when posting the messages from Java to JS (to find the filter
associated with the message port). Maybe it is better if I add the logic when I
need it.

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

https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
android_webview/native/aw_message_port_service_impl.h:20: // java class for an
explanation of use, ownership and lifetime.
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> I would recommend adding comments and asserts about threading assumptions, as
> this object is intended to be used on IO thread.

Done.

https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
File android_webview/renderer/aw_message_port_client.h (right):

https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
android_webview/renderer/aw_message_port_client.h:24: // RenderFrame::Observer
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> RenderFrameObserver

Done.

https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
File content/browser/message_port_provider.cc (right):

https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
content/browser/message_port_provider.cc:20: void
MessagePortProvider::PostMessageToFrame(
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> Why do we need to do this on IO thread? Can we prepare all the data, except
the
> new routing ids on UI thread?

Done.

https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
content/browser/message_port_provider.cc:26:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> nit: use DCHECK_CURRENTLY_ON(BrowserThread::IO)

Done.

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

https://codereview.chromium.org/831523004/diff/60001/content/public/browser/m...
content/public/browser/message_port_provider.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> nit: remove (c)

Done.

[sgurun-gerrit only, 2015-01-10 02:37:19]

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

[sgurun-gerrit only, 2015-01-10 02:37:56]

On 2015/01/10 02:36:29, sgurun wrote:
>
https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
> File content/browser/message_port_provider.cc (right):
> 
>
https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
> content/browser/message_port_provider.cc:22: const base::string16&
> source_origin,
> On 2015/01/06 17:57:06, sgurun wrote:
> > passing web_contents to IO thread is not safe. We need to fix that.
> 
> Done.
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/browser/...
> File android_webview/browser/aw_message_port_message_filter.h (right):
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/browser/...
> android_webview/browser/aw_message_port_message_filter.h:19:
> AwMessagePortMessageFilter(int routing_id);
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > explicit AwMessagePortMessageFilter(int route_id)
> 
> Done.
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/common/a...
> File android_webview/common/aw_message_port_messages.h (right):
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/common/a...
> android_webview/common/aw_message_port_messages.h:1: // Copyright (c) 2014 The
> Chromium Authors. All rights reserved.
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > nit: remove "(c)"
> 
> Done.
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/java/src...
> File
>
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
> (right):
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/java/src...
>
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:36:
> private SparseArray<MessagePort> mMessagePorts = new
SparseArray<MessagePort>();
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > This class is only used on UI thread, right? It's just not obvious due to
> > Java<->C++ and thread transitions in C++.
> 
> yes, good point. clarified on class documentation
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/javatest...
> File
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
> (right):
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/javatest...
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:207:
> channelContainer.waitForMessage();
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > BTW, do we allow blocking UI thread while waiting for a message from the JS
> > side? In your test, you have the instrumentation thread, so this always
works.
> I
> > guess, it will not work for an app that uses the main thread for waiting,
due
> to
> > those occasional transitions through UI thread.
> > 
> > Are we intentionally not supporting this? Java Bridge allows such
> > synchronization, and people do really use it. How we will be retiring JB
then?
> 
> Acknowledged. will do on monday
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> File android_webview/native/aw_message_port_service_impl.cc (right):
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> android_webview/native/aw_message_port_service_impl.cc:129: // TODO(sgurun)
> seems like we are accessing ports_ from both UI and IO thread.
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > What do we use |ports_| for?
> 
> This will be used when posting the messages from Java to JS (to find the
filter
> associated with the message port). Maybe it is better if I add the logic when
I
> need it.
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> File android_webview/native/aw_message_port_service_impl.h (right):
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> android_webview/native/aw_message_port_service_impl.h:20: // java class for an
> explanation of use, ownership and lifetime.
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > I would recommend adding comments and asserts about threading assumptions,
as
> > this object is intended to be used on IO thread.
> 
> Done.
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
> File android_webview/renderer/aw_message_port_client.h (right):
> 
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
> android_webview/renderer/aw_message_port_client.h:24: // RenderFrame::Observer
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > RenderFrameObserver
> 
> Done.
> 
>
https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
> File content/browser/message_port_provider.cc (right):
> 
>
https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
> content/browser/message_port_provider.cc:20: void
> MessagePortProvider::PostMessageToFrame(
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > Why do we need to do this on IO thread? Can we prepare all the data, except
> the
> > new routing ids on UI thread?
> 
> Done.
> 
>
https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
> content/browser/message_port_provider.cc:26:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > nit: use DCHECK_CURRENTLY_ON(BrowserThread::IO)
> 
> Done.
> 
>
https://codereview.chromium.org/831523004/diff/60001/content/public/browser/m...
> File content/public/browser/message_port_provider.h (right):
> 
>
https://codereview.chromium.org/831523004/diff/60001/content/public/browser/m...
> content/public/browser/message_port_provider.h:1: // Copyright (c) 2014 The
> Chromium Authors. All rights reserved.
> On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > nit: remove (c)
> 
> Done.

jochen please review content/

[sgurun-gerrit only, 2015-01-10 02:39:18]

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

[sgurun-gerrit only, 2015-01-10 02:39:56]

On 2015/01/10 02:37:56, sgurun wrote:
> On 2015/01/10 02:36:29, sgurun wrote:
> >
>
https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
> > File content/browser/message_port_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
> > content/browser/message_port_provider.cc:22: const base::string16&
> > source_origin,
> > On 2015/01/06 17:57:06, sgurun wrote:
> > > passing web_contents to IO thread is not safe. We need to fix that.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/browser/...
> > File android_webview/browser/aw_message_port_message_filter.h (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/browser/...
> > android_webview/browser/aw_message_port_message_filter.h:19:
> > AwMessagePortMessageFilter(int routing_id);
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > explicit AwMessagePortMessageFilter(int route_id)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/common/a...
> > File android_webview/common/aw_message_port_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/common/a...
> > android_webview/common/aw_message_port_messages.h:1: // Copyright (c) 2014
The
> > Chromium Authors. All rights reserved.
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > nit: remove "(c)"
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/java/src...
> > File
> >
>
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/java/src...
> >
>
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:36:
> > private SparseArray<MessagePort> mMessagePorts = new
> SparseArray<MessagePort>();
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > This class is only used on UI thread, right? It's just not obvious due to
> > > Java<->C++ and thread transitions in C++.
> > 
> > yes, good point. clarified on class documentation
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/javatest...
> > File
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/javatest...
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:207:
> > channelContainer.waitForMessage();
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > BTW, do we allow blocking UI thread while waiting for a message from the
JS
> > > side? In your test, you have the instrumentation thread, so this always
> works.
> > I
> > > guess, it will not work for an app that uses the main thread for waiting,
> due
> > to
> > > those occasional transitions through UI thread.
> > > 
> > > Are we intentionally not supporting this? Java Bridge allows such
> > > synchronization, and people do really use it. How we will be retiring JB
> then?
> > 
> > Acknowledged. will do on monday
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> > File android_webview/native/aw_message_port_service_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> > android_webview/native/aw_message_port_service_impl.cc:129: // TODO(sgurun)
> > seems like we are accessing ports_ from both UI and IO thread.
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > What do we use |ports_| for?
> > 
> > This will be used when posting the messages from Java to JS (to find the
> filter
> > associated with the message port). Maybe it is better if I add the logic
when
> I
> > need it.
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> > File android_webview/native/aw_message_port_service_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> > android_webview/native/aw_message_port_service_impl.h:20: // java class for
an
> > explanation of use, ownership and lifetime.
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > I would recommend adding comments and asserts about threading assumptions,
> as
> > > this object is intended to be used on IO thread.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
> > File android_webview/renderer/aw_message_port_client.h (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
> > android_webview/renderer/aw_message_port_client.h:24: //
RenderFrame::Observer
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > RenderFrameObserver
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
> > File content/browser/message_port_provider.cc (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
> > content/browser/message_port_provider.cc:20: void
> > MessagePortProvider::PostMessageToFrame(
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > Why do we need to do this on IO thread? Can we prepare all the data,
except
> > the
> > > new routing ids on UI thread?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
> > content/browser/message_port_provider.cc:26:
> > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > nit: use DCHECK_CURRENTLY_ON(BrowserThread::IO)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/public/browser/m...
> > File content/public/browser/message_port_provider.h (right):
> > 
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/public/browser/m...
> > content/public/browser/message_port_provider.h:1: // Copyright (c) 2014 The
> > Chromium Authors. All rights reserved.
> > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > nit: remove (c)
> > 
> > Done.
> 
> jochen please review content/

palmer please review ipc/ipc_message_start.h

[sgurun-gerrit only, 2015-01-13 00:31:30]

Patchset #7 (id:120001) has been deleted

[sgurun-gerrit only, 2015-01-13 00:39:19]

On 2015/01/10 02:39:56, sgurun wrote:
> On 2015/01/10 02:37:56, sgurun wrote:
> > On 2015/01/10 02:36:29, sgurun wrote:
> > >
> >
>
https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
> > > File content/browser/message_port_provider.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/40001/content/browser/message_...
> > > content/browser/message_port_provider.cc:22: const base::string16&
> > > source_origin,
> > > On 2015/01/06 17:57:06, sgurun wrote:
> > > > passing web_contents to IO thread is not safe. We need to fix that.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/browser/...
> > > File android_webview/browser/aw_message_port_message_filter.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/browser/...
> > > android_webview/browser/aw_message_port_message_filter.h:19:
> > > AwMessagePortMessageFilter(int routing_id);
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > explicit AwMessagePortMessageFilter(int route_id)
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/common/a...
> > > File android_webview/common/aw_message_port_messages.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/common/a...
> > > android_webview/common/aw_message_port_messages.h:1: // Copyright (c) 2014
> The
> > > Chromium Authors. All rights reserved.
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > nit: remove "(c)"
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/java/src...
> > > File
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/java/src...
> > >
> >
>
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:36:
> > > private SparseArray<MessagePort> mMessagePorts = new
> > SparseArray<MessagePort>();
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > This class is only used on UI thread, right? It's just not obvious due
to
> > > > Java<->C++ and thread transitions in C++.
> > > 
> > > yes, good point. clarified on class documentation
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/javatest...
> > > File
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/javatest...
> > >
> >
>
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:207:
> > > channelContainer.waitForMessage();
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > BTW, do we allow blocking UI thread while waiting for a message from the
> JS
> > > > side? In your test, you have the instrumentation thread, so this always
> > works.
> > > I
> > > > guess, it will not work for an app that uses the main thread for
waiting,
> > due
> > > to
> > > > those occasional transitions through UI thread.
> > > > 
> > > > Are we intentionally not supporting this? Java Bridge allows such
> > > > synchronization, and people do really use it. How we will be retiring JB
> > then?
> > > 
> > > Acknowledged. will do on monday
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> > > File android_webview/native/aw_message_port_service_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> > > android_webview/native/aw_message_port_service_impl.cc:129: //
TODO(sgurun)
> > > seems like we are accessing ports_ from both UI and IO thread.
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > What do we use |ports_| for?
> > > 
> > > This will be used when posting the messages from Java to JS (to find the
> > filter
> > > associated with the message port). Maybe it is better if I add the logic
> when
> > I
> > > need it.
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> > > File android_webview/native/aw_message_port_service_impl.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/native/a...
> > > android_webview/native/aw_message_port_service_impl.h:20: // java class
for
> an
> > > explanation of use, ownership and lifetime.
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > I would recommend adding comments and asserts about threading
assumptions,
> > as
> > > > this object is intended to be used on IO thread.
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
> > > File android_webview/renderer/aw_message_port_client.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/android_webview/renderer...
> > > android_webview/renderer/aw_message_port_client.h:24: //
> RenderFrame::Observer
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > RenderFrameObserver
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
> > > File content/browser/message_port_provider.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
> > > content/browser/message_port_provider.cc:20: void
> > > MessagePortProvider::PostMessageToFrame(
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > Why do we need to do this on IO thread? Can we prepare all the data,
> except
> > > the
> > > > new routing ids on UI thread?
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/browser/message_...
> > > content/browser/message_port_provider.cc:26:
> > > DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > nit: use DCHECK_CURRENTLY_ON(BrowserThread::IO)
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/public/browser/m...
> > > File content/public/browser/message_port_provider.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/831523004/diff/60001/content/public/browser/m...
> > > content/public/browser/message_port_provider.h:1: // Copyright (c) 2014
The
> > > Chromium Authors. All rights reserved.
> > > On 2015/01/07 11:43:54, mnaganov (cr) wrote:
> > > > nit: remove (c)
> > > 
> > > Done.
> > 
> > jochen please review content/
> 
> palmer please review ipc/ipc_message_start.h

mnaganov: I have added receiving messages in a background thread for Java side.
Ptal, thx

[mnaganov (inactive), 2015-01-13 10:37:51]

No issues, except for formatting and naming, so LGTM.

Are you planning adding a test verifying that it is actually possible to receive
a message from the page while the main app thread is blocked, as you have
enabled that?

https://codereview.chromium.org/831523004/diff/160001/android_webview/browser...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/831523004/diff/160001/android_webview/browser...
android_webview/browser/aw_browser_context.h:107: 
nit: extra blank line

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

https://codereview.chromium.org/831523004/diff/160001/android_webview/browser...
android_webview/browser/aw_message_port_message_filter.h:19: explicit
AwMessagePortMessageFilter(int routing_id);
nit: route_id

https://codereview.chromium.org/831523004/diff/160001/android_webview/browser...
android_webview/browser/aw_message_port_message_filter.h:30: void
SendMessagesAreQueued(int route_id) override;
The name is misleading. Sounds like as if the function returns a boolean, but it
is not. Should it be 'SendQueuedMessages'?

https://codereview.chromium.org/831523004/diff/160001/android_webview/common/...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/831523004/diff/160001/android_webview/common/...
android_webview/common/aw_message_port_messages.h:30: int /* recipient message
port id */,
nit: strange indentation

https://codereview.chromium.org/831523004/diff/160001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(right):

https://codereview.chromium.org/831523004/diff/160001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:106:

nit: extra blank line

https://codereview.chromium.org/831523004/diff/160001/content/browser/message...
File content/browser/message_port_provider.cc (right):

https://codereview.chromium.org/831523004/diff/160001/content/browser/message...
content/browser/message_port_provider.cc:21: void
PostMessageOnIOThread(MessagePortMessageFilter* filter,
Perhaps, you can now make this function a public method of
MessagePortMessageFilter itself?

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

can you add an implementation of the API to content shell and add a content
browser test for this?

[sgurun-gerrit only, 2015-01-14 23:53:13]

addressed Mikhail's comments. Note that I modified the existing test to wait in
UI thread and this also caught a call to UI thread that would cause a deadlock.
It is also fixed in this patch.

I have not addressed Jochen's comments yet.

https://codereview.chromium.org/831523004/diff/160001/android_webview/browser...
File android_webview/browser/aw_browser_context.h (right):

https://codereview.chromium.org/831523004/diff/160001/android_webview/browser...
android_webview/browser/aw_browser_context.h:107: 
On 2015/01/13 10:37:50, mnaganov (cr) wrote:
> nit: extra blank line

Done.

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

https://codereview.chromium.org/831523004/diff/160001/android_webview/browser...
android_webview/browser/aw_message_port_message_filter.h:19: explicit
AwMessagePortMessageFilter(int routing_id);
On 2015/01/13 10:37:50, mnaganov (cr) wrote:
> nit: route_id

Done.

https://codereview.chromium.org/831523004/diff/160001/android_webview/browser...
android_webview/browser/aw_message_port_message_filter.h:30: void
SendMessagesAreQueued(int route_id) override;
On 2015/01/13 10:37:50, mnaganov (cr) wrote:
> The name is misleading. Sounds like as if the function returns a boolean, but
it
> is not. Should it be 'SendQueuedMessages'?

Yes, quite confusing. It originates from MessagePortMessageFilter

void MessagePortMessageFilter::SendMessagesAreQueued(int route_id) {
  Send(new MessagePortMsg_MessagesQueued(route_id));
}

I think the name Send is to emphasize that it is sending an IPC:

// Tells the Message Port Channel object that there are no more in-flight
// messages arriving.
IPC_MESSAGE_ROUTED0(MessagePortMsg_MessagesQueued)

the IPC tells the port implementation at the renderer that the messages will
start to be queued at the browser side.
I think in the delegate we can easily drop the name Send, and call it
MessagesAreQueued. I am also open to any other name. I will put a TODO here to
address it when I start working on queuing messages.

https://codereview.chromium.org/831523004/diff/160001/android_webview/common/...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/831523004/diff/160001/android_webview/common/...
android_webview/common/aw_message_port_messages.h:30: int /* recipient message
port id */,
On 2015/01/13 10:37:50, mnaganov (cr) wrote:
> nit: strange indentation

Done.

https://codereview.chromium.org/831523004/diff/160001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java
(right):

https://codereview.chromium.org/831523004/diff/160001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMessagePortService.java:106:

On 2015/01/13 10:37:50, mnaganov (cr) wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/831523004/diff/160001/content/browser/message...
File content/browser/message_port_provider.cc (right):

https://codereview.chromium.org/831523004/diff/160001/content/browser/message...
content/browser/message_port_provider.cc:21: void
PostMessageOnIOThread(MessagePortMessageFilter* filter,
On 2015/01/13 10:37:51, mnaganov (cr) wrote:
> Perhaps, you can now make this function a public method of
> MessagePortMessageFilter itself?

Let's leave it to later in case there is a demand, since it is pretty simple to
do. Or let me know if you feel strongly and I will do it.

[mnaganov (inactive), 2015-01-15 09:49:33]

Still LGTM!

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

https://codereview.chromium.org/831523004/diff/160001/android_webview/browser...
android_webview/browser/aw_message_port_message_filter.h:30: void
SendMessagesAreQueued(int route_id) override;
On 2015/01/14 23:53:13, sgurun wrote:
> On 2015/01/13 10:37:50, mnaganov (cr) wrote:
> > The name is misleading. Sounds like as if the function returns a boolean,
but
> it
> > is not. Should it be 'SendQueuedMessages'?
> 
> Yes, quite confusing. It originates from MessagePortMessageFilter
> 
> void MessagePortMessageFilter::SendMessagesAreQueued(int route_id) {
>   Send(new MessagePortMsg_MessagesQueued(route_id));
> }
> 
> I think the name Send is to emphasize that it is sending an IPC:
> 
> // Tells the Message Port Channel object that there are no more in-flight
> // messages arriving.
> IPC_MESSAGE_ROUTED0(MessagePortMsg_MessagesQueued)
> 
> the IPC tells the port implementation at the renderer that the messages will
> start to be queued at the browser side.
> I think in the delegate we can easily drop the name Send, and call it
> MessagesAreQueued. I am also open to any other name. I will put a TODO here to
> address it when I start working on queuing messages.

Ah, sorry -- I actually missed somehow the fact that this method is inherited :)

https://codereview.chromium.org/831523004/diff/160001/content/browser/message...
File content/browser/message_port_provider.cc (right):

https://codereview.chromium.org/831523004/diff/160001/content/browser/message...
content/browser/message_port_provider.cc:21: void
PostMessageOnIOThread(MessagePortMessageFilter* filter,
On 2015/01/14 23:53:13, sgurun wrote:
> On 2015/01/13 10:37:51, mnaganov (cr) wrote:
> > Perhaps, you can now make this function a public method of
> > MessagePortMessageFilter itself?
> 
> Let's leave it to later in case there is a demand, since it is pretty simple
to
> do. Or let me know if you feel strongly and I will do it.

Oh, again -- I was wrongly supposing that MessagePortMessageFilter is entirely
yours, but now I see that it is actually a generic class used by all postMessage
implementations, in that case modifying it just for this case doesn't really
make sense. Sorry!

https://codereview.chromium.org/831523004/diff/180001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/831523004/diff/180001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:221:

nit: extra new lines

[palmer, 2015-01-15 20:31:51]

https://codereview.chromium.org/831523004/diff/180001/android_webview/common/...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/831523004/diff/180001/android_webview/common/...
android_webview/common/aw_message_port_messages.h:18: // format to a
base::Value. Due to the complexities of renderer/browser
Nit: The type in the response message is base::ListValue. The comment and the
code should match.

https://codereview.chromium.org/831523004/diff/180001/android_webview/common/...
android_webview/common/aw_message_port_messages.h:22: base::string16 /* message
*/,
Are there any lexical, syntactic, or size constraints on |message|? Can they be
better expressed with a type — e.g. WebSerializeScript, or other?

[sgurun-gerrit only, 2015-01-15 21:21:32]

https://codereview.chromium.org/831523004/diff/180001/android_webview/common/...
File android_webview/common/aw_message_port_messages.h (right):

https://codereview.chromium.org/831523004/diff/180001/android_webview/common/...
android_webview/common/aw_message_port_messages.h:18: // format to a
base::Value. Due to the complexities of renderer/browser
On 2015/01/15 20:31:51, Chromium Palmer wrote:
> Nit: The type in the response message is base::ListValue. The comment and the
> code should match.

Done.

https://codereview.chromium.org/831523004/diff/180001/android_webview/common/...
android_webview/common/aw_message_port_messages.h:22: base::string16 /* message
*/,
On 2015/01/15 20:31:51, Chromium Palmer wrote:
> Are there any lexical, syntactic, or size constraints on |message|? Can they
be
> better expressed with a type — e.g. WebSerializeScript, or other?

Not that I know of. There is one other place that I know the messages cross
renderer-browser boundary, and there they are converted to a base::String16
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/vie...

used in:

bool RenderFrameImpl::willCheckAndDispatchMessageEvent(
    blink::WebLocalFrame* source_frame,
    blink::WebFrame* target_frame,
    blink::WebSecurityOrigin target_origin,
    blink::WebDOMMessageEvent event) {
  DCHECK(!frame_ || frame_ == target_frame);

  if (!is_swapped_out_)
    return false;

  ViewMsg_PostMessage_Params params;
  params.is_data_raw_string = false;
  params.data = event.data().toString();

...

https://codereview.chromium.org/831523004/diff/180001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java
(right):

https://codereview.chromium.org/831523004/diff/180001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/PostMessageTest.java:221:

On 2015/01/15 09:49:33, mnaganov (cr) wrote:
> nit: extra new lines

Done.

[sgurun-gerrit only, 2015-01-16 02:40:44]

Jochen: I added browser tests to verify the functionality of message port
provider. I will add more as we add more functionality (such as queueing, etc).

[sgurun-gerrit only, 2015-01-16 08:09:52]

On 2015/01/16 02:40:44, sgurun wrote:
> Jochen: I added browser tests to verify the functionality of message port
> provider. I will add more as we add more functionality (such as queueing,
etc).

himm, I am guessing I need to add some CONTENT_EXPORTs in MessagePortService to
fix linker errros for tests. I will try this tomorrow.

[mnaganov (inactive), 2015-01-16 10:06:15]

On 2015/01/16 08:09:52, sgurun wrote:
> On 2015/01/16 02:40:44, sgurun wrote:
> > Jochen: I added browser tests to verify the functionality of message port
> > provider. I will add more as we add more functionality (such as queueing,
> etc).
> 
> himm, I am guessing I need to add some CONTENT_EXPORTs in MessagePortService
to
> fix linker errros for tests. I will try this tomorrow.

Pro-tip: try doing a component build locally (add "component=shared_library" to
GYP_DEFINES)

[sgurun-gerrit only, 2015-01-16 17:42:55]

On 2015/01/16 10:06:15, mnaganov (cr) wrote:
> On 2015/01/16 08:09:52, sgurun wrote:
> > On 2015/01/16 02:40:44, sgurun wrote:
> > > Jochen: I added browser tests to verify the functionality of message port
> > > provider. I will add more as we add more functionality (such as queueing,
> > etc).
> > 
> > himm, I am guessing I need to add some CONTENT_EXPORTs in MessagePortService
> to
> > fix linker errros for tests. I will try this tomorrow.
> 
> Pro-tip: try doing a component build locally (add "component=shared_library"
to
> GYP_DEFINES)

Thanks Mikhail. With a component build, I now locally verified that
CONTENT_EXPORT fixed the problem.

Jochen PTAL,

Thanks!

[jochen (gone - plz use gerrit), 2015-01-19 12:47:57]

thanks for the test. One more question: is it possible to also test the other
direction, i.e. JS posting to browser?

My understanding is that this change is supposed to make that possible, right?

[mnaganov (inactive), 2015-01-19 13:27:10]

On 2015/01/19 12:47:57, jochen (slow) wrote:
> thanks for the test. One more question: is it possible to also test the other
> direction, i.e. JS posting to browser?
> 
> My understanding is that this change is supposed to make that possible, right?

I believe, this is done in PostMessageTest.testCreateChannel. Perhaps, its name
just wasn't updated, sorry for missing that on my review.

[jochen (gone - plz use gerrit), 2015-01-19 13:32:17]

right, but that's a webview test, right? How much effort is it to add a browser
test for this?

[mnaganov (inactive), 2015-01-19 13:47:37]

On 2015/01/19 13:32:17, jochen (slow) wrote:
> right, but that's a webview test, right? How much effort is it to add a
browser
> test for this?

Correct, this is a webview-only test.

But what clients are you considering for a generic case? I guess, it should be
some Java code running in the browser process, which I think we only have on
Android. And the current trend for enabling direct JS-Java communication is to
use mojo.

[sgurun-gerrit only, 2015-01-19 18:23:14]

On 2015/01/19 13:47:37, mnaganov (cr) wrote:
> On 2015/01/19 13:32:17, jochen (slow) wrote:
> > right, but that's a webview test, right? How much effort is it to add a
> browser
> > test for this?
> 
> Correct, this is a webview-only test.
> 
> But what clients are you considering for a generic case? I guess, it should be
> some Java code running in the browser process, which I think we only have on
> Android. And the current trend for enabling direct JS-Java communication is to
> use mojo.

Yes, as Mikhail mentioned, I did not want to duplicate the test that already
exists in Webview side. Jochen, do you feel very strongly about it. I can look
into it tomorrow (today is holiday here)

[jochen (gone - plz use gerrit), 2015-01-20 15:54:50]

On 2015/01/19 at 18:23:14, sgurun wrote:
> On 2015/01/19 13:47:37, mnaganov (cr) wrote:
> > On 2015/01/19 13:32:17, jochen (slow) wrote:
> > > right, but that's a webview test, right? How much effort is it to add a
> > browser
> > > test for this?
> > 
> > Correct, this is a webview-only test.
> > 
> > But what clients are you considering for a generic case? I guess, it should
be
> > some Java code running in the browser process, which I think we only have on
> > Android. And the current trend for enabling direct JS-Java communication is
to
> > use mojo.
> 
> Yes, as Mikhail mentioned, I did not want to duplicate the test that already
exists in Webview side. Jochen, do you feel very strongly about it. I can look
into it tomorrow (today is holiday here)

Historically, the message port code suffered from very poor test coverage.
Anyway, I guess the java test might be good enough, so lgtm

[sgurun-gerrit only, 2015-01-20 17:48:22]

On 2015/01/20 15:54:50, jochen (slow) wrote:
> On 2015/01/19 at 18:23:14, sgurun wrote:
> > On 2015/01/19 13:47:37, mnaganov (cr) wrote:
> > > On 2015/01/19 13:32:17, jochen (slow) wrote:
> > > > right, but that's a webview test, right? How much effort is it to add a
> > > browser
> > > > test for this?
> > > 
> > > Correct, this is a webview-only test.
> > > 
> > > But what clients are you considering for a generic case? I guess, it
should
> be
> > > some Java code running in the browser process, which I think we only have
on
> > > Android. And the current trend for enabling direct JS-Java communication
is
> to
> > > use mojo.
> > 
> > Yes, as Mikhail mentioned, I did not want to duplicate the test that already
> exists in Webview side. Jochen, do you feel very strongly about it. I can look
> into it tomorrow (today is holiday here)
> 
> Historically, the message port code suffered from very poor test coverage.
> Anyway, I guess the java test might be good enough, so lgtm

thanks!. palmer, ping for ipc

[palmer, 2015-01-21 01:56:19]

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

https://codereview.chromium.org/831523004/diff/260001/android_webview/browser...
android_webview/browser/aw_message_port_message_filter.h:28: const
base::string16& message,
I don't quite believe :) that |message| does not have *any* lexical, syntactic,
or even size constraints. What is an example of a typical value for |message|?

If it's truly an opaque blob of data, you're better off expressing that with
std::vector<uint8_t>.

[sgurun-gerrit only, 2015-01-21 02:25:54]

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

https://codereview.chromium.org/831523004/diff/260001/android_webview/browser...
android_webview/browser/aw_message_port_message_filter.h:28: const
base::string16& message,
On 2015/01/21 01:56:19, Chromium Palmer wrote:
> I don't quite believe :) that |message| does not have *any* lexical,
syntactic,
> or even size constraints. What is an example of a typical value for |message|?
> 
> If it's truly an opaque blob of data, you're better off expressing that with
> std::vector<uint8_t>.

I think my answer was not well worded before, sorry about that. I don't mean
that there are no constraints on the format, the format is already known, it has
to be a WebSerializedScriptValue. But browser does not have logic to
deserialize/serialize a Webserializedscriptvalue.

Normally for a browser, this is a truly opaque data that browser does not
operate on it. It simply routes it to the receiver message port. 

Here browser routes this opaque data to renderer so that it can be converted to
a format it understands.

The message port logic already uses base::string16 rather then
std::vector<uint8_t> (here we simply use the existing logic). If
std::vector<uint8_t> a better choice, an owner of message port service, or maybe
even I can take a look it but I think it should be a separate CL.

[sgurun-gerrit only, 2015-01-22 16:02:43]

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

[sgurun-gerrit only, 2015-01-22 16:09:55]

nasko: please review ipc/  and android_webview/common/
palmer:  I figured you may not have enough bandwidth to review this CL at the
moment so I am asking nasko to take a look at ipc/ related parts.  We still need
a train of CLs to have this feature implemented so if you have any comments we
can address them easily in future CLs. Thanks for your review!

[palmer, 2015-01-22 19:46:22]

Yeah, it's probably better for someone else to pick this up. I am pretty full
lately. Sorry!

[palmer, 2015-01-22 19:47:53]

palmer@chromium.org changed reviewers:
- palmer@chromium.org

[nasko, 2015-01-23 17:01:08]

Unfortunately, I'm in the same boat as Chris. We are running an event next week
and I don't have the time to review this CL until I'm back later next week. Feel
free to pick another IPC reviewer for faster turnaround time.

[sgurun-gerrit only, 2015-01-23 17:06:32]

On 2015/01/23 17:01:08, nasko (very slow until 02-02) wrote:
> Unfortunately, I'm in the same boat as Chris. We are running an event next
week
> and I don't have the time to review this CL until I'm back later next week.
Feel
> free to pick another IPC reviewer for faster turnaround time.

well, bummer. thanks, will see if anybody else is willing to look at it.

[sgurun-gerrit only, 2015-01-23 17:10:38]

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

[sgurun-gerrit only, 2015-01-23 17:11:25]

tsepez: can you please review ipc/ and android_webview/common. 
Thanks!

[Tom Sepez, 2015-01-23 17:30:58]

The ipc/ and android_webview/common implementations look fine, the question I
have is what is the consequence if an untrusted renderer makes calls with
crafted port numbers?  This may have been addressed earlier in the thread, but I
didn't dig through the whole thing.

[sgurun-gerrit only, 2015-01-23 18:59:42]

On 2015/01/23 17:30:58, Tom Sepez wrote:
> The ipc/ and android_webview/common implementations look fine, the question I
> have is what is the consequence if an untrusted renderer makes calls with
> crafted port numbers?  This may have been addressed earlier in the thread, but
I
> didn't dig through the whole thing.

This is a pretty interesting question. 

Android_webview is single process, so unfortunately, if renderer is malicious,
the existence of ipc message layer won't be a very protective layer.

For multi-process scenario, the renderer to browser boundary is passed the first
time using this message:

// Sends a message to a message port.  Optionally sends a message port as
// as well if sent_message_port_id != MSG_ROUTING_NONE.
IPC_MESSAGE_CONTROL3(MessagePortHostMsg_PostMessage,
                     int /* sender_message_port_id */,
                     base::string16 /* message */,
                     std::vector<int> /* sent_message_port_ids */)

The JS does not directly have access to sender_message_port_id, so another
vulnerability need to be used to fake it. Once this happens,
these checks are done at the browser side:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/me...

1. if the port number is not already stored in the known message channel message
port container, or
2. if the entangled port number (the recipient), is not already stored in the
message channel message port container, or
3. the entangled port number does not have a valid routing id,
the message is dropped silently.

otherwise the message is sent to the entangled port, which normally lives in
another renderer.

in android_webview case, the message is sent to the webview. For using the
message in Java, we convert it to a known format, in which case the message
passes the renderer->browser boundary a second time:


 // Response to AwMessagePortMessage_ConvertMessage
 IPC_MESSAGE_ROUTED3(AwMessagePortHostMsg_ConvertedMessage,
                     int /* recipient message port id */,
                     base::ListValue /* converted message */,
                      std::vector<int> /* sent message port_ids */)

here the browser side keeps a mapping of known message ports in
AwMessagePortService.java. However, we do not check if the message port id
already exists in the map anymore:
 // Called on IO thread.
 122     @CalledByNative
 123     private void onPostMessage(int portId, String message, int[] ports) {
 124         PostMessage m = new PostMessage(mPortStorage.get(portId), message);
 125         Handler handler = mMessageHandler.getHandler();
 126         Message msg = handler.obtainMessage(POST_MESSAGE, m);
 127         handler.sendMessage(msg);
 128     }
 129 


thanks for looking into this!

[Tom Sepez, 2015-01-23 19:03:58]

Ok. That seems reasonable. LGTM.

[sgurun-gerrit only, 2015-01-23 19:05:42]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2015-01-23 19:06:33]

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

[commit-bot: I haz the power, 2015-01-23 20:54:37]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2015-01-23 20:56:00]

Patchset 13 (id:??) landed as
https://crrev.com/d2a4306024cc3bd6c52c70646ad6ec37f0259622
Cr-Commit-Position: refs/heads/master@{#312922}