Simplify ChildProcessLauncher

Simplify ChildProcessLauncher

Remove the refcounted internal state object (nested class 'Context').
This refactor makes it more obvious what happens on what thread,
and avoids the need to pass refptrs around.

TBR=bradnelson@chromium.org
BUG=469248
Committed: https://crrev.com/954e37aad499a68771f3c517d1e4760e38986e0e
Cr-Commit-Position: refs/heads/master@{#322695}

[no sievers, 2015-03-26 18:36:14]

sievers@chromium.org changed reviewers:
+ rvargas@chromium.org

[no sievers, 2015-03-26 18:36:33]

Ok, I feel this is going somewhere now. PTAL.

[no sievers, 2015-03-26 21:27:50]

ptal. Moved all private static functions to the source file (with the exception
of DidLaunch, which is sort of the static portion of Notify and needs to call
the latter, which is private).

[jam, 2015-03-26 22:24:46]

nice simplification using bind & weak ptr :) (didn't review in detail)

[rvargas (doing something else), 2015-03-27 22:01:22]

Nice indeed!

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:416: if (process.IsValid() &&
terminate_on_shutdown) {
We should terminate the process regardless of terminate_on_shutdown because we
are not telling anyone about it.

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
content/browser/child_process_launcher.h:118: // shutdown. Default behavior is
to terminate the child.
nit: There is no default behavior anymore.

Note however that there is an specific exception that allows default arguments
for constructors, so we could have a default behavior.

[no sievers, 2015-03-27 22:23:04]

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:416: if (process.IsValid() &&
terminate_on_shutdown) {
On 2015/03/27 22:01:22, rvargas (slow to respond) wrote:
> We should terminate the process regardless of terminate_on_shutdown because we
> are not telling anyone about it.

Isn't that a behavioral change though (which I've tried to avoid). Before we'd
have taken line 508 on the left, which would also have skipped calling
TerminateInternal() if |!terminate_child_on_shutdown_|?

Also, it looks like !terminate is only used for PluginProcessHost where it says
that it needs to shutdown gracefully. Wouldn't that be regardless of the client
going away or not?

[rvargas (doing something else), 2015-03-27 22:56:00]

lgtm

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:416: if (process.IsValid() &&
terminate_on_shutdown) {
On 2015/03/27 22:23:04, sievers wrote:
> On 2015/03/27 22:01:22, rvargas (slow to respond) wrote:
> > We should terminate the process regardless of terminate_on_shutdown because
we
> > are not telling anyone about it.
> 
> Isn't that a behavioral change though (which I've tried to avoid). Before we'd
> have taken line 508 on the left, which would also have skipped calling
> TerminateInternal() if |!terminate_child_on_shutdown_|?

You're right, that's the current behavior. Sounds like a bug to me, but it's
better not to change behavior in this CL.

> 
> Also, it looks like !terminate is only used for PluginProcessHost where it
says
> that it needs to shutdown gracefully. Wouldn't that be regardless of the
client
> going away or not?

I think the problem is that the delegate is never informed of the process being
created at all, so nobody will perform any cleanup and the process will just
leak. On the other hand, there may be some platform dependent behavior here
because I think on posix it's possible to start receiving IPCs before the
delegate is notified.

[no sievers, 2015-03-27 23:13:37]

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.h (right):

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
content/browser/child_process_launcher.h:118: // shutdown. Default behavior is
to terminate the child.
On 2015/03/27 22:01:22, rvargas (slow to respond) wrote:
> nit: There is no default behavior anymore.
> 
> Note however that there is an specific exception that allows default arguments
> for constructors, so we could have a default behavior.

Good idea. Made it a default argument. (Unfortunately all call sites except for
the renderer go through BrowserChildProcessHost::Launch() and still need to be
adapted.)

[no sievers, 2015-03-27 23:14:20]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-03-27 23:14:20]

The patchset sent to the CQ was uploaded after l-g-t-m from rvargas@chromium.org
Link to the patchset: https://codereview.chromium.org/1022703007/#ps120001
(title: "default arg, rebase")

[commit-bot: I haz the power, 2015-03-27 23:14:37]

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

[no sievers, 2015-03-27 23:15:35]

The CQ bit was unchecked by sievers@chromium.org

[no sievers, 2015-03-28 01:08:08]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2015-03-28 01:08:15]

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

[commit-bot: I haz the power, 2015-03-28 01:50:34]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-28 01:51:22]

Patchset 7 (id:??) landed as
https://crrev.com/954e37aad499a68771f3c517d1e4760e38986e0e
Cr-Commit-Position: refs/heads/master@{#322695}

[Will Harris, 2015-09-29 01:31:55]

wfh@chromium.org changed reviewers:
+ wfh@chromium.org

[Will Harris, 2015-09-29 01:31:56]

Hi sievers -

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:311: base::Passed(&process_)));
This CL changes behavior of destructor to call TerminateOnLauncherThread where
previously it did not.

I think this could be the root-cause of crbug.com/493452 since the majority of
the crashes appear to be from this PostTask.

WDYT?

[no sievers, 2015-09-29 17:16:59]

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
content/browser/child_process_launcher.cc:311: base::Passed(&process_)));
On 2015/09/29 01:31:55, Will Harris wrote:
> This CL changes behavior of destructor to call TerminateOnLauncherThread where
> previously it did not.
> 
> I think this could be the root-cause of crbug.com/493452 since the majority of
> the crashes appear to be from this PostTask.
> 
> WDYT?

There is no functional behavior here, or at least none intended.
The code here was previously performed through the Context destructor where it
calls Terminate(). Did you check if the crash previously happened with the old
signature? We can probably discuss in the bug.

[Will Harris, 2015-09-29 17:19:30]

On 2015/09/29 17:16:59, sievers wrote:
>
https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
> File content/browser/child_process_launcher.cc (right):
> 
>
https://codereview.chromium.org/1022703007/diff/100001/content/browser/child_...
> content/browser/child_process_launcher.cc:311: base::Passed(&process_)));
> On 2015/09/29 01:31:55, Will Harris wrote:
> > This CL changes behavior of destructor to call TerminateOnLauncherThread
where
> > previously it did not.
> > 
> > I think this could be the root-cause of crbug.com/493452 since the majority
of
> > the crashes appear to be from this PostTask.
> > 
> > WDYT?
> 
> There is no functional behavior here, or at least none intended.
> The code here was previously performed through the Context destructor where it
> calls Terminate(). Did you check if the crash previously happened with the old
> signature? We can probably discuss in the bug.

ah yes you're right I didn't see that destructor... oh well back to drawing
board. I cannot see how this change would cause the issues I am seeing in 493452
anyway. The code looks correct. sigh. Happy to discuss on the bug.