Stop early exit watcher in BrowserChildProcessHostImpl::OnChildDisconnected

Stop early exit watcher in BrowserChildProcessHostImpl::OnChildDisconnected

The early exit watcher holds on to a reference to the waitable event which ChildProcessLauncher::GetChildTerminationStatus deletes, so it should be stopped first.

BUG=424024
Committed: https://crrev.com/319402eea01f491047b41b7b1267f10ae81e490d
Cr-Commit-Position: refs/heads/master@{#300518}

[jbauman, 2014-10-16 23:21:49]

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

[rvargas (doing something else), 2014-10-17 01:00:08]

https://codereview.chromium.org/658253002/diff/1/content/browser/browser_chil...
File content/browser/browser_child_process_host_impl.cc (right):

https://codereview.chromium.org/658253002/diff/1/content/browser/browser_chil...
content/browser/browser_child_process_host_impl.cc:275:
early_exit_watcher_.StopWatching();
How does this work after https://codereview.chromium.org/651253002/ lands?

Do we still have to call StopWatching here? We actually stopped watching when
OnChannelConnected was received (which tells me nothing else is needed).

[jbauman, 2014-10-17 01:10:41]

On 2014/10/17 01:00:08, rvargas wrote:
>
https://codereview.chromium.org/658253002/diff/1/content/browser/browser_chil...
> File content/browser/browser_child_process_host_impl.cc (right):
> 
>
https://codereview.chromium.org/658253002/diff/1/content/browser/browser_chil...
> content/browser/browser_child_process_host_impl.cc:275:
> early_exit_watcher_.StopWatching();
> How does this work after https://codereview.chromium.org/651253002/ lands?
> 
> Do we still have to call StopWatching here? We actually stopped watching when
> OnChannelConnected was received (which tells me nothing else is needed).

I think StopWatching is still necessary. I'm not certain that OnChannelConnected
necessarily has to be called before OnChannelError (maybe the channel is dying
as it's starting up). Although, maybe your patch will fix this bug as well.

[rvargas (doing something else), 2014-10-17 01:34:07]

On 2014/10/17 01:10:41, jbauman wrote:
> On 2014/10/17 01:00:08, rvargas wrote:
> >
>
https://codereview.chromium.org/658253002/diff/1/content/browser/browser_chil...
> > File content/browser/browser_child_process_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/658253002/diff/1/content/browser/browser_chil...
> > content/browser/browser_child_process_host_impl.cc:275:
> > early_exit_watcher_.StopWatching();
> > How does this work after https://codereview.chromium.org/651253002/ lands?
> > 
> > Do we still have to call StopWatching here? We actually stopped watching
when
> > OnChannelConnected was received (which tells me nothing else is needed).
> 
> I think StopWatching is still necessary. I'm not certain that
OnChannelConnected
> necessarily has to be called before OnChannelError (maybe the channel is dying
> as it's starting up). Although, maybe your patch will fix this bug as well.

Hmm... twisted logic: ChannelWin::HandleInternalMessage has a NOTREACHED which
would mean that this is not part of the contract, but also handles the case
(sending the error notification), which means that it can happen.

oh well.

lgtm

[jbauman, 2014-10-18 00:16:02]

jbauman@chromium.org changed reviewers:
+ sky@chromium.org

[jbauman, 2014-10-18 00:16:03]

Also adding sky@ for an OWNERS review.

[sky, 2014-10-20 16:27:34]

LGTM

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

https://codereview.chromium.org/658253002/diff/20001/content/browser/browser_...
content/browser/browser_child_process_host_impl.cc:268: #if defined(OS_WIN)
Document why this is needed.

[jbauman, 2014-10-20 21:11:27]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-10-20 21:13:14]

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

[commit-bot: I haz the power, 2014-10-20 21:37:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-20 21:37:52]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/70930)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jbauman, 2014-10-21 00:17:02]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:19:32]

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

[commit-bot: I haz the power, 2014-10-21 02:21:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-21 02:21:48]

Exceeded time limit waiting for builds to trigger.

[jbauman, 2014-10-21 16:42:15]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-10-21 16:44:20]

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

[commit-bot: I haz the power, 2014-10-21 17:52:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-21 17:53:30]

Patchset 3 (id:??) landed as
https://crrev.com/319402eea01f491047b41b7b1267f10ae81e490d
Cr-Commit-Position: refs/heads/master@{#300518}