Revert of https://codereview.chromium.org/71013004/

Reason for revert: Causing compile failure in chrome_util.cc on "Google Chrome Win" http://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Win/builds/21803

TBR=cpu@chromium.org,jvoung@chromium.org,thakis@chromium.org,sergeyu@chromium.org,grt@chromium.org,gene@chromium.org,youngki@chromium.org,rvargas@chromium.org
NOTREECHECKS=true
NOTRY=true

Review URL: https://codereview.chromium.org/90963002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@237541 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 124 insertions, 61 deletions

diff --git a/base/base.gyp b/base/base.gyp
index 8a73064..71b2615 100644
--- a/base/base.gyp
+++ b/base/base.gyp
@@ -680,6 +680,7 @@
         'win/registry_unittest.cc',
         'win/scoped_bstr_unittest.cc',
         'win/scoped_comptr_unittest.cc',
+        'win/scoped_handle_unittest.cc',
         'win/scoped_process_information_unittest.cc',
         'win/scoped_variant_unittest.cc',
         'win/shortcut_unittest.cc',
diff --git a/base/memory/shared_memory_unittest.cc b/base/memory/shared_memory_unittest.cc
index a1aa9e32..2b3ad7a 100644
--- a/base/memory/shared_memory_unittest.cc
+++ b/base/memory/shared_memory_unittest.cc
@@ -425,18 +425,16 @@ TEST(SharedMemoryTest, ShareReadOnly) {
   EXPECT_EQ(NULL, MapViewOfFile(handle, FILE_MAP_WRITE, 0, 0, 0))
       << "Shouldn't be able to map memory writable.";
 
-  HANDLE temp_handle;
-  BOOL rv = ::DuplicateHandle(GetCurrentProcess(),
+  base::win::ScopedHandle writable_handle;
+  EXPECT_EQ(0,
+            ::DuplicateHandle(GetCurrentProcess(),
                               handle,
                               GetCurrentProcess,
-                              &temp_handle,
+                              writable_handle.Receive(),
                               FILE_MAP_ALL_ACCESS,
                               false,
-                              0);
-  EXPECT_EQ(FALSE, rv)
+                              0))
       << "Shouldn't be able to duplicate the handle into a writable one.";
-  if (rv)
-    base::win::ScopedHandle writable_handle(temp_handle);
 #else
 #error Unexpected platform; write a test that tries to make 'handle' writable.
 #endif  // defined(OS_POSIX) || defined(OS_WIN)
diff --git a/base/process/launch.h b/base/process/launch.h
index f5664fb..ac2df5e 100644
--- a/base/process/launch.h
+++ b/base/process/launch.h
@@ -21,7 +21,6 @@
 #include "base/posix/file_descriptor_shuffle.h"
 #elif defined(OS_WIN)
 #include <windows.h>
-#include "base/win/scoped_handle.h"
 #endif
 
 class CommandLine;
@@ -147,7 +146,7 @@ BASE_EXPORT bool LaunchProcess(const CommandLine& cmdline,
 //  cmdline = "c:\windows\explorer.exe" -foo "c:\bar\"
 BASE_EXPORT bool LaunchProcess(const string16& cmdline,
                                const LaunchOptions& options,
-                               win::ScopedHandle* process_handle);
+                               ProcessHandle* process_handle);
 
 #elif defined(OS_POSIX)
 // A POSIX-specific version of LaunchProcess that takes an argv array
diff --git a/base/process/launch_win.cc b/base/process/launch_win.cc
index 34f84c3..da913ef 100644
--- a/base/process/launch_win.cc
+++ b/base/process/launch_win.cc
@@ -103,7 +103,7 @@ void RouteStdioToConsole() {
 
 bool LaunchProcess(const string16& cmdline,
                    const LaunchOptions& options,
-                   win::ScopedHandle* process_handle) {
+                   ProcessHandle* process_handle) {
   STARTUPINFO startup_info = {};
   startup_info.cb = sizeof(startup_info);
   if (options.empty_desktop_name)
@@ -136,7 +136,7 @@ bool LaunchProcess(const string16& cmdline,
   if (options.force_breakaway_from_job_)
     flags |= CREATE_BREAKAWAY_FROM_JOB;
 
-  PROCESS_INFORMATION temp_process_info = {};
+  base::win::ScopedProcessInformation process_info;
 
   if (options.as_user) {
     flags |= CREATE_UNICODE_ENVIRONMENT;
@@ -152,7 +152,7 @@ bool LaunchProcess(const string16& cmdline,
                             const_cast<wchar_t*>(cmdline.c_str()),
                             NULL, NULL, options.inherit_handles, flags,
                             enviroment_block, NULL, &startup_info,
-                            &temp_process_info);
+                            process_info.Receive());
     DestroyEnvironmentBlock(enviroment_block);
     if (!launched) {
       DPLOG(ERROR);
@@ -162,12 +162,11 @@ bool LaunchProcess(const string16& cmdline,
     if (!CreateProcess(NULL,
                        const_cast<wchar_t*>(cmdline.c_str()), NULL, NULL,
                        options.inherit_handles, flags, NULL, NULL,
-                       &startup_info, &temp_process_info)) {
+                       &startup_info, process_info.Receive())) {
       DPLOG(ERROR);
       return false;
     }
   }
-  base::win::ScopedProcessInformation process_info(temp_process_info);
 
   if (options.job_handle) {
     if (0 == AssignProcessToJobObject(options.job_handle,
@@ -185,7 +184,7 @@ bool LaunchProcess(const string16& cmdline,
 
   // If the caller wants the process handle, we won't close it.
   if (process_handle)
-    process_handle->Set(process_info.TakeProcessHandle());
+    *process_handle = process_info.TakeProcessHandle();
 
   return true;
 }
@@ -193,13 +192,7 @@ bool LaunchProcess(const string16& cmdline,
 bool LaunchProcess(const CommandLine& cmdline,
                    const LaunchOptions& options,
                    ProcessHandle* process_handle) {
-  if (!process_handle)
-    return LaunchProcess(cmdline.GetCommandLineString(), options, NULL);
-
-  win::ScopedHandle process;
-  bool rv = LaunchProcess(cmdline.GetCommandLineString(), options, &process);
-  *process_handle = process.Take();
-  return rv;
+  return LaunchProcess(cmdline.GetCommandLineString(), options, process_handle);
 }
 
 bool SetJobObjectLimitFlags(HANDLE job_object, DWORD limit_flags) {
@@ -240,7 +233,8 @@ bool GetAppOutput(const CommandLine& cl, std::string* output) {
 
   FilePath::StringType writable_command_line_string(cl.GetCommandLineString());
 
-  STARTUPINFO start_info = {};
+  base::win::ScopedProcessInformation proc_info;
+  STARTUPINFO start_info = { 0 };
 
   start_info.cb = sizeof(STARTUPINFO);
   start_info.hStdOutput = out_write;
@@ -250,16 +244,14 @@ bool GetAppOutput(const CommandLine& cl, std::string* output) {
   start_info.dwFlags |= STARTF_USESTDHANDLES;
 
   // Create the child process.
-  PROCESS_INFORMATION temp_process_info = {};
   if (!CreateProcess(NULL,
                      &writable_command_line_string[0],
                      NULL, NULL,
                      TRUE,  // Handles are inherited.
-                     0, NULL, NULL, &start_info, &temp_process_info)) {
+                     0, NULL, NULL, &start_info, proc_info.Receive())) {
     NOTREACHED() << "Failed to start process";
     return false;
   }
-  base::win::ScopedProcessInformation proc_info(temp_process_info);
 
   // Close our writing end of pipe now. Otherwise later read would not be able
   // to detect end of child's output.
diff --git a/base/win/scoped_handle.h b/base/win/scoped_handle.h
index 0d038e0..d3f5c22 100644
--- a/base/win/scoped_handle.h
+++ b/base/win/scoped_handle.h
@@ -39,6 +39,22 @@ class GenericScopedHandle {
  public:
   typedef typename Traits::Handle Handle;
 
+  // Helper object to contain the effect of Receive() to the function that needs
+  // a pointer, and allow proper tracking of the handle.
+  class Receiver {
+   public:
+    explicit Receiver(GenericScopedHandle* owner)
+        : handle_(Traits::NullHandle()),
+          owner_(owner) {}
+    ~Receiver() { owner_->Set(handle_); }
+
+    operator Handle*() { return &handle_; }
+
+   private:
+    Handle handle_;
+    GenericScopedHandle* owner_;
+  };
+
   GenericScopedHandle() : handle_(Traits::NullHandle()) {}
 
   explicit GenericScopedHandle(Handle handle) : handle_(Traits::NullHandle()) {
@@ -86,6 +102,16 @@ class GenericScopedHandle {
     return handle_;
   }
 
+  // This method is intended to be used with functions that require a pointer to
+  // a destination handle, like so:
+  //    void CreateRequiredHandle(Handle* out_handle);
+  //    ScopedHandle a;
+  //    CreateRequiredHandle(a.Receive());
+  Receiver Receive() {
+    DCHECK(!Traits::IsHandleValid(handle_)) << "Handle must be NULL";
+    return Receiver(this);
+  }
+
   // Transfers ownership away from this object.
   Handle Take() {
     Handle temp = handle_;
diff --git a/base/win/scoped_handle_unittest.cc b/base/win/scoped_handle_unittest.cc
new file mode 100644
index 0000000..e0d8b46
--- /dev/null
+++ b/base/win/scoped_handle_unittest.cc
@@ -0,0 +1,31 @@
+// Copyright 2013 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.
+
+#include "base/win/scoped_handle.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+void CreateHandle(int value, HANDLE* result) {
+  *result = reinterpret_cast<HANDLE>(value);
+}
+
+TEST(ScopedHandleTest, Receive) {
+  base::win::ScopedHandle handle;
+  int value = 51;
+
+  {
+    // This is not really the expected use case, but it is a very explicit test.
+    base::win::ScopedHandle::Receiver a = handle.Receive();
+    HANDLE* pointer = a;
+    *pointer = reinterpret_cast<HANDLE>(value);
+  }
+
+  EXPECT_EQ(handle.Get(), reinterpret_cast<HANDLE>(value));
+  HANDLE to_discard = handle.Take();
+
+  // The standard use case:
+  value = 183;
+  CreateHandle(value, handle.Receive());
+  EXPECT_EQ(handle.Get(), reinterpret_cast<HANDLE>(value));
+  to_discard = handle.Take();
+}
diff --git a/base/win/scoped_process_information.cc b/base/win/scoped_process_information.cc
index bb24637..cb7a30e 100644
--- a/base/win/scoped_process_information.cc
+++ b/base/win/scoped_process_information.cc
@@ -15,7 +15,7 @@ namespace {
 // Duplicates source into target, returning true upon success. |target| is
 // guaranteed to be untouched in case of failure. Succeeds with no side-effects
 // if source is NULL.
-bool CheckAndDuplicateHandle(HANDLE source, ScopedHandle* target) {
+bool CheckAndDuplicateHandle(HANDLE source, HANDLE* target) {
   if (!source)
     return true;
 
@@ -26,7 +26,7 @@ bool CheckAndDuplicateHandle(HANDLE source, ScopedHandle* target) {
     DPLOG(ERROR) << "Failed to duplicate a handle.";
     return false;
   }
-  target->Set(temp);
+  *target = temp;
   return true;
 }
 
@@ -36,15 +36,15 @@ ScopedProcessInformation::ScopedProcessInformation()
     : process_id_(0), thread_id_(0) {
 }
 
-ScopedProcessInformation::ScopedProcessInformation(
-    const PROCESS_INFORMATION& process_info) : process_id_(0), thread_id_(0) {
-  Set(process_info);
-}
-
 ScopedProcessInformation::~ScopedProcessInformation() {
   Close();
 }
 
+ScopedProcessInformation::Receiver ScopedProcessInformation::Receive() {
+  DCHECK(!IsValid()) << "process_information_ must be NULL";
+  return Receiver(this);
+}
+
 bool ScopedProcessInformation::IsValid() const {
   return process_id_ || process_handle_.Get() ||
          thread_id_ || thread_handle_.Get();
@@ -72,8 +72,10 @@ bool ScopedProcessInformation::DuplicateFrom(
   DCHECK(!IsValid()) << "target ScopedProcessInformation must be NULL";
   DCHECK(other.IsValid()) << "source ScopedProcessInformation must be valid";
 
-  if (CheckAndDuplicateHandle(other.process_handle(), &process_handle_) &&
-      CheckAndDuplicateHandle(other.thread_handle(), &thread_handle_)) {
+  if (CheckAndDuplicateHandle(other.process_handle(),
+                              process_handle_.Receive()) &&
+      CheckAndDuplicateHandle(other.thread_handle(),
+                              thread_handle_.Receive())) {
     process_id_ = other.process_id();
     thread_id_ = other.thread_id();
     return true;
diff --git a/base/win/scoped_process_information.h b/base/win/scoped_process_information.h
index 2e24054..1f404c2 100644
--- a/base/win/scoped_process_information.h
+++ b/base/win/scoped_process_information.h
@@ -18,10 +18,33 @@ namespace win {
 // structures. Allows clients to take ownership of either handle independently.
 class BASE_EXPORT ScopedProcessInformation {
  public:
+  // Helper object to contain the effect of Receive() to the funtion that needs
+  // a pointer.
+  class Receiver {
+   public:
+    explicit Receiver(ScopedProcessInformation* owner)
+        : info_(),
+          owner_(owner) {}
+    ~Receiver() { owner_->Set(info_); }
+
+    operator PROCESS_INFORMATION*() { return &info_; }
+
+   private:
+    PROCESS_INFORMATION info_;
+    ScopedProcessInformation* owner_;
+  };
+
   ScopedProcessInformation();
-  explicit ScopedProcessInformation(const PROCESS_INFORMATION& process_info);
   ~ScopedProcessInformation();
 
+  // Returns an object that may be passed to API calls such as CreateProcess.
+  // DCHECKs that the object is not currently holding any handles.
+  // HANDLEs stored in the returned PROCESS_INFORMATION will be owned by this
+  // instance.
+  // The intended use case is something like this:
+  //   if (::CreateProcess(..., startup_info, scoped_proces_info.Receive()))
+  Receiver Receive();
+
   // Returns true iff this instance is holding a thread and/or process handle.
   bool IsValid() const;
 
diff --git a/base/win/scoped_process_information_unittest.cc b/base/win/scoped_process_information_unittest.cc
index 550076e..b8ffc44 100644
--- a/base/win/scoped_process_information_unittest.cc
+++ b/base/win/scoped_process_information_unittest.cc
@@ -19,13 +19,11 @@ const DWORD kThreadId = 1234;
 const HANDLE kProcessHandle = reinterpret_cast<HANDLE>(7651);
 const HANDLE kThreadHandle = reinterpret_cast<HANDLE>(1567);
 
-void MockCreateProcess(base::win::ScopedProcessInformation* process_info) {
-  PROCESS_INFORMATION process_information = {};
-  process_information.dwProcessId = kProcessId;
-  process_information.dwThreadId = kThreadId;
-  process_information.hProcess = kProcessHandle;
-  process_information.hThread = kThreadHandle;
-  process_info->Set(process_information);
+void MockCreateProcess(PROCESS_INFORMATION* process_info) {
+  process_info->dwProcessId = kProcessId;
+  process_info->dwThreadId = kThreadId;
+  process_info->hProcess = kProcessHandle;
+  process_info->hThread = kThreadHandle;
 }
 
 }  // namespace
@@ -64,7 +62,7 @@ TEST_F(ScopedProcessInformationTest, InitiallyInvalid) {
 
 TEST_F(ScopedProcessInformationTest, Receive) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(&process_info);
+  MockCreateProcess(process_info.Receive());
 
   EXPECT_TRUE(process_info.IsValid());
   EXPECT_EQ(kProcessId, process_info.process_id());
@@ -76,7 +74,7 @@ TEST_F(ScopedProcessInformationTest, Receive) {
 
 TEST_F(ScopedProcessInformationTest, TakeProcess) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(&process_info);
+  MockCreateProcess(process_info.Receive());
 
   HANDLE process = process_info.TakeProcessHandle();
   EXPECT_EQ(kProcessHandle, process);
@@ -88,7 +86,7 @@ TEST_F(ScopedProcessInformationTest, TakeProcess) {
 
 TEST_F(ScopedProcessInformationTest, TakeThread) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(&process_info);
+  MockCreateProcess(process_info.Receive());
 
   HANDLE thread = process_info.TakeThreadHandle();
   EXPECT_EQ(kThreadHandle, thread);
@@ -100,7 +98,7 @@ TEST_F(ScopedProcessInformationTest, TakeThread) {
 
 TEST_F(ScopedProcessInformationTest, TakeBoth) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(&process_info);
+  MockCreateProcess(process_info.Receive());
 
   HANDLE process = process_info.TakeProcessHandle();
   HANDLE thread = process_info.TakeThreadHandle();
@@ -110,7 +108,7 @@ TEST_F(ScopedProcessInformationTest, TakeBoth) {
 
 TEST_F(ScopedProcessInformationTest, TakeWholeStruct) {
   base::win::ScopedProcessInformation process_info;
-  MockCreateProcess(&process_info);
+  MockCreateProcess(process_info.Receive());
 
   PROCESS_INFORMATION to_discard = process_info.Take();
   EXPECT_EQ(kProcessId, to_discard.dwProcessId);
@@ -121,11 +119,8 @@ TEST_F(ScopedProcessInformationTest, TakeWholeStruct) {
 }
 
 TEST_F(ScopedProcessInformationTest, Duplicate) {
-  PROCESS_INFORMATION temp_process_information;
-  DoCreateProcess("ReturnSeven", &temp_process_information);
   base::win::ScopedProcessInformation process_info;
-  process_info.Set(temp_process_information);
-
+  DoCreateProcess("ReturnSeven", process_info.Receive());
   base::win::ScopedProcessInformation duplicate;
   duplicate.DuplicateFrom(process_info);
 
@@ -151,17 +146,15 @@ TEST_F(ScopedProcessInformationTest, Duplicate) {
 }
 
 TEST_F(ScopedProcessInformationTest, Set) {
-  base::win::ScopedProcessInformation base_process_info;
+  PROCESS_INFORMATION base_process_info = {};
   MockCreateProcess(&base_process_info);
 
-  PROCESS_INFORMATION base_struct = base_process_info.Take();
-
   base::win::ScopedProcessInformation process_info;
-  process_info.Set(base_struct);
+  process_info.Set(base_process_info);
 
   EXPECT_EQ(kProcessId, process_info.process_id());
   EXPECT_EQ(kThreadId, process_info.thread_id());
   EXPECT_EQ(kProcessHandle, process_info.process_handle());
   EXPECT_EQ(kThreadHandle, process_info.thread_handle());
-  base_struct = process_info.Take();
+  base_process_info = process_info.Take();
 }
diff --git a/base/win/startup_information_unittest.cc b/base/win/startup_information_unittest.cc
index d637ebd..1903564 100644
--- a/base/win/startup_information_unittest.cc
+++ b/base/win/startup_information_unittest.cc
@@ -38,6 +38,7 @@ TEST_F(StartupInformationTest, InheritStdOut) {
   if (base::win::GetVersion() < base::win::VERSION_VISTA)
     return;
 
+  base::win::ScopedProcessInformation process_info;
   base::win::StartupInformation startup_info;
 
   HANDLE section = ::CreateFileMappingW(INVALID_HANDLE_VALUE, NULL,
@@ -64,13 +65,10 @@ TEST_F(StartupInformationTest, InheritStdOut) {
   std::wstring cmd_line =
       this->MakeCmdLine("FireInheritedEvents", false).GetCommandLineString();
 
-  PROCESS_INFORMATION temp_process_info = {};
   ASSERT_TRUE(::CreateProcess(NULL, const_cast<wchar_t*>(cmd_line.c_str()),
                               NULL, NULL, true, EXTENDED_STARTUPINFO_PRESENT,
                               NULL, NULL, startup_info.startup_info(),
-                              &temp_process_info)) << ::GetLastError();
-  base::win::ScopedProcessInformation process_info(temp_process_info);
-
+                              process_info.Receive())) << ::GetLastError();
   // Only the first event should be signalled
   EXPECT_EQ(WAIT_OBJECT_0, ::WaitForMultipleObjects(2, events, false,
                                                     4000));