Use StopChildProcess instead of base::KillProcess to kill a renderer process

Use StopChildProcess instead of base::KillProcess to kill a renderer process

base::KillProcess doesn't work on android because a renderer process is running as an isolated process.
So we need to use StopChildProcess to kill it.

BUG=433068
Committed: https://crrev.com/37e572a3b60109e457a09df566d15c6f96b79210
Cr-Commit-Position: refs/heads/master@{#306931}

[Jaekyun Seok (inactive), 2014-11-24 10:04:20]

jaekyun@chromium.org changed reviewers:
+ benwells@chromium.org, creis@chromium.org, sievers@chromium.org,
thakis@chromium.org, thestig@chromium.org, yoz@chromium.org

[Jaekyun Seok (inactive), 2014-11-24 10:04:21]

Please review this CL to use StopChildProcess instead of base::KillProcess to
kill a renderer process.

Thanks,

[Charlie Reis, 2014-11-24 17:53:59]

I'm looking forward to having these crash-inducing tests work on Android!

However, ifdef'ing every callsite of base::KillProcess is not a good long-term
solution.  We'll easily miss future calls to KillProcess this way.

If base::KillProcess itself can't be updated, can we abstract this into another
function that takes care of the ifdef?

[no sievers, 2014-11-24 21:35:26]

On 2014/11/24 17:53:59, Charlie Reis wrote:
> I'm looking forward to having these crash-inducing tests work on Android!
> 
> However, ifdef'ing every callsite of base::KillProcess is not a good long-term
> solution.  We'll easily miss future calls to KillProcess this way.
> 
> If base::KillProcess itself can't be updated, can we abstract this into
another
> function that takes care of the ifdef?

+1  Let's unify the codepaths. And file a separate bug to eliminate other
KillProcess call sites. Maybe we can remove KillProcess entirely and have
everything shut down things through the child process host.

Let's also unify the child shutdown codepaths. I *think* what's needed here is
something similar to FastShutdownIfPossible() but without the "*if* possible"
(forceful shutdown).

[Jaekyun Seok (inactive), 2014-11-24 22:27:18]

On 2014/11/24 21:35:26, sievers wrote:
> On 2014/11/24 17:53:59, Charlie Reis wrote:
> +1  Let's unify the codepaths. And file a separate bug to eliminate other
> KillProcess call sites. Maybe we can remove KillProcess entirely and have
> everything shut down things through the child process host.

I will file a bug after this issue is fixed. But I'm not sure whether all other
KillProcess call sites could be removed because it isn't clear which processes
are killed by some of them.

> 
> Let's also unify the child shutdown codepaths. I *think* what's needed here is
> something similar to FastShutdownIfPossible() but without the "*if* possible"
> (forceful shutdown).

I will add and use FastShutdown instead of ifdef'ing every callsite.

[no sievers, 2014-11-24 22:44:36]

On 2014/11/24 22:27:18, Jaekyun Seok wrote:
> On 2014/11/24 21:35:26, sievers wrote:
> > On 2014/11/24 17:53:59, Charlie Reis wrote:
> > +1  Let's unify the codepaths. And file a separate bug to eliminate other
> > KillProcess call sites. Maybe we can remove KillProcess entirely and have
> > everything shut down things through the child process host.
> 
> I will file a bug after this issue is fixed. But I'm not sure whether all
other
> KillProcess call sites could be removed because it isn't clear which processes
> are killed by some of them.

There are tests that kill processes based on an executable commandline. I'd be
surprised if anything in production code attempts to kill a process that's not
one that the browser launched. So we can probably clean all of this up. But I'd
do it one by one and start with this one call site you are interested in.

> 
> > 
> > Let's also unify the child shutdown codepaths. I *think* what's needed here
is
> > something similar to FastShutdownIfPossible() but without the "*if*
possible"
> > (forceful shutdown).
> 
> I will add and use FastShutdown instead of ifdef'ing every callsite.

[Jaekyun Seok (inactive), 2014-11-25 03:25:46]

PTAL. I added RenderProcessHost::FastShutdown and replace KillProcess with it.

On 2014/11/24 22:44:36, sievers wrote:
> There are tests that kill processes based on an executable commandline. I'd be
> surprised if anything in production code attempts to kill a process that's not
> one that the browser launched. So we can probably clean all of this up. But
I'd
> do it one by one and start with this one call site you are interested in.

I see. I will file a new issue after this issue is fixed to clean up remaining
KillProcess call sites.

[Jaekyun Seok (inactive), 2014-11-27 01:19:08]

Ping?

[benwells, 2014-11-27 02:21:01]

On 2014/11/27 01:19:08, Jaekyun Seok wrote:
> Ping?

You've got a lot of reviewers. I assume you just want me to look at the
extensions stuff, but I'll look at that once the meat of the change is approved.

It would help if you told each reviewer what you wanted them to look at.

[Jaekyun Seok (inactive), 2014-11-27 02:37:10]

On 2014/11/27 02:21:01, benwells wrote:
> It would help if you told each reviewer what you wanted them to look at.

Sorry for the ambiguous review request. I will ask you to review again after
content/* is reviewed first.

Charlie and Daniel, could you please take another look at content/*?

[Charlie Reis, 2014-12-01 19:44:10]

Sorry for the delay!  I like having it on RenderProcessHost.  Just one request
about the name and it should be good to go.

https://codereview.chromium.org/740813004/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/740813004/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1345: return false; 
// Single process mode never shutdown the renderer.
nit: shutdown -> shuts down

https://codereview.chromium.org/740813004/diff/160001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/740813004/diff/160001/content/public/browser/...
content/public/browser/render_process_host.h:114: // Returns True if it was able
to do fast shutdown.
nit: Don't capitalize true.  Also see comment below about "fast shutdown."

https://codereview.chromium.org/740813004/diff/160001/content/public/browser/...
content/public/browser/render_process_host.h:115: virtual bool FastShutdown(int
exit_code, bool wait) = 0;
This doesn't really overlap with any of the FastShutdownIfPossible or
FastShutdownStarted logic, so I think calling it FastShutdown is confusing.

Can we call it KillProcess (or just Kill) instead?  I'd list it above
FastShutdownIfPossible since it's a more basic version.

[no sievers, 2014-12-01 21:32:13]

https://codereview.chromium.org/740813004/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/740813004/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1347: #if
defined(OS_ANDROID)
I'm still confused why we need an ifdef here.
Why can't all platforms go through the same shutdown path?

We have already abstracted the Android-specifics in ChildProcessLauncher.
Can't we just do the same as the function above, which is call ProcessDied(false
/* already dead */) (this should probably ), which then kills the channel and
Context which in turn causes StopChildProcess() to be called?

Is there really a reason why we want to defer the ProcessDied() here, i.e. it
getting called from OnChannelError instead with |already_dead == true|?

[no sievers, 2014-12-01 21:34:16]

On 2014/12/01 21:32:13, sievers wrote:
>
https://codereview.chromium.org/740813004/diff/160001/content/browser/rendere...
> File content/browser/renderer_host/render_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/740813004/diff/160001/content/browser/rendere...
> content/browser/renderer_host/render_process_host_impl.cc:1347: #if
> defined(OS_ANDROID)
> I'm still confused why we need an ifdef here.
> Why can't all platforms go through the same shutdown path?
> 
> We have already abstracted the Android-specifics in ChildProcessLauncher.
> Can't we just do the same as the function above, which is call
ProcessDied(false
> /* already dead */) (this should probably ), which then kills the channel and
> Context which in turn causes StopChildProcess() to be called?
> 
> Is there really a reason why we want to defer the ProcessDied() here, i.e. it
> getting called from OnChannelError instead with |already_dead == true|?

And the advantage being that we can work towards deprecating KillProcess().
If KillProcess() doesn't work on Android, and given that it's not particularly
useful (maybe just a lazy way of doing things), we should then remove it.
Otherwise we will always have these corner case call sites that don't work on
Android. And we might not even notice.

[Jaekyun Seok (inactive), 2014-12-02 00:50:39]

PTAL.

I modified FastShutdown to call ProcessDied internally.

https://codereview.chromium.org/740813004/diff/160001/content/browser/rendere...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/740813004/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1345: return false; 
// Single process mode never shutdown the renderer.
On 2014/12/01 19:44:09, Charlie Reis wrote:
> nit: shutdown -> shuts down

Done.

https://codereview.chromium.org/740813004/diff/160001/content/browser/rendere...
content/browser/renderer_host/render_process_host_impl.cc:1347: #if
defined(OS_ANDROID)
On 2014/12/01 21:32:13, sievers wrote:
> I'm still confused why we need an ifdef here.
> Why can't all platforms go through the same shutdown path?
> 
> We have already abstracted the Android-specifics in ChildProcessLauncher.
> Can't we just do the same as the function above, which is call
ProcessDied(false
> /* already dead */) (this should probably ), which then kills the channel and
> Context which in turn causes StopChildProcess() to be called?
> 
> Is there really a reason why we want to defer the ProcessDied() here, i.e. it
> getting called from OnChannelError instead with |already_dead == true|?

I wanted not to change any execution flow on non-android platforms. But using
ProcessDied() seems reasonable.

I will use it.

https://codereview.chromium.org/740813004/diff/160001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/740813004/diff/160001/content/public/browser/...
content/public/browser/render_process_host.h:114: // Returns True if it was able
to do fast shutdown.
On 2014/12/01 19:44:10, Charlie Reis wrote:
> nit: Don't capitalize true.  Also see comment below about "fast shutdown."

Done.

https://codereview.chromium.org/740813004/diff/160001/content/public/browser/...
content/public/browser/render_process_host.h:115: virtual bool FastShutdown(int
exit_code, bool wait) = 0;
On 2014/12/01 19:44:09, Charlie Reis wrote:
> This doesn't really overlap with any of the FastShutdownIfPossible or
> FastShutdownStarted logic, so I think calling it FastShutdown is confusing.
> 
> Can we call it KillProcess (or just Kill) instead?  I'd list it above
> FastShutdownIfPossible since it's a more basic version.

I modified FastShutdown to use ProcessDied, and so I believe that the method
name is reasonable now.

[Jaekyun Seok (inactive), 2014-12-02 05:42:53]

PTAL.

I revived StopChildProcess and KillProcess because ProcessDied failed in some
tests. And I changed the method name to "Shutdown".

[Charlie Reis, 2014-12-02 17:13:37]

Patch set 11 LGTM with the nits below.

I'm not ok with the ProcessDied approach in patch set 10 unless we can reason
through the implications of removing the result codes, which appear to matter in
some cases.  I can see sievers@'s argument for unifying the two approaches, but
perhaps that can be attempted in a later CL.

https://codereview.chromium.org/740813004/diff/200001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/740813004/diff/200001/content/public/browser/...
content/public/browser/render_process_host.h:105: // Try to shutdown the
associated renderer process forcedly, giving it the
nit: shut down
nit: drop "forcedly," instead say "without running unload handlers, etc"

https://codereview.chromium.org/740813004/diff/200001/content/public/browser/...
content/public/browser/render_process_host.h:113: // function does nothing.  The
current implementation uses TerminateProcess.
Might as well drop the "current implementation" sentence, since it's stale.

[no sievers, 2014-12-02 18:13:42]

On 2014/12/02 17:13:37, Charlie Reis wrote:
> Patch set 11 LGTM with the nits below.
> 
> I'm not ok with the ProcessDied approach in patch set 10 unless we can reason
> through the implications of removing the result codes, which appear to matter
in
> some cases.  I can see sievers@'s argument for unifying the two approaches,
but
> perhaps that can be attempted in a later CL.

sgtm. But sounds like there would still be failing tests on Android because we
cannot pass the RESULT_CODE_KILLED_BAD_MESSAGE exit code there. 
So we might have to override the termination status from the browser anyways to
fix that (and then maybe that works for all platforms- we also wouldn't have to
block in waitpid()).

[Jaekyun Seok (inactive), 2014-12-03 02:49:18]

> sgtm. But sounds like there would still be failing tests on Android because we
> cannot pass the RESULT_CODE_KILLED_BAD_MESSAGE exit code there. 
> So we might have to override the termination status from the browser anyways
to
> fix that (and then maybe that works for all platforms- we also wouldn't have
to
> block in waitpid()).

There are no failing tests on Android, and the exit_code doesn't seem to be used
inside base::KillProcess
(https://cs.corp.google.com/#clankium/src/base/process/kill_posix.cc&l=139).

When I saw the failed test logs of patchset 10, there was an error on locking at
https://cs.corp.google.com/#clankium/src/base/synchronization/lock_impl_posix...
.

[Jaekyun Seok (inactive), 2014-12-03 02:51:35]

https://codereview.chromium.org/740813004/diff/200001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/740813004/diff/200001/content/public/browser/...
content/public/browser/render_process_host.h:105: // Try to shutdown the
associated renderer process forcedly, giving it the
On 2014/12/02 17:13:37, Charlie Reis wrote:
> nit: shut down
> nit: drop "forcedly," instead say "without running unload handlers, etc"

Done.

https://codereview.chromium.org/740813004/diff/200001/content/public/browser/...
content/public/browser/render_process_host.h:113: // function does nothing.  The
current implementation uses TerminateProcess.
On 2014/12/02 17:13:37, Charlie Reis wrote:
> Might as well drop the "current implementation" sentence, since it's stale.

Done.

[Charlie Reis, 2014-12-03 17:41:37]

On 2014/12/03 02:49:18, Jaekyun Seok wrote:
> There are no failing tests on Android, and the exit_code doesn't seem to be
used
> inside base::KillProcess
> (https://cs.corp.google.com/#clankium/src/base/process/kill_posix.cc&l=139).

That's not quite true.  It's not used there, but it is used on Windows (in
kill_win.cc).  This is one of the reasons I was skeptical about removing it
without more careful investigation.

https://codereview.chromium.org/740813004/diff/220001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/740813004/diff/220001/content/public/browser/...
content/public/browser/render_process_host.h:110: // function does nothing. 
This function uses TerminateProcess.
Ah, I meant drop the whole sentence.  It doesn't use TerminateProcess anymore,
and we don't need to expose the implementation details in the public interface
comment.

[Jaekyun Seok (inactive), 2014-12-04 01:30:04]

> That's not quite true.  It's not used there, but it is used > on Windows (in
> kill_win.cc).  This is one of the reasons I was skeptical about removing it
> without more careful investigation.

Sorry for my misunderstanding.
As you suggested, it seems that we need to investigate more to remove
KillProcess. So let's do that as a follow-up after this change because that
isn't urgent for now.

https://codereview.chromium.org/740813004/diff/220001/content/public/browser/...
File content/public/browser/render_process_host.h (right):

https://codereview.chromium.org/740813004/diff/220001/content/public/browser/...
content/public/browser/render_process_host.h:110: // function does nothing. 
This function uses TerminateProcess.
On 2014/12/03 17:41:37, Charlie Reis wrote:
> Ah, I meant drop the whole sentence.  It doesn't use TerminateProcess anymore,
> and we don't need to expose the implementation details in the public interface
> comment.

Done.

[Jaekyun Seok (inactive), 2014-12-04 01:34:27]

benwells and yoz, could you please review changes on extensions/ and
chrome/browser/extensions?

And thestig and thakis, could please review changes on chrome/browser/?

Thanks,

[Lei Zhang, 2014-12-04 02:45:32]

chrome/browser is consistent -> lgtm

[benwells, 2014-12-04 04:55:26]

c/b/extensions, extensions/ lgtm

[Jaekyun Seok (inactive), 2014-12-04 22:19:28]

The CQ bit was checked by jaekyun@chromium.org

[commit-bot: I haz the power, 2014-12-04 22:20:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/740813004/240001

[commit-bot: I haz the power, 2014-12-04 23:34:01]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2014-12-04 23:34:43]

Patchset 13 (id:??) landed as
https://crrev.com/37e572a3b60109e457a09df566d15c6f96b79210
Cr-Commit-Position: refs/heads/master@{#306931}