Notify observers if a running browser child process is shut down by deleting the BrowserChildProces…

Notify observers if a running browser child process is shut down by deleting the BrowserChildProcessHostImpl.

BUG=598775
Committed: https://crrev.com/012289f7b6689dff062785907d2475ab4d9b9496
Cr-Commit-Position: refs/heads/master@{#384454}

[Anand Mistry (off Chromium), 2016-03-31 00:21:21]

amistry@chromium.org changed reviewers:
+ nick@chromium.org

[Anand Mistry (off Chromium), 2016-03-31 00:21:21]

WDYT?

[ncarter (slow), 2016-03-31 15:59:58]

lgtm

thanks!

[ncarter (slow), 2016-03-31 16:06:15]

https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:202:
notify_child_disconnected_ = true;
Actually, is this line necessary? If we're only notifying on disconnect, then I
think we only need to set the bit to true when the channel connects.

[Anand Mistry (off Chromium), 2016-03-31 18:51:33]

https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:202:
notify_child_disconnected_ = true;
On 2016/03/31 16:06:15, ncarter wrote:
> Actually, is this line necessary? If we're only notifying on disconnect, then
I
> think we only need to set the bit to true when the channel connects.

I believe so. I thought about this, but then I notice this little comment in
OnChildDisconnected:
// OnChildDisconnected may be called without OnChannelConnected, so stop the
// early exit watcher so GetTerminationStatus can close the process handle.

[ncarter (slow), 2016-03-31 20:17:31]

https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:202:
notify_child_disconnected_ = true;
On 2016/03/31 18:51:33, Anand Mistry wrote:
> On 2016/03/31 16:06:15, ncarter wrote:
> > Actually, is this line necessary? If we're only notifying on disconnect,
then
> I
> > think we only need to set the bit to true when the channel connects.
> 
> I believe so. I thought about this, but then I notice this little comment in
> OnChildDisconnected:
> // OnChildDisconnected may be called without OnChannelConnected, so stop the
> // early exit watcher so GetTerminationStatus can close the process handle.

That comment is correct: OnProcessLaunched() can occur either before, or after,
OnChannelConnected(). But when OnProcessLaunched() happens first, we don't
notify the observers about anything until OnChannelConnected() happens.

We're just trying to make it so that NotifyProcessHostConnected is always
balanced with NotifyProcessHostDisconnected, so it seems like this needs to be
set to true only from OnChannelConnected().

[Anand Mistry (off Chromium), 2016-03-31 22:14:10]

https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
content/browser/browser_child_process_host_impl.cc:202:
notify_child_disconnected_ = true;
On 2016/03/31 20:17:31, ncarter wrote:
> On 2016/03/31 18:51:33, Anand Mistry wrote:
> > On 2016/03/31 16:06:15, ncarter wrote:
> > > Actually, is this line necessary? If we're only notifying on disconnect,
> then
> > I
> > > think we only need to set the bit to true when the channel connects.
> > 
> > I believe so. I thought about this, but then I notice this little comment in
> > OnChildDisconnected:
> > // OnChildDisconnected may be called without OnChannelConnected, so stop the
> > // early exit watcher so GetTerminationStatus can close the process handle.
> 
> That comment is correct: OnProcessLaunched() can occur either before, or
after,
> OnChannelConnected(). But when OnProcessLaunched() happens first, we don't
> notify the observers about anything until OnChannelConnected() happens.
> 
> We're just trying to make it so that NotifyProcessHostConnected is always
> balanced with NotifyProcessHostDisconnected, so it seems like this needs to be
> set to true only from OnChannelConnected().

It seems that the current behaviour isn't balanced. OnChildDisconnected() always
runs NotifyProcessHostDisconnected, regardless of whether or not
OnChannelConnected() was run. Maybe this is a bug, I don't know. I'm just trying
to preserve existing behaviour.

There's another detail which might help explain why I've done this change as it
is... in-process unit tests. In this case, I believe OnChannelConnected() is run
without OnProcessLauncher() being run or Launch() being called.

[ncarter (slow), 2016-03-31 22:27:30]

On 2016/03/31 22:14:10, Anand Mistry wrote:
>
https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
> File content/browser/browser_child_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1843663003/diff/20001/content/browser/browser...
> content/browser/browser_child_process_host_impl.cc:202:
> notify_child_disconnected_ = true;
> On 2016/03/31 20:17:31, ncarter wrote:
> > On 2016/03/31 18:51:33, Anand Mistry wrote:
> > > On 2016/03/31 16:06:15, ncarter wrote:
> > > > Actually, is this line necessary? If we're only notifying on disconnect,
> > then
> > > I
> > > > think we only need to set the bit to true when the channel connects.
> > > 
> > > I believe so. I thought about this, but then I notice this little comment
in
> > > OnChildDisconnected:
> > > // OnChildDisconnected may be called without OnChannelConnected, so stop
the
> > > // early exit watcher so GetTerminationStatus can close the process
handle.
> > 
> > That comment is correct: OnProcessLaunched() can occur either before, or
> after,
> > OnChannelConnected(). But when OnProcessLaunched() happens first, we don't
> > notify the observers about anything until OnChannelConnected() happens.
> > 
> > We're just trying to make it so that NotifyProcessHostConnected is always
> > balanced with NotifyProcessHostDisconnected, so it seems like this needs to
be
> > set to true only from OnChannelConnected().
> 
> It seems that the current behaviour isn't balanced. OnChildDisconnected()
always
> runs NotifyProcessHostDisconnected, regardless of whether or not
> OnChannelConnected() was run. Maybe this is a bug, I don't know. I'm just
trying
> to preserve existing behaviour.
> 
> There's another detail which might help explain why I've done this change as
it
> is... in-process unit tests. In this case, I believe OnChannelConnected() is
run
> without OnProcessLauncher() being run or Launch() being called.

Ah, I see. This whole file is very subtle, and so I'm fine with preserving
existing behavior.

lgtm as originally proposed

[Anand Mistry (off Chromium), 2016-03-31 22:45:21]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2016-03-31 22:45:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1843663003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1843663003/20001

[commit-bot: I haz the power, 2016-04-01 00:56:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-01 00:57:58]

Description was changed from

==========
Notify observers if a running browser child process is shut down by deleting the
BrowserChildProcessHostImpl.

BUG=598775
==========

to

==========
Notify observers if a running browser child process is shut down by deleting the
BrowserChildProcessHostImpl.

BUG=598775

Committed: https://crrev.com/012289f7b6689dff062785907d2475ab4d9b9496
Cr-Commit-Position: refs/heads/master@{#384454}
==========

[commit-bot: I haz the power, 2016-04-01 00:57:58]

Patchset 2 (id:??) landed as
https://crrev.com/012289f7b6689dff062785907d2475ab4d9b9496
Cr-Commit-Position: refs/heads/master@{#384454}