Avoid exiting the renderer process if it has a pending render view.

Avoid exiting the renderer process if it has a pending render view.

This addresses an issue for a future CL when we switch to swapping
RenderViewHosts in and out, rather than throwing them away each time.

BUG=65953
TEST=None

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

[Charlie Reis, 2011-05-04 16:48:16]

Thanks for the comments on http://codereview.chromium.org/6909016/.  I've
switched to using the ShutdownRequest message in BrowserRenderProcessHost
instead.  Sound reasonable?

Once we've reviewed it, I think this should be safe to land separately from the
rest of http://codereview.chromium.org/6319001/.

[jam, 2011-05-06 17:45:11]

whoever reviewd the renderviewhostmanager changes last time should review the
changes to that file, since i'm not familiar with it.

http://codereview.chromium.org/6927014/diff/2002/content/browser/renderer_hos...
File content/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/6927014/diff/2002/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.h:63: virtual int
PendingViewCount() const;
I don't see this being used anywhere?

http://codereview.chromium.org/6927014/diff/2002/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6927014/diff/2002/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:2153: case
NotificationType::RENDERER_PROCESS_CLOSING:
why do you listen to this in TabContents and then just have it call
RenderManager?  i.e. why doesn't render_manager_ listen to the notification
itself?

[Charlie Reis, 2011-05-10 00:57:03]

On 2011/05/06 17:45:11, John Abd-El-Malek wrote:
> whoever reviewd the renderviewhostmanager changes last time should review the
> changes to that file, since i'm not familiar with it.

Matt, would you be willing to take a look at the RVHM changes here?  (Mainly
where we increase and decrease the pending view count.)  This is a subset of the
larger CL we're reviewing at http://codereview.chromium.org/6319001/, in an
attempt to handle part of it separately.

John, I've addressed your other feedback below.

Thanks!

http://codereview.chromium.org/6927014/diff/2002/content/browser/renderer_hos...
File content/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/6927014/diff/2002/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.h:63: virtual int
PendingViewCount() const;
On 2011/05/06 17:45:11, John Abd-El-Malek wrote:
> I don't see this being used anywhere?

Done.

http://codereview.chromium.org/6927014/diff/2002/content/browser/tab_contents...
File content/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6927014/diff/2002/content/browser/tab_contents...
content/browser/tab_contents/tab_contents.cc:2153: case
NotificationType::RENDERER_PROCESS_CLOSING:
On 2011/05/06 17:45:11, John Abd-El-Malek wrote:
> why do you listen to this in TabContents and then just have it call
> RenderManager?  i.e. why doesn't render_manager_ listen to the notification
> itself?

RenderViewHostManager wasn't a NotificationObserver, and adding a registrar
seemed unnecessary if TabContents is already notifying RVHM about events from
the outside world.

I've switched it over as you suggest, though, which lets us avoid exposing the
RendererProcessClosing function.

[Matt Perry, 2011-05-10 01:14:02]

LGTM

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host.h (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host.h:196: virtual void
AddPendingView() = 0;
since this is an interface on the class already, why not just make it
non-virtual and implement it here? The impl doesn't seem to do anything specific
to chrome.

http://codereview.chromium.org/6927014/diff/6001/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.cc:696:
pending_render_view_host->process()->RemovePendingView();
does it matter if this comes before or after Shutdown? (I'm wondering if
Shutdown might synchronously NULL-out process).

[Matt Perry, 2011-05-10 01:14:50]

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.cc:473:
pending_views_--;
does this need to send a shutdown if pending_views_ reaches 0?

[Charlie Reis, 2011-05-10 01:33:09]

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/browser_render_process_host.cc:473:
pending_views_--;
On 2011/05/10 01:14:50, Matt Perry wrote:
> does this need to send a shutdown if pending_views_ reaches 0?

Nope-- it'll also reach 0 if we commit the view and have it showing.  The only
period of risk (once we do the rest of the swapping changes in the other CL) is
while it's pending.

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host.h (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host.h:196: virtual void
AddPendingView() = 0;
On 2011/05/10 01:14:02, Matt Perry wrote:
> since this is an interface on the class already, why not just make it
> non-virtual and implement it here? The impl doesn't seem to do anything
specific
> to chrome.

Actually, I'm a little unclear on the difference between RenderProcessHost and
BrowserRenderProcessHost, since both are in content, not chrome.  Right now, the
test version in MockRPH doesn't care about this, so that's the difference
between the two.

John, if you agree that we should move the implementation here, I'm happy to do
that.

http://codereview.chromium.org/6927014/diff/6001/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.cc:696:
pending_render_view_host->process()->RemovePendingView();
On 2011/05/10 01:14:02, Matt Perry wrote:
> does it matter if this comes before or after Shutdown? (I'm wondering if
> Shutdown might synchronously NULL-out process).

That's a good question-- Shutdown doesn't null out the process, but it does send
a shutdown-related message.  It should probably come before to avoid a race. 
I'll fix it in the next patch set.

[jam, 2011-05-10 04:45:18]

lgtm

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host.h (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host.h:196: virtual void
AddPendingView() = 0;
On 2011/05/10 01:33:09, creis wrote:
> On 2011/05/10 01:14:02, Matt Perry wrote:
> > since this is an interface on the class already, why not just make it
> > non-virtual and implement it here? The impl doesn't seem to do anything
> specific
> > to chrome.
> 
> Actually, I'm a little unclear on the difference between RenderProcessHost and
> BrowserRenderProcessHost, since both are in content, not chrome.  Right now,
the
> test version in MockRPH doesn't care about this, so that's the difference
> between the two.
> 
> John, if you agree that we should move the implementation here, I'm happy to
do
> that.

RenderProcessHost was added so that we can have a mock object that implements
the bare interface and use it in tests instead of BrowserRenderProcessHost,
which actually launches processes etc.  I don't have strong opinions on where
the implementation goes.

http://codereview.chromium.org/6927014/diff/6001/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager.h (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.h:118: //
NotificationObserver.
nit: the usual style is to add the inherited methods in order that the
interfaces are listed.  so this should go at line 181

[Charlie Reis, 2011-05-10 18:42:19]

Thanks-- I've fixed the remaining issues you mentioned.  Just waiting for the
try servers.

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host.h (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host.h:196: virtual void
AddPendingView() = 0;
On 2011/05/10 04:45:18, John Abd-El-Malek wrote:
> On 2011/05/10 01:33:09, creis wrote:
> > On 2011/05/10 01:14:02, Matt Perry wrote:
> > > since this is an interface on the class already, why not just make it
> > > non-virtual and implement it here? The impl doesn't seem to do anything
> > specific
> > > to chrome.
> > 
> > Actually, I'm a little unclear on the difference between RenderProcessHost
and
> > BrowserRenderProcessHost, since both are in content, not chrome.  Right now,
> the
> > test version in MockRPH doesn't care about this, so that's the difference
> > between the two.
> > 
> > John, if you agree that we should move the implementation here, I'm happy to
> do
> > that.
> 
> RenderProcessHost was added so that we can have a mock object that implements
> the bare interface and use it in tests instead of BrowserRenderProcessHost,
> which actually launches processes etc.  I don't have strong opinions on where
> the implementation goes.

Ok.  Since there's no harm in having these counters in the tests (and possibly
some value), I've moved the implementation to RenderProcessHost as Matt
suggests.

http://codereview.chromium.org/6927014/diff/6001/content/browser/tab_contents...
File content/browser/tab_contents/render_view_host_manager.h (right):

http://codereview.chromium.org/6927014/diff/6001/content/browser/tab_contents...
content/browser/tab_contents/render_view_host_manager.h:118: //
NotificationObserver.
On 2011/05/10 04:45:18, John Abd-El-Malek wrote:
> nit: the usual style is to add the inherited methods in order that the
> interfaces are listed.  so this should go at line 181

Done.