Delete TODO-comment about guest process/frame IDs in MessageService::OpenChannelImpl

Delete TODO-comment about guest process/frame IDs in MessageService::OpenChannelImpl

Deleted a comment in MessageService::OpenChannelImpl that
questioned the use of |guest_process_id| and
|guest_render_frame_routing_id|. These ID appear to be used by some
component extensions (bug 424762) so they cannot be removed.

Committed: https://crrev.com/bfdcca7e3c8e8f96108fb07f5ac76aa36495e568
Cr-Commit-Position: refs/heads/master@{#439504}

[robwu, 2016-11-27 00:46:22]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-11-27 00:46:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robwu, 2016-11-27 00:57:01]

rob@robwu.nl changed reviewers:
+ dcheng@chromium.org, rdevlin.cronin@chromium.org

[robwu, 2016-11-27 00:57:02]

devlin: PTAL

dcheng: Please rubberstamp the small change in
extensions/common/extension_messages.h

[dcheng, 2016-11-27 01:08:19]

ipc lgtm

https://codereview.chromium.org/2529213002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/messaging/extension_message_port.cc (right):

https://codereview.chromium.org/2529213002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/messaging/extension_message_port.cc:218:
info.guest_process_id = include_guest_process_info
I thought we try not to plumb process ID information from browser to renderer...
I guess it's unavoidable here due to the api exposed by extensions.

[dcheng, 2016-11-27 01:08:47]

(Btw, please update the CL description to describe what this fixes and how too)

[commit-bot: I haz the power, 2016-11-27 02:10:16]

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

[commit-bot: I haz the power, 2016-11-27 02:10:17]

Dry run: This issue passed the CQ dry run.

[robwu, 2016-11-27 10:40:08]

Description was changed from

==========
Do not re-open extension ports after disconnect in a same-process sender

BUG=597698
TEST=./browser_tests --gtest_filter=ExtensionApiTest.MessagingBackgroundOnly
==========

to

==========
Do not re-open extension ports after disconnect in a same-process sender

When an extension message channel is created (via chrome.runtime.connect
or chrome.runtime.sendMessage), the renderer stores an identifier of the
opened port for the context/frame from where the API call was made.    
                                                                       
When the channel is not targeting a specific frame in the extension, the
IPC message to create the port for recipients is sent to the extension 
process instead of a specific RenderFrame.Normally, the above identifier
is sufficient to avoid dispatching the chrome.runtime.onConnect or     
chrome.runtime.onMessage events at the sender. But when the sender     
immediately disconnects the message port after creation, (e.g. calling 
chrome.runtime.sendMessage without callback, or port.disconnect()), then
this identifier is not present and the event is erroneously fired.     
                                                                       
To fix this, the RenderFrame ID is now included in the IPC, plus a flag
that tells whether the sender and recipient are running in the same    
process (because RenderFrame IDs are only unique within one process).  
This enables the renderer to prevent messages from reaching the frame  
that initiated the message channel.                                    
                                                                       
I also deleted a comment in MessageService::OpenChannelImpl that       
questioned the use of |guest_process_id| and                           
|guest_render_frame_routing_id|. These ID appear to be used by some    
scomponent extensions (bug 424762) so they cannot be removed.          

BUG=597698
TEST=./browser_tests --gtest_filter=ExtensionApiTest.MessagingBackgroundOnly
==========

[robwu, 2016-11-27 10:40:58]

Description was changed from

==========
Do not re-open extension ports after disconnect in a same-process sender

When an extension message channel is created (via chrome.runtime.connect
or chrome.runtime.sendMessage), the renderer stores an identifier of the
opened port for the context/frame from where the API call was made.    
                                                                       
When the channel is not targeting a specific frame in the extension, the
IPC message to create the port for recipients is sent to the extension 
process instead of a specific RenderFrame.Normally, the above identifier
is sufficient to avoid dispatching the chrome.runtime.onConnect or     
chrome.runtime.onMessage events at the sender. But when the sender     
immediately disconnects the message port after creation, (e.g. calling 
chrome.runtime.sendMessage without callback, or port.disconnect()), then
this identifier is not present and the event is erroneously fired.     
                                                                       
To fix this, the RenderFrame ID is now included in the IPC, plus a flag
that tells whether the sender and recipient are running in the same    
process (because RenderFrame IDs are only unique within one process).  
This enables the renderer to prevent messages from reaching the frame  
that initiated the message channel.                                    
                                                                       
I also deleted a comment in MessageService::OpenChannelImpl that       
questioned the use of |guest_process_id| and                           
|guest_render_frame_routing_id|. These ID appear to be used by some    
scomponent extensions (bug 424762) so they cannot be removed.          

BUG=597698
TEST=./browser_tests --gtest_filter=ExtensionApiTest.MessagingBackgroundOnly
==========

to

==========
Do not re-open extension ports after disconnect in a same-process sender

When an extension message channel is created (via chrome.runtime.connect
or chrome.runtime.sendMessage), the renderer stores an identifier of the
opened port for the context/frame from where the API call was made.    
                                                                       
When the channel is not targeting a specific frame in the extension, the
IPC message to create the port for recipients is sent to the extension 
process instead of a specific RenderFrame.Normally, the above identifier
is sufficient to avoid dispatching the chrome.runtime.onConnect or     
chrome.runtime.onMessage events at the sender. But when the sender     
immediately disconnects the message port after creation, (e.g. calling 
chrome.runtime.sendMessage without callback, or port.disconnect()), then
this identifier is not present and the event is erroneously fired.     
                                                                       
To fix this, the RenderFrame ID is now included in the IPC, plus a flag
that tells whether the sender and recipient are running in the same    
process (because RenderFrame IDs are only unique within one process).  
This enables the renderer to prevent messages from reaching the frame  
that initiated the message channel.                                    
                                                                       
I also deleted a comment in MessageService::OpenChannelImpl that       
questioned the use of |guest_process_id| and                           
|guest_render_frame_routing_id|. These ID appear to be used by some    
component extensions (bug 424762) so they cannot be removed.          

BUG=597698
TEST=./browser_tests --gtest_filter=ExtensionApiTest.MessagingBackgroundOnly
==========

[Devlin, 2016-11-28 23:49:51]

I agree with dcheng that it'd be nice to limit what we send - I'd like to see
how much we can avoid passing more information to the renderer in these types of
messages (or to the browser, for that matter).

In theory, we should be able to tell which ports were used by a given context. 
Would it make sense to track them renderer side instead, and not pass around
this information?  It would mean that we need to maintain a set of ints for all
contexts, but hopefully there aren't enough messages that that's a problem. 
WDYT?

[robwu, 2016-11-28 23:55:46]

On 2016/11/28 23:49:51, Devlin wrote:
> I agree with dcheng that it'd be nice to limit what we send - I'd like to see
> how much we can avoid passing more information to the renderer in these types
of
> messages (or to the browser, for that matter).
> 
> In theory, we should be able to tell which ports were used by a given context.

> Would it make sense to track them renderer side instead, and not pass around
> this information?  It would mean that we need to maintain a set of ints for
all
> contexts, but hopefully there aren't enough messages that that's a problem. 
> WDYT?

I have thought about this. That would then require keeping the port IDs valid
until the message has been acknowledged, and also maintain a flag to keep track
of whether the port ID is still valid. I went for the current approach because
it is much simpler than the bookkeeping that the other method would need.

If you want to limit the information even more, then I can send the render frame
ID down only if the sender is a webview, or for same processes (=sender and
receiver are both in an extension process) (in other cases the render frame ID
is not used). Should I update the patch?

[Devlin, 2016-11-29 00:09:34]

On 2016/11/28 23:55:46, robwu wrote:
> On 2016/11/28 23:49:51, Devlin wrote:
> > I agree with dcheng that it'd be nice to limit what we send - I'd like to
see
> > how much we can avoid passing more information to the renderer in these
types
> of
> > messages (or to the browser, for that matter).
> > 
> > In theory, we should be able to tell which ports were used by a given
context.
> 
> > Would it make sense to track them renderer side instead, and not pass around
> > this information?  It would mean that we need to maintain a set of ints for
> all
> > contexts, but hopefully there aren't enough messages that that's a problem. 
> > WDYT?
> 
> I have thought about this. That would then require keeping the port IDs valid
> until the message has been acknowledged, and also maintain a flag to keep
track
> of whether the port ID is still valid. I went for the current approach because
> it is much simpler than the bookkeeping that the other method would need.
> 
> If you want to limit the information even more, then I can send the render
frame
> ID down only if the sender is a webview, or for same processes (=sender and
> receiver are both in an extension process) (in other cases the render frame ID
> is not used). Should I update the patch?

Why would we need a flag?  We never reuse port ids, so we should just be able to
track the ones we've ever opened, right?

[robwu, 2016-11-29 00:12:52]

On 2016/11/29 00:09:34, Devlin wrote:
> On 2016/11/28 23:55:46, robwu wrote:
> > On 2016/11/28 23:49:51, Devlin wrote:
> > > I agree with dcheng that it'd be nice to limit what we send - I'd like to
> see
> > > how much we can avoid passing more information to the renderer in these
> types
> > of
> > > messages (or to the browser, for that matter).
> > > 
> > > In theory, we should be able to tell which ports were used by a given
> context.
> > 
> > > Would it make sense to track them renderer side instead, and not pass
around
> > > this information?  It would mean that we need to maintain a set of ints
for
> > all
> > > contexts, but hopefully there aren't enough messages that that's a
problem. 
> > > WDYT?
> > 
> > I have thought about this. That would then require keeping the port IDs
valid
> > until the message has been acknowledged, and also maintain a flag to keep
> track
> > of whether the port ID is still valid. I went for the current approach
because
> > it is much simpler than the bookkeeping that the other method would need.
> > 
> > If you want to limit the information even more, then I can send the render
> frame
> > ID down only if the sender is a webview, or for same processes (=sender and
> > receiver are both in an extension process) (in other cases the render frame
ID
> > is not used). Should I update the patch?
> 
> Why would we need a flag?  We never reuse port ids, so we should just be able
to
> track the ones we've ever opened, right?

Either a flag with the ID, or a separate set for keeping the IDs. Either way is
more complex than the current patch.

[Devlin, 2016-11-29 01:03:01]

On 2016/11/29 00:12:52, robwu wrote:
> On 2016/11/29 00:09:34, Devlin wrote:
> > On 2016/11/28 23:55:46, robwu wrote:
> > > On 2016/11/28 23:49:51, Devlin wrote:
> > > > I agree with dcheng that it'd be nice to limit what we send - I'd like
to
> > see
> > > > how much we can avoid passing more information to the renderer in these
> > types
> > > of
> > > > messages (or to the browser, for that matter).
> > > > 
> > > > In theory, we should be able to tell which ports were used by a given
> > context.
> > > 
> > > > Would it make sense to track them renderer side instead, and not pass
> around
> > > > this information?  It would mean that we need to maintain a set of ints
> for
> > > all
> > > > contexts, but hopefully there aren't enough messages that that's a
> problem. 
> > > > WDYT?
> > > 
> > > I have thought about this. That would then require keeping the port IDs
> valid
> > > until the message has been acknowledged, and also maintain a flag to keep
> > track
> > > of whether the port ID is still valid. I went for the current approach
> because
> > > it is much simpler than the bookkeeping that the other method would need.
> > > 
> > > If you want to limit the information even more, then I can send the render
> > frame
> > > ID down only if the sender is a webview, or for same processes (=sender
and
> > > receiver are both in an extension process) (in other cases the render
frame
> ID
> > > is not used). Should I update the patch?
> > 
> > Why would we need a flag?  We never reuse port ids, so we should just be
able
> to
> > track the ones we've ever opened, right?
> 
> Either a flag with the ID, or a separate set for keeping the IDs. Either way
is
> more complex than the current patch.

It seems like we should be able to add a set, and then check it in
messaging_bindings.cc, but thus avoid any changes to the browser side or IPC
messages.  I don't see why the set would be very complicated, so it seems like
that should be simpler, or at worst on par with, the current patch in terms of
complexity.  Is there a catch that I'm missing?  (I haven't investigated this
thoroughly, so right now it just seems like it should work.)

[robwu, 2016-11-29 01:34:26]

On 2016/11/29 01:03:01, Devlin wrote:
> On 2016/11/29 00:12:52, robwu wrote:
> > On 2016/11/29 00:09:34, Devlin wrote:
> > > On 2016/11/28 23:55:46, robwu wrote:
> > > > On 2016/11/28 23:49:51, Devlin wrote:
> > > > > I agree with dcheng that it'd be nice to limit what we send - I'd like
> to
> > > see
> > > > > how much we can avoid passing more information to the renderer in
these
> > > types
> > > > of
> > > > > messages (or to the browser, for that matter).
> > > > > 
> > > > > In theory, we should be able to tell which ports were used by a given
> > > context.
> > > > 
> > > > > Would it make sense to track them renderer side instead, and not pass
> > around
> > > > > this information?  It would mean that we need to maintain a set of
ints
> > for
> > > > all
> > > > > contexts, but hopefully there aren't enough messages that that's a
> > problem. 
> > > > > WDYT?
> > > > 
> > > > I have thought about this. That would then require keeping the port IDs
> > valid
> > > > until the message has been acknowledged, and also maintain a flag to
keep
> > > track
> > > > of whether the port ID is still valid. I went for the current approach
> > because
> > > > it is much simpler than the bookkeeping that the other method would
need.
> > > > 
> > > > If you want to limit the information even more, then I can send the
render
> > > frame
> > > > ID down only if the sender is a webview, or for same processes (=sender
> and
> > > > receiver are both in an extension process) (in other cases the render
> frame
> > ID
> > > > is not used). Should I update the patch?
> > > 
> > > Why would we need a flag?  We never reuse port ids, so we should just be
> able
> > to
> > > track the ones we've ever opened, right?
> > 
> > Either a flag with the ID, or a separate set for keeping the IDs. Either way
> is
> > more complex than the current patch.
> 
> It seems like we should be able to add a set, and then check it in
> messaging_bindings.cc, but thus avoid any changes to the browser side or IPC
> messages.  I don't see why the set would be very complicated, so it seems like
> that should be simpler, or at worst on par with, the current patch in terms of
> complexity.  Is there a catch that I'm missing?  (I haven't investigated this
> thoroughly, so right now it just seems like it should work.)

(From the top of my head, based on how the ports were implemented before you
introduced the local/global port IDs):
- When a port is opened, the browser does not send an ack message to the opener.
- When a port is disconnected explicitly, the browser does not send an explicit
ack to the caller of disconnect().
- If a port is opened and immediately disconnected, the port should be marked as
deleted, and erased from the set once it's known that no further messages for
onConnect will arrive.
- In the same-process case, we could wait until the onConnect message has been
received.
- But in the cross-process case, this is not possible (because of the lack of
acknowledgement messages)

(Yet an alternative way of fixing the issue is to keep the messages in the
process, without consulting the browser (when the sender and recipients are in
the same process). This is more risky to implement, so I want to fix the current
implementation before even considering this option.)

[Devlin, 2016-11-30 19:05:25]

On 2016/11/29 01:34:26, robwu wrote:
> On 2016/11/29 01:03:01, Devlin wrote:
> > On 2016/11/29 00:12:52, robwu wrote:
> > > On 2016/11/29 00:09:34, Devlin wrote:
> > > > On 2016/11/28 23:55:46, robwu wrote:
> > > > > On 2016/11/28 23:49:51, Devlin wrote:
> > > > > > I agree with dcheng that it'd be nice to limit what we send - I'd
like
> > to
> > > > see
> > > > > > how much we can avoid passing more information to the renderer in
> these
> > > > types
> > > > > of
> > > > > > messages (or to the browser, for that matter).
> > > > > > 
> > > > > > In theory, we should be able to tell which ports were used by a
given
> > > > context.
> > > > > 
> > > > > > Would it make sense to track them renderer side instead, and not
pass
> > > around
> > > > > > this information?  It would mean that we need to maintain a set of
> ints
> > > for
> > > > > all
> > > > > > contexts, but hopefully there aren't enough messages that that's a
> > > problem. 
> > > > > > WDYT?
> > > > > 
> > > > > I have thought about this. That would then require keeping the port
IDs
> > > valid
> > > > > until the message has been acknowledged, and also maintain a flag to
> keep
> > > > track
> > > > > of whether the port ID is still valid. I went for the current approach
> > > because
> > > > > it is much simpler than the bookkeeping that the other method would
> need.
> > > > > 
> > > > > If you want to limit the information even more, then I can send the
> render
> > > > frame
> > > > > ID down only if the sender is a webview, or for same processes
(=sender
> > and
> > > > > receiver are both in an extension process) (in other cases the render
> > frame
> > > ID
> > > > > is not used). Should I update the patch?
> > > > 
> > > > Why would we need a flag?  We never reuse port ids, so we should just be
> > able
> > > to
> > > > track the ones we've ever opened, right?
> > > 
> > > Either a flag with the ID, or a separate set for keeping the IDs. Either
way
> > is
> > > more complex than the current patch.
> > 
> > It seems like we should be able to add a set, and then check it in
> > messaging_bindings.cc, but thus avoid any changes to the browser side or IPC
> > messages.  I don't see why the set would be very complicated, so it seems
like
> > that should be simpler, or at worst on par with, the current patch in terms
of
> > complexity.  Is there a catch that I'm missing?  (I haven't investigated
this
> > thoroughly, so right now it just seems like it should work.)
> 
> (From the top of my head, based on how the ports were implemented before you
> introduced the local/global port IDs):
> - When a port is opened, the browser does not send an ack message to the
opener.
> - When a port is disconnected explicitly, the browser does not send an
explicit
> ack to the caller of disconnect().
> - If a port is opened and immediately disconnected, the port should be marked
as
> deleted, and erased from the set once it's known that no further messages for
> onConnect will arrive.
> - In the same-process case, we could wait until the onConnect message has been
> received.
> - But in the cross-process case, this is not possible (because of the lack of
> acknowledgement messages)
> 
> (Yet an alternative way of fixing the issue is to keep the messages in the
> process, without consulting the browser (when the sender and recipients are in
> the same process). This is more risky to implement, so I want to fix the
current
> implementation before even considering this option.)

This is what I was trying to describe:
https://codereview.chromium.org/2540783005

Note that's only a proof-of-concept, and something to commit in its current
state - but works to illustrate what I'm describing.  It seems to fix the
problem; apart from the growing set for ids, is there a downside?

[robwu, 2016-11-30 21:41:32]

On 2016/11/30 19:05:25, Devlin wrote:
> On 2016/11/29 01:34:26, robwu wrote:
> > On 2016/11/29 01:03:01, Devlin wrote:
> > > It seems like we should be able to add a set, and then check it in
> > > messaging_bindings.cc, but thus avoid any changes to the browser side or
IPC
> > > messages.  I don't see why the set would be very complicated, so it seems
> like
> > > that should be simpler, or at worst on par with, the current patch in
terms
> of
> > > complexity.  Is there a catch that I'm missing?  (I haven't investigated
> this
> > > thoroughly, so right now it just seems like it should work.)
> > 
> > (From the top of my head, based on how the ports were implemented before you
> > introduced the local/global port IDs):
> > - When a port is opened, the browser does not send an ack message to the
> opener.
> > - When a port is disconnected explicitly, the browser does not send an
> explicit
> > ack to the caller of disconnect().
> > - If a port is opened and immediately disconnected, the port should be
marked
> as
> > deleted, and erased from the set once it's known that no further messages
for
> > onConnect will arrive.
> > - In the same-process case, we could wait until the onConnect message has
been
> > received.
> > - But in the cross-process case, this is not possible (because of the lack
of
> > acknowledgement messages)
> > 
> > (Yet an alternative way of fixing the issue is to keep the messages in the
> > process, without consulting the browser (when the sender and recipients are
in
> > the same process). This is more risky to implement, so I want to fix the
> current
> > implementation before even considering this option.)
> 
> This is what I was trying to describe:
> https://codereview.chromium.org/2540783005
> 
> Note that's only a proof-of-concept, and something to commit in its current
> state - but works to illustrate what I'm describing.  It seems to fix the
> problem; apart from the growing set for ids, is there a downside?

If you think that the ever-growing set of IDs is not a problem, then there is no
downside - your patch fixes the bug.
However, I think that we should not have an ever-growing set of IDs if we can
avoid it. So we would have to erase the port ID when the ID is no longer
relevant. We only care about onConnect because there are no user-visible
consequences if an onMessage or onDisconnected message arrives for a
non-existing port.

A port can be disconnected in two ways: 1) explicitly via disconnect() and 2)
implicitly via unloading the context. I guess that the second won't cause issues
in practice (i.e. connect, navigate to a different page and receive onConnect in
that page for the connect from the previous page), so let's focus on the first
one.

If you open a port via connect(), then you have to move the port ID
(local/global, whichever applies) to a set of "deleted" port IDs, and then in
DidCreatePortWithGlobalId check whether the ID is in this set. This set of
deleted IDs should live until the browser has sent messages for onConnect.
For messages to the same extension process, we know that the browser will send a
message, and so we can remove the ID from the set when the onConnect message is
received.
For messages to a different process, there is no signal that the browser
finished sending onConnect messages (if you were thinking of onDisconnect - this
event is only fired on the opposite port). So the list of deleted port IDs would
stay around forever (until the context is destroyed).

[Devlin, 2016-11-30 22:48:37]

On 2016/11/30 21:41:32, robwu wrote:
> If you think that the ever-growing set of IDs is not a problem, then there is
no
> downside - your patch fixes the bug.
> However, I think that we should not have an ever-growing set of IDs if we can
> avoid it. So we would have to erase the port ID when the ID is no longer
> relevant. We only care about onConnect because there are no user-visible
> consequences if an onMessage or onDisconnected message arrives for a
> non-existing port.
> 
> A port can be disconnected in two ways: 1) explicitly via disconnect() and 2)
> implicitly via unloading the context. I guess that the second won't cause
issues
> in practice (i.e. connect, navigate to a different page and receive onConnect
in
> that page for the connect from the previous page), so let's focus on the first
> one.
> 
> If you open a port via connect(), then you have to move the port ID
> (local/global, whichever applies) to a set of "deleted" port IDs, and then in
> DidCreatePortWithGlobalId check whether the ID is in this set. This set of
> deleted IDs should live until the browser has sent messages for onConnect.
> For messages to the same extension process, we know that the browser will send
a
> message, and so we can remove the ID from the set when the onConnect message
is
> received.
> For messages to a different process, there is no signal that the browser
> finished sending onConnect messages (if you were thinking of onDisconnect -
this
> event is only fired on the opposite port). So the list of deleted port IDs
would
> stay around forever (until the context is destroyed).

Hmm... I'm not really comfortable with this version, either.

Okay.  I think that I might be able to kill two birds with one stone here and
tackle the TODO at [1] while also fixing this.  Lemme hack around a bit and get
back to you. :)

[1]
https://cs.chromium.org/chromium/src/extensions/renderer/messaging_bindings.c...

[Devlin, 2016-12-09 16:25:19]

With
https://chromium.googlesource.com/chromium/src/+/8e744a465d70e2ab1bf7959a66c3...
landed, can we close this patch?

[robwu, 2016-12-19 07:53:34]

Description was changed from

==========
Do not re-open extension ports after disconnect in a same-process sender

When an extension message channel is created (via chrome.runtime.connect
or chrome.runtime.sendMessage), the renderer stores an identifier of the
opened port for the context/frame from where the API call was made.    
                                                                       
When the channel is not targeting a specific frame in the extension, the
IPC message to create the port for recipients is sent to the extension 
process instead of a specific RenderFrame.Normally, the above identifier
is sufficient to avoid dispatching the chrome.runtime.onConnect or     
chrome.runtime.onMessage events at the sender. But when the sender     
immediately disconnects the message port after creation, (e.g. calling 
chrome.runtime.sendMessage without callback, or port.disconnect()), then
this identifier is not present and the event is erroneously fired.     
                                                                       
To fix this, the RenderFrame ID is now included in the IPC, plus a flag
that tells whether the sender and recipient are running in the same    
process (because RenderFrame IDs are only unique within one process).  
This enables the renderer to prevent messages from reaching the frame  
that initiated the message channel.                                    
                                                                       
I also deleted a comment in MessageService::OpenChannelImpl that       
questioned the use of |guest_process_id| and                           
|guest_render_frame_routing_id|. These ID appear to be used by some    
component extensions (bug 424762) so they cannot be removed.          

BUG=597698
TEST=./browser_tests --gtest_filter=ExtensionApiTest.MessagingBackgroundOnly
==========

to

==========
Delete TODO-comment about guest process/frame IDs in
MessageService::OpenChannelImpl

Deleted a comment in MessageService::OpenChannelImpl that
questioned the use of |guest_process_id| and
|guest_render_frame_routing_id|. These ID appear to be used by some
component extensions (bug 424762) so they cannot be removed.
==========

[robwu, 2016-12-19 07:53:34]

rob@robwu.nl changed reviewers:
- dcheng@chromium.org

[robwu, 2016-12-19 07:55:16]

On 2016/12/09 16:25:19, Devlin wrote:
> With
>
https://chromium.googlesource.com/chromium/src/+/8e744a465d70e2ab1bf7959a66c3...
> landed, can we close this patch?

I reverted all changes (since your patch supersedes this one), except for
deletion of a TODO comment.
Can you take a look?

Removed dcheng as reviewer because the changes are not related to IPC any more
(for some reason codereview still shows dcheng at the sidebar, but w/e).

[Devlin, 2016-12-19 16:53:12]

I still don't like the fact we need these, but that seems to be a very deep
rabbit hole indeed. :(

lgtm

[robwu, 2016-12-19 17:19:26]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-12-19 17:19:26]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2529213002/#ps20001
(title: "Revert all except for comment change")

[commit-bot: I haz the power, 2016-12-19 17:19:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 18:05:26]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482167966094330,
"parent_rev": "b7bc72aeae97f244cda31be4ec1121cf7b466721", "commit_rev":
"45274c4148447ac733fc2108d743e9ae84e2860f"}

[commit-bot: I haz the power, 2016-12-19 18:05:52]

Description was changed from

==========
Delete TODO-comment about guest process/frame IDs in
MessageService::OpenChannelImpl

Deleted a comment in MessageService::OpenChannelImpl that
questioned the use of |guest_process_id| and
|guest_render_frame_routing_id|. These ID appear to be used by some
component extensions (bug 424762) so they cannot be removed.
==========

to

==========
Delete TODO-comment about guest process/frame IDs in
MessageService::OpenChannelImpl

Deleted a comment in MessageService::OpenChannelImpl that
questioned the use of |guest_process_id| and
|guest_render_frame_routing_id|. These ID appear to be used by some
component extensions (bug 424762) so they cannot be removed.

Review-Url: https://codereview.chromium.org/2529213002
==========

[commit-bot: I haz the power, 2016-12-19 18:05:53]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-19 18:11:38]

Description was changed from

==========
Delete TODO-comment about guest process/frame IDs in
MessageService::OpenChannelImpl

Deleted a comment in MessageService::OpenChannelImpl that
questioned the use of |guest_process_id| and
|guest_render_frame_routing_id|. These ID appear to be used by some
component extensions (bug 424762) so they cannot be removed.

Review-Url: https://codereview.chromium.org/2529213002
==========

to

==========
Delete TODO-comment about guest process/frame IDs in
MessageService::OpenChannelImpl

Deleted a comment in MessageService::OpenChannelImpl that
questioned the use of |guest_process_id| and
|guest_render_frame_routing_id|. These ID appear to be used by some
component extensions (bug 424762) so they cannot be removed.

Committed: https://crrev.com/bfdcca7e3c8e8f96108fb07f5ac76aa36495e568
Cr-Commit-Position: refs/heads/master@{#439504}
==========

[commit-bot: I haz the power, 2016-12-19 18:11:39]

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