Wait properly for renderer crashes

Wait properly for renderer crashes

This replaces a Sleep in automation with a wait for renderer crash. It turns out that our IPC on POSIX had one loophole that caused it not to notice very early crashes, so I also fixed that.

The problem was that when the child process died before connecting to the parent's IPC channel, the parent wouldn't notice the crash because the child end of the IPC pipe was kept open for too long. This change makes the code close the child end of the pipe right after forking the child.

This might also help with automation not noticing the browser crash during initial launch, or at least should be a good step toward fixing that problem.

BUG=38497, 90489
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101760

[Paweł Hajdan Jr., 2011-09-12 22:15:46]

Please review:

Darin: ipc, content, chrome
Al (apatrick): content/common/gpu
Scott: chrome/test

[sky, 2011-09-12 22:36:25]

chrome/test LGTM

[apatrick_chromium, 2011-09-12 22:38:36]

content/common/gpu LGTM

[apatrick_chromium, 2011-09-13 18:54:11]

+vangelis who is tracking a bug that might be related.

[Paweł Hajdan Jr., 2011-09-14 22:39:53]

Darin: ping, please take a look.

[darin (slow to review), 2011-09-15 05:43:37]

http://codereview.chromium.org/7870008/diff/12001/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/7870008/diff/12001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:142: // Treat every
closing render process as a crash.
It might help to explain why instead of just summarizing what the code does. 
Why do you want to map the closed notification to crashes?  How do you know that
it will always mean that a renderer crashed?

http://codereview.chromium.org/7870008/diff/12001/chrome/browser/importer/fir...
File chrome/browser/importer/firefox_importer_unittest_utils_mac.cc (right):

http://codereview.chromium.org/7870008/diff/12001/chrome/browser/importer/fir...
chrome/browser/importer/firefox_importer_unittest_utils_mac.cc:49: if (ipcfd >
-1) {
shouldn't this be "ipcfd != -1" ?

http://codereview.chromium.org/7870008/diff/12001/chrome/common/logging_chrom...
File chrome/common/logging_chrome_uitest.cc (left):

http://codereview.chromium.org/7870008/diff/12001/chrome/common/logging_chrom...
chrome/common/logging_chrome_uitest.cc:96: // (since TAU wouldn't be able to
find the browser window).
lol... TAU!  TAU was the very first automation system we used for driving Chrome
(pre-IPC automation).  It was super flaky!

http://codereview.chromium.org/7870008/diff/12001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/7870008/diff/12001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.h:226: int TakeClientFileDescriptor();
this method is a little bit odd here.  it isn't clear what ClientFileDescriptor
means in the context of the AutomationProxy.

you could name this method TakeClientFileDescriptorFromChannel, or perhaps you
should just expose a channel getter?

http://codereview.chromium.org/7870008/diff/12001/content/plugin/plugin_chann...
File content/plugin/plugin_channel.h (right):

http://codereview.chromium.org/7870008/diff/12001/content/plugin/plugin_chann...
content/plugin/plugin_channel.h:54: int TakeClientFileDescriptor() {
would it help to name this TakeRendererFileDescriptor just as we do for
GpuChannel?

[Paweł Hajdan Jr., 2011-09-15 18:36:44]

Darin, PTAL.

http://codereview.chromium.org/7870008/diff/12001/chrome/browser/automation/a...
File chrome/browser/automation/automation_provider_observers.cc (right):

http://codereview.chromium.org/7870008/diff/12001/chrome/browser/automation/a...
chrome/browser/automation/automation_provider_observers.cc:142: // Treat every
closing render process as a crash.
On 2011/09/15 05:43:37, darin wrote:
> It might help to explain why instead of just summarizing what the code does. 
> Why do you want to map the closed notification to crashes?  How do you know
that
> it will always mean that a renderer crashed?

I just removed that assumption then. Probably better to fix than to explain.

http://codereview.chromium.org/7870008/diff/12001/chrome/browser/importer/fir...
File chrome/browser/importer/firefox_importer_unittest_utils_mac.cc (right):

http://codereview.chromium.org/7870008/diff/12001/chrome/browser/importer/fir...
chrome/browser/importer/firefox_importer_unittest_utils_mac.cc:49: if (ipcfd >
-1) {
On 2011/09/15 05:43:37, darin wrote:
> shouldn't this be "ipcfd != -1" ?

Done.

http://codereview.chromium.org/7870008/diff/12001/chrome/test/automation/auto...
File chrome/test/automation/automation_proxy.h (right):

http://codereview.chromium.org/7870008/diff/12001/chrome/test/automation/auto...
chrome/test/automation/automation_proxy.h:226: int TakeClientFileDescriptor();
On 2011/09/15 05:43:37, darin wrote:
> this method is a little bit odd here.  it isn't clear what
ClientFileDescriptor
> means in the context of the AutomationProxy.
> 
> you could name this method TakeClientFileDescriptorFromChannel, or perhaps you
> should just expose a channel getter?

Done.

http://codereview.chromium.org/7870008/diff/12001/content/plugin/plugin_chann...
File content/plugin/plugin_channel.h (right):

http://codereview.chromium.org/7870008/diff/12001/content/plugin/plugin_chann...
content/plugin/plugin_channel.h:54: int TakeClientFileDescriptor() {
On 2011/09/15 05:43:37, darin wrote:
> would it help to name this TakeRendererFileDescriptor just as we do for
> GpuChannel?

Done.

[darin (slow to review), 2011-09-19 15:18:54]

LGTM

http://codereview.chromium.org/7870008/diff/20002/chrome/test/automation/prox...
File chrome/test/automation/proxy_launcher.cc (right):

http://codereview.chromium.org/7870008/diff/20002/chrome/test/automation/prox...
chrome/test/automation/proxy_launcher.cc:478:
fds.push_back(std::make_pair(ipcfd, kPrimaryIPCChannel + 3));
looks like this worked out well...