Task manager tracking RenderProcessHosts processes

chrome/browser/task_manager/providers/render_process_host_task_provider.cc

Index: chrome/browser/task_manager/providers/render_process_host_task_provider.cc
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
new file mode 100644
index 0000000000000000000000000000000000000000..4ae69a6a78783d03d4b09407d4289714b4d863ff
--- /dev/null
+++ b/chrome/browser/task_manager/providers/render_process_host_task_provider.cc
@@ -0,0 +1,142 @@
+// Copyright 2015 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 "chrome/browser/task_manager/providers/render_process_host_task_provider.h"
+
+#include "base/process/process.h"
+#include "chrome/browser/task_manager/providers/child_process_task.h"
+#include "content/public/browser/browser_child_process_host_iterator.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/child_process_data.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_types.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/common/process_type.h"
+#include "extensions/features/features.h"
+
+using content::RenderProcessHost;
+using content::BrowserThread;
+using content::ChildProcessData;
+
+#if BUILDFLAG(ENABLE_EXTENSIONS)
+#include "extensions/browser/process_map.h"
+#endif
+
+namespace task_manager {
+
+RenderProcessHostTaskProvider::RenderProcessHostTaskProvider()
+    : weak_ptr_factory_(this) {
+  registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED,
+                 content::NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                 content::NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
+                 content::NotificationService::AllBrowserContextsAndSources());
+}
+
+RenderProcessHostTaskProvider::~RenderProcessHostTaskProvider() {}
+
+Task* RenderProcessHostTaskProvider::GetTaskOfUrlRequest(int origin_pid,
+                                                         int child_id,
+                                                         int route_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  auto itr = tasks_by_pid_.find(static_cast<base::ProcessId>(origin_pid));
+  if (itr == tasks_by_pid_.end())
+    return nullptr;

[ncarter (slow), 2017/07/26 21:22:34]

Did you experiment with doing: tasks_by_rph_id_.find(child_id) ?

If that works for network activity in the ServiceWorker case, then it's probably
all we need. And if so, then I don't think we need tasks_by_pid_ at all.

However, I'm also totally fine with keeping tasks_by_pid_.

[cburn, 2017/07/27 19:28:29]

Done. This was the only reason tasks_by_pid_.

+
+  return itr->second;
+}
+
+void RenderProcessHostTaskProvider::StartUpdating() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK(tasks_by_rph_id_.empty());
+  DCHECK(tasks_by_pid_.empty());
+  for (RenderProcessHost::iterator it(RenderProcessHost::AllHostsIterator());
+       !it.IsAtEnd(); it.Advance()) {
+    RenderProcessHost* host = it.GetCurrentValue();
+    ChildProcessData data(content::PROCESS_TYPE_RENDERER);
+    data.handle = host->GetHandle();
+    data.id = host->GetID();
+    CreateTask(data);

[ncarter (slow), 2017/07/26 21:22:34]

What if host->IsReady() is false when the TaskManager is first shown?

That should be a pretty rare race condition, but it shouldn't be too hard to get
right -- the idea is to do something like the following pseudocode:

if (host->IsReady()) {
  CreateTask(host);
} else {
  host->PostTaskWhenReady(... &CreateTask);
}

However, given the observation below about the other call to
PostTaskWhenProcessIsReady(), I'm thinking maybe this class doesn't need to
worry itself with PostTaskWhenProcessIsReady at all!  An alternative might be as
simple as:

if (host->GetHandle()) {
  CreateTask(host);
} else {
   // There is no else clause; we'll just ignore this RPH,
   // and rely on NOTIFICATION_RENDERER_PROCESS_CREATED to happen
   // when the process launches.
}

[cburn, 2017/07/27 19:28:29]

I like the second option. It looks cleaner to me and easier to understand when
the next person comes along and reads this class. 

+  }
+}
+
+void RenderProcessHostTaskProvider::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();
+
+  // Then delete all tasks (if any).
+  tasks_by_rph_id_.clear();
+  tasks_by_pid_.clear();
+}
+
+void RenderProcessHostTaskProvider::CreateTask(
+    const content::ChildProcessData& data) {

[ncarter (slow), 2017/07/26 21:22:34]

I'm thinking this argument should either be an RPH, or else an rph_id that we
use to look up an RPH.

We should only need to create a ChildProcessData temporarily.

[cburn, 2017/07/27 19:28:29]

I agree. I went with rph_id so it is parallel with the |DeleteTask| signature. 

+  // Checks that the task by RenderProcessHost ID isn't already a task in the
+  // map and deletes it if it is so they new task can be cleanly added without
+  // causing crashes.
+  std::unique_ptr<ChildProcessTask>& check_task = tasks_by_rph_id_[data.id];
+  if (check_task.get() != nullptr) {
+    DeleteTask(data.id);

[ncarter (slow), 2017/07/26 21:22:34]

After DeleteTask returns, check_task references freed memory. Arguably that's an
accident waiting to happen; and even if there's no bug, it makes the rest of the
function a little harder to follow, since |task| and |check_task| look
redundant.

What I might do here, would be to just call DeleteTask(data.id) unconditionally,
as the first line of the function, since it's resilient to being called with an
unknown ID. Then, you can just proceed with task creation.

[cburn, 2017/07/27 19:28:30]

Done.

+  }
+  std::unique_ptr<ChildProcessTask>& task = tasks_by_rph_id_[data.id];
+  // Create the task and notify the observer.
+  task.reset(new ChildProcessTask(data));
+  tasks_by_pid_[task->process_id()] = task.get();
+  NotifyObserverTaskAdded(task.get());
+}
+
+void RenderProcessHostTaskProvider::DeleteTask(
+    const int render_process_host_ID) {
+  auto itr = tasks_by_rph_id_.find(render_process_host_ID);
+  // The following case should never happen since we start observing
+  // |RenderProcessHostObserver| only after we collect all pre-existing child
+  // processes and are notified (on the UI thread) that the collection is
+  // completed at |ChildProcessDataCollected()|.

[ncarter (slow), 2017/07/26 21:22:34]

Does this comment make sense?

[cburn, 2017/07/27 19:28:30]

It does not. 

+  if (itr == tasks_by_rph_id_.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.
+    // NOTREACHED();

[ncarter (slow), 2017/07/26 21:22:34]

Delete this comment, it looks like it's a copy/paste artifact; and moreover, I
think sometimes we'll want to intentionally call DeleteTask with a potentially
invalid ID.

[cburn, 2017/07/27 19:28:30]

Done.

+    return;
+  }
+
+  NotifyObserverTaskRemoved(itr->second.get());
+
+  // Clear from the pid index.
+  tasks_by_pid_.erase(itr->second->process_id());
+
+  // Finally delete the task.
+  tasks_by_rph_id_.erase(itr);
+}
+
+void RenderProcessHostTaskProvider::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  content::RenderProcessHost* host =
+      content::Source<content::RenderProcessHost>(source).ptr();
+  ChildProcessData data(content::PROCESS_TYPE_RENDERER);
+  switch (type) {
+    case content::NOTIFICATION_RENDERER_PROCESS_CREATED:
+      data.id = host->GetID();
+      data.handle = host->GetHandle();

[ncarter (slow), 2017/07/26 21:22:34]

It looks like NOTIFICATION_RENDERER_PROCESS_CREATED is sent from
ProcessLaunched, which is exactly the moment when the process handle is known.
So reading the handle here should be okay.

However, if we known the handle, is it necessary to do
PostTaskWhenProcessIsReady?

[cburn, 2017/07/27 19:28:29]

I don't think we need to do the PostTaskWhenProcessIsReady reading over this
again. 

+      host->PostTaskWhenProcessIsReady(
+          base::Bind(&RenderProcessHostTaskProvider::CreateTask,
+                     weak_ptr_factory_.GetWeakPtr(), data));
+      break;
+    case content::NOTIFICATION_RENDERER_PROCESS_CLOSED:
+    case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED:
+      DeleteTask(host->GetID());
+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+}
+
+}  // namespace task_manager