Stop using renderer specific host ids in ResourceDispatcher. This allows it ...

Stop having renderers use both pids and a monotonically increasing "host_id".  This allows ResourceDispatcher to be used by child processes other than renderers.  I've done minor related cleanup on the way to make the code simpler.

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

[Matt Perry, 2009-03-12 01:11:16]

Looks good overall.  It's pretty much guaranteed to break the build when you
check it in, though :).  Be sure to run as many tests as you can locally.

http://codereview.chromium.org/42054/diff/59/3013
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/42054/diff/59/3013#newcode727
Line 727: sent_requests_.erase(sent_requests_.begin());
I'm not sure I understand this change.  Is the idea that OnChannelCreated is
called in order for each sent_request, so you know that the first sent_request_
in the queue is the one corresponding to this channel?

Consider using a std::queue for send_requests_, which would improve readability
and performance.

http://codereview.chromium.org/42054/diff/59/3022
File chrome/browser/tab_contents/site_instance.h (right):

http://codereview.chromium.org/42054/diff/59/3022#newcode148
Line 148: RenderProcessHost* process_;
From reading the comment on the old code: when a RPH dies, the host_id_ used to
remain the same until a new process was created, so the tabs retain their
grouping.  With the new code, it seems like that grouping is lost since process_
is NULLed out.

Also, another concern: we use host_id in places where it's not safe to hold on
to a RPH pointer (because RPH can only be accessed on the UI thread).  It might
not be safe to keep a pointer here (not sure if SiteInstance is only used on the
UI thread or what).

[jam, 2009-03-12 01:21:45]

I had run all the unit/ui tests and try bots seem fine, so crossing fingers. 
Will watch the tree carefully :)

http://codereview.chromium.org/42054/diff/59/3013
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/42054/diff/59/3013#newcode727
Line 727: sent_requests_.erase(sent_requests_.begin());
On 2009/03/12 01:11:16, Matt Perry wrote:
> I'm not sure I understand this change.  Is the idea that OnChannelCreated is
> called in order for each sent_request, so you know that the first
sent_request_
> in the queue is the one corresponding to this channel?

The idea behind this is that it doesn't actually matter if we "match up" the
requests with the created channels, since the channel creation requests don't
carry any state.

> 
> Consider using a std::queue for send_requests_, which would improve
readability
> and performance.

Yeah I thought it looked bad (but I don't think it hurts performance, since
it'll usually just be one).  Just changed it to use queue.

http://codereview.chromium.org/42054/diff/59/3022
File chrome/browser/tab_contents/site_instance.h (right):

http://codereview.chromium.org/42054/diff/59/3022#newcode148
Line 148: RenderProcessHost* process_;
On 2009/03/12 01:11:16, Matt Perry wrote:
> From reading the comment on the old code: when a RPH dies, the host_id_ used
to
> remain the same until a new process was created, so the tabs retain their
> grouping.  With the new code, it seems like that grouping is lost since
process_
> is NULLed out.

I think what that pointer means is that the RenderViewHosts are still associated
with each other through SiteInstance.  That behavior isn't changed, since even
though process_ is NULLed, it'll get created again when GetProcess() is called.

> 
> Also, another concern: we use host_id in places where it's not safe to hold on
> to a RPH pointer (because RPH can only be accessed on the UI thread).  It
might
> not be safe to keep a pointer here (not sure if SiteInstance is only used on
the
> UI thread or what).

Right.  The only place where I changed this is SiteInstance, since it's created
before RenderProcessHost::Init is called, and I didn't want to make RPH know
about SiteInstance.  However since it uses the notification service, the pointer
stashing is safe.

[Matt Perry, 2009-03-12 01:31:30]

http://codereview.chromium.org/42054/diff/59/3013
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/42054/diff/59/3013#newcode727
Line 727: sent_requests_.erase(sent_requests_.begin());
On 2009/03/12 01:21:46, John Abd-El-Malek wrote:
> On 2009/03/12 01:11:16, Matt Perry wrote:
> > I'm not sure I understand this change.  Is the idea that OnChannelCreated is
> > called in order for each sent_request, so you know that the first
> sent_request_
> > in the queue is the one corresponding to this channel?
> 
> The idea behind this is that it doesn't actually matter if we "match up" the
> requests with the created channels, since the channel creation requests don't
> carry any state.

If they didn't match up, wouldn't we be sending the wrong channel name to the
renderer that asked for a plugin?

http://codereview.chromium.org/42054/diff/59/3022
File chrome/browser/tab_contents/site_instance.h (right):

http://codereview.chromium.org/42054/diff/59/3022#newcode148
Line 148: RenderProcessHost* process_;
On 2009/03/12 01:21:46, John Abd-El-Malek wrote:
> On 2009/03/12 01:11:16, Matt Perry wrote:
> > From reading the comment on the old code: when a RPH dies, the host_id_ used
> to
> > remain the same until a new process was created, so the tabs retain their
> > grouping.  With the new code, it seems like that grouping is lost since
> process_
> > is NULLed out.
> 
> I think what that pointer means is that the RenderViewHosts are still
associated
> with each other through SiteInstance.  That behavior isn't changed, since even
> though process_ is NULLed, it'll get created again when GetProcess() is
called.

If the grouping is done by RPH, wouldn't 2 (previously separate) crashed tabs be
grouped together, since they now have NULL RPHs?

[jam, 2009-03-12 01:36:28]

http://codereview.chromium.org/42054/diff/59/3013
File chrome/browser/plugin_process_host.cc (right):

http://codereview.chromium.org/42054/diff/59/3013#newcode727
Line 727: sent_requests_.erase(sent_requests_.begin());
On 2009/03/12 01:31:30, Matt Perry wrote:
> On 2009/03/12 01:21:46, John Abd-El-Malek wrote:
> > On 2009/03/12 01:11:16, Matt Perry wrote:
> > > I'm not sure I understand this change.  Is the idea that OnChannelCreated
is
> > > called in order for each sent_request, so you know that the first
> > sent_request_
> > > in the queue is the one corresponding to this channel?
> > 
> > The idea behind this is that it doesn't actually matter if we "match up" the
> > requests with the created channels, since the channel creation requests
don't
> > carry any state.
> 
> If they didn't match up, wouldn't we be sending the wrong channel name to the
> renderer that asked for a plugin?

All a renderer cares is that it gets a channel to a specific plugin process. 
Since the queuing up of plugin channel requests is in PluginProcessHost, there's
no need to match up requests with responses.

http://codereview.chromium.org/42054/diff/59/3022
File chrome/browser/tab_contents/site_instance.h (right):

http://codereview.chromium.org/42054/diff/59/3022#newcode148
Line 148: RenderProcessHost* process_;
On 2009/03/12 01:31:30, Matt Perry wrote:
> On 2009/03/12 01:21:46, John Abd-El-Malek wrote:
> > On 2009/03/12 01:11:16, Matt Perry wrote:
> > > From reading the comment on the old code: when a RPH dies, the host_id_
used
> > to
> > > remain the same until a new process was created, so the tabs retain their
> > > grouping.  With the new code, it seems like that grouping is lost since
> > process_
> > > is NULLed out.
> > 
> > I think what that pointer means is that the RenderViewHosts are still
> associated
> > with each other through SiteInstance.  That behavior isn't changed, since
even
> > though process_ is NULLed, it'll get created again when GetProcess() is
> called.
> 
> If the grouping is done by RPH, wouldn't 2 (previously separate) crashed tabs
be
> grouped together, since they now have NULL RPHs?

From what I understand, the grouping is done through SiteInstance.  Even if
RenderViewHosts crash and they were together, when the process is restarted
they'll still have the same SiteInstance and hence process.  Same if they had
different SiteInstances, when their RenderProcessHosts are recreated, this will
be done through their separate SiteInstances.  I've verified this through
testing killing processes that were together/separate and then reloading the
page.

[Matt Perry, 2009-03-12 01:46:20]

LGTM