Don't allow a renderer to exit if we are using it in other views.

Don't allow a renderer to exit if we are using it in other views.

BUG=87176
TEST=See bug, comment 35.


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

[Charlie Reis, 2012-03-20 17:14:15]

I'd like to add a regression test or two for this, but I'm not sure how to delay
IPC messages in browser_tests or test RenderProcessHostImpl in a unit_test. 
Ideas welcome.

Basic steps to repro:
1) Start Chrome with an NTP.
2) Attach a debugger to the NTP process and set a breakpoint in
ChildProcess::ReleaseProcess.
3) Navigate the tab to any page (e.g., google.com).  The breakpoint will hit,
but just let it sit there for the moment.
4) Open a new tab.  It will try to load in the halted NTP process.
5) Resume the NTP process.  Sad tab.

[jam, 2012-03-21 18:04:47]

lgtm, thanks for fixing this, i've seen it before but did't know what it was!

[Charlie Reis, 2012-03-21 21:45:41]

Thanks.  I took your advice to add a custom ContentBrowserClient and message
filter to test it, and it worked.  Can you review the test as well?

[jam, 2012-03-21 23:30:12]

http://codereview.chromium.org/9751001/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

http://codereview.chromium.org/9751001/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_browsertest.cc:9: #include
"chrome/browser/chrome_content_browser_client.h"
you're going to get a checkdeps failure for this.. i should have thought about
this more before my suggestion over email. you can't derive from the chrome impl
since you're in content.

[Charlie Reis, 2012-03-21 23:44:14]

http://codereview.chromium.org/9751001/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

http://codereview.chromium.org/9751001/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_browsertest.cc:9: #include
"chrome/browser/chrome_content_browser_client.h"
On 2012/03/21 23:30:12, John Abd-El-Malek wrote:
> you're going to get a checkdeps failure for this.. i should have thought about
> this more before my suggestion over email. you can't derive from the chrome
impl
> since you're in content.

Yeah, I realized that...  This seems like a limitation on what we can test in
browser_tests, since those have a full Chrome instance that we'd like to
interact with.  Should browsertest.cc files live in chrome, or have an exception
that allows them to include chrome files (like the chrome/browser/ui/browser.h
below)?

If we had browser_tests that just ran on content_shell, that would probably work
too, but we're not there yet.

I suppose the best option at the moment is to create a new test file in chrome
for just this new test?

[Charlie Reis, 2012-03-21 23:51:17]

http://codereview.chromium.org/9751001/diff/7001/content/browser/renderer_hos...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

http://codereview.chromium.org/9751001/diff/7001/content/browser/renderer_hos...
content/browser/renderer_host/render_process_host_browsertest.cc:9: #include
"chrome/browser/chrome_content_browser_client.h"
Actually, we've already got a render_process_host_chrome_browsertest.cc in
chrome!  I'll put it there.

[Charlie Reis, 2012-03-22 00:27:13]

http://codereview.chromium.org/9751001/diff/6005/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

http://codereview.chromium.org/9751001/diff/6005/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:13:
#include "content/common/child_process_messages.h"
Drat.  I was able to get all of it to work in chrome except this-- I need to
name the ChildProcessHostMsg_ShutdownRequest message in the test, and that's off
limits.

Can you think of a way to make this work?  I'd like to keep this test if
possible.

[jam, 2012-03-22 18:09:42]

On 2012/03/22 00:27:13, creis wrote:
>
http://codereview.chromium.org/9751001/diff/6005/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
> (right):
> 
>
http://codereview.chromium.org/9751001/diff/6005/chrome/browser/renderer_host...
> chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:13:
> #include "content/common/child_process_messages.h"
> Drat.  I was able to get all of it to work in chrome except this-- I need to
> name the ChildProcessHostMsg_ShutdownRequest message in the test, and that's
off
> limits.
> 
> Can you think of a way to make this work?  I'd like to keep this test if
> possible.

I can't think of a way at this point, it's not possible until we have
content_browsertests. that is on my plate though, I'm going to start this soon.

[Charlie Reis, 2012-03-22 18:17:50]

On 2012/03/22 18:09:42, John Abd-El-Malek wrote:
> > Can you think of a way to make this work?  I'd like to keep this test if
> > possible.
> 
> I can't think of a way at this point, it's not possible until we have
> content_browsertests. that is on my plate though, I'm going to start this
soon.

Ok.  At least I've got a draft of the test for when the time comes.  I'll make a
note to land it when content_browsertests comes along.

For now, I'll commit just the fix from patch set 1.  Thanks for your help.

[commit-bot: I haz the power, 2012-03-22 18:21:22]

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

[commit-bot: I haz the power, 2012-03-22 19:50:53]

Change committed as 128274