[PM] Make PerformanceManager(Impl)::CallOnGraph thread-safe.

With this CL, PerformanceManager(Impl)::CallOnGraph can safely be
called from any sequence, at any time. The callback does not run if
the call is made before creation of after destruction of the
Performance Manager.

Implementation details: CallOnGraph always posts a task to the
Performance Manager sequence. When the task runs on the
Performance Manager sequence, it checks if a global pointer to
the Performance Manager if set. This is not racy because the
pointer is only modified from the Performance Manager sequence.
If the pointer is set, the task gets the Graph from it and invokes
the callback that was passed to CallOnGraph. Otherwise, the
callback does not run.

Bug: 980533
Change-Id: I3d640e0ae13d012d76bab1483a5d59afb977c1a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849213
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704602}

chrome/browser/performance_manager/browser_child_process_watcher.cc

@@ -95,10 +95,9 @@ void BrowserChildProcessWatcher::GPUProcessExited(int id, int exit_code) {
   // specifically on crash.
   if (base::Contains(gpu_process_nodes_, id)) {
     auto* process_node = gpu_process_nodes_[id].get();
-    PerformanceManagerImpl* performance_manager =
-        PerformanceManagerImpl::GetInstance();
 
-    performance_manager->task_runner()->PostTask(
+    DCHECK(PerformanceManagerImpl::GetInstance());
+    PerformanceManagerImpl::GetTaskRunner()->PostTask(
         FROM_HERE, base::BindOnce(&ProcessNodeImpl::SetProcessExitStatus,
                                   base::Unretained(process_node), exit_code));
   }
@@ -118,9 +117,8 @@ void BrowserChildProcessWatcher::OnProcessLaunched(
       process.CreationTime();
 #endif
 
-  PerformanceManagerImpl* performance_manager =
-      PerformanceManagerImpl::GetInstance();
-  performance_manager->task_runner()->PostTask(
+  DCHECK(PerformanceManagerImpl::GetInstance());
+  PerformanceManagerImpl::GetTaskRunner()->PostTask(
       FROM_HERE, base::BindOnce(&ProcessNodeImpl::SetProcess,
                                 base::Unretained(process_node),
                                 process.Duplicate(), launch_time));

chrome/browser/performance_manager/chrome_content_browser_client_performance_manager_part.cc

@@ -31,11 +31,8 @@ void BindProcessNode(
       performance_manager::RenderProcessUserData::GetForRenderProcessHost(
           render_process_host);
 
-  performance_manager::PerformanceManagerImpl* performance_manager =
-      performance_manager::PerformanceManagerImpl::GetInstance();
-  DCHECK(performance_manager);
-
-  performance_manager->task_runner()->PostTask(
+  DCHECK(performance_manager::PerformanceManagerImpl::GetInstance());
+  performance_manager::PerformanceManagerImpl::GetTaskRunner()->PostTask(
       FROM_HERE, base::BindOnce(&performance_manager::ProcessNodeImpl::Bind,
                                 base::Unretained(user_data->process_node()),
                                 std::move(receiver)));

components/performance_manager/performance_manager.cc

@@ -21,14 +21,10 @@ void PerformanceManager::CallOnGraph(const base::Location& from_here,
                                      GraphCallback callback) {
   DCHECK(callback);
 
-  // Passing |pm| unretained is safe as it is actually destroyed on the
-  // destination sequence, and g_performance_manager.Get() would return nullptr
-  // if its deletion task was already posted.
-  auto* pm = PerformanceManagerImpl::GetInstance();
   // TODO(siggi): Unwrap this by binding the loose param.
-  pm->task_runner_->PostTask(
+  PerformanceManagerImpl::GetTaskRunner()->PostTask(
       from_here, base::BindOnce(&PerformanceManagerImpl::RunCallbackWithGraph,
-                                base::Unretained(pm), std::move(callback)));
+                                std::move(callback)));
 }
 
 // static
@@ -36,14 +32,17 @@ void PerformanceManager::PassToGraph(const base::Location& from_here,
                                      std::unique_ptr<GraphOwned> graph_owned) {
   DCHECK(graph_owned);
 
-  // Passing |graph_| unretained is safe as it is actually destroyed on the
-  // destination sequence, and g_performance_manager.Get() would return nullptr
-  // if its deletion task was already posted.
-  auto* pm = PerformanceManagerImpl::GetInstance();
-  pm->task_runner_->PostTask(
+  // PassToGraph() should only be called when a graph is available to take
+  // ownership of |graph_owned|.
+  DCHECK(IsAvailable());
+
+  PerformanceManagerImpl::CallOnGraphImpl(
       from_here,
-      base::BindOnce(&GraphImpl::PassToGraph, base::Unretained(&pm->graph_),
-                     std::move(graph_owned)));
+      base::BindOnce(
+          [](std::unique_ptr<GraphOwned> graph_owned, GraphImpl* graph) {
+            graph->PassToGraph(std::move(graph_owned));
+          },
+          std::move(graph_owned)));
 }
 
 // static

components/performance_manager/performance_manager_impl.cc

@@ -12,6 +12,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/task/lazy_task_runner.h"
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
 #include "components/performance_manager/graph/frame_node_impl.h"
@@ -24,89 +25,76 @@ namespace performance_manager {
 
 namespace {
 
-class Singleton {
- public:
-  Singleton() = default;
-  ~Singleton() = default;
-
-  // Safe to call from any thread.
-  PerformanceManagerImpl* Get() {
-    return instance_.load(std::memory_order_acquire);
-  }
-
-  // Should be called from the main thread only.
-  void Set(PerformanceManagerImpl* instance) {
-    DCHECK_NE(nullptr, instance);
-    auto* old_value = instance_.exchange(instance, std::memory_order_release);
-    DCHECK_EQ(nullptr, old_value);
-  }
-
-  // Should be called from the main thread only.
-  void Clear(PerformanceManagerImpl* instance) {
-    DCHECK_NE(nullptr, instance);
-    auto* old_value = instance_.exchange(nullptr, std::memory_order_release);
-    DCHECK_EQ(instance, old_value);
-  }
-
- private:
-  std::atomic<PerformanceManagerImpl*> instance_{nullptr};
-};
-Singleton g_performance_manager;
-
-scoped_refptr<base::SequencedTaskRunner> CreateTaskRunner() {
-  return base::CreateSequencedTaskRunner(
-      {base::ThreadPool(), base::TaskPriority::USER_VISIBLE,
-       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN, base::MayBlock()});
-}
+// Singleton instance of PerformanceManagerImpl. Set from
+// PerformanceManagerImpl::StartImpl() and reset from the destructor of
+// PerformanceManagerImpl (PM sequence). Accesses should be on the PM sequence.
+PerformanceManagerImpl* g_performance_manager_from_pm_sequence = nullptr;
+
+// Singleton instance of PerformanceManagerImpl. Set from Create() and reset
+// from Destroy() (external sequence). Accesses can be on any sequence but must
+// not race with Create() or Destroy().
+//
+// TODO(https://crbug.com/1013127): Get rid of
+// PerformanceManagerImpl::GetInstance(). Callers can probably use
+// PerformanceManagerImpl::CallOnGraphImpl().
+PerformanceManagerImpl* g_performance_manager_from_any_sequence = nullptr;
+
+// The performance manager TaskRunner.
+base::LazySequencedTaskRunner g_performance_manager_task_runner =
+    LAZY_SEQUENCED_TASK_RUNNER_INITIALIZER(
+        base::TaskTraits(base::ThreadPool(),
+                         base::TaskPriority::USER_VISIBLE,
+                         base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
+                         base::MayBlock()));
 
 }  // namespace
 
 PerformanceManagerImpl::~PerformanceManagerImpl() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK_EQ(g_performance_manager_from_pm_sequence, this);
   // TODO(https://crbug.com/966840): Move this to a TearDown function.
   graph_.TearDown();
+  g_performance_manager_from_pm_sequence = nullptr;
 }
 
 // static
 void PerformanceManagerImpl::CallOnGraphImpl(const base::Location& from_here,
                                              GraphImplCallback callback) {
   DCHECK(callback);
-
-  // Passing |pm| unretained is safe as it is actually destroyed on the
-  // destination sequence, and g_performance_manager.Get() would return nullptr
-  // if its deletion task was already posted.
-  auto* pm = g_performance_manager.Get();
-  pm->task_runner_->PostTask(
+  GetTaskRunner()->PostTask(
       from_here,
       base::BindOnce(&PerformanceManagerImpl::RunCallbackWithGraphImpl,
-                     base::Unretained(pm), std::move(callback)));
+                     std::move(callback)));
 }
 
 PerformanceManagerImpl* PerformanceManagerImpl::GetInstance() {
-  return g_performance_manager.Get();
+  return g_performance_manager_from_any_sequence;
 }
 
 // static
 std::unique_ptr<PerformanceManagerImpl> PerformanceManagerImpl::Create(
     GraphImplCallback on_start) {
+  DCHECK(!g_performance_manager_from_any_sequence);
+
   std::unique_ptr<PerformanceManagerImpl> instance =
       base::WrapUnique(new PerformanceManagerImpl());
 
-  instance->task_runner()->PostTask(
+  g_performance_manager_from_any_sequence = instance.get();
+
+  GetTaskRunner()->PostTask(
       FROM_HERE,
       base::BindOnce(&PerformanceManagerImpl::OnStartImpl,
                      base::Unretained(instance.get()), std::move(on_start)));
 
-  g_performance_manager.Set(instance.get());
-
   return instance;
 }
 
 // static
 void PerformanceManagerImpl::Destroy(
     std::unique_ptr<PerformanceManagerImpl> instance) {
-  g_performance_manager.Clear(instance.get());
-  instance->task_runner_->DeleteSoon(FROM_HERE, instance.release());
+  DCHECK_EQ(instance.get(), g_performance_manager_from_any_sequence);
+  g_performance_manager_from_any_sequence = nullptr;
+  GetTaskRunner()->DeleteSoon(FROM_HERE, instance.release());
 }
 
 std::unique_ptr<FrameNodeImpl> PerformanceManagerImpl::CreateFrameNode(
@@ -155,13 +143,12 @@ std::unique_ptr<WorkerNodeImpl> PerformanceManagerImpl::CreateWorkerNode(
 
 void PerformanceManagerImpl::BatchDeleteNodes(
     std::vector<std::unique_ptr<NodeBase>> nodes) {
-  task_runner_->PostTask(
+  GetTaskRunner()->PostTask(
       FROM_HERE, base::BindOnce(&PerformanceManagerImpl::BatchDeleteNodesImpl,
                                 base::Unretained(this), std::move(nodes)));
 }
 
-PerformanceManagerImpl::PerformanceManagerImpl()
-    : task_runner_(CreateTaskRunner()) {
+PerformanceManagerImpl::PerformanceManagerImpl() {
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
@@ -187,7 +174,7 @@ std::unique_ptr<NodeType> PerformanceManagerImpl::CreateNodeImpl(
     Args&&... constructor_args) {
   std::unique_ptr<NodeType> new_node = std::make_unique<NodeType>(
       &graph_, std::forward<Args>(constructor_args)...);
-  task_runner_->PostTask(
+  GetTaskRunner()->PostTask(
       FROM_HERE, base::BindOnce(&AddNodeAndInvokeCreationCallback<NodeType>,
                                 std::move(creation_callback),
                                 base::Unretained(new_node.get()),
@@ -196,7 +183,7 @@ std::unique_ptr<NodeType> PerformanceManagerImpl::CreateNodeImpl(
 }
 
 void PerformanceManagerImpl::PostDeleteNode(std::unique_ptr<NodeBase> node) {
-  task_runner_->PostTask(
+  GetTaskRunner()->PostTask(
       FROM_HERE, base::BindOnce(&PerformanceManagerImpl::DeleteNodeImpl,
                                 base::Unretained(this), std::move(node)));
 }
@@ -269,23 +256,39 @@ void PerformanceManagerImpl::BatchDeleteNodesImpl(
   // When |nodes| goes out of scope, all nodes are deleted.
 }
 
+// static
+scoped_refptr<base::SequencedTaskRunner>
+PerformanceManagerImpl::GetTaskRunner() {
+  return g_performance_manager_task_runner.Get();
+}
+
+// static
 void PerformanceManagerImpl::RunCallbackWithGraphImpl(
     GraphImplCallback graph_callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(GetTaskRunner()->RunsTasksInCurrentSequence());
 
-  std::move(graph_callback).Run(&graph_);
+  if (g_performance_manager_from_pm_sequence) {
+    std::move(graph_callback)
+        .Run(&g_performance_manager_from_pm_sequence->graph_);
+  }
 }
 
+// static
 void PerformanceManagerImpl::RunCallbackWithGraph(
     GraphCallback graph_callback) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(GetTaskRunner()->RunsTasksInCurrentSequence());
 
-  std::move(graph_callback).Run(&graph_);
+  if (g_performance_manager_from_pm_sequence) {
+    std::move(graph_callback)
+        .Run(&g_performance_manager_from_pm_sequence->graph_);
+  }
 }
 
 void PerformanceManagerImpl::OnStartImpl(GraphImplCallback on_start) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!g_performance_manager_from_pm_sequence);
 
+  g_performance_manager_from_pm_sequence = this;
   graph_.set_ukm_recorder(ukm::UkmRecorder::Get());
   std::move(on_start).Run(&graph_);
 }

components/performance_manager/performance_manager_impl.h

@@ -40,8 +40,9 @@ class PerformanceManagerImpl : public PerformanceManager {
   ~PerformanceManagerImpl() override;
 
   // Posts a callback that will run on the PM sequence, and be provided a
-  // pointer to the Graph. Valid to be called from the main thread only, and
-  // only if "IsAvailable" returns true.
+  // pointer to the Graph. Valid to call from any sequence, but |graph_callback|
+  // won't run if this is called before Create() or after Destroy().
+  //
   // TODO(chrisha): Move this to the public interface.
   using GraphImplCallback = base::OnceCallback<void(GraphImpl*)>;
   static void CallOnGraphImpl(const base::Location& from_here,
@@ -57,10 +58,10 @@ class PerformanceManagerImpl : public PerformanceManager {
       base::OnceCallback<TaskReturnType(GraphImpl*)> task,
       base::OnceCallback<void(TaskReturnType)> reply);
 
-  // Retrieves the currently registered instance.
-  // The caller needs to ensure that the lifetime of the registered instance
-  // exceeds the use of this function and the retrieved pointer.
-  // This function can be called from any sequence with those caveats.
+  // Retrieves the currently registered instance. Calls must not race with
+  // Create() or Destroy(). The returned pointer must not be used after
+  // Destroy(). This function can be called from any sequence with those
+  // caveats.
   static PerformanceManagerImpl* GetInstance();
 
   // Creates, initializes and registers an instance.
@@ -112,17 +113,16 @@ class PerformanceManagerImpl : public PerformanceManager {
   // in topological order and destroying them.
   void BatchDeleteNodes(std::vector<std::unique_ptr<NodeBase>> nodes);
 
+  // Returns the performance manager TaskRunner.
   // TODO(chrisha): Hide this after the last consumer stops using it!
-  scoped_refptr<base::SequencedTaskRunner> task_runner() const {
-    return task_runner_;
-  }
+  static scoped_refptr<base::SequencedTaskRunner> GetTaskRunner();
 
   content::LockObserver* lock_observer() { return &lock_observer_; }
 
   // Indicates whether or not the caller is currently running on the PM task
   // runner.
   bool OnPMTaskRunnerForTesting() const {
-    return task_runner_->RunsTasksInCurrentSequence();
+    return GetTaskRunner()->RunsTasksInCurrentSequence();
   }
 
  private:
@@ -140,15 +140,13 @@ class PerformanceManagerImpl : public PerformanceManager {
   void BatchDeleteNodesImpl(std::vector<std::unique_ptr<NodeBase>> nodes);
 
   void OnStartImpl(GraphImplCallback graph_callback);
-  void RunCallbackWithGraphImpl(GraphImplCallback graph_callback);
-  void RunCallbackWithGraph(GraphCallback graph_callback);
+  static void RunCallbackWithGraphImpl(GraphImplCallback graph_callback);
+  static void RunCallbackWithGraph(GraphCallback graph_callback);
 
   template <typename TaskReturnType>
   TaskReturnType RunCallbackWithGraphAndReplyWithResult(
       base::OnceCallback<TaskReturnType(GraphImpl*)> task);
 
-  // The performance task runner.
-  const scoped_refptr<base::SequencedTaskRunner> task_runner_;
   GraphImpl graph_;
 
   // The LockObserver registered with //content to track lock acquisitions.
@@ -171,7 +169,7 @@ void PerformanceManagerImpl::CallOnGraphAndReplyWithResult(
     base::OnceCallback<void(TaskReturnType)> reply) {
   auto* pm = GetInstance();
   base::PostTaskAndReplyWithResult(
-      pm->task_runner_.get(), from_here,
+      GetTaskRunner().get(), from_here,
       base::BindOnce(
           &PerformanceManagerImpl::RunCallbackWithGraphAndReplyWithResult<
               TaskReturnType>,

components/performance_manager/performance_manager_tab_helper.cc

@@ -232,7 +232,7 @@ void PerformanceManagerTabHelper::RenderFrameHostChanged(
     return;
 
   // Perform the swap in the graph.
-  performance_manager_->task_runner()->PostTask(
+  PerformanceManagerImpl::GetTaskRunner()->PostTask(
       FROM_HERE, base::BindOnce(
                      [](FrameNodeImpl* old_frame, FrameNodeImpl* new_frame) {
                        if (old_frame) {
@@ -393,7 +393,7 @@ void PerformanceManagerTabHelper::PostToGraph(const base::Location& from_here,
                                               Args&&... args) {
   static_assert(std::is_base_of<NodeBase, NodeType>::value,
                 "NodeType must be descended from NodeBase");
-  performance_manager_->task_runner()->PostTask(
+  PerformanceManagerImpl::GetTaskRunner()->PostTask(
       from_here, base::BindOnce(functor, base::Unretained(node),
                                 std::forward<Args>(args)...));
 }

components/performance_manager/public/performance_manager.h

@@ -26,8 +26,8 @@ class PerformanceManager {
   static bool IsAvailable();
 
   // Posts a callback that will run on the PM sequence, and be provided a
-  // pointer to the Graph. Valid to call from the main thread only, and only
-  // if "IsAvailable" returns true.
+  // pointer to the Graph. Valid to call from any sequence, but |graph_callback|
+  // won't run if "IsAvailable" returns false.
   using GraphCallback = base::OnceCallback<void(Graph*)>;
   static void CallOnGraph(const base::Location& from_here,
                           GraphCallback graph_callback);

components/performance_manager/render_process_user_data.cc

@@ -88,9 +88,6 @@ RenderProcessUserData* RenderProcessUserData::GetOrCreateForRenderProcessHost(
 
 void RenderProcessUserData::RenderProcessReady(
     content::RenderProcessHost* host) {
-  PerformanceManagerImpl* performance_manager =
-      PerformanceManagerImpl::GetInstance();
-
   const base::Time launch_time =
 #if defined(OS_ANDROID)
       // Process::CreationTime() is not available on Android. Since this
@@ -101,7 +98,7 @@ void RenderProcessUserData::RenderProcessReady(
       host->GetProcess().CreationTime();
 #endif
 
-  performance_manager->task_runner()->PostTask(
+  PerformanceManagerImpl::GetTaskRunner()->PostTask(
       FROM_HERE, base::BindOnce(&ProcessNodeImpl::SetProcess,
                                 base::Unretained(process_node_.get()),
                                 host->GetProcess().Duplicate(), launch_time));
@@ -110,10 +107,7 @@ void RenderProcessUserData::RenderProcessReady(
 void RenderProcessUserData::RenderProcessExited(
     content::RenderProcessHost* host,
     const content::ChildProcessTerminationInfo& info) {
-  PerformanceManagerImpl* performance_manager =
-      PerformanceManagerImpl::GetInstance();
-
-  performance_manager->task_runner()->PostTask(
+  PerformanceManagerImpl::GetTaskRunner()->PostTask(
       FROM_HERE,
       base::BindOnce(&ProcessNodeImpl::SetProcessExitStatus,
                      base::Unretained(process_node_.get()), info.exit_code));