Browser Plugin: Removed BrowserPluginWebContentsObserver (to be replaced by rewritten BrowserPlugi…

Browser Plugin: Removed BrowserPluginWebContentsObserver (to be replaced by rewritten BrowserPluginHost in subsequent patch). Plumbed embedder_process_id and embedder_routing_id.

BUG=127987
TEST=manually

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137319

[Fady Samuel, 2012-05-10 15:23:22]

Hi Charlie,

This is the beginning of the plumbing for cross-process navigation support in
the browser plugin. Some older upstreamed code is being removed because it
doesn't do what we want. Will send out some other patches for review in the near
future. I'm trying to get all the changes that touch files outside of
"browser_plugin" directories landed first and then land "browser_plugin"
directory changes in one fell swoop.

John: Review as OWNER please.

Thanks,

Fady

[Charlie Reis, 2012-05-11 17:41:03]

Sorry for the delay.  Mostly looks good, but see the question about exposing the
pending RVH.

Also, please give the CL a bug number, since this is an important effort we'd
like to be able to track later.  Thanks!

http://codereview.chromium.org/10378089/diff/2001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.h:239: // RenderView is told
to start issuing page IDs at |max_page_id| + 1.
Nit: Why move the opener_route_id comment to the end if it's the second
parameter?

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
File content/browser/web_contents/interstitial_page_impl.cc (right):

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
content/browser/web_contents/interstitial_page_impl.cc:510: max_page_id, 0, 0);
It might be more consistent with other ID schemes to use -1 or a constant.  For
example, the second one should be MSG_ROUTING_NONE.

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:144:
GetPendingRenderProcessHost() const OVERRIDE;
This is already accessible via the RenderViewHost.  We should not expose it
here.

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:146: virtual
content::RenderViewHost* GetPendingRenderViewHost() const OVERRIDE;
I'm hesitant to expose this outside of WebContentsImpl, and certainly outside of
the content module.  It exposes a great deal of complexity that has been best to
keep inside WebContentsImpl so far.

I could be convinced if there's sufficient cause, but we may also want to think
if there's a better way to achieve the same goal.

http://codereview.chromium.org/10378089/diff/2001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/common/view_message...
content/common/view_messages.h:31: #include "ppapi/c/pp_instance.h"
I don't see where you're using this.

http://codereview.chromium.org/10378089/diff/2001/content/common/view_message...
content/common/view_messages.h:654: // The process id of the RenderProcessHost
of the embedder of this view if it
nit: id -> ID

http://codereview.chromium.org/10378089/diff/2001/content/common/view_message...
content/common/view_messages.h:659: // The process ID + routing ID uniquely
identify the RenderViewHost
Please be specific about embedder_process_id and embedder_routing_id, since
"process ID + routing ID" refer to the current RVH, not the embedder.

http://codereview.chromium.org/10378089/diff/2001/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/10378089/diff/2001/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:881: false,
Is it too soon to remove this parameter and pass in the new IDs?

[Fady Samuel, 2012-05-11 22:03:34]

http://codereview.chromium.org/10378089/diff/2001/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.h:239: // RenderView is told
to start issuing page IDs at |max_page_id| + 1.
On 2012/05/11 17:41:03, creis wrote:
> Nit: Why move the opener_route_id comment to the end if it's the second
> parameter?

Done.

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
File content/browser/web_contents/interstitial_page_impl.cc (right):

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
content/browser/web_contents/interstitial_page_impl.cc:510: max_page_id, 0, 0);
On 2012/05/11 17:41:03, creis wrote:
> It might be more consistent with other ID schemes to use -1 or a constant. 
For
> example, the second one should be MSG_ROUTING_NONE.

Done.

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:144:
GetPendingRenderProcessHost() const OVERRIDE;
On 2012/05/11 17:41:03, creis wrote:
> This is already accessible via the RenderViewHost.  We should not expose it
> here.

Done.

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:146: virtual
content::RenderViewHost* GetPendingRenderViewHost() const OVERRIDE;
On 2012/05/11 17:41:03, creis wrote:
> I'm hesitant to expose this outside of WebContentsImpl, and certainly outside
of
> the content module.  It exposes a great deal of complexity that has been best
to
> keep inside WebContentsImpl so far.
> 
> I could be convinced if there's sufficient cause, but we may also want to
think
> if there's a better way to achieve the same goal.


This doesn't really need to be exposed outside the content module. We need it in
WebContentsImpl though because BrowserPluginHost needs to direct messages to the
new render process to tell it to begin compositing.

http://codereview.chromium.org/10378089/diff/2001/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/common/view_message...
content/common/view_messages.h:31: #include "ppapi/c/pp_instance.h"
On 2012/05/11 17:41:03, creis wrote:
> I don't see where you're using this.

Done.

http://codereview.chromium.org/10378089/diff/2001/content/common/view_message...
content/common/view_messages.h:654: // The process id of the RenderProcessHost
of the embedder of this view if it
On 2012/05/11 17:41:03, creis wrote:
> nit: id -> ID

Done.

http://codereview.chromium.org/10378089/diff/2001/content/common/view_message...
content/common/view_messages.h:659: // The process ID + routing ID uniquely
identify the RenderViewHost
On 2012/05/11 17:41:03, creis wrote:
> Please be specific about embedder_process_id and embedder_routing_id, since
> "process ID + routing ID" refer to the current RVH, not the embedder.

Done.

http://codereview.chromium.org/10378089/diff/2001/content/renderer/render_thr...
File content/renderer/render_thread_impl.cc (right):

http://codereview.chromium.org/10378089/diff/2001/content/renderer/render_thr...
content/renderer/render_thread_impl.cc:881: false,
On 2012/05/11 17:41:03, creis wrote:
> Is it too soon to remove this parameter and pass in the new IDs?

Yea, I currently don't pass these to RenderViewImpl, instead I have a
BrowserPluginChannelManager that creates or looks up an existing
GuestToEmbedderChannel. I need to land BrowserPluginChannelManager and
GuestToEmbedderChannel to make that change to RenderViewImpl. This will come in
a subsequent patch.

[jam, 2012-05-11 22:06:41]

http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
File content/public/browser/web_contents.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
content/public/browser/web_contents.h:102: virtual RenderProcessHost*
GetPendingRenderProcessHost() const = 0;
1) only stuff used by embedders of content should be in the public API
2) the concept of pending RVH is an internal implementation detail of
WebContentsImpl that we don't expose by design

happy to talk more about this on Monday, or you can ask brettw who wrote this

[Fady Samuel, 2012-05-11 22:08:01]

http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
File content/public/browser/web_contents.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
content/public/browser/web_contents.h:102: virtual RenderProcessHost*
GetPendingRenderProcessHost() const = 0;
On 2012/05/11 22:06:43, John Abd-El-Malek wrote:
> 1) only stuff used by embedders of content should be in the public API
> 2) the concept of pending RVH is an internal implementation detail of
> WebContentsImpl that we don't expose by design
> 
> happy to talk more about this on Monday, or you can ask brettw who wrote this

Yup, just got rid of it. This is not absolutely necessary, but I'll need to cast
from WebContents to WebContentsImpl elsewhere.

[Charlie Reis, 2012-05-11 22:16:47]

Thanks.  One question below, and don't forget the bug number on the CL.

Apart from that it looks ready.

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
content/browser/web_contents/web_contents_impl.h:146: virtual
content::RenderViewHost* GetPendingRenderViewHost() const OVERRIDE;
On 2012/05/11 22:03:34, Fady Samuel wrote:
> On 2012/05/11 17:41:03, creis wrote:
> > I'm hesitant to expose this outside of WebContentsImpl, and certainly
outside
> of
> > the content module.  It exposes a great deal of complexity that has been
best
> to
> > keep inside WebContentsImpl so far.
> > 
> > I could be convinced if there's sufficient cause, but we may also want to
> think
> > if there's a better way to achieve the same goal.
> 
> 
> This doesn't really need to be exposed outside the content module. We need it
in
> WebContentsImpl though because BrowserPluginHost needs to direct messages to
the
> new render process to tell it to begin compositing.

Would it be possible to put a method on WebContentsImpl to accomplish this for
you, rather than exposing the pending RVH outside of WebContentsImpl (even
within the content module)?

[Fady Samuel, 2012-05-14 02:05:20]

On 2012/05/11 22:16:47, creis wrote:
> Thanks.  One question below, and don't forget the bug number on the CL.
> 
> Apart from that it looks ready.
> 
>
http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
> File content/browser/web_contents/web_contents_impl.h (right):
> 
>
http://codereview.chromium.org/10378089/diff/2001/content/browser/web_content...
> content/browser/web_contents/web_contents_impl.h:146: virtual
> content::RenderViewHost* GetPendingRenderViewHost() const OVERRIDE;
> On 2012/05/11 22:03:34, Fady Samuel wrote:
> > On 2012/05/11 17:41:03, creis wrote:
> > > I'm hesitant to expose this outside of WebContentsImpl, and certainly
> outside
> > of
> > > the content module.  It exposes a great deal of complexity that has been
> best
> > to
> > > keep inside WebContentsImpl so far.
> > > 
> > > I could be convinced if there's sufficient cause, but we may also want to
> > think
> > > if there's a better way to achieve the same goal.
> > 
> > 
> > This doesn't really need to be exposed outside the content module. We need
it
> in
> > WebContentsImpl though because BrowserPluginHost needs to direct messages to
> the
> > new render process to tell it to begin compositing.
> 
> Would it be possible to put a method on WebContentsImpl to accomplish this for
> you, rather than exposing the pending RVH outside of WebContentsImpl (even
> within the content module)?

Updated with bug #. I'm not sure if there's a good way to do this.

In BrowserPluginHost (see
http://codereview.chromium.org/9609008/patch/144002/144007) there are two uses
for the pending RenderViewHost/RenderProcessHost:

1. On BrowserPluginHost::OnMapInstance to send a message to the right
RenderView.
2. BrowserPluginHost::OnConnectToChannel to get the right RenderProcessHost's
ID.

[jam, 2012-05-14 04:48:15]

http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
File content/public/browser/web_contents.h (right):

http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
content/public/browser/web_contents.h:102: virtual RenderProcessHost*
GetPendingRenderProcessHost() const = 0;
On 2012/05/11 22:08:01, Fady Samuel wrote:
> On 2012/05/11 22:06:43, John Abd-El-Malek wrote:
> > 1) only stuff used by embedders of content should be in the public API
> > 2) the concept of pending RVH is an internal implementation detail of
> > WebContentsImpl that we don't expose by design
> > 
> > happy to talk more about this on Monday, or you can ask brettw who wrote
this
> 
> Yup, just got rid of it. This is not absolutely necessary, but I'll need to
cast
> from WebContents to WebContentsImpl elsewhere.

what I meant by 2 is that the whole concept of pending RVHs shouldn't be exposed
outside of WebContents. a lot of work was done to hide this (i.e. see how
RenderViewHostManager isn't exposed now, except only for tests)

[jam, 2012-05-14 04:49:21]

http://codereview.chromium.org/10378089/diff/13001/content/common/view_messag...
File content/common/view_messages.h (right):

http://codereview.chromium.org/10378089/diff/13001/content/common/view_messag...
content/common/view_messages.h:656: IPC_STRUCT_MEMBER(int32,
embedder_process_id)
we have avoided making renderers aware of process ids. why is this needed? can't
we keep this bookkeeping in the browser?

[Fady Samuel, 2012-05-14 11:33:19]

On 2012/05/14 04:49:21, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/10378089/diff/13001/content/common/view_messag...
> File content/common/view_messages.h (right):
> 
>
http://codereview.chromium.org/10378089/diff/13001/content/common/view_messag...
> content/common/view_messages.h:656: IPC_STRUCT_MEMBER(int32,
> embedder_process_id)
> we have avoided making renderers aware of process ids. why is this needed?
can't
> we keep this bookkeeping in the browser?

I need a way to uniquely identify an embedder's process in the renderer so that:

1. Create a unique channel handle (I haven't looked at the internal
implementation of channel creation on all platforms but I assume the channel
name needs to be unique?

2. Store GuestToEmbedderChannels in a map that is keyed on the process id. 

Embedder Routing ID doesn't actually seem to be necessary here anymore. A recent
refactoring has rendered it unused.

[Fady Samuel, 2012-05-14 11:38:35]

On 2012/05/14 04:48:15, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
> File content/public/browser/web_contents.h (right):
> 
>
http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
> content/public/browser/web_contents.h:102: virtual RenderProcessHost*
> GetPendingRenderProcessHost() const = 0;
> On 2012/05/11 22:08:01, Fady Samuel wrote:
> > On 2012/05/11 22:06:43, John Abd-El-Malek wrote:
> > > 1) only stuff used by embedders of content should be in the public API
> > > 2) the concept of pending RVH is an internal implementation detail of
> > > WebContentsImpl that we don't expose by design
> > > 
> > > happy to talk more about this on Monday, or you can ask brettw who wrote
> this
> > 
> > Yup, just got rid of it. This is not absolutely necessary, but I'll need to
> cast
> > from WebContents to WebContentsImpl elsewhere.
> 
> what I meant by 2 is that the whole concept of pending RVHs shouldn't be
exposed
> outside of WebContents. a lot of work was done to hide this (i.e. see how
> RenderViewHostManager isn't exposed now, except only for tests)

I want to expose the concept of "current" RenderViewHost to the browser plugin:

GetPendingRenderViewHost() ? GetPendingRenderViewHost() : GetRenderViewHost().

Does this seem reasonable?

Without exposing this, messages get sent to the wrong place, because the browser
plugin needs to hook into cross-process navigation.

Is there a better way to do? Perhaps adding a Send method to WebContents?

[Fady Samuel, 2012-05-14 14:44:09]

Removed need for embedder_routing_id.

Currently the only need for the embedder_process_id is to be able to uniquely
key GuestToEmbedderChannels based on the embedder. Is there an alternative way
to do this?

[Fady Samuel, 2012-05-14 14:44:10]

Removed need for embedder_routing_id.

Currently the only need for the embedder_process_id is to be able to uniquely
key GuestToEmbedderChannels based on the embedder. Is there an alternative way
to do this?

[jam, 2012-05-14 15:30:06]

On 2012/05/14 11:33:19, Fady Samuel wrote:
> On 2012/05/14 04:49:21, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/10378089/diff/13001/content/common/view_messag...
> > File content/common/view_messages.h (right):
> > 
> >
>
http://codereview.chromium.org/10378089/diff/13001/content/common/view_messag...
> > content/common/view_messages.h:656: IPC_STRUCT_MEMBER(int32,
> > embedder_process_id)
> > we have avoided making renderers aware of process ids. why is this needed?
> can't
> > we keep this bookkeeping in the browser?
> 
> I need a way to uniquely identify an embedder's process in the renderer so
that:
> 
> 1. Create a unique channel handle (I haven't looked at the internal
> implementation of channel creation on all platforms but I assume the channel
> name needs to be unique?

how about having the browser process send down the channel name instead of the
process id?

> 
> 2. Store GuestToEmbedderChannels in a map that is keyed on the process id. 

with the above, you can have the map key be the channel name
> 
> Embedder Routing ID doesn't actually seem to be necessary here anymore. A
recent
> refactoring has rendered it unused.

[jam, 2012-05-14 15:33:49]

On 2012/05/14 11:38:35, Fady Samuel wrote:
> On 2012/05/14 04:48:15, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
> > File content/public/browser/web_contents.h (right):
> > 
> >
>
http://codereview.chromium.org/10378089/diff/2001/content/public/browser/web_...
> > content/public/browser/web_contents.h:102: virtual RenderProcessHost*
> > GetPendingRenderProcessHost() const = 0;
> > On 2012/05/11 22:08:01, Fady Samuel wrote:
> > > On 2012/05/11 22:06:43, John Abd-El-Malek wrote:
> > > > 1) only stuff used by embedders of content should be in the public API
> > > > 2) the concept of pending RVH is an internal implementation detail of
> > > > WebContentsImpl that we don't expose by design
> > > > 
> > > > happy to talk more about this on Monday, or you can ask brettw who wrote
> > this
> > > 
> > > Yup, just got rid of it. This is not absolutely necessary, but I'll need
to
> > cast
> > > from WebContents to WebContentsImpl elsewhere.
> > 
> > what I meant by 2 is that the whole concept of pending RVHs shouldn't be
> exposed
> > outside of WebContents. a lot of work was done to hide this (i.e. see how
> > RenderViewHostManager isn't exposed now, except only for tests)
> 
> I want to expose the concept of "current" RenderViewHost to the browser
plugin:
> 
> GetPendingRenderViewHost() ? GetPendingRenderViewHost() : GetRenderViewHost().
> 
> Does this seem reasonable?

I understand why you're doing this, but stepping back a bit, what I've been
trying to say is that we've specifically avoided exposing pending RVH through
WebContents.

> 
> Without exposing this, messages get sent to the wrong place, because the
browser
> plugin needs to hook into cross-process navigation.

Have a look at RenderViewHostObserver which got added for this reason (devtools
code had similar issues with messages not going to pending RVHs).
> 
> Is there a better way to do? Perhaps adding a Send method to WebContents?

This wouldn't work because it's not clear that when someone calls
WebContents::Send, they always want it to go to the pending RVH if it exists..

[Fady Samuel, 2012-05-14 23:02:08]

Hi John,

I've addressed your comment by removing GetPendingRenderViewHost(). I will do
the rest of the plumbing (in RenderView) in another patch.

Thanks,

Fady

[jam, 2012-05-15 00:05:06]

lgtm but wait for Justin's lgtm too as well.

Justin: see the comment

http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:260:
StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id);
someone from the security team should take a look at this. we've had problems in
the past with processes connecting to processes that they shouldn't. in this
case it's a process that has the lowest privilege, but it would be good to
double check.

[jschuh, 2012-05-15 00:43:35]

http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:260:
StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id);
On 2012/05/15 00:05:06, John Abd-El-Malek wrote:
> someone from the security team should take a look at this. we've had problems
in
> the past with processes connecting to processes that they shouldn't. in this
> case it's a process that has the lowest privilege, but it would be good to
> double check.

Thanks John. It should probably be changed to something like:

Channel::GenerateVerifiedChannelID(
    StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id));

There's no overhead to speak of, and it avoids vulnerabilities for things like
privileged renderers or if the code changes in the future.

[Fady Samuel, 2012-05-15 02:09:13]

http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:260:
StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id);
On 2012/05/15 00:43:35, Justin Schuh wrote:
> On 2012/05/15 00:05:06, John Abd-El-Malek wrote:
> > someone from the security team should take a look at this. we've had
problems
> in
> > the past with processes connecting to processes that they shouldn't. in this
> > case it's a process that has the lowest privilege, but it would be good to
> > double check.
> 
> Thanks John. It should probably be changed to something like:
> 
> Channel::GenerateVerifiedChannelID(
>     StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id));
> 
> There's no overhead to speak of, and it avoids vulnerabilities for things like
> privileged renderers or if the code changes in the future.

Hi Justin,

I don't quite understand the purpose of this function? This attaches a random ID
at the end of the string. In the renderer, I use this string as a key to decide
whether I have an existing channel to the embedder or whether I need to create a
new one. I can't do this if I have a random string. Thanks.

[jschuh, 2012-05-15 15:49:43]

http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
File content/browser/renderer_host/render_view_host_impl.cc (right):

http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
content/browser/renderer_host/render_view_host_impl.cc:260:
StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id);
On 2012/05/15 02:09:15, Fady Samuel wrote:
> Hi Justin,
> 
> I don't quite understand the purpose of this function? This attaches a random
ID
> at the end of the string. In the renderer, I use this string as a key to
decide
> whether I have an existing channel to the embedder or whether I need to create
a
> new one. I can't do this if I have a random string. Thanks.

The client gets a channel ID including the nonce via IPC or the command line.
When connecting, the client passes the random nonce to the server, which
verifies it. Otherwise the channel is vulnerable to hijack in the window between
creation and connection (one of the Pwnium exploits used that trick).

Also, it sounds like you're relying on process IDs not getting reused?
Unfortunately, that's not a safe assumption on Windows.

The GPU and plugin processes both do something similar to what you're
describing. IIRC they add an incrementing ID field for uniquely tracking the
channel in a map, and use GenerateVerifiedChannelID to generate the actual
channel.

[Fady Samuel, 2012-05-15 16:05:02]

On 2012/05/15 15:49:43, Justin Schuh wrote:
>
http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
> content/browser/renderer_host/render_view_host_impl.cc:260:
> StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id);
> On 2012/05/15 02:09:15, Fady Samuel wrote:
> > Hi Justin,
> > 
> > I don't quite understand the purpose of this function? This attaches a
random
> ID
> > at the end of the string. In the renderer, I use this string as a key to
> decide
> > whether I have an existing channel to the embedder or whether I need to
create
> a
> > new one. I can't do this if I have a random string. Thanks.
> 
> The client gets a channel ID including the nonce via IPC or the command line.
> When connecting, the client passes the random nonce to the server, which
> verifies it. Otherwise the channel is vulnerable to hijack in the window
between
> creation and connection (one of the Pwnium exploits used that trick).
> 
> Also, it sounds like you're relying on process IDs not getting reused?
> Unfortunately, that's not a safe assumption on Windows.
> 
> The GPU and plugin processes both do something similar to what you're
> describing. IIRC they add an incrementing ID field for uniquely tracking the
> channel in a map, and use GenerateVerifiedChannelID to generate the actual
> channel.

Ok, that is what I'm doing, I think?

In this case, the server is the guest renderer. I pass the guest the string:

StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id);

BrowserPluginChannelManager (see
http://codereview.chromium.org/9609008/patch/145044/138023) generates a channel
ID from that string and gives the channel handle to the browser process (the
client) via BrowserPluginHostMsg_ConnectToChannel.

Does this sound reasonable?

[jschuh, 2012-05-15 16:11:03]

On 2012/05/15 16:05:02, Fady Samuel wrote:
> On 2012/05/15 15:49:43, Justin Schuh wrote:
> >
>
http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
> > File content/browser/renderer_host/render_view_host_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/10378089/diff/16011/content/browser/renderer_h...
> > content/browser/renderer_host/render_view_host_impl.cc:260:
> > StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id);
> > On 2012/05/15 02:09:15, Fady Samuel wrote:
> > > Hi Justin,
> > > 
> > > I don't quite understand the purpose of this function? This attaches a
> random
> > ID
> > > at the end of the string. In the renderer, I use this string as a key to
> > decide
> > > whether I have an existing channel to the embedder or whether I need to
> create
> > a
> > > new one. I can't do this if I have a random string. Thanks.
> > 
> > The client gets a channel ID including the nonce via IPC or the command
line.
> > When connecting, the client passes the random nonce to the server, which
> > verifies it. Otherwise the channel is vulnerable to hijack in the window
> between
> > creation and connection (one of the Pwnium exploits used that trick).
> > 
> > Also, it sounds like you're relying on process IDs not getting reused?
> > Unfortunately, that's not a safe assumption on Windows.
> > 
> > The GPU and plugin processes both do something similar to what you're
> > describing. IIRC they add an incrementing ID field for uniquely tracking the
> > channel in a map, and use GenerateVerifiedChannelID to generate the actual
> > channel.
> 
> Ok, that is what I'm doing, I think?
> 
> In this case, the server is the guest renderer. I pass the guest the string:
> 
> StringPrintf("%d.r%d", GetProcess()->GetID(), embedder_process_id);
> 
> BrowserPluginChannelManager (see
> http://codereview.chromium.org/9609008/patch/145044/138023) generates a
channel
> ID from that string and gives the channel handle to the browser process (the
> client) via BrowserPluginHostMsg_ConnectToChannel.
> 
> Does this sound reasonable?

Ah, that's the piece I was missing. Yes, that seems like it should be okay. lgtm

[commit-bot: I haz the power, 2012-05-15 16:12:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10378089/16011

[commit-bot: I haz the power, 2012-05-15 16:29:49]

Try job failure for 10378089-16011 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-05-15 19:05:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10378089/17001

[commit-bot: I haz the power, 2012-05-15 19:29:59]

Try job failure for 10378089-17001 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-15 20:27:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10378089/1019

[commit-bot: I haz the power, 2012-05-15 20:52:00]

Try job failure for 10378089-1019 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-05-15 21:28:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10378089/4044

[commit-bot: I haz the power, 2012-05-15 21:59:42]

Try job failure for 10378089-4044 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-05-15 22:26:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10378089/4044

[commit-bot: I haz the power, 2012-05-15 22:51:20]

Try job failure for 10378089-4044 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-15 22:51:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10378089/4048

[commit-bot: I haz the power, 2012-05-15 22:51:42]

Presubmit check for 10378089-4048 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/visitedlink

Presubmit checks took 2.4s to calculate.

[Fady Samuel, 2012-05-15 22:54:53]

+brettw for OWNER of chrome/browser/visitedlink

[brettw, 2012-05-15 22:55:39]

lgtm

[commit-bot: I haz the power, 2012-05-15 22:56:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/10378089/4048

[commit-bot: I haz the power, 2012-05-16 00:36:52]

Change committed as 137319