Remove uses of CloseProcessHandle.

Remove uses of CloseProcessHandle.

BUG=417532
Committed: https://crrev.com/d5626f6497c92188cc5d88b6778f7bae0b33c1b3
Cr-Commit-Position: refs/heads/master@{#314846}

[rvargas (doing something else), 2015-01-24 02:54:23]

Patchset #1 (id:1) has been deleted

[rvargas (doing something else), 2015-01-24 03:43:03]

rvargas@chromium.org changed reviewers:
+ siggi@chromium.org, wez@chromium.org

[rvargas (doing something else), 2015-01-24 03:43:04]

PTAL

[Sergey Ulanov, 2015-01-24 18:04:28]

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

[Sergey Ulanov, 2015-01-24 18:04:29]

lgtm, but please see my comment

https://chromiumcodereview.appspot.com/859413003/diff/20001/remoting/host/ipc...
File remoting/host/ipc_desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/859413003/diff/20001/remoting/host/ipc...
remoting/host/ipc_desktop_environment.cc:197: desktop_process =
base::Process::Current();
nit: please add {} here and in the else case

Can this logic be moved in the base::Process constructor?

[Sigurður Ásgeirsson, 2015-01-26 14:21:29]

browser_watcher lgtm with a nit & suggestion.

https://chromiumcodereview.appspot.com/859413003/diff/20001/components/browse...
File components/browser_watcher/exit_code_watcher_win_unittest.cc (left):

https://chromiumcodereview.appspot.com/859413003/diff/20001/components/browse...
components/browser_watcher/exit_code_watcher_win_unittest.cc:92: if (process_ !=
base::kNullProcessHandle) {
Ah - that was pretty redundant - thanks for the cleanup!

https://chromiumcodereview.appspot.com/859413003/diff/20001/components/browse...
File components/browser_watcher/watcher_client_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/859413003/diff/20001/components/browse...
components/browser_watcher/watcher_client_win_unittest.cc:144: //
WaitForExitCode closes it(!!!).
Stale comment. 
I think the dupe is still necessary, but only because the client has an accessor
to process_.Handle() - this is for testing only IIRC.
Maybe cleaner to change the accessor to return a const Process& and use
Duplicate() to draw a new Process instance?

If you don't want to do this in this CL, I can take care of it - leave me a TODO
or a bug in that case, please?

PS: thanks for cleaning up ProcessHandle ownership - the old way drove me nuts!

[rvargas (doing something else), 2015-02-02 20:57:00]

Thanks.

https://chromiumcodereview.appspot.com/859413003/diff/20001/components/browse...
File components/browser_watcher/watcher_client_win_unittest.cc (right):

https://chromiumcodereview.appspot.com/859413003/diff/20001/components/browse...
components/browser_watcher/watcher_client_win_unittest.cc:144: //
WaitForExitCode closes it(!!!).
On 2015/01/26 14:21:29, Sigurður Ásgeirsson wrote:
> Stale comment. 
> I think the dupe is still necessary, but only because the client has an
accessor
> to process_.Handle() - this is for testing only IIRC.
> Maybe cleaner to change the accessor to return a const Process& and use
> Duplicate() to draw a new Process instance?
> 
> If you don't want to do this in this CL, I can take care of it - leave me a
TODO
> or a bug in that case, please?
> 
> PS: thanks for cleaning up ProcessHandle ownership - the old way drove me
nuts!

Done.

https://chromiumcodereview.appspot.com/859413003/diff/20001/remoting/host/ipc...
File remoting/host/ipc_desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/859413003/diff/20001/remoting/host/ipc...
remoting/host/ipc_desktop_environment.cc:197: desktop_process =
base::Process::Current();
On 2015/01/24 18:04:29, Sergey Ulanov wrote:
> nit: please add {} here and in the else case

done
 
> Can this logic be moved in the base::Process constructor?

Unfortunately, it cannot. This is not pretty because it is not a safe construct.
This code is basically equivalent to

if (handle == INVALID_HANDLE_VALUE)
  Do whatever I'm doing on the current process;

which is pretty bad!

IMO The problem is that the IPC code is venting a raw handle instead of an
object. If this code could receive a Process, the unit test (which is the source
of CurrentProcess) could pass the right (and safe) thing.

I thought about hacking something at the test side insteadof doing this, but
although that solves the test failure, it leaves the problem open for the
future: Do you expect any valid caller to pass CurrentProcess? If the answer is
no, I can fix the test... otherwise, we are forced to having unsafe code here.

Your call.

(BTW, I believe the right fix resides in the IPC layer, but we are going to mojo
so it's not really worth it... not that mojo fixes anything here).

[Sergey Ulanov, 2015-02-02 21:36:31]

https://chromiumcodereview.appspot.com/859413003/diff/20001/remoting/host/ipc...
File remoting/host/ipc_desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/859413003/diff/20001/remoting/host/ipc...
remoting/host/ipc_desktop_environment.cc:197: desktop_process =
base::Process::Current();
On 2015/02/02 20:57:00, rvargas (Out OO) wrote:
> On 2015/01/24 18:04:29, Sergey Ulanov wrote:
> > nit: please add {} here and in the else case
> 
> done
>  
> > Can this logic be moved in the base::Process constructor?
> 
> Unfortunately, it cannot. This is not pretty because it is not a safe
construct.
> This code is basically equivalent to
> 
> if (handle == INVALID_HANDLE_VALUE)
>   Do whatever I'm doing on the current process;
> 
> which is pretty bad!
> 
> IMO The problem is that the IPC code is venting a raw handle instead of an
> object. If this code could receive a Process, the unit test (which is the
source
> of CurrentProcess) could pass the right (and safe) thing.
> 
> I thought about hacking something at the test side insteadof doing this, but
> although that solves the test failure, it leaves the problem open for the
> future: Do you expect any valid caller to pass CurrentProcess? If the answer
is
> no, I can fix the test... otherwise, we are forced to having unsafe code here.
> 
> Your call.

Current process handle is used in tests only, so it would be ideal if this
function could get base::Process instead of base::ProcessHandle. In the same
time if it takes too much of your time to fix it then I'm fine keeping it as is
for now, maybe with a TODO to fix later.

> 
> (BTW, I believe the right fix resides in the IPC layer, but we are going to
mojo
> so it's not really worth it... not that mojo fixes anything here).

[rvargas (doing something else), 2015-02-04 02:11:10]

Patchset #3 (id:60001) has been deleted

[rvargas (doing something else), 2015-02-04 03:21:38]

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

[rvargas (doing something else), 2015-02-04 03:21:38]

+cpu for base

https://chromiumcodereview.appspot.com/859413003/diff/20001/remoting/host/ipc...
File remoting/host/ipc_desktop_environment.cc (right):

https://chromiumcodereview.appspot.com/859413003/diff/20001/remoting/host/ipc...
remoting/host/ipc_desktop_environment.cc:197: desktop_process =
base::Process::Current();
On 2015/02/02 21:36:31, Sergey Ulanov wrote:
> On 2015/02/02 20:57:00, rvargas (Out OO) wrote:
> > On 2015/01/24 18:04:29, Sergey Ulanov wrote:
> > > nit: please add {} here and in the else case
> > 
> > done
> >  
> > > Can this logic be moved in the base::Process constructor?
> > 
> > Unfortunately, it cannot. This is not pretty because it is not a safe
> construct.
> > This code is basically equivalent to
> > 
> > if (handle == INVALID_HANDLE_VALUE)
> >   Do whatever I'm doing on the current process;
> > 
> > which is pretty bad!
> > 
> > IMO The problem is that the IPC code is venting a raw handle instead of an
> > object. If this code could receive a Process, the unit test (which is the
> source
> > of CurrentProcess) could pass the right (and safe) thing.
> > 
> > I thought about hacking something at the test side insteadof doing this, but
> > although that solves the test failure, it leaves the problem open for the
> > future: Do you expect any valid caller to pass CurrentProcess? If the answer
> is
> > no, I can fix the test... otherwise, we are forced to having unsafe code
here.
> > 
> > Your call.
> 
> Current process handle is used in tests only, so it would be ideal if this
> function could get base::Process instead of base::ProcessHandle. In the same
> time if it takes too much of your time to fix it then I'm fine keeping it as
is
> for now, maybe with a TODO to fix later.
> 
> > 
> > (BTW, I believe the right fix resides in the IPC layer, but we are going to
> mojo
> > so it's not really worth it... not that mojo fixes anything here).
> 

Done.

[cpu_(ooo_6.6-7.5), 2015-02-05 03:20:18]

lgtm on the base file.

[rvargas (doing something else), 2015-02-05 18:48:43]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2015-02-05 18:49:31]

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

[commit-bot: I haz the power, 2015-02-05 19:09:53]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2015-02-05 19:11:46]

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