plugins: use OnChannelError to detect when the channel goes away

plugins: use OnChannelError to detect when the channel goes away

Previously we used a special Watcher object to watch the process,
but that is not portable to POSIX and this is simpler anyway.
With this change, I now see ~PluginProcessHost() running when a
plugin crashes.

If you recall all the way back to
  http://codereview.chromium.org/16814
we did a similar thing to the renderer host.

[Evan Martin, 2009-07-09 22:46:28]

John, I'm especially interested in possible test cases I can run to triple-check
this doesn't break anything on Windows.

[agl, 2009-07-09 22:52:49]

LGTM

[Antoine Labour, 2009-07-09 22:56:48]

The linux bits look good. For the windows part, I assume this means we now
detect the child has gone away unexpectedly only when trying to talk to it,
instead of immediately. I'm ok with it, but I want to make sure I understand
correctly.

[Evan Martin, 2009-07-09 23:01:40]

On 2009/07/09 22:56:48, Antoine Labour wrote:
> The linux bits look good. For the windows part, I assume this means we now
> detect the child has gone away unexpectedly only when trying to talk to it,
> instead of immediately. I'm ok with it, but I want to make sure I understand
> correctly.

My understanding is that OnChannelError happens immediately when the child has
gone away.  But that's why I bug John with these reviews.  :)

[jam, 2009-07-09 23:03:00]

lgtm (I hope that OnChannelError gets called all the time!)

http://codereview.chromium.org/155331/diff/1001/13
File chrome/common/child_process_host.cc (right):

http://codereview.chromium.org/155331/diff/1001/13#newcode114
Line 114: set_handle(NULL);
why is this needed?

[Evan Martin, 2009-07-09 23:06:15]

http://codereview.chromium.org/155331/diff/1001/13
File chrome/common/child_process_host.cc (right):

http://codereview.chromium.org/155331/diff/1001/13#newcode114
Line 114: set_handle(NULL);
On 2009/07/09 23:03:00, John Abd-El-Malek wrote:
> why is this needed?

In the destructor,  ProcessWatcher::EnsureProcessTerminated(handle());
would do the wrong thing because handle() is already gone.

It already had a NULL check so I didn't have to change it

[jam, 2009-07-09 23:10:05]

http://codereview.chromium.org/155331/diff/1001/13
File chrome/common/child_process_host.cc (right):

http://codereview.chromium.org/155331/diff/1001/13#newcode114
Line 114: set_handle(NULL);
On 2009/07/09 23:06:15, Evan Martin wrote:
> On 2009/07/09 23:03:00, John Abd-El-Malek wrote:
> > why is this needed?
> 
> In the destructor,  ProcessWatcher::EnsureProcessTerminated(handle());
> would do the wrong thing because handle() is already gone.
> 
> It already had a NULL check so I didn't have to change it

This works fine on Windows.  Is the issue that
ProcessWatcher::EnsureProcessTerminated misbehave on POSIX because handle isn't
valid anymore?  But I thought the whole point of EnsureProcessTerminated is that
you can call it after a process crashed (and hence with a stale handle).

[Evan Martin, 2009-07-09 23:14:46]

On 2009/07/09 23:10:05, John Abd-El-Malek wrote:
> http://codereview.chromium.org/155331/diff/1001/13
> File chrome/common/child_process_host.cc (right):
> 
> http://codereview.chromium.org/155331/diff/1001/13#newcode114
> Line 114: set_handle(NULL);
> On 2009/07/09 23:06:15, Evan Martin wrote:
> > On 2009/07/09 23:03:00, John Abd-El-Malek wrote:
> > > why is this needed?
> > 
> > In the destructor,  ProcessWatcher::EnsureProcessTerminated(handle());
> > would do the wrong thing because handle() is already gone.
> > 
> > It already had a NULL check so I didn't have to change it
> 
> This works fine on Windows.  Is the issue that
> ProcessWatcher::EnsureProcessTerminated misbehave on POSIX because handle
isn't
> valid anymore?  But I thought the whole point of EnsureProcessTerminated is
that
> you can call it after a process crashed (and hence with a stale handle).

Both EnsureProcessTerminated and DidProcessCrash call waitpid() to get the exit
code of the process, and only one of them will succeed.  This function calls the
latter of the two, so we can't allow it.

BTW, DidProcessCrash returns a bool that lets you knwo if the process exited
(which is always true on POSIX).  I could check that if you want me to on
Windows, but I think it's more special-casing than necessary.

[Evan Martin, 2009-07-09 23:16:01]

That was a terrible explanation, sorry.  All I meant was that you can only call
waitpid() once on a pid, after which that pid is spent; another process could
come along and use the same pid.

There's definitely an impedance mismatch between Windows process handles and
POSIX pids.  On the plus side, it's impossible to leak PIDs because they're just
integers.  ;)

[jam, 2009-07-09 23:19:06]

On 2009/07/09 23:16:01, Evan Martin wrote:
> That was a terrible explanation, sorry.  All I meant was that you can only
call
> waitpid() once on a pid, after which that pid is spent; another process could
> come along and use the same pid.
> 
> There's definitely an impedance mismatch between Windows process handles and
> POSIX pids.  On the plus side, it's impossible to leak PIDs because they're
just
> integers.  ;)

Thanks for the explanation.  Can you add a small comment so a clueless Windows
developer doesn't mess up POSIX builds by taking out this line that doesn't look
needed to them? :)

[Evan Martin, 2009-07-10 18:19:18]

Tony: I'm retrying this change I had to revert.  All of the code is the same
aside from the changes to utility_process_host_unittest.  Please review those. 
:)

[tony, 2009-07-10 18:28:58]

utility_process_host_unittest change LGTM.  Maybe verify that it doesn't leak in
valgrind?