Move OpenProcessHandle to Process::Open.

Move OpenProcessHandle to Process::Open.

This removes another source of raw process handles.

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

[rvargas (doing something else), 2015-01-23 23:01:05]

Patchset #1 (id:1) has been deleted

[rvargas (doing something else), 2015-01-24 00:42:27]

Patchset #1 (id:20001) has been deleted

[rvargas (doing something else), 2015-01-24 00:42:34]

Patchset #1 (id:40001) has been deleted

[rvargas (doing something else), 2015-01-24 00:42:40]

Patchset #1 (id:60001) has been deleted

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

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

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

PTAL

[cpu_(ooo_6.6-7.5), 2015-01-28 17:17:51]

I think I can review all except chrome/test is probably best an owner looks at
them.

I have a philosophical question. We are returning processes with
PROCESS_TERMINATE which seems a way for somebody to cause trouble down the line.

Could it be possible to remove this right and have the function that kills
processes reopen the handle with terminate right, kill and then close that
handle?

https://codereview.chromium.org/868543002/diff/80001/base/process/process_han...
File base/process/process_handle_posix.cc (left):

https://codereview.chromium.org/868543002/diff/80001/base/process/process_han...
base/process/process_handle_posix.cc:23: return true;
is zero special?

as in returning false?

[jln (very slow on Chromium), 2015-01-28 18:39:09]

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

[jln (very slow on Chromium), 2015-01-28 18:39:09]

https://chromiumcodereview.appspot.com/868543002/diff/80001/sandbox/linux/ser...
File sandbox/linux/services/unix_domain_socket_unittest.cc (left):

https://chromiumcodereview.appspot.com/868543002/diff/80001/sandbox/linux/ser...
sandbox/linux/services/unix_domain_socket_unittest.cc:62: base::ProcessId ret =
base::GetParentProcessId(pid);
Not related to your CL, but this should have been handle, not pid, right?

[rvargas (doing something else), 2015-02-02 19:47:09]

> I think I can review all except chrome/test is probably best an owner looks at
> them.

Sure, I'll add someone when we're happy :)

> I have a philosophical question. We are returning processes with
> PROCESS_TERMINATE which seems a way for somebody to cause trouble down the
line.

This sounds like the common case (most callers I've seen want to terminate the
process, or at least be able to).

On the other hand, I was thinking about moving some callers of OpenWithAccess()
to Open(), given that they want reduce the default access for generic tests. The
last thing to consider is that OpenWithAccess is now Windows only so if
multiplatform code wants to open to kill, we would have to write #ifdefs.

I don't have a clear answer.

> Could it be possible to remove this right and have the function that kills
> processes reopen the handle with terminate right, kill and then close that
> handle?

We could. If we do that, it should go into a dedicated CL (I'm trying not to
change behavior here).

https://chromiumcodereview.appspot.com/868543002/diff/80001/base/process/proc...
File base/process/process_handle_posix.cc (left):

https://chromiumcodereview.appspot.com/868543002/diff/80001/base/process/proc...
base/process/process_handle_posix.cc:23: return true;
On 2015/01/28 17:17:50, cpu wrote:
> is zero special?
> 
> as in returning false?  

Zero is the value for invalid handle / pid. This should be implicitly covered by
the fact that the new Process will return IsValid() as false.

https://chromiumcodereview.appspot.com/868543002/diff/80001/sandbox/linux/ser...
File sandbox/linux/services/unix_domain_socket_unittest.cc (left):

https://chromiumcodereview.appspot.com/868543002/diff/80001/sandbox/linux/ser...
sandbox/linux/services/unix_domain_socket_unittest.cc:62: base::ProcessId ret =
base::GetParentProcessId(pid);
On 2015/01/28 18:39:09, jln wrote:
> Not related to your CL, but this should have been handle, not pid, right?

I believe so. The comment says that we need a handle to be correct but then
doesn't use it.

[cpu_(ooo_6.6-7.5), 2015-02-02 22:59:29]

yeah best not to change behavior on this CL, then lgtm

[rvargas (doing something else), 2015-02-03 21:56:17]

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

[rvargas (doing something else), 2015-02-03 21:56:17]

+phajdan (chrome/test)

[rvargas (doing something else), 2015-02-04 00:05:28]

rvargas@chromium.org changed reviewers:
+ sky@chromium.org
- phajdan.jr@chromium.org

[rvargas (doing something else), 2015-02-04 00:05:29]

Pawel is OOO, so +sky

[sky, 2015-02-04 17:14:52]

LGTM

https://codereview.chromium.org/868543002/diff/80001/chrome/test/base/chrome_...
File chrome/test/base/chrome_process_util.cc (right):

https://codereview.chromium.org/868543002/diff/80001/chrome/test/base/chrome_...
chrome/test/base/chrome_process_util.cc:57: base::KillProcess(process.Handle(),
content::RESULT_CODE_KILLED, true);
Seems like it would be nice to do process.Kill(...) rather than an intermediary.

[rvargas (doing something else), 2015-02-04 18:19:51]

Thanks everyone!.

https://codereview.chromium.org/868543002/diff/80001/chrome/test/base/chrome_...
File chrome/test/base/chrome_process_util.cc (right):

https://codereview.chromium.org/868543002/diff/80001/chrome/test/base/chrome_...
chrome/test/base/chrome_process_util.cc:57: base::KillProcess(process.Handle(),
content::RESULT_CODE_KILLED, true);
On 2015/02/04 17:14:52, sky wrote:
> Seems like it would be nice to do process.Kill(...) rather than an
intermediary.

Yes, that's part of the plan (remove the utility functions that take a
ProcessHandle)

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

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2015-02-04 18:20:58]

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

[commit-bot: I haz the power, 2015-02-04 18:26:27]

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

[commit-bot: I haz the power, 2015-02-04 18:26:29]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[rvargas (doing something else), 2015-02-04 19:30:54]

New patchsets have been uploaded after l-g-t-m from sky@chromium.org

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

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2015-02-04 19:32:12]

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

[commit-bot: I haz the power, 2015-02-04 20:00:48]

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

[commit-bot: I haz the power, 2015-02-04 20:00:49]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[rvargas (doing something else), 2015-02-04 20:46:19]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2015-02-04 20:47:29]

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

[commit-bot: I haz the power, 2015-02-04 21:12:12]

Committed patchset #2 (id:100001)

[commit-bot: I haz the power, 2015-02-04 21:13:28]

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