Enforce handle ownership in base::Process

Enforce handle ownership in base::Process.

The main user (and the immediate reason for the change) is to improve handle
ownership in content::ChildProcessLauncher.

This CL is not enforcing clean ownership beyond ChildProcessLauncher; that is
to be covered by subsequent CLs.

BUG=417532
TEST=base_unittests
R=scottmg@chromium.org, thestig@chromium.org

Committed: https://crrev.com/079d184e2786686546385df92c7e72d8023f2941
Cr-Commit-Position: refs/heads/master@{#300180}

[scottmg, 2014-10-14 18:51:03]

https://codereview.chromium.org/651253002/diff/1/base/process/process.h
File base/process/process.h (right):

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:21: // TODO(rvargas) crbug.com/417532: Reaname this class
Process after switching
nit; "Rename"

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:21: // TODO(rvargas) crbug.com/417532: Reaname this class
Process after switching
Any chance we could do that upfront? i.e. switch Process to ProcessRaw and then
call this Process. Because I don't think anyone will notice "Deprecated!. Use
ProcessObject instead."

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:27: explicit ProcessObject(ProcessHandle handle =
kNullProcessHandle);
nit; I think default args are against "the rules" in preference to separate
ctors

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:32: // The destructor does not terminate the process.
Is it possible the handle could be reused if ProcessObject lasted much longer
than the process?

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:84: bool is_current_process_;
nit; indent

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:85: win::ScopedHandle process_;
Could you explain why we use Scoped on Windows, but just a number on others?

https://codereview.chromium.org/651253002/diff/1/base/process/process_win.cc
File base/process/process_win.cc (right):

https://codereview.chromium.org/651253002/diff/1/base/process/process_win.cc#...
base/process/process_win.cc:16: CHECK_NE(handle, ::GetCurrentProcess());
why does the current process need to be handled specially?

https://codereview.chromium.org/651253002/diff/1/content/browser/child_proces...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/651253002/diff/1/content/browser/child_proces...
content/browser/child_process_launcher.cc:113:
this_object->Notify(base::ProcessObject(handle));
I'm a little worried that it's going to be difficult to make progress on
ownership, since handle<->ProcessObject isn't restricted.

[rvargas (doing something else), 2014-10-14 19:56:55]

Thanks.

https://codereview.chromium.org/651253002/diff/1/base/process/process.h
File base/process/process.h (right):

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:21: // TODO(rvargas) crbug.com/417532: Reaname this class
Process after switching
On 2014/10/14 18:51:02, scottmg wrote:
> Any chance we could do that upfront? i.e. switch Process to ProcessRaw and
then
> call this Process. Because I don't think anyone will notice "Deprecated!. Use
> ProcessObject instead."

I was trying to avoid a huge change right now, but there is a huge change at the
end anyway so I guess it makes sense.

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:27: explicit ProcessObject(ProcessHandle handle =
kNullProcessHandle);
On 2014/10/14 18:51:02, scottmg wrote:
> nit; I think default args are against "the rules" in preference to separate
> ctors

I though that too until some recent thread involving c++ 11 made me look at the
style guide again, and now there are explicit exceptions about default arguments
for constructors (dunno when that changed)

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:32: // The destructor does not terminate the process.
On 2014/10/14 18:51:03, scottmg wrote:
> Is it possible the handle could be reused if ProcessObject lasted much longer
> than the process?

That should not be the case. The handle should be unrelated to the lifetime of
the actual process, but it should be valid until the last reference to the
process is released. (for Windows, which keeps a real handle)

For posix though, the "handle" is just the pid so it should be reusable
independently of this object :(. I should add a comment about it (I don't see
much more to do)

https://codereview.chromium.org/651253002/diff/1/base/process/process.h#newco...
base/process/process.h:85: win::ScopedHandle process_;
On 2014/10/14 18:51:03, scottmg wrote:
> Could you explain why we use Scoped on Windows, but just a number on others?

I'be been back and forth about how to clean up this mess. I considered writing a
scoped class to encapsulate ProcessHandle so that content & co. can pass that
around... but then what would be this class encapsulating at all? So my current
thinking is that this class should be the object that encapsulates the process
and ownership of the "handle", and leave base::ProcessHandle to be a raw
handle/pid.

That means that this class has a member that has to be properly owned, and for
windows that means ScopedHandle. For posix, that means we simulate move only
behavior for an underlying int, as there is nothing to do to "close" that
number. There is still some worrisome behavior about reuse of that pid
(background or terminate the wrong process?), but I have no idea how to fix
that, other than maybe a side effect of making all callers do the right thing
whenever they deal with this object.

https://codereview.chromium.org/651253002/diff/1/base/process/process_win.cc
File base/process/process_win.cc (right):

https://codereview.chromium.org/651253002/diff/1/base/process/process_win.cc#...
base/process/process_win.cc:16: CHECK_NE(handle, ::GetCurrentProcess());
On 2014/10/14 18:51:03, scottmg wrote:
> why does the current process need to be handled specially?

It doesn't _have_to_, but once we have a dedicated method to get an object for
the current process (also present on the old class), it makes sense to enforce
that nobody does

HANDLE handle = INVALID_HANDLE_VALUE;
Process process(handle);
process.IsValid() -> true :(

https://codereview.chromium.org/651253002/diff/1/content/browser/child_proces...
File content/browser/child_process_launcher.cc (right):

https://codereview.chromium.org/651253002/diff/1/content/browser/child_proces...
content/browser/child_process_launcher.cc:113:
this_object->Notify(base::ProcessObject(handle));
On 2014/10/14 18:51:03, scottmg wrote:
> I'm a little worried that it's going to be difficult to make progress on
> ownership, since handle<->ProcessObject isn't restricted.

The problem is that restricting the behavior implies fixing potentially hundreds
of callers at the same time :(.

The idea for this code is that StartSandboxedProcess,
base::LaunchElevatedProcess, StartChildProcess, etc, will return a proper
base::Process, but we have to be able to change a few files at a time.

[scottmg, 2014-10-14 20:17:48]

OK, lgtm then with the minor tweaks, but I'm not owners on anything here.

(I don't think it's urgent to do the rename, but it seems just easy to do it
before as at the end, and might avoid some new code using the old api.)

[rvargas (doing something else), 2014-10-16 02:13:57]

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

[rvargas (doing something else), 2014-10-16 02:17:05]

Thanks.

I wrote a CL to rename Process -> ProcessRaw, but then I realized I was
overestimating the number of real users, so I'm just removing the old class
right away.

Lei, please take a look at base and chrome.

[Lei Zhang, 2014-10-16 02:50:09]

https://codereview.chromium.org/651253002/diff/20001/base/process/process.h
File base/process/process.h (right):

https://codereview.chromium.org/651253002/diff/20001/base/process/process.h#n...
base/process/process.h:40: Process(RValue other);
should this be explicit?

https://codereview.chromium.org/651253002/diff/20001/base/process/process_uni...
File base/process/process_unittest.cc (right):

https://codereview.chromium.org/651253002/diff/20001/base/process/process_uni...
base/process/process_unittest.cc:29: base::Process
process(SpawnChild("SimpleChildProcess"));
nit: put the entire test in namespace base.

https://codereview.chromium.org/651253002/diff/20001/base/process/process_win.cc
File base/process/process_win.cc (right):

https://codereview.chromium.org/651253002/diff/20001/base/process/process_win...
base/process/process_win.cc:89: DCHECK(IsValid());
Can you give some reassurances that these DCHECKs won't fail?

https://codereview.chromium.org/651253002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/651253002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:65: if
(handle == GetCurrentProcess())
nit: funny indentation

[rvargas (doing something else), 2014-10-16 19:06:10]

Thanks

https://codereview.chromium.org/651253002/diff/20001/base/process/process.h
File base/process/process.h (right):

https://codereview.chromium.org/651253002/diff/20001/base/process/process.h#n...
base/process/process.h:40: Process(RValue other);
On 2014/10/16 02:50:08, Lei Zhang wrote:
> should this be explicit?

Nope. There's a note about that on move.h

https://codereview.chromium.org/651253002/diff/20001/base/process/process_uni...
File base/process/process_unittest.cc (right):

https://codereview.chromium.org/651253002/diff/20001/base/process/process_uni...
base/process/process_unittest.cc:29: base::Process
process(SpawnChild("SimpleChildProcess"));
On 2014/10/16 02:50:08, Lei Zhang wrote:
> nit: put the entire test in namespace base.

:(  I like to treat tests as consumers of the code being tested, not part of it.

done

https://codereview.chromium.org/651253002/diff/20001/base/process/process_win.cc
File base/process/process_win.cc (right):

https://codereview.chromium.org/651253002/diff/20001/base/process/process_win...
base/process/process_win.cc:89: DCHECK(IsValid());
On 2014/10/16 02:50:08, Lei Zhang wrote:
> Can you give some reassurances that these DCHECKs won't fail?

The DCHECKs are going through internal state only so they do not depend on
anything that for example the OS would do (like kill a process).

The intention is for them to fire as soon as someone misuses the method... like
trying to terminate a process that was never created properly in the first
place.

https://codereview.chromium.org/651253002/diff/20001/chrome/browser/renderer_...
File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
(right):

https://codereview.chromium.org/651253002/diff/20001/chrome/browser/renderer_...
chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:65: if
(handle == GetCurrentProcess())
On 2014/10/16 02:50:08, Lei Zhang wrote:
> nit: funny indentation

Done.

[Lei Zhang, 2014-10-16 20:46:37]

lgtm

[rvargas (doing something else), 2014-10-16 21:39:57]

rvargas@chromium.org changed reviewers:
+ dmichael@chromium.org, joaodasilva@chromium.org, sievers@chromium.org

[rvargas (doing something else), 2014-10-16 21:39:58]

+
joaodasilva -> components
sievers -> content
dmichael -> ppapi

[dmichael (off chromium), 2014-10-16 21:44:49]

ppapi lgtm

[Joao da Silva, 2014-10-17 06:24:26]

components/policy lgtm

[no sievers, 2014-10-17 18:43:16]

lgtm

https://codereview.chromium.org/651253002/diff/40001/content/browser/browser_...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/651253002/diff/40001/content/browser/browser_...
content/browser/browser_child_process_host_impl.h:110: // ObjectWatcher
implementation.
nit: ObjectWatcher::Delegate implementation.

[rvargas (doing something else), 2014-10-17 19:12:28]

The CQ bit was checked by rvargas@chromium.org

[rvargas (doing something else), 2014-10-17 19:12:54]

Thanks

https://codereview.chromium.org/651253002/diff/40001/content/browser/browser_...
File content/browser/browser_child_process_host_impl.h (right):

https://codereview.chromium.org/651253002/diff/40001/content/browser/browser_...
content/browser/browser_child_process_host_impl.h:110: // ObjectWatcher
implementation.
On 2014/10/17 18:43:16, sievers wrote:
> nit: ObjectWatcher::Delegate implementation.

Done.

[commit-bot: I haz the power, 2014-10-17 19:14:25]

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

[commit-bot: I haz the power, 2014-10-17 19:23:30]

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

[commit-bot: I haz the power, 2014-10-17 19:23:36]

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), 2014-10-17 19:28:34]

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

[rvargas (doing something else), 2014-10-17 19:28:36]

oops. missed device/ owner.

+ rockot

[Ken Rockot(use gerrit already), 2014-10-17 19:29:20]

On 2014/10/17 19:28:36, rvargas wrote:
> oops. missed device/ owner.
> 
> + rockot

lgtm

[rvargas (doing something else), 2014-10-17 20:38:41]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-10-17 20:39:45]

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

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

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

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

Try jobs failed on following builders:
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[rvargas (doing something else), 2014-10-17 22:04:41]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-10-17 22:06:30]

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

[commit-bot: I haz the power, 2014-10-17 22:32:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-10-17 22:33:18]

Patchset 5 (id:??) landed as
https://crrev.com/079d184e2786686546385df92c7e72d8023f2941
Cr-Commit-Position: refs/heads/master@{#300180}