Simplify task_manager code now that ProcessHostOnUI is the only path.
Bug: 904556
Change-Id: Ib8a2407f33ef7ac735148aa09f2f5e630ef28758
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3171458
Auto-Submit: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/main@{#923645}
diff --git a/chrome/browser/task_manager/providers/child_process_task.cc b/chrome/browser/task_manager/providers/child_process_task.cc
index e18b71a..b8404219 100644
--- a/chrome/browser/task_manager/providers/child_process_task.cc
+++ b/chrome/browser/task_manager/providers/child_process_task.cc
@@ -25,7 +25,6 @@
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/child_process_data.h"
#include "content/public/common/child_process_host.h"
-#include "content/public/common/content_features.h"
#include "content/public/common/process_type.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/common/extension_set.h"
@@ -121,38 +120,18 @@
return result_title;
}
-// Connects the |resource_reporter| to the InterfaceRegistry of the
-// BrowserChildProcessHost whose unique ID is |unique_child_process_id|.
-void ConnectResourceReporterOnIOThread(
- int unique_child_process_id,
- mojo::PendingReceiver<content::mojom::ResourceUsageReporter>
- resource_reporter) {
- DCHECK_CURRENTLY_ON(base::FeatureList::IsEnabled(features::kProcessHostOnUI)
- ? content::BrowserThread::UI
- : content::BrowserThread::IO);
-
- content::BrowserChildProcessHost* host =
- content::BrowserChildProcessHost::FromID(unique_child_process_id);
- if (!host)
- return;
-
- host->GetHost()->BindReceiver(std::move(resource_reporter));
-}
-
// Creates the Mojo service wrapper that will be used to sample the V8 memory
// usage of the browser child process whose unique ID is
// |unique_child_process_id|.
ProcessResourceUsage* CreateProcessResourcesSampler(
int unique_child_process_id) {
mojo::PendingRemote<content::mojom::ResourceUsageReporter> usage_reporter;
- auto task_runner = base::FeatureList::IsEnabled(features::kProcessHostOnUI)
- ? content::GetUIThreadTaskRunner({})
- : content::GetIOThreadTaskRunner({});
- task_runner->PostTask(
- FROM_HERE,
- base::BindOnce(&ConnectResourceReporterOnIOThread,
- unique_child_process_id,
- usage_reporter.InitWithNewPipeAndPassReceiver()));
+ content::BrowserChildProcessHost* host =
+ content::BrowserChildProcessHost::FromID(unique_child_process_id);
+ auto receiver = usage_reporter.InitWithNewPipeAndPassReceiver();
+ if (host)
+ host->GetHost()->BindReceiver(std::move(receiver));
+
return new ProcessResourceUsage(std::move(usage_reporter));
}
diff --git a/chrome/browser/task_manager/providers/child_process_task_provider.cc b/chrome/browser/task_manager/providers/child_process_task_provider.cc
index f966fdc3..ce86c82 100644
--- a/chrome/browser/task_manager/providers/child_process_task_provider.cc
+++ b/chrome/browser/task_manager/providers/child_process_task_provider.cc
@@ -13,7 +13,6 @@
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
#include "content/public/browser/child_process_data.h"
-#include "content/public/common/content_features.h"
using content::BrowserChildProcessHostIterator;
using content::BrowserThread;
@@ -21,34 +20,6 @@
namespace task_manager {
-namespace {
-
-// Collects and returns the child processes data on the IO thread to get all the
-// pre-existing child process before we start observing
-// |BrowserChildProcessObserver|.
-std::unique_ptr<std::vector<ChildProcessData>> CollectChildProcessData() {
- // The |BrowserChildProcessHostIterator| must only be used on the IO thread.
- DCHECK_CURRENTLY_ON(base::FeatureList::IsEnabled(features::kProcessHostOnUI)
- ? content::BrowserThread::UI
- : content::BrowserThread::IO);
-
- std::unique_ptr<std::vector<ChildProcessData>> child_processes(
- new std::vector<ChildProcessData>());
- for (BrowserChildProcessHostIterator itr; !itr.Done(); ++itr) {
- const ChildProcessData& process_data = itr.GetData();
-
- // Only add processes that have already started, i.e. with valid handles.
- if (!process_data.GetProcess().IsValid())
- continue;
-
- child_processes->push_back(process_data.Duplicate());
- }
-
- return child_processes;
-}
-
-} // namespace
-
ChildProcessTaskProvider::ChildProcessTaskProvider() {}
ChildProcessTaskProvider::~ChildProcessTaskProvider() {
@@ -85,22 +56,28 @@
DCHECK(tasks_by_child_id_.empty());
// First, get the pre-existing child processes data.
- auto task_runner = base::FeatureList::IsEnabled(features::kProcessHostOnUI)
- ? content::GetUIThreadTaskRunner({})
- : content::GetIOThreadTaskRunner({});
- task_runner->PostTaskAndReplyWithResult(
- FROM_HERE, base::BindOnce(&CollectChildProcessData),
- base::BindOnce(&ChildProcessTaskProvider::ChildProcessDataCollected,
- weak_ptr_factory_.GetWeakPtr()));
+ std::unique_ptr<std::vector<ChildProcessData>> child_processes(
+ new std::vector<ChildProcessData>());
+ for (BrowserChildProcessHostIterator itr; !itr.Done(); ++itr) {
+ const ChildProcessData& process_data = itr.GetData();
+
+ // Only add processes that have already started, i.e. with valid handles.
+ if (!process_data.GetProcess().IsValid())
+ continue;
+
+ child_processes->push_back(process_data.Duplicate());
+ }
+
+ for (const auto& process_data : *child_processes)
+ CreateTask(process_data);
+
+ // Now start observing.
+ BrowserChildProcessObserver::Add(this);
}
void ChildProcessTaskProvider::StopUpdating() {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
- // ChildProcessDataCollected() should never be called after this, and hence
- // we must invalidate the weak pointers.
- weak_ptr_factory_.InvalidateWeakPtrs();
-
// First, stop observing.
BrowserChildProcessObserver::Remove(this);
@@ -112,18 +89,6 @@
tasks_by_child_id_.clear();
}
-void ChildProcessTaskProvider::ChildProcessDataCollected(
- std::unique_ptr<const std::vector<content::ChildProcessData>>
- child_processes) {
- DCHECK_CURRENTLY_ON(BrowserThread::UI);
-
- for (const auto& process_data : *child_processes)
- CreateTask(process_data);
-
- // Now start observing.
- BrowserChildProcessObserver::Add(this);
-}
-
void ChildProcessTaskProvider::CreateTask(
const content::ChildProcessData& data) {
std::unique_ptr<ChildProcessTask>& task =
diff --git a/chrome/browser/task_manager/providers/child_process_task_provider.h b/chrome/browser/task_manager/providers/child_process_task_provider.h
index ab83196..fe5b05d 100644
--- a/chrome/browser/task_manager/providers/child_process_task_provider.h
+++ b/chrome/browser/task_manager/providers/child_process_task_provider.h
@@ -10,7 +10,6 @@
#include <vector>
#include "base/containers/flat_map.h"
-#include "base/memory/weak_ptr.h"
#include "chrome/browser/task_manager/providers/task_provider.h"
#include "content/public/browser/browser_child_process_observer.h"
@@ -49,13 +48,6 @@
void StartUpdating() override;
void StopUpdating() override;
- // The pre-existing child processes data will be collected on the IO thread.
- // When that is done, we will be notified on the UI thread by receiving a call
- // to this method.
- void ChildProcessDataCollected(
- std::unique_ptr<const std::vector<content::ChildProcessData>>
- child_processes);
-
// Creates a ChildProcessTask from the given |data| and notifies the observer
// of its addition.
void CreateTask(const content::ChildProcessData& data);
@@ -74,10 +66,6 @@
// A map to track ChildProcessTask's by their child process unique ids.
base::flat_map<int, ChildProcessTask*> tasks_by_child_id_;
-
- // Always keep this the last member of this class to make sure it's the
- // first thing to be destructed.
- base::WeakPtrFactory<ChildProcessTaskProvider> weak_ptr_factory_{this};
};
} // namespace task_manager
diff --git a/chrome/browser/task_manager/sampling/task_group.cc b/chrome/browser/task_manager/sampling/task_group.cc
index 50f9f5d..b56ca8f 100644
--- a/chrome/browser/task_manager/sampling/task_group.cc
+++ b/chrome/browser/task_manager/sampling/task_group.cc
@@ -17,7 +17,6 @@
#include "components/nacl/browser/nacl_browser.h"
#include "content/public/browser/browser_task_traits.h"
#include "content/public/browser/browser_thread.h"
-#include "content/public/common/content_features.h"
#include "gpu/ipc/common/memory_stats.h"
#if defined(OS_WIN)
@@ -305,20 +304,13 @@
#if BUILDFLAG(ENABLE_NACL)
void TaskGroup::RefreshNaClDebugStubPort(int child_process_unique_id) {
- if (base::FeatureList::IsEnabled(features::kProcessHostOnUI)) {
- base::SequencedTaskRunnerHandle::Get()->PostTask(
- FROM_HERE, base::BindOnce(&TaskGroup::OnRefreshNaClDebugStubPortDone,
- weak_ptr_factory_.GetWeakPtr(),
- GetNaClDebugStubPortOnProcessThread(
- child_process_unique_id)));
- } else {
- content::GetIOThreadTaskRunner({})->PostTaskAndReplyWithResult(
- FROM_HERE,
- base::BindOnce(&GetNaClDebugStubPortOnProcessThread,
- child_process_unique_id),
- base::BindOnce(&TaskGroup::OnRefreshNaClDebugStubPortDone,
- weak_ptr_factory_.GetWeakPtr()));
- }
+ // Note this needs to be in a PostTask to avoid a use-after-free (see
+ // https://crbug.com/1221406).
+ base::SequencedTaskRunnerHandle::Get()->PostTask(
+ FROM_HERE, base::BindOnce(&TaskGroup::OnRefreshNaClDebugStubPortDone,
+ weak_ptr_factory_.GetWeakPtr(),
+ GetNaClDebugStubPortOnProcessThread(
+ child_process_unique_id)));
}
void TaskGroup::OnRefreshNaClDebugStubPortDone(int nacl_debug_stub_port) {
