Conv CHECK->DCHECK in CloseHandle (top crasher & data enough to debug)

Conv CHECK->DCHECK in CloseHandle (top crasher & data enough to debug)
We seem to have gathered enough data to find out debug further cause/reason for calling CloseHandle on already closed/invalid handle.
There are couple of theories which need to be investigated further like policy ownership of TargetProcess and thread watching for JOB_OBJECT_MSG_ACTIVE_PROCESS_ZERO.
CHECK seem to have introduced with CL https://codereview.chromium.org/1045513004

BUG=480639
R=cpu@chromium.org,grt@chromium.org

Committed: https://crrev.com/dfb73a76e24eadbb2311c7b756c0383acefc8f06
Cr-Commit-Position: refs/heads/master@{#327348}

[Will Harris, 2015-04-27 23:59:45]

drive-by comment - won't removing this CHECK mean that these CloseHandle
failures become impossible to debug, since the only data we get on these is from
crash reports in the wild?

[Shrikant Kelkar, 2015-04-28 00:07:57]

On 2015/04/27 23:59:45, Will Harris wrote:
> drive-by comment - won't removing this CHECK mean that these CloseHandle
> failures become impossible to debug, since the only data we get on these is
from
> crash reports in the wild?

Agreed, we can certainly turn it back on whenever needed. Since this is on for 2
weeks and we have got so many crashes to analyse I thought we can turn it off
and whenever any CL which addresses issue we can turn it back on.

[grt (UTC plus 2), 2015-04-28 01:54:03]

https://codereview.chromium.org/1108953003/diff/1/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/1108953003/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:44: DCHECK(false);
return false in this case. how about:
  if (::CloseHandle(handle))
    return true;
  NOTREACHED():
  return false;
to avoid extra braces.

[Shrikant Kelkar, 2015-04-28 17:59:10]

https://codereview.chromium.org/1108953003/diff/1/base/win/scoped_handle.cc
File base/win/scoped_handle.cc (right):

https://codereview.chromium.org/1108953003/diff/1/base/win/scoped_handle.cc#n...
base/win/scoped_handle.cc:44: DCHECK(false);
On 2015/04/28 01:54:03, grt wrote:
> return false in this case. how about:
>   if (::CloseHandle(handle))
>     return true;
>   NOTREACHED():
>   return false;
> to avoid extra braces.

Done.

[grt (UTC plus 2), 2015-04-28 18:23:38]

lgtm

[cpu_(ooo_6.6-7.5), 2015-04-28 18:24:26]

yeah, don't know if there is a master switch but we have plenty of crashes
already.

lgtm

[cpu_(ooo_6.6-7.5), 2015-04-28 18:24:41]

The CQ bit was checked by cpu@chromium.org

[commit-bot: I haz the power, 2015-04-28 18:25:11]

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

[commit-bot: I haz the power, 2015-04-28 19:35:24]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-28 19:36:23]

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

[rvargas (doing something else), 2015-04-30 01:18:19]

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

[rvargas (doing something else), 2015-04-30 01:18:19]

Please don't do this!.
(AKA, revert this change).

The contract for a lot of stuff that Closes a handle is void (intentionally). 

We want to crash when close fails, this is not a case of gathering extra debug
data, it is a case of not going on after a critical failure: if we are dealing
with invalid handles we don't know what are we doing and it is not safe to
continue working under those conditions.

[grt (UTC plus 2), 2015-04-30 02:46:39]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1109423003/ by grt@chromium.org.

The reason for reverting is: rvargas said so..