Reland "Fix invalid handle errors found with App Verifier"

This is a reland of c31de326. The CHECK
that was triggering (crbug.com/868341) has been removed but the change
is otherwise the same. The CHECK will be readded once all of the bugs
that it reveals are fixed (crbug.com/869154)

Original change's description:
> 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}

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

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