Enforce handle ownership in base::Process

base/process/process.h

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_PROCESS_PROCESS_PROCESS_H_
 #define BASE_PROCESS_PROCESS_PROCESS_H_
 
 #include "base/base_export.h"
 #include "base/basictypes.h"
+#include "base/move.h"
 #include "base/process/process_handle.h"
 #include "build/build_config.h"
 
+#if defined(OS_WIN)
+#include "base/win/scoped_handle.h"
+#endif
+
 namespace base {
 
+// Provides a move-only implementation of Process.
+// TODO(rvargas) crbug.com/417532: Reaname this class Process after switching

[scottmg, 2014/10/14 18:51:02]

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."

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

nit; "Rename"

[rvargas (doing something else), 2014/10/14 19:56:54]

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.

+// all users and deleting the old implementation.
+class BASE_EXPORT ProcessObject {
+  MOVE_ONLY_TYPE_FOR_CPP_03(ProcessObject, RValue)
+
+ public:
+  explicit ProcessObject(ProcessHandle handle = kNullProcessHandle);

[scottmg, 2014/10/14 18:51:02]

nit; I think default args are against "the rules" in preference to separate
ctors

[rvargas (doing something else), 2014/10/14 19:56:54]

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)

+
+  // Move constructor for C++03 move emulation of this type.
+  ProcessObject(RValue other);
+
+  // The destructor does not terminate the process.

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

Is it possible the handle could be reused if ProcessObject lasted much longer
than the process?

[rvargas (doing something else), 2014/10/14 19:56:54]

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)

+  ~ProcessObject() {}
+
+  // Move operator= for C++03 move emulation of this type.
+  ProcessObject& operator=(RValue other);
+
+  // Returns an object for the current process.
+  static ProcessObject Current();
+
+  // Returns true if processes can be backgrounded.
+  static bool CanBackgroundProcesses();
+
+  // Returns true if this objects represents a valid process.
+  bool IsValid() const;
+
+  // Returns a handle for this process. There is no guarantee about when that
+  // handle becomes invalid because this object retains ownership.
+  ProcessHandle Handle() const;
+
+  // Returns a second object that represents this process.
+  ProcessObject Duplicate() const;
+
+  // Get the PID for this process.
+  ProcessId pid() const;
+
+  // Returns true if this process is the current process.
+  bool is_current() const;
+
+  // Close the process handle. This will not terminate the process.
+  void Close();
+
+  // Terminates the process with extreme prejudice. The given |result_code| will
+  // be the exit code of the process.
+  // NOTE: On POSIX |result_code| is ignored.
+  void Terminate(int result_code);
+
+  // A process is backgrounded when it's priority is lower than normal.
+  // Return true if this process is backgrounded, false otherwise.
+  bool IsProcessBackgrounded() const;
+
+  // Set a process as backgrounded. If value is true, the priority of the
+  // process will be lowered. If value is false, the priority of the process
+  // will be made "normal" - equivalent to default process priority.
+  // Returns true if the priority was changed, false otherwise.
+  bool SetProcessBackgrounded(bool value);
+
+  // Returns an integer representing the priority of a process. The meaning
+  // of this value is OS dependent.
+  int GetPriority() const;
+
+ private:
+#if defined(OS_WIN)
+   bool is_current_process_;

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

nit; indent

+   win::ScopedHandle process_;

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

Could you explain why we use Scoped on Windows, but just a number on others?

[rvargas (doing something else), 2014/10/14 19:56:54]

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.

+#else
+  ProcessHandle process_;
+#endif
+};
+
+// Deprecated!. Use ProcessObject instead.
 class BASE_EXPORT Process {
  public:
   Process() : process_(kNullProcessHandle) {
   }
 
   explicit Process(ProcessHandle handle) : process_(handle) {
   }
 
   // A handle to the current process.
   static Process Current();
@@ skipping 36 matching lines @@
   // of this value is OS dependent.
   int GetPriority() const;
 
  private:
   ProcessHandle process_;
 };
 
 }  // namespace base
 
 #endif  // BASE_PROCESS_PROCESS_PROCESS_H_