Fix invalid handle errors found with App Verifier

ChildProcessData was holding a raw ProcessHandle which meant that there
was no guarantee that the handle would be valid when it was used. This
change switches to a ScopedHandle on Windows, with the associated
duplication of the handle, so that it is guaranteed to be valid. This
fixes a frequently encountered use of an invalid handle that was
detected by Application Verifier. This change also adds a CHECK to
base::GetProcId to check for use of invalid handles even when not using
Application Verifier.

ChildProcessData::handle is now a private member so all references to
it were updated to use accessors.

When ChildProcessData::SetHandle is called the handle is duplicated.
When a ChildProcessData object needs to be copied an explicit
Duplicate function is called.

An IsHandleValid helper function was added.

An invalid test was removed because it was triggering the CHECK in
base::GetProcId.

Bug: 417532,821453,865805
Change-Id: I7e38140335c0140536919341f011f144f150c88f
Reviewed-on: https://chromium-review.googlesource.com/1145767
Commit-Queue: Bruce Dawson <brucedawson@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578419}

base/process/process_handle_win.cc

@@ -21,8 +21,14 @@ ProcessHandle GetCurrentProcessHandle() {
 }
 
 ProcessId GetProcId(ProcessHandle process) {
+  if (process == base::kNullProcessHandle)
+    return 0;
   // This returns 0 if we have insufficient rights to query the process handle.
-  return GetProcessId(process);
+  // Invalid handles or non-process handles will cause a hard failure.
+  ProcessId result = GetProcessId(process);
+  CHECK(result != 0 || GetLastError() != ERROR_INVALID_HANDLE)
+      << "process handle = " << process;
+  return result;
 }
 
 ProcessId GetParentProcessId(ProcessHandle process) {

chrome/browser/android/compositor/compositor_view.cc

@@ -255,7 +255,7 @@ void CompositorView::BrowserChildProcessKilled(
     const content::ChildProcessData& data,
     const content::ChildProcessTerminationInfo& info) {
   LOG(WARNING) << "Child process died (type=" << data.process_type
-               << ") pid=" << data.handle << ")";
+               << ") pid=" << data.GetHandle() << ")";
   if (base::android::BuildInfo::GetInstance()->sdk_int() <=
           base::android::SDK_VERSION_JELLY_BEAN_MR2 &&
       data.process_type == content::PROCESS_TYPE_GPU) {

chrome/browser/android/feedback/process_id_feedback_source.cc

@@ -65,7 +65,8 @@ void ProcessIdFeedbackSource::PrepareProcessIdsOnIOThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   for (content::BrowserChildProcessHostIterator iter; !iter.Done(); ++iter)
-    process_ids_[iter.GetData().process_type].push_back(iter.GetData().handle);
+    process_ids_[iter.GetData().process_type].push_back(
+        iter.GetData().GetHandle());
 
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,

chrome/browser/chrome_content_browser_client_browsertest_chromeos.cc

@@ -26,7 +26,7 @@ namespace {
 void GetUtilityProcessPidsOnIOThread(std::vector<pid_t>* pids) {
   for (BrowserChildProcessHostIterator it(content::PROCESS_TYPE_UTILITY);
        !it.Done(); ++it) {
-    pid_t pid = it.GetData().handle;
+    pid_t pid = it.GetData().GetHandle();
     pids->push_back(pid);
   }
 }

chrome/browser/chrome_plugin_browsertest.cc

@@ -154,7 +154,7 @@ class ChromePluginTest : public InProcessBrowserTest {
       if (iter.GetData().process_type != content::PROCESS_TYPE_PPAPI_PLUGIN)
         continue;
       base::Process process = base::Process::DeprecatedGetProcessFromHandle(
-          iter.GetData().handle);
+          iter.GetData().GetHandle());
       process.Terminate(0, true);
       found = true;
     }

chrome/browser/chromeos/app_mode/app_session.cc

@@ -78,12 +78,12 @@ void DumpPluginProcessOnIOThread(const std::set<int>& child_ids) {
     const content::ChildProcessData& data = iter.GetData();
     if (child_ids.count(data.id) == 1) {
       // Send a signal to dump the plugin process.
-      if (kill(data.handle, SIGFPE) == 0) {
+      if (kill(data.GetHandle(), SIGFPE) == 0) {
         dump_requested = true;
       } else {
         LOG(WARNING) << "Failed to send SIGFPE to plugin process"
                      << ", errno=" << errno
-                     << ", pid=" << data.handle
+                     << ", pid=" << data.GetHandle()
                      << ", type=" << data.process_type
                      << ", name=" << data.name;
       }

chrome/browser/extensions/api/processes/processes_api.cc

@@ -520,7 +520,7 @@ base::ProcessHandle ProcessesTerminateFunction::GetProcessHandleOnIO(
 
   auto* host = content::BrowserChildProcessHost::FromID(child_process_host_id);
   if (host)
-    return host->GetData().handle;
+    return host->GetData().GetHandle();
 
   return base::kNullProcessHandle;
 }

chrome/browser/image_decoder_browsertest.cc

@@ -118,13 +118,12 @@ class KillProcessObserver : public content::BrowserChildProcessObserver {
   void BrowserChildProcessHostConnected(
       const content::ChildProcessData& data) override {
     DCHECK_CURRENTLY_ON(BrowserThread::UI);
-    if (data.handle == base::kNullProcessHandle ||
-        data.name != utility_process_name_) {
+    if (!data.IsHandleValid() || data.name != utility_process_name_) {
       return;
     }
 
     ASSERT_FALSE(did_kill_);
-    base::ProcessHandle handle = data.handle;
+    base::ProcessHandle handle = data.GetHandle();
 
 #if defined(OS_WIN)
     // On windows, duplicate the process handle since base::Process closes it on

chrome/browser/memory_details.cc

@@ -197,9 +197,9 @@ void MemoryDetails::CollectChildInfoOnIOThread() {
   // the process is being launched, so we skip it.
   for (BrowserChildProcessHostIterator iter; !iter.Done(); ++iter) {
     ProcessMemoryInformation info;
-    if (!iter.GetData().handle)
+    if (!iter.GetData().GetHandle())
       continue;
-    info.pid = base::GetProcId(iter.GetData().handle);
+    info.pid = base::GetProcId(iter.GetData().GetHandle());
     if (!info.pid)
       continue;
 

chrome/browser/performance_monitor/performance_monitor.cc

@@ -150,7 +150,7 @@ void PerformanceMonitor::GatherMetricsMapOnIOThread(
   // See https://crbug.com/821453.
   for (content::BrowserChildProcessHostIterator iter; !iter.Done(); ++iter) {
     ProcessMetricsMetadata child_process_data;
-    child_process_data.handle = iter.GetData().handle;
+    child_process_data.handle = iter.GetData().GetHandle();
     child_process_data.process_type = iter.GetData().process_type;
 
     if (iter.GetData().name == base::ASCIIToUTF16(content::kFlashPluginName)) {

chrome/browser/resource_coordinator/browser_child_process_watcher.cc

@@ -32,7 +32,8 @@ void BrowserChildProcessWatcher::BrowserChildProcessLaunchedAndConnected(
             content::ServiceManagerConnection::GetForProcess()->GetConnector());
 
     gpu_process_resource_coordinator_->SetLaunchTime(base::Time::Now());
-    gpu_process_resource_coordinator_->SetPID(base::GetProcId(data.handle));
+    gpu_process_resource_coordinator_->SetPID(
+        base::GetProcId(data.GetHandle()));
   }
 }
 

chrome/browser/task_manager/providers/child_process_task.cc

@@ -158,7 +158,7 @@ ChildProcessTask::ChildProcessTask(const content::ChildProcessData& data)
     : Task(GetLocalizedTitle(data.name, data.process_type),
            base::UTF16ToUTF8(data.name),
            FetchIcon(IDR_PLUGINS_FAVICON, &s_icon_),
-           data.handle),
+           data.GetHandle()),
       process_resources_sampler_(CreateProcessResourcesSampler(data.id)),
       v8_memory_allocated_(-1),
       v8_memory_used_(-1),

chrome/browser/task_manager/providers/child_process_task_provider.cc

@@ -31,10 +31,10 @@ std::unique_ptr<std::vector<ChildProcessData>> CollectChildProcessData() {
     const ChildProcessData& process_data = itr.GetData();
 
     // Only add processes that have already started, i.e. with valid handles.
-    if (process_data.handle == base::kNullProcessHandle)
+    if (!process_data.IsHandleValid())
       continue;
 
-    child_processes->push_back(process_data);
+    child_processes->push_back(process_data.Duplicate());
   }
 
   return child_processes;
@@ -62,7 +62,7 @@ Task* ChildProcessTaskProvider::GetTaskOfUrlRequest(int child_id,
 void ChildProcessTaskProvider::BrowserChildProcessLaunchedAndConnected(
     const content::ChildProcessData& data) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (data.handle == base::kNullProcessHandle)
+  if (!data.IsHandleValid())
     return;
 
   CreateTask(data);
@@ -71,12 +71,12 @@ void ChildProcessTaskProvider::BrowserChildProcessLaunchedAndConnected(
 void ChildProcessTaskProvider::BrowserChildProcessHostDisconnected(
     const content::ChildProcessData& data) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DeleteTask(data.handle);
+  DeleteTask(data.GetHandle());
 }
 
 void ChildProcessTaskProvider::StartUpdating() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(tasks_by_handle_.empty());
+  DCHECK(tasks_by_processid_.empty());
   DCHECK(tasks_by_child_id_.empty());
 
   // First, get the pre-existing child processes data.
@@ -102,7 +102,7 @@ void ChildProcessTaskProvider::StopUpdating() {
   // StopUpdating() is called after the observer has been cleared.
 
   // Then delete all tasks (if any).
-  tasks_by_handle_.clear();
+  tasks_by_processid_.clear();
   tasks_by_child_id_.clear();
 }
 
@@ -120,7 +120,8 @@ void ChildProcessTaskProvider::ChildProcessDataCollected(
 
 void ChildProcessTaskProvider::CreateTask(
     const content::ChildProcessData& data) {
-  std::unique_ptr<ChildProcessTask>& task = tasks_by_handle_[data.handle];
+  std::unique_ptr<ChildProcessTask>& task =
+      tasks_by_processid_[base::GetProcId(data.GetHandle())];
   if (task) {
     // This task is already known to us. This case can happen when some of the
     // child process data we collect upon StartUpdating() might be of
@@ -136,13 +137,13 @@ void ChildProcessTaskProvider::CreateTask(
 }
 
 void ChildProcessTaskProvider::DeleteTask(base::ProcessHandle handle) {
-  auto itr = tasks_by_handle_.find(handle);
+  auto itr = tasks_by_processid_.find(base::GetProcId(handle));
 
   // The following case should never happen since we start observing
   // |BrowserChildProcessObserver| only after we collect all pre-existing child
   // processes and are notified (on the UI thread) that the collection is
   // completed at |ChildProcessDataCollected()|.
-  if (itr == tasks_by_handle_.end()) {
+  if (itr == tasks_by_processid_.end()) {
     // BUG(crbug.com/611067): Temporarily removing due to test flakes. The
     // reason why this happens is well understood (see bug), but there's no
     // quick and easy fix.
@@ -156,7 +157,7 @@ void ChildProcessTaskProvider::DeleteTask(base::ProcessHandle handle) {
   tasks_by_child_id_.erase(itr->second->GetChildProcessUniqueID());
 
   // Finally delete the task.
-  tasks_by_handle_.erase(itr);
+  tasks_by_processid_.erase(itr);
 }
 
 }  // namespace task_manager

chrome/browser/task_manager/providers/child_process_task_provider.h

@@ -65,11 +65,11 @@ class ChildProcessTaskProvider
 
   // A map to track ChildProcessTasks by their handles.
   //
-  // This uses handles instead of pids because on windows (where pids and
-  // handles differ), BrowserChildProcessObserver gives us a handle instead of a
-  // pid.
-  std::map<base::ProcessHandle, std::unique_ptr<ChildProcessTask>>
-      tasks_by_handle_;
+  // This uses pids instead of handles because on windows (where pids and
+  // handles differ), there may be multiple different handles to the same
+  // process.
+  std::map<base::ProcessId, std::unique_ptr<ChildProcessTask>>
+      tasks_by_processid_;
 
   // A map to track ChildProcessTask's by their child process unique ids.
   base::flat_map<int, ChildProcessTask*> tasks_by_child_id_;

chrome/browser/task_manager/providers/child_process_task_unittest.cc

@@ -69,7 +69,7 @@ class ChildProcessTaskTest
 
   bool AreProviderContainersEmpty(
       const ChildProcessTaskProvider& provider) const {
-    return provider.tasks_by_handle_.empty() &&
+    return provider.tasks_by_processid_.empty() &&
            provider.tasks_by_child_id_.empty();
   }
 
@@ -106,7 +106,7 @@ TEST_F(ChildProcessTaskTest, TestAll) {
   // The following process which has handle = base::kNullProcessHandle, won't be
   // added.
   ChildProcessData data1(0);
-  ASSERT_EQ(base::kNullProcessHandle, data1.handle);
+  ASSERT_EQ(base::kNullProcessHandle, data1.GetHandle());
   provider.BrowserChildProcessLaunchedAndConnected(data1);
   EXPECT_TRUE(provided_tasks_.empty());
 
@@ -116,14 +116,15 @@ TEST_F(ChildProcessTaskTest, TestAll) {
       IDS_TASK_MANAGER_PLUGIN_PREFIX, name));
 
   ChildProcessData data2(content::PROCESS_TYPE_PPAPI_PLUGIN);
-  data2.handle = base::GetCurrentProcessHandle();
+  data2.SetHandle(base::GetCurrentProcessHandle());
   data2.name = name;
   data2.id = unique_id;
   provider.BrowserChildProcessLaunchedAndConnected(data2);
   ASSERT_EQ(1U, provided_tasks_.size());
 
   Task* task = provided_tasks_.begin()->second;
-  EXPECT_EQ(base::GetCurrentProcessHandle(), task->process_handle());
+  // Process handles may not match, but process IDs must:
+  EXPECT_EQ(base::GetCurrentProcId(), base::GetProcId(task->process_handle()));
   EXPECT_EQ(base::GetCurrentProcId(), task->process_id());
   EXPECT_EQ(expected_name, task->title());
   EXPECT_EQ(Task::PLUGIN, task->GetType());
@@ -162,11 +163,12 @@ TEST_F(ChildProcessTaskTest, ProcessTypeToTaskType) {
   for (const auto& types_pair : process_task_types_pairs) {
     // Add the task.
     ChildProcessData data(types_pair.process_type_);
-    data.handle = base::GetCurrentProcessHandle();
+    data.SetHandle(base::GetCurrentProcessHandle());
     provider.BrowserChildProcessLaunchedAndConnected(data);
     ASSERT_EQ(1U, provided_tasks_.size());
     Task* task = provided_tasks_.begin()->second;
-    EXPECT_EQ(base::GetCurrentProcessHandle(), task->process_handle());
+    EXPECT_EQ(base::GetCurrentProcId(),
+              base::GetProcId(task->process_handle()));
     EXPECT_EQ(types_pair.expected_task_type_, task->GetType());
 
     // Remove the task.

chrome/browser/task_manager/providers/render_process_host_task_provider.cc

@@ -82,9 +82,7 @@ void RenderProcessHostTaskProvider::CreateTask(
   // TODO(cburn): plumb out something from RPH so the title can be set here.
   // Create the task and notify the observer.
   ChildProcessData data(content::PROCESS_TYPE_RENDERER);
-  // TODO(siggi): Investigate whether this is also a handle race, per
-  //     https://crbug.com/821453.
-  data.handle = host->GetProcess().Handle();
+  data.SetHandle(host->GetProcess().Handle());
   data.id = host->GetID();
   task.reset(new ChildProcessTask(data));
   NotifyObserverTaskAdded(task.get());

chrome/browser/ui/hung_plugin_tab_helper.cc

@@ -38,9 +38,9 @@ void KillPluginOnIOThread(int child_id) {
   while (!iter.Done()) {
     const content::ChildProcessData& data = iter.GetData();
     if (data.id == child_id) {
-      CrashDumpHungChildProcess(data.handle);
+      CrashDumpHungChildProcess(data.GetHandle());
       base::Process process =
-          base::Process::DeprecatedGetProcessFromHandle(data.handle);
+          base::Process::DeprecatedGetProcessFromHandle(data.GetHandle());
       process.Terminate(content::RESULT_CODE_HUNG, false);
       break;
     }

chrome/browser/ui/webui/memory_internals_ui.cc

@@ -279,7 +279,7 @@ void MemoryInternalsDOMHandler::GetChildProcessesOnIOThread(
     const content::ChildProcessData& data = iter.GetData();
 
     if (data.process_type == content::PROCESS_TYPE_GPU) {
-      result.push_back(MakeProcessInfo(base::GetProcId(data.handle),
+      result.push_back(MakeProcessInfo(base::GetProcId(data.GetHandle()),
                                        GetChildDescription(data)));
     }
   }

components/browser_watcher/exit_code_watcher_win_unittest.cc

@@ -119,16 +119,6 @@ class ExitCodeWatcherTest : public testing::Test {
 
 }  // namespace
 
-TEST_F(ExitCodeWatcherTest, ExitCodeWatcherInvalidHandleFailsInit) {
-  ExitCodeWatcher watcher(kRegistryPath);
-
-  // A waitable event has a non process-handle.
-  base::Process event(::CreateEvent(nullptr, false, false, nullptr));
-
-  // A non-process handle should fail.
-  EXPECT_FALSE(watcher.Initialize(std::move(event)));
-}
-
 TEST_F(ExitCodeWatcherTest, ExitCodeWatcherNoAccessHandleFailsInit) {
   ExitCodeWatcher watcher(kRegistryPath);
 

components/crash/content/browser/child_exit_observer_android.cc

@@ -152,7 +152,7 @@ void ChildExitObserver::BrowserChildProcessHostDisconnected(
     browser_child_process_info_.erase(it);
   } else {
     info.process_host_id = data.id;
-    info.pid = data.handle;
+    info.pid = data.GetHandle();
     info.process_type = static_cast<content::ProcessType>(data.process_type);
     info.app_state = base::android::ApplicationStatusListener::GetState();
     info.normal_termination = true;
@@ -167,7 +167,7 @@ void ChildExitObserver::BrowserChildProcessKilled(
   DCHECK(!base::ContainsKey(browser_child_process_info_, data.id));
   TerminationInfo info;
   info.process_host_id = data.id;
-  info.pid = data.handle;
+  info.pid = data.GetHandle();
   info.process_type = static_cast<content::ProcessType>(data.process_type);
   info.app_state = base::android::ApplicationStatusListener::GetState();
   PopulateTerminationInfo(content_info, &info);

components/heap_profiling/client_connection_manager.cc

@@ -165,7 +165,7 @@ void StartProfilingPidOnIOThread(base::WeakPtr<Controller> controller,
   for (content::BrowserChildProcessHostIterator browser_child_iter;
        !browser_child_iter.Done(); ++browser_child_iter) {
     const content::ChildProcessData& data = browser_child_iter.GetData();
-    if (base::GetProcId(data.handle) == pid) {
+    if (base::GetProcId(data.GetHandle()) == pid) {
       StartProfilingNonRendererChildOnIOThread(controller, data, pid);
       return;
     }
@@ -188,9 +188,9 @@ void StartProfilingNonRenderersIfNecessaryOnIOThread(
        !browser_child_iter.Done(); ++browser_child_iter) {
     const content::ChildProcessData& data = browser_child_iter.GetData();
     if (ShouldProfileNonRendererProcessType(mode, data.process_type) &&
-        data.handle != base::kNullProcessHandle) {
-      StartProfilingNonRendererChildOnIOThread(controller, data,
-                                               base::GetProcId(data.handle));
+        data.IsHandleValid()) {
+      StartProfilingNonRendererChildOnIOThread(
+          controller, data, base::GetProcId(data.GetHandle()));
     }
   }
 }
@@ -303,7 +303,7 @@ void ClientConnectionManager::StartProfilingNonRendererChild(
       ->PostTask(
           FROM_HERE,
           base::BindOnce(&StartProfilingNonRendererChildOnIOThread, controller_,
-                         data, base::GetProcId(data.handle)));
+                         data.Duplicate(), base::GetProcId(data.GetHandle())));
 }
 
 void ClientConnectionManager::Observe(

components/nacl/browser/nacl_broker_host_win.cc

@@ -103,7 +103,7 @@ bool NaClBrokerHost::LaunchDebugExceptionHandler(
     int32_t pid,
     base::ProcessHandle process_handle,
     const std::string& startup_info) {
-  base::ProcessHandle broker_process = process_->GetData().handle;
+  base::ProcessHandle broker_process = process_->GetData().GetHandle();
   base::ProcessHandle handle_in_broker_process;
   if (!DuplicateHandle(::GetCurrentProcess(), process_handle,
                        broker_process, &handle_in_broker_process,

components/nacl/browser/nacl_process_host.cc

@@ -250,7 +250,7 @@ NaClProcessHost::NaClProcessHost(
 
 NaClProcessHost::~NaClProcessHost() {
   // Report exit status only if the process was successfully started.
-  if (process_->GetData().handle != base::kNullProcessHandle) {
+  if (!process_->GetData().IsHandleValid()) {
     content::ChildProcessTerminationInfo info =
         process_->GetTerminationInfo(false /* known_dead */);
     std::string message =
@@ -644,8 +644,7 @@ void NaClProcessHost::ReplyToRenderer(
       NaClLaunchResult(ppapi_channel_handle.release(),
                        trusted_channel_handle.release(),
                        manifest_service_channel_handle.release(),
-                       base::GetProcId(data.handle),
-                       data.id,
+                       base::GetProcId(data.GetHandle()), data.id,
                        crash_info_shmem_renderer_handle),
       error_message);
 
@@ -764,7 +763,7 @@ bool NaClProcessHost::StartNaClExecution() {
   if (uses_nonsfi_mode_) {
     // Currently, non-SFI mode is supported only on Linux.
     if (enable_nacl_debug) {
-      base::ProcessId pid = base::GetProcId(process_->GetData().handle);
+      base::ProcessId pid = base::GetProcId(process_->GetData().GetHandle());
       LOG(WARNING) << "nonsfi nacl plugin running in " << pid;
     }
   } else {
@@ -911,11 +910,8 @@ bool NaClProcessHost::StartPPAPIProxy(
   // browser process.
   ppapi_host_.reset(content::BrowserPpapiHost::CreateExternalPluginProcess(
       ipc_proxy_channel_.get(),  // sender
-      permissions_,
-      process_->GetData().handle,
-      ipc_proxy_channel_.get(),
-      nacl_host_message_filter_->render_process_id(),
-      render_view_id_,
+      permissions_, process_->GetData().GetHandle(), ipc_proxy_channel_.get(),
+      nacl_host_message_filter_->render_process_id(), render_view_id_,
       profile_directory_));
 
   ppapi::PpapiNaClPluginArgs args;
@@ -1110,7 +1106,7 @@ bool NaClProcessHost::AttachDebugExceptionHandler(const std::string& info,
   }
   debug_exception_handler_requested_ = true;
 
-  base::ProcessId nacl_pid = base::GetProcId(process_->GetData().handle);
+  base::ProcessId nacl_pid = base::GetProcId(process_->GetData().GetHandle());
   // We cannot use process_->GetData().handle because it does not have
   // the necessary access rights.  We open the new handle here rather
   // than in the NaCl broker process in case the NaCl loader process

content/browser/browser_child_process_host_impl.cc

@@ -186,7 +186,7 @@ BrowserChildProcessHostImpl::~BrowserChildProcessHostImpl() {
   if (notify_child_disconnected_) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::BindOnce(&NotifyProcessHostDisconnected, data_));
+        base::BindOnce(&NotifyProcessHostDisconnected, data_.Duplicate()));
   }
 }
 
@@ -316,7 +316,7 @@ void BrowserChildProcessHostImpl::SetMetricsName(
 
 void BrowserChildProcessHostImpl::SetHandle(base::ProcessHandle handle) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  data_.handle = handle;
+  data_.SetHandle(handle);
 }
 
 service_manager::mojom::ServiceRequest
@@ -358,7 +358,8 @@ ChildProcessTerminationInfo BrowserChildProcessHostImpl::GetTerminationInfo(
   if (!child_process_) {
     // If the delegate doesn't use Launch() helper.
     ChildProcessTerminationInfo info;
-    info.status = base::GetTerminationStatus(data_.handle, &info.exit_code);
+    info.status =
+        base::GetTerminationStatus(data_.GetHandle(), &info.exit_code);
     return info;
   }
   return child_process_->GetChildTerminationInfo(known_dead);
@@ -381,8 +382,9 @@ void BrowserChildProcessHostImpl::OnChannelConnected(int32_t peer_pid) {
   early_exit_watcher_.StopWatching();
 #endif
 
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::BindOnce(&NotifyProcessHostConnected, data_));
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&NotifyProcessHostConnected, data_.Duplicate()));
 
   delegate_->OnChannelConnected(peer_pid);
 
@@ -390,7 +392,7 @@ void BrowserChildProcessHostImpl::OnChannelConnected(int32_t peer_pid) {
     ShareMetricsAllocatorToProcess();
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::BindOnce(&NotifyProcessLaunchedAndConnected, data_));
+        base::BindOnce(&NotifyProcessLaunchedAndConnected, data_.Duplicate()));
   }
 }
 
@@ -435,13 +437,14 @@ void BrowserChildProcessHostImpl::OnChildDisconnected() {
   // early exit watcher so GetTerminationStatus can close the process handle.
   early_exit_watcher_.StopWatching();
 #endif
-  if (child_process_.get() || data_.handle) {
+  if (child_process_.get() || data_.GetHandle()) {
     ChildProcessTerminationInfo info =
         GetTerminationInfo(true /* known_dead */);
 #if defined(OS_ANDROID)
     delegate_->OnProcessCrashed(info.exit_code);
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                            base::BindOnce(&NotifyProcessKilled, data_, info));
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::BindOnce(&NotifyProcessKilled, data_.Duplicate(), info));
 #else  // OS_ANDROID
     switch (info.status) {
       case base::TERMINATION_STATUS_PROCESS_CRASHED:
@@ -449,7 +452,7 @@ void BrowserChildProcessHostImpl::OnChildDisconnected() {
         delegate_->OnProcessCrashed(info.exit_code);
         BrowserThread::PostTask(
             BrowserThread::UI, FROM_HERE,
-            base::BindOnce(&NotifyProcessCrashed, data_, info));
+            base::BindOnce(&NotifyProcessCrashed, data_.Duplicate(), info));
         UMA_HISTOGRAM_ENUMERATION("ChildProcess.Crashed2",
                                   static_cast<ProcessType>(data_.process_type),
                                   PROCESS_TYPE_MAX);
@@ -462,7 +465,7 @@ void BrowserChildProcessHostImpl::OnChildDisconnected() {
         delegate_->OnProcessCrashed(info.exit_code);
         BrowserThread::PostTask(
             BrowserThread::UI, FROM_HERE,
-            base::BindOnce(&NotifyProcessKilled, data_, info));
+            base::BindOnce(&NotifyProcessKilled, data_.Duplicate(), info));
         // Report that this child process was killed.
         UMA_HISTOGRAM_ENUMERATION("ChildProcess.Killed2",
                                   static_cast<ProcessType>(data_.process_type),
@@ -600,15 +603,14 @@ void BrowserChildProcessHostImpl::OnProcessLaunched() {
   early_exit_watcher_.StartWatchingOnce(process.Handle(), this);
 #endif
 
-  // TODO(rvargas) crbug.com/417532: Don't store a handle.
-  data_.handle = process.Handle();
+  data_.SetHandle(process.Handle());
   delegate_->OnProcessLaunched();
 
   if (is_channel_connected_) {
     ShareMetricsAllocatorToProcess();
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
-        base::BindOnce(&NotifyProcessLaunchedAndConnected, data_));
+        base::BindOnce(&NotifyProcessLaunchedAndConnected, data_.Duplicate()));
   }
 }
 

content/browser/histogram_controller.cc

@@ -153,7 +153,7 @@ void HistogramController::GetHistogramDataFromChildProcesses(
     // example, the GPU process may not exist and there may instead just be a
     // GPU thread in the browser process). If that's the case, then the process
     // handle will be base::kNullProcessHandle and we shouldn't ask it for data.
-    if (data.handle == base::kNullProcessHandle)
+    if (!data.IsHandleValid())
       continue;
 
     if (auto* child_histogram_fetcher =

content/browser/ppapi_plugin_process_host.cc

@@ -509,8 +509,8 @@ void PpapiPluginProcessHost::OnRendererPluginChannelCreated(
   sent_requests_.pop();
 
   const ChildProcessData& data = process_->GetData();
-  client->OnPpapiChannelOpened(channel_handle, base::GetProcId(data.handle),
-                               data.id);
+  client->OnPpapiChannelOpened(channel_handle,
+                               base::GetProcId(data.GetHandle()), data.id);
 }
 
 }  // namespace content

content/browser/renderer_host/media/audio_service_listener.cc

@@ -182,7 +182,7 @@ void AudioServiceListener::OnServiceStopped(
 void AudioServiceListener::BrowserChildProcessHostDisconnected(
     const ChildProcessData& data) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
-  if (base::GetProcId(data.handle) != process_id_)
+  if (base::GetProcId(data.GetHandle()) != process_id_)
     return;
   process_id_ = base::kNullProcessId;
   metrics_.ServiceProcessTerminated(
@@ -193,7 +193,7 @@ void AudioServiceListener::BrowserChildProcessCrashed(
     const ChildProcessData& data,
     const ChildProcessTerminationInfo& info) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
-  if (base::GetProcId(data.handle) != process_id_)
+  if (base::GetProcId(data.GetHandle()) != process_id_)
     return;
   process_id_ = base::kNullProcessId;
   metrics_.ServiceProcessTerminated(
@@ -204,7 +204,7 @@ void AudioServiceListener::BrowserChildProcessKilled(
     const ChildProcessData& data,
     const ChildProcessTerminationInfo& info) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(owning_sequence_);
-  if (base::GetProcId(data.handle) != process_id_)
+  if (base::GetProcId(data.GetHandle()) != process_id_)
     return;
   process_id_ = base::kNullProcessId;
   metrics_.ServiceProcessTerminated(

content/browser/renderer_host/media/audio_service_listener_unittest.cc

@@ -194,7 +194,7 @@ TEST(AudioServiceListenerTest,
       MakeTestServiceInfo(audio_service_identity, pid));
   audio_service_listener.OnServiceStarted(audio_service_identity, pid);
   ChildProcessData data(content::ProcessType::PROCESS_TYPE_UTILITY);
-  data.handle = handle;
+  data.SetHandle(handle);
   audio_service_listener.BrowserChildProcessHostDisconnected(data);
   histogram_tester.ExpectUniqueSample(
       "Media.AudioService.ObservedProcessTerminationStatus",
@@ -215,7 +215,7 @@ TEST(AudioServiceListenerTest,
       MakeTestServiceInfo(audio_service_identity, pid));
   audio_service_listener.OnServiceStarted(audio_service_identity, pid);
   ChildProcessData data(content::ProcessType::PROCESS_TYPE_UTILITY);
-  data.handle = handle;
+  data.SetHandle(handle);
   audio_service_listener.BrowserChildProcessCrashed(
       data, content::ChildProcessTerminationInfo());
   histogram_tester.ExpectUniqueSample(
@@ -236,7 +236,7 @@ TEST(AudioServiceListenerTest,
       MakeTestServiceInfo(audio_service_identity, pid));
   audio_service_listener.OnServiceStarted(audio_service_identity, pid);
   ChildProcessData data(content::ProcessType::PROCESS_TYPE_UTILITY);
-  data.handle = handle;
+  data.SetHandle(handle);
   audio_service_listener.BrowserChildProcessKilled(
       data, content::ChildProcessTerminationInfo());
   histogram_tester.ExpectUniqueSample(

content/browser/service_manager/service_manager_context.cc

@@ -721,7 +721,7 @@ bool ServiceManagerContext::HasValidProcessForProcessGroup(
   auto iter = g_active_process_groups.Get().find(process_group_name);
   if (iter == g_active_process_groups.Get().end() || !iter->second)
     return false;
-  return iter->second->GetData().handle != base::kNullProcessHandle;
+  return iter->second->GetData().IsHandleValid();
 }
 
 // static

content/public/browser/BUILD.gn

@@ -87,6 +87,7 @@ jumbo_source_set("browser_sources") {
     "cache_storage_usage_info.h",
     "cdm_registry.h",
     "certificate_request_result_type.h",
+    "child_process_data.cc",
     "child_process_data.h",
     "child_process_launcher_utils.h",
     "child_process_security_policy.h",

content/public/browser/child_process_data.cc

+// Copyright 2018 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 "content/public/browser/child_process_data.h"
+
+#if defined(OS_WIN)
+#include <Windows.h>
+#endif
+
+namespace content {
+
+#if defined(OS_WIN)
+void ChildProcessData::SetHandle(base::ProcessHandle process) {
+  HANDLE handle_to_set;
+  if (process == base::kNullProcessHandle) {
+    handle_to_set = base::kNullProcessHandle;
+  } else {
+    BOOL result =
+        ::DuplicateHandle(::GetCurrentProcess(), process, ::GetCurrentProcess(),
+                          &handle_to_set, PROCESS_QUERY_INFORMATION, FALSE, 0);
+    auto err = GetLastError();
+    CHECK(result) << process << " " << err;
+  }
+  handle_ = base::win::ScopedHandle(handle_to_set);
+}
+#endif
+
+ChildProcessData::ChildProcessData(int process_type)
+    : process_type(process_type), id(0), handle_(base::kNullProcessHandle) {}
+
+ChildProcessData::ChildProcessData(ChildProcessData&& rhs)
+    : process_type(rhs.process_type),
+      name(rhs.name),
+      metrics_name(rhs.metrics_name),
+      id(rhs.id) {
+#if defined(OS_WIN)
+  handle_.Set(rhs.handle_.Take());
+#else
+  handle_ = rhs.handle_;
+  rhs.handle_ = base::kNullProcessHandle;
+#endif
+}
+
+ChildProcessData::~ChildProcessData() {}
+
+ChildProcessData ChildProcessData::Duplicate() const {
+  ChildProcessData result(process_type);
+  result.name = name;
+  result.metrics_name = metrics_name;
+  result.id = id;
+#if defined(OS_WIN)
+  result.SetHandle(handle_.Get());
+#else
+  result.SetHandle(handle_);
+#endif
+  return result;
+}
+
+}  // namespace content

content/public/browser/child_process_data.h

@@ -7,12 +7,17 @@
 
 #include "base/process/process.h"
 #include "base/strings/string16.h"
+#include "build/build_config.h"
 #include "content/common/content_export.h"
 
+#if defined(OS_WIN)
+#include "base/win/scoped_handle.h"
+#endif
+
 namespace content {
 
 // Holds information about a child process.
-struct ChildProcessData {
+struct CONTENT_EXPORT ChildProcessData {
   // The type of the process. See the content::ProcessType enum for the
   // well-known process types.
   int process_type;
@@ -29,15 +34,37 @@ struct ChildProcessData {
   // one run of the browser.
   int id;
 
-  // The handle to the process. May have value kNullProcessHandle if no process
-  // exists - either because it hasn't been started yet or it's running in the
-  // current process.
-  base::ProcessHandle handle;
+#if defined(OS_WIN)
+  base::ProcessHandle GetHandle() const { return handle_.Get(); }
+  // Will duplicate the handle and assume ownership of the duplicate.
+  void SetHandle(base::ProcessHandle process);
+#else
+  base::ProcessHandle GetHandle() const { return handle_; }
+  void SetHandle(base::ProcessHandle process) { handle_ = process; }
+#endif
+
+  bool IsHandleValid() const { return GetHandle() != base::kNullProcessHandle; }
+
+  explicit ChildProcessData(int process_type);
+  ~ChildProcessData();
+
+  ChildProcessData(ChildProcessData&& rhs);
+
+  // Copying these objects requires duplicating the handle which is moderately
+  // expensive, so make it an explicit action.
+  ChildProcessData Duplicate() const;
 
-  explicit ChildProcessData(int process_type)
-      : process_type(process_type),
-        id(0),
-        handle(base::kNullProcessHandle) {}
+ private:
+// The handle to the process. May have value kNullProcessHandle if no process
+// exists - either because it hasn't been started yet or it's running in the
+// current process.
+#if defined(OS_WIN)
+  // Must be a scoped handle on Windows holding a duplicated handle or else
+  // there are no guarantees the handle will still be valid when used.
+  base::win::ScopedHandle handle_;
+#else
+  base::ProcessHandle handle_;
+#endif
 };
 
 }  // namespace content