Fix a process handle usage race in resource coordinator render probe.

The GetHandle function of RenderProcessHost returned a base::ProcessHandle, which cannot be passed around to other threads because the caller does not have ownership. This lead to handle abuse problems on Windows if the RPH goes away as the handle is being bounced around. This CL replaces the GetHandle function of RPH with a GetProcess function, which returns a const base::Process&. This in turn allows callers to duplicate the underlying base::Process, and the duplicate can then be safely bounced around to other threads. Bug: 821453 Change-Id: I7d9956c3ab92352d99ccea70fde3f2013aa66d33 Reviewed-on: https://chromium-review.googlesource.com/998213 Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org> Reviewed-by: Chris Hamilton <chrisha@chromium.org> Reviewed-by: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/heads/master@{#550988}
cbchan · Apr 16, 2018 · eb95666 · eb95666
1 parent 99a348c
commit eb95666
Show file tree

Hide file tree

Showing 42 changed files with 158 additions and 127 deletions.
diff --git a/chrome/browser/android/tab_android.cc b/chrome/browser/android/tab_android.cc
@@ -932,7 +932,7 @@ jint TabAndroid::GetCurrentRenderProcessId(JNIEnv* env,
   content::RenderProcessHost* render_process = host->GetProcess();
   DCHECK(render_process);
   if (render_process->HasConnection())
-    return render_process->GetHandle();
+    return render_process->GetProcess().Handle();
   return 0;
 }
 

diff --git a/chrome/browser/apps/guest_view/web_view_browsertest.cc b/chrome/browser/apps/guest_view/web_view_browsertest.cc
@@ -3423,7 +3423,10 @@ IN_PROC_BROWSER_TEST_P(
 
   // Kill the embedder's render process, so the webview will go as well.
   base::Process process = base::Process::DeprecatedGetProcessFromHandle(
-      embedder_web_contents->GetMainFrame()->GetProcess()->GetHandle());
+      embedder_web_contents->GetMainFrame()
+          ->GetProcess()
+          ->GetProcess()
+          .Handle());
   process.Terminate(0, false);
   observer->WaitForEmbedderRenderProcessTerminate();
 

diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
@@ -3245,14 +3245,18 @@ void ChromeContentBrowserClient::ExposeInterfacesToRenderer(
     // callback will be invoked in the context of ModuleDatabase::GetInstance,
     // which is invoked by Mojo initialization, which occurs while the
     // |render_process_host| is alive.
-    auto get_process = base::Bind(&content::RenderProcessHost::GetHandle,
-                                  base::Unretained(render_process_host));
+    auto get_process = base::BindRepeating(
+        [](content::RenderProcessHost* host) -> base::ProcessHandle {
+          return host->GetProcess().Handle();
+        },
+        base::Unretained(render_process_host));
     // The ModuleDatabase is a global singleton so passing an unretained pointer
     // is safe.
     registry->AddInterface(
-        base::Bind(&ModuleEventSinkImpl::Create, std::move(get_process),
-                   content::PROCESS_TYPE_RENDERER,
-                   base::Unretained(ModuleDatabase::GetInstance())),
+        base::BindRepeating(&ModuleEventSinkImpl::Create,
+                            std::move(get_process),
+                            content::PROCESS_TYPE_RENDERER,
+                            base::Unretained(ModuleDatabase::GetInstance())),
         ui_task_runner);
   }
 #endif

diff --git a/chrome/browser/chromeos/power/renderer_freezer.cc b/chrome/browser/chromeos/power/renderer_freezer.cc
@@ -155,7 +155,8 @@ void RendererFreezer::OnScreenLockStateChanged(chromeos::ScreenLocker* locker,
     content::WebContents* web_contents = locker->delegate()->GetWebContents();
     if (web_contents) {
       delegate_->SetShouldFreezeRenderer(
-          web_contents->GetMainFrame()->GetProcess()->GetHandle(), false);
+          web_contents->GetMainFrame()->GetProcess()->GetProcess().Handle(),
+          false);
     }
   }
 }
@@ -195,7 +196,7 @@ void RendererFreezer::OnRenderProcessCreated(content::RenderProcessHost* rph) {
 
     // This renderer has an extension that is using GCM.  Make sure it is not
     // frozen during suspend.
-    delegate_->SetShouldFreezeRenderer(rph->GetHandle(), false);
+    delegate_->SetShouldFreezeRenderer(rph->GetProcess().Handle(), false);
     gcm_extension_processes_.insert(rph_id);
 
     // Watch to see if the renderer process or the RenderProcessHost is
@@ -206,7 +207,7 @@ void RendererFreezer::OnRenderProcessCreated(content::RenderProcessHost* rph) {
 
   // We didn't find an extension in this RenderProcessHost that is using GCM so
   // we can go ahead and freeze it on suspend.
-  delegate_->SetShouldFreezeRenderer(rph->GetHandle(), true);
+  delegate_->SetShouldFreezeRenderer(rph->GetProcess().Handle(), true);
 }
 
 }  // namespace chromeos
diff --git a/chrome/browser/extensions/api/processes/processes_api.cc b/chrome/browser/extensions/api/processes/processes_api.cc
@@ -500,7 +500,8 @@ ExtensionFunction::ResponseAction ProcessesTerminateFunction::Run() {
   auto* render_process_host =
       content::RenderProcessHost::FromID(child_process_host_id_);
   if (render_process_host)
-    return RespondNow(TerminateIfAllowed(render_process_host->GetHandle()));
+    return RespondNow(
+        TerminateIfAllowed(render_process_host->GetProcess().Handle()));
 
   // This could be a non-renderer child process like a plugin or a nacl
   // process. Try to get its handle from the BrowserChildProcessHost on the

diff --git a/chrome/browser/media/webrtc/webrtc_video_display_perf_browsertest.cc b/chrome/browser/media/webrtc/webrtc_video_display_perf_browsertest.cc
@@ -149,8 +149,8 @@ class WebRtcVideoDisplayPerfBrowserTest
         OpenPageAndGetUserMediaInNewTabWithConstraints(
             embedded_test_server()->GetURL(kMainWebrtcTestHtmlPage),
             "{audio: true, video: false}");
-    const int process_id = base::GetProcId(
-        right_tab->GetRenderViewHost()->GetProcess()->GetHandle());
+    const int process_id =
+        right_tab->GetRenderViewHost()->GetProcess()->GetProcess().Pid();
 
     const std::string disable_cpu_adaptation_constraint(
         "{'optional': [{'googCpuOveruseDetection': false}]}");

diff --git a/chrome/browser/memory_details.cc b/chrome/browser/memory_details.cc
@@ -230,7 +230,7 @@ void MemoryDetails::CollectChildInfoOnUIThread() {
     // or processes that are still launching.
     if (!widget->GetProcess()->IsReady())
       continue;
-    base::ProcessId pid = base::GetProcId(widget->GetProcess()->GetHandle());
+    base::ProcessId pid = widget->GetProcess()->GetProcess().Pid();
     widgets_by_pid[pid].push_back(widget);
   }
 

diff --git a/chrome/browser/metrics/process_memory_metrics_emitter.cc b/chrome/browser/metrics/process_memory_metrics_emitter.cc
@@ -360,7 +360,7 @@ int ProcessMemoryMetricsEmitter::GetNumberOfExtensions(base::ProcessId pid) {
   int rph_id = -1;
   auto iter = content::RenderProcessHost::AllHostsIterator();
   while (!iter.IsAtEnd()) {
-    if (base::GetProcId(iter.GetCurrentValue()->GetHandle()) == pid) {
+    if (iter.GetCurrentValue()->GetProcess().Pid() == pid) {
       rph_id = iter.GetCurrentValue()->GetID();
       break;
     }

diff --git a/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc b/chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
@@ -142,10 +142,11 @@ DataReductionProxyMetricsObserver::OnCommit(
   if (!data || !data->used_data_reduction_proxy())
     return STOP_OBSERVING;
   data_ = data->DeepCopy();
-  process_id_ = base::GetProcId(navigation_handle->GetWebContents()
-                                    ->GetMainFrame()
-                                    ->GetProcess()
-                                    ->GetHandle());
+  process_id_ = navigation_handle->GetWebContents()
+                    ->GetMainFrame()
+                    ->GetProcess()
+                    ->GetProcess()
+                    .Pid();
   render_process_host_id_ = navigation_handle->GetWebContents()
                                 ->GetMainFrame()
                                 ->GetProcess()

diff --git a/chrome/browser/performance_monitor/performance_monitor.cc b/chrome/browser/performance_monitor/performance_monitor.cc
@@ -102,7 +102,7 @@ void PerformanceMonitor::GatherMetricsMapOnUIThread() {
     content::RenderProcessHost* host = rph_iter.GetCurrentValue();
     ProcessMetricsMetadata data;
     data.process_type = content::PROCESS_TYPE_RENDERER;
-    data.handle = host->GetHandle();
+    data.handle = host->GetProcess().Handle();
 
     GatherMetricsForRenderProcess(host, &data);
     MarkProcessAsAlive(data, current_update_sequence);
@@ -146,6 +146,8 @@ void PerformanceMonitor::GatherMetricsMapOnIOThread(
 
   // Find all child processes (does not include renderers), which has to be
   // done on the IO thread.
+  // This creates a race on usage of the child process handles on Windows.
+  // See https://crbug.com/821453.
   for (content::BrowserChildProcessHostIterator iter; !iter.Done(); ++iter) {
     ProcessMetricsMetadata child_process_data;
     child_process_data.handle = iter.GetData().handle;

diff --git a/chrome/browser/prerender/prerender_contents.cc b/chrome/browser/prerender/prerender_contents.cc
@@ -643,11 +643,11 @@ void PrerenderContents::DestroyWhenUsingTooManyResources() {
     if (!rph)
       return;
 
-    base::ProcessHandle handle = rph->GetHandle();
+    base::ProcessHandle handle = rph->GetProcess().Handle();
     if (handle == base::kNullProcessHandle)
       return;
 
-    process_pid_ = base::GetProcId(handle);
+    process_pid_ = rph->GetProcess().Pid();
   }
 
   if (process_pid_ == base::kNullProcessId)

diff --git a/chrome/browser/profiling_host/profiling_test_driver.cc b/chrome/browser/profiling_host/profiling_test_driver.cc
@@ -67,7 +67,7 @@ bool RenderersAreBeingProfiled(
   size_t renderer_count = 0;
   for (auto iter = content::RenderProcessHost::AllHostsIterator();
        !iter.IsAtEnd(); iter.Advance()) {
-    base::ProcessId pid = base::GetProcId(iter.GetCurrentValue()->GetHandle());
+    base::ProcessId pid = iter.GetCurrentValue()->GetProcess().Pid();
     if (std::find(profiled_pids.begin(), profiled_pids.end(), pid) ==
         profiled_pids.end()) {
       return false;

diff --git a/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc b/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
@@ -100,7 +100,8 @@ class ChromeRenderProcessHostTest : public ExtensionBrowserTest {
 
     WaitForLauncherThread();
     WaitForMessageProcessing(wc);
-    return ProcessFromHandle(wc->GetMainFrame()->GetProcess()->GetHandle());
+    return ProcessFromHandle(
+        wc->GetMainFrame()->GetProcess()->GetProcess().Handle());
   }
 
   // Loads the given url in a new background tab and returns the handle of its
@@ -117,7 +118,8 @@ class ChromeRenderProcessHostTest : public ExtensionBrowserTest {
 
     WaitForLauncherThread();
     WaitForMessageProcessing(wc);
-    return ProcessFromHandle(wc->GetMainFrame()->GetProcess()->GetHandle());
+    return ProcessFromHandle(
+        wc->GetMainFrame()->GetProcess()->GetProcess().Handle());
   }
 
   // Ensures that the backgrounding / foregrounding gets a chance to run.
@@ -626,8 +628,10 @@ class ChromeRenderProcessHostBackgroundingTest
 
     // Create a new tab for the no audio page and confirm that the process of
     // each tab is different and that both are valid.
-    audio_process_ = ProcessFromHandle(
-        audio_tab_web_contents_->GetMainFrame()->GetProcess()->GetHandle());
+    audio_process_ = ProcessFromHandle(audio_tab_web_contents_->GetMainFrame()
+                                           ->GetProcess()
+                                           ->GetProcess()
+                                           .Handle());
     no_audio_process_ = ShowSingletonTab(no_audio_url_);
     ASSERT_NE(audio_process_.Pid(), no_audio_process_.Pid());
     ASSERT_TRUE(no_audio_process_.IsValid());

diff --git a/chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.cc b/chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.cc
@@ -115,22 +115,26 @@ void ResourceCoordinatorRenderProcessProbe::
            content::RenderProcessHost::AllHostsIterator();
        !rph_iter.IsAtEnd(); rph_iter.Advance()) {
     content::RenderProcessHost* host = rph_iter.GetCurrentValue();
-    base::ProcessHandle handle = host->GetHandle();
     // Process may not be valid yet.
-    if (handle == base::kNullProcessHandle) {
+    if (!host->GetProcess().IsValid()) {
       continue;
     }
 
-    auto& render_process_info = render_process_info_map_[handle];
+    // Duplicate the process to retain ownership of it through the thread
+    // bouncing.
+    base::Process process(host->GetProcess().Duplicate());
+    auto& render_process_info = render_process_info_map_[process.Handle()];
     render_process_info.last_gather_cycle_active = current_gather_cycle_;
+    render_process_info.process = std::move(process);
 
     if (render_process_info.metrics.get() == nullptr) {
 #if defined(OS_MACOSX)
       render_process_info.metrics = base::ProcessMetrics::CreateProcessMetrics(
-          handle, content::BrowserChildProcessHost::GetPortProvider());
+          render_process_info.process.Handle(),
+          content::BrowserChildProcessHost::GetPortProvider());
 #else
-      render_process_info.metrics =
-          base::ProcessMetrics::CreateProcessMetrics(handle);
+      render_process_info.metrics = base::ProcessMetrics::CreateProcessMetrics(
+          render_process_info.process.Handle());
 #endif
       render_process_info.render_process_host_id = host->GetID();
     }

diff --git a/chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.h b/chrome/browser/resource_coordinator/resource_coordinator_render_process_probe.h
@@ -11,7 +11,7 @@
 
 #include "base/lazy_instance.h"
 #include "base/macros.h"
-#include "base/process/process_handle.h"
+#include "base/process/process.h"
 #include "base/process/process_metrics.h"
 #include "base/timer/timer.h"
 
@@ -20,6 +20,7 @@ namespace resource_coordinator {
 struct RenderProcessInfo {
   RenderProcessInfo();
   ~RenderProcessInfo();
+  base::Process process;
   double cpu_usage = -1.0;
   // This structure bounces from the UI thread to blocking threads and back.
   // It's therefore not safe to store RenderProcessHost pointers, so the ID is

diff --git a/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc b/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc
@@ -103,7 +103,7 @@ base::ProcessHandle TabLifecycleUnitSource::TabLifecycleUnit::GetProcessHandle()
   content::RenderProcessHost* process = main_frame->GetProcess();
   if (!process)
     return base::ProcessHandle();
-  return process->GetHandle();
+  return process->GetProcess().Handle();
 }
 
 LifecycleUnit::SortKey TabLifecycleUnitSource::TabLifecycleUnit::GetSortKey()

diff --git a/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc b/chrome/browser/resource_coordinator/tab_manager_delegate_chromeos.cc
@@ -297,7 +297,8 @@ void TabManagerDelegate::OnBrowserSetLastActive(Browser* browser) {
   if (!contents)
     return;
 
-  base::ProcessHandle pid = contents->GetMainFrame()->GetProcess()->GetHandle();
+  base::ProcessHandle pid =
+      contents->GetMainFrame()->GetProcess()->GetProcess().Handle();
   AdjustFocusedTabScore(pid);
 }
 
@@ -419,7 +420,7 @@ void TabManagerDelegate::Observe(int type,
     case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED: {
       content::RenderProcessHost* host =
           content::Source<content::RenderProcessHost>(source).ptr();
-      oom_score_map_.erase(host->GetHandle());
+      oom_score_map_.erase(host->GetProcess().Handle());
       // Coming here we know that a renderer was just killed and memory should
       // come back into the pool. However - the memory pressure observer did
       // not yet update its status and therefore we ask it to redo the
@@ -442,7 +443,7 @@ void TabManagerDelegate::Observe(int type,
             content::Source<content::RenderWidgetHost>(source)
                 .ptr()
                 ->GetProcess();
-        AdjustFocusedTabScore(render_host->GetHandle());
+        AdjustFocusedTabScore(render_host->GetProcess().Handle());
       }
       // Do not handle the "else" case when it changes to invisible because
       // 1. The behavior is a bit awkward in that when switching from tab A to

diff --git a/chrome/browser/spellchecker/spellcheck_service.cc b/chrome/browser/spellchecker/spellcheck_service.cc
@@ -337,7 +337,7 @@ void SpellcheckService::InitForAllRenderers() {
           content::RenderProcessHost::AllHostsIterator());
        !i.IsAtEnd(); i.Advance()) {
     content::RenderProcessHost* process = i.GetCurrentValue();
-    if (process && process->GetHandle())
+    if (process && process->GetProcess().Handle())
       InitForRenderer(process->GetChildIdentity());
   }
 }

diff --git a/chrome/browser/task_manager/providers/render_process_host_task_provider.cc b/chrome/browser/task_manager/providers/render_process_host_task_provider.cc
@@ -53,7 +53,7 @@ void RenderProcessHostTaskProvider::StartUpdating() {
   for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
        !it.IsAtEnd(); it.Advance()) {
     RenderProcessHost* host = it.GetCurrentValue();
-    if (host->GetHandle()) {
+    if (host->GetProcess().IsValid()) {
       CreateTask(host->GetID());
     } else {
       // If the host isn't ready do nothing and we will learn of its creation
@@ -82,7 +82,9 @@ 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);
-  data.handle = host->GetHandle();
+  // TODO(siggi): Investigate whether this is also a handle race, per
+  //     https://crbug.com/821453.
+  data.handle = host->GetProcess().Handle();
   data.id = host->GetID();
   task.reset(new ChildProcessTask(data));
   NotifyObserverTaskAdded(task.get());

diff --git a/chrome/browser/task_manager/providers/web_contents/renderer_task.cc b/chrome/browser/task_manager/providers/web_contents/renderer_task.cc
@@ -80,7 +80,7 @@ RendererTask::RendererTask(const base::string16& title,
     : Task(title,
            GetRapporSampleName(web_contents),
            icon,
-           render_process_host->GetHandle()),
+           render_process_host->GetProcess().Handle()),
       web_contents_(web_contents),
       render_process_host_(render_process_host),
       renderer_resources_sampler_(

diff --git a/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc b/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
@@ -195,7 +195,7 @@ void WebContentsEntry::RenderFrameReady(int render_process_id,
 
   const base::ProcessId determine_pid_from_handle = base::kNullProcessId;
   provider_->UpdateTaskProcessInfoAndNotifyObserver(
-      task, render_frame_host->GetProcess()->GetHandle(),
+      task, render_frame_host->GetProcess()->GetProcess().Handle(),
       determine_pid_from_handle);
 }
 

diff --git a/chrome/browser/ui/views/hung_renderer_view.cc b/chrome/browser/ui/views/hung_renderer_view.cc
@@ -377,7 +377,7 @@ bool HungRendererDialogView::Cancel() {
   if (rph) {
 #if defined(OS_WIN)
     // Try to generate a crash report for the hung process.
-    CrashDumpAndTerminateHungChildProcess(rph->GetHandle());
+    CrashDumpAndTerminateHungChildProcess(rph->GetProcess().Handle());
 #else
     rph->Shutdown(content::RESULT_CODE_HUNG);
 #endif

diff --git a/chrome/browser/ui/webui/memory_internals_ui.cc b/chrome/browser/ui/webui/memory_internals_ui.cc
@@ -298,8 +298,7 @@ void MemoryInternalsDOMHandler::ReturnProcessListOnUIThread(
   // Append renderer processes.
   auto iter = content::RenderProcessHost::AllHostsIterator();
   while (!iter.IsAtEnd()) {
-    base::ProcessHandle renderer_handle = iter.GetCurrentValue()->GetHandle();
-    base::ProcessId renderer_pid = base::GetProcId(renderer_handle);
+    base::ProcessId renderer_pid = iter.GetCurrentValue()->GetProcess().Pid();
     if (renderer_pid != 0) {
       // TODO(brettw) make a better description of the process, maybe see
       // what TaskManager does to get the page title.

diff --git a/components/crash/content/browser/crash_dump_observer_android.cc b/components/crash/content/browser/crash_dump_observer_android.cc
@@ -147,14 +147,14 @@ void CrashDumpObserver::Observe(int type,
     case content::NOTIFICATION_RENDERER_PROCESS_CREATED: {
       // The child process pid isn't available when process is gone, keep a
       // mapping between process_host_id and pid, so we can find it later.
-      process_host_id_to_pid_[rph->GetID()] = rph->GetHandle();
+      process_host_id_to_pid_[rph->GetID()] = rph->GetProcess().Handle();
       return;
     }
     default:
       NOTREACHED();
       return;
   }
-  base::ProcessHandle pid = rph->GetHandle();
+  base::ProcessHandle pid = rph->GetProcess().Handle();
   const auto& iter = process_host_id_to_pid_.find(rph->GetID());
   if (iter != process_host_id_to_pid_.end()) {
     if (pid == base::kNullProcessHandle) {