Enable messaging over a navigator.connect initiated channel.

Enable messaging over a navigator.connect initiated channel.

This refactors MessagePortService to decouple it from MessagePortMessageFilter, allowing a new NavigatorConnectContext class to intercept messages sent to a service worker.

Layout tests for this are in https://codereview.chromium.org/812853002
BUG=426458
Committed: https://crrev.com/e6b8303df3dd0dbb81c494c395976fb6a17390a1
Cr-Commit-Position: refs/heads/master@{#309291}

[Marijn Kruisselbrink, 2014-12-17 19:17:35]

mek@chromium.org changed reviewers:
+ michaeln@chromium.org, scheib@chromium.org

[scheib, 2014-12-18 00:24:10]

lgtm presuming IPC message sanitization is addressed or TODO'ed with an issue.

https://codereview.chromium.org/785133006/diff/30001/content/browser/message_...
File content/browser/message_port_message_filter.cc (right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/message_...
content/browser/message_port_message_filter.cc:66: // If a message port was sent
around, the new location will need a routing
This code block is more indirect now that a nesting of two helpers are doing the
work. The comment helps, but may be clearer as:

When |sent_message_port_ids| is non-empty generate and send |new_routing_ids| to
avoid waiting for the created ports to send a sync message with routing ids.

https://codereview.chromium.org/785133006/diff/30001/content/browser/message_...
File content/browser/message_port_service.cc (right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/message_...
content/browser/message_port_service.cc:15: // |delegate| is just a weak pointer
since we get notified when its process
Old comment out of date, I suggest:
"|delegate| is a raw pointer, it calls OnMessagePortDelegateClosing before it is
destroyed."

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_context.cc (right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_context.cc:32: // Use
message_port_id as routing ID since there is no reason to have yet
Perhaps this comment would be best at the definition of connections_ in the .h?

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_context.cc:37: Connection&
connection = connections_[client.message_port_id];
See comment at NavigatorConnectDispatcherHost::OnConnect.

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_context.cc:51: // Hold
messages on all transfered ports.
comment suggestion: 
Hold messages while service worker is found, activated, and message sent causing
ServiceWorkerScriptContext::OnCrossOriginMessageToWorker to construct
WebMessagePortChannelImpl instances which send
MessagePortHostMsg_ReleaseMessages.

(explains why the call to HoldMessages)
(explains where/when to expect ReleaseMessages - a bit involved)

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_context.h (right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_context.h:19: // Class that
keeps track of all active navigator.connect connections. This
Tracks all active navigator.connect connections, wakes up service workers when a
message arrives, and routes the messages.

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_context.h:32: // Registers
an accepted connection with this class. This method takes care
Registers an accepted connection and updates MessagePortService with the new
route/delegate for the message port associated with this connection.

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_dispatcher_host.cc
(right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_dispatcher_host.cc:43:
MessagePortService::GetInstance()->HoldMessages(client.message_port_id);
What sanitizing / protection from a rogue Renderer process should be done on
these parameters, specifically client.message_port_id? I think of this when
seeing NavigatorConnectContext::RegisterConnection taking the message_port_id to
replace a Connection. If a Renderer is compromised, could it redirect messages
to its self that were intended for another renderer? This is the first
processing of the IPC here, so if we can validate that no harm is underway here
it would be best. Else, perhaps harden up downstream usage. Or both.

[michaeln, 2014-12-18 23:58:24]

i think this lgtm, vincent's comment word smithing is nice

[Marijn Kruisselbrink, 2014-12-19 00:09:21]

https://codereview.chromium.org/785133006/diff/30001/content/browser/message_...
File content/browser/message_port_message_filter.cc (right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/message_...
content/browser/message_port_message_filter.cc:66: // If a message port was sent
around, the new location will need a routing
On 2014/12/18 00:24:10, scheib - OOO Until Jan 5 wrote:
> This code block is more indirect now that a nesting of two helpers are doing
the
> work. The comment helps, but may be clearer as:
> 
> When |sent_message_port_ids| is non-empty generate and send |new_routing_ids|
to
> avoid waiting for the created ports to send a sync message with routing ids.

Okay, slightly rephrased the comment.

https://codereview.chromium.org/785133006/diff/30001/content/browser/message_...
File content/browser/message_port_service.cc (right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/message_...
content/browser/message_port_service.cc:15: // |delegate| is just a weak pointer
since we get notified when its process
On 2014/12/18 00:24:10, scheib - OOO Until Jan 5 wrote:
> Old comment out of date, I suggest:
> "|delegate| is a raw pointer, it calls OnMessagePortDelegateClosing before it
is
> destroyed."

Done.

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_context.cc (right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_context.cc:32: // Use
message_port_id as routing ID since there is no reason to have yet
On 2014/12/18 00:24:10, scheib - OOO Until Jan 5 wrote:
> Perhaps this comment would be best at the definition of connections_ in the
.h?
Ah yes, good point.

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_context.cc:51: // Hold
messages on all transfered ports.
On 2014/12/18 00:24:10, scheib - OOO Until Jan 5 wrote:
> comment suggestion: 
> Hold messages while service worker is found, activated, and message sent
causing
> ServiceWorkerScriptContext::OnCrossOriginMessageToWorker to construct
> WebMessagePortChannelImpl instances which send
> MessagePortHostMsg_ReleaseMessages.
> 
> (explains why the call to HoldMessages)
> (explains where/when to expect ReleaseMessages - a bit involved)

Done.

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_context.h (right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_context.h:19: // Class that
keeps track of all active navigator.connect connections. This
On 2014/12/18 00:24:10, scheib - OOO Until Jan 5 wrote:
> Tracks all active navigator.connect connections, wakes up service workers when
a
> message arrives, and routes the messages.

Done.

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_context.h:32: // Registers
an accepted connection with this class. This method takes care
On 2014/12/18 00:24:10, scheib - OOO Until Jan 5 wrote:
> Registers an accepted connection and updates MessagePortService with the new
> route/delegate for the message port associated with this connection.

Done.

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
File content/browser/navigator_connect/navigator_connect_dispatcher_host.cc
(right):

https://codereview.chromium.org/785133006/diff/30001/content/browser/navigato...
content/browser/navigator_connect/navigator_connect_dispatcher_host.cc:43:
MessagePortService::GetInstance()->HoldMessages(client.message_port_id);
On 2014/12/18 00:24:10, scheib - OOO Until Jan 5 wrote:
> What sanitizing / protection from a rogue Renderer process should be done on
> these parameters, specifically client.message_port_id? I think of this when
> seeing NavigatorConnectContext::RegisterConnection taking the message_port_id
to
> replace a Connection. If a Renderer is compromised, could it redirect messages
> to its self that were intended for another renderer? This is the first
> processing of the IPC here, so if we can validate that no harm is underway
here
> it would be best. Else, perhaps harden up downstream usage. Or both.
There are much easier ways a compromised renderer could redirect messages to
itself. A renderer could simply call MessagePortHostMsg_PostMessage to post a
message to a port it controls with any arbitrary message_port_id it wants to be
transferred to itself, so this certainly doesn't open up any holes that don't
already exist.

[Marijn Kruisselbrink, 2014-12-19 00:17:26]

mek@chromium.org changed reviewers:
+ jam@chromium.org, tsepez@chromium.org

[Marijn Kruisselbrink, 2014-12-19 00:17:27]

+jam for content/ OWNERS review
+tsepez for IPC OWNERS review

[Tom Sepez, 2014-12-19 20:54:56]

Messages LGTM

[Marijn Kruisselbrink, 2014-12-19 21:47:18]

mek@chromium.org changed reviewers:
+ avi@chromium.org

[Marijn Kruisselbrink, 2014-12-19 21:47:19]

+avi for content/ OWNERS, since jam appears to be OOO

[Avi (use Gerrit), 2014-12-19 21:58:14]

lgtm

Basically stampity stamp.

The thing that stands out to me is the use of vector for an unordered list
everywhere. I see that vector is used with a related message, but making it a
set helps for semantic understanding.

https://codereview.chromium.org/785133006/diff/70001/content/browser/message_...
File content/browser/message_port_delegate.h (right):

https://codereview.chromium.org/785133006/diff/70001/content/browser/message_...
content/browser/message_port_delegate.h:9: #include "base/strings/string16.h"
space before this line

https://codereview.chromium.org/785133006/diff/70001/content/browser/message_...
content/browser/message_port_delegate.h:23: const std::vector<int>&
sent_message_port_ids) = 0;
set?

[Marijn Kruisselbrink, 2014-12-19 22:13:07]

https://codereview.chromium.org/785133006/diff/70001/content/browser/message_...
File content/browser/message_port_delegate.h (right):

https://codereview.chromium.org/785133006/diff/70001/content/browser/message_...
content/browser/message_port_delegate.h:9: #include "base/strings/string16.h"
On 2014/12/19 21:58:14, Avi wrote:
> space before this line

Done.

https://codereview.chromium.org/785133006/diff/70001/content/browser/message_...
content/browser/message_port_delegate.h:23: const std::vector<int>&
sent_message_port_ids) = 0;
On 2014/12/19 21:58:14, Avi wrote:
> set?
I'm pretty sure this actually has to be an ordered list. The postMessage and
MessageEvent spec seem to be quite explicit that order matters for this list
(this list directly gets translated into the MessageEvents ports attribute)

[Marijn Kruisselbrink, 2014-12-19 22:28:57]

The CQ bit was checked by mek@chromium.org

[commit-bot: I haz the power, 2014-12-19 22:29:57]

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

[commit-bot: I haz the power, 2014-12-19 23:36:04]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2014-12-19 23:38:44]

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