Fix various TaskManager bugs and add new enhancements

chrome/browser/task_management/providers/child_process_task.cc

Index: chrome/browser/task_management/providers/child_process_task.cc
diff --git a/chrome/browser/task_management/providers/child_process_task.cc b/chrome/browser/task_management/providers/child_process_task.cc
index b5a8cb7e5e74ff0f5a954dcee2036daa34443b88..475ad20345b814474575be98f892dcd2d5b0e93a 100644
--- a/chrome/browser/task_management/providers/child_process_task.cc
+++ b/chrome/browser/task_management/providers/child_process_task.cc
@@ -6,7 +6,10 @@
 
 #include "base/i18n/rtl.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/process_resource_usage.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/task_management/task_manager_observer.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/nacl/common/nacl_process_type.h"
@@ -15,6 +18,8 @@
 #include "content/public/browser/child_process_data.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/service_registry.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/common/extension_set.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
@@ -72,9 +77,19 @@ base::string16 GetLocalizedTitle(const base::string16& title,
                                         result_title);
     case PROCESS_TYPE_NACL_BROKER:
       return l10n_util::GetStringUTF16(IDS_TASK_MANAGER_NACL_BROKER_PREFIX);
-    case PROCESS_TYPE_NACL_LOADER:
+    case PROCESS_TYPE_NACL_LOADER: {
+      if (g_browser_process->profile_manager()) {
+        auto& enabled_extensions =
+            extensions::ExtensionRegistry::Get(
+                ProfileManager::GetActiveUserProfile())->enabled_extensions();

[afakhry, 2015/11/19 01:12:01]

What about this? We still need a profile or a browser context to get the
extension registry.

[ncarter (slow), 2015/11/19 20:06:44]

It is not correct to call GetActiveUserProfile like this. In this case,
different profiles have different sets of installed extensions.

To be truly correct we need to plumb the profile in. It looks like the nacl
process host has access to its profile path (good!), so if you can figure out
how to plumb that value here, you ought to be able to do
ProfileManager::GetProfileByPath() to recover the right profile here. But again
-- that's probably best tackled as a follow on change.

Without a plumbed-in profile, we can try looping over the
ProfileManager::GetLoadedProfiles(). The potential bug with looping would be
pretty mild: it's not guaranteed that if the same extension is installed in 2
profiles, it has the same name. In theory the names could be localized
differently. In practice I doubt this actually matters -- we apparently have
much bigger bugs with applying different display languages to different
profiles.

So I say: put in the loop hack for now, have test coverage for it if possible,
and leave a TODO() to remove the loop once we plumb the profile in?

[afakhry, 2015/11/20 20:55:43]

Done.

+        auto extension =
+            enabled_extensions.GetExtensionOrAppByURL(GURL(result_title));
+        if (extension)
+          result_title = base::UTF8ToUTF16(extension->name());
+      }
       return l10n_util::GetStringFUTF16(IDS_TASK_MANAGER_NACL_PREFIX,
                                         result_title);
+    }
     // These types don't need display names or get them from elsewhere.
     case content::PROCESS_TYPE_BROWSER:
     case content::PROCESS_TYPE_RENDERER:
@@ -127,6 +142,40 @@ ProcessResourceUsage* CreateProcessResourcesSampler(
   return new ProcessResourceUsage(service.Pass());
 }
 
+// Based on |process_type| we return the name of the active user profile that
+// started this task (at time of its construction). Some processes are shared
+// across profiles and hence we don't show a profile name for them.
+base::string16 GetChildProcessProfileName(int process_type) {
+  if (!g_browser_process->profile_manager())
+    return base::string16();
+
+  switch (process_type) {
+    case content::PROCESS_TYPE_BROWSER:
+    case content::PROCESS_TYPE_GPU:
+    case content::PROCESS_TYPE_ZYGOTE:
+    case content::PROCESS_TYPE_SANDBOX_HELPER:
+    case content::PROCESS_TYPE_MAX:
+    case content::PROCESS_TYPE_UNKNOWN:
+      return base::string16();
+
+    default:
+      return Task::GetProfileNameFromProfile(
+          ProfileManager::GetActiveUserProfile());

[ncarter (slow), 2015/11/18 18:39:56]

I don't think there's any reason to expect it belongs to the active profile? In
fact the comment on GetActiveUserProfile says to "avoid using it at all cost."

Bug https://code.google.com/p/chromium/issues/detail?id=552559 is about why this
API can be dangerous.

On non-cros/non-ash, you can test this with multiple profiles, and load the same
Flash page in each profile (say, homestarrunner.com), the task manager will show
two "Plugin: Shockwave Flash" rows. They belong to different profiles, and
should be shown as such.

I'm pretty sure that what you need to do, is to plumb in or look up the Profile
or the BrowsingContext associated with this task. (a BrowsingContext can be
converted into a Profile). That looks like a substantial amount of work, since
the BrowserChildProcessHosts seem not to interact with Profiles (or
ProfileIOData, since they're on the IO thread).

For PluginProcessHost, it does look like you can get a ResourceContext, which
you can convert into a ProfileIOData on the IO thread. That object exists 1:1
with the profile, but it's not clear to me if there's any way currently to look
up the Profile from its ProfileIOData -- you might have to add something for
that (say, a base::WeakPtr<Profile> profile_handle_for_posting_to_ui_thread()).

Let's drop the "profile name" part of this patch, and tackle that as a follow
on?

[afakhry, 2015/11/19 01:12:01]

I assumed that at the time of this task's construction getting the current
"active" user profile would correspond to the valid profile of the user that
started this process.

If it requires a bit of work, we can postpone it to a later CL.

[ncarter (slow), 2015/11/19 20:06:44]

No such guarantee; there's no such thing as a current profile, just the default
one. GetActiveUserProfile is mostly called from cros code, where it makes
considerably more sense, since multi-user works differently there. For non-cros,
it's basically just the default profile: it always returns the same Profile no
matter when you call it.

+  }
+}
+
+bool UsesV8Memory(int process_type) {
+  switch (process_type) {
+    case content::PROCESS_TYPE_UTILITY:
+    case content::PROCESS_TYPE_BROWSER:
+    case content::PROCESS_TYPE_RENDERER:
+      return true;
+
+    default:
+      return false;
+  }
+}
+
 }  // namespace
 
 ChildProcessTask::ChildProcessTask(const content::ChildProcessData& data)
@@ -136,8 +185,10 @@ ChildProcessTask::ChildProcessTask(const content::ChildProcessData& data)
       process_resources_sampler_(CreateProcessResourcesSampler(data.id)),
       v8_memory_allocated_(-1),
       v8_memory_used_(-1),
+      profile_name_(GetChildProcessProfileName(data.process_type)),
       unique_child_process_id_(data.id),
-      process_type_(data.process_type) {
+      process_type_(data.process_type),
+      uses_v8_memory_(UsesV8Memory(process_type_)) {
 }
 
 ChildProcessTask::~ChildProcessTask() {
@@ -150,6 +201,9 @@ void ChildProcessTask::Refresh(const base::TimeDelta& update_interval,
   if ((refresh_flags & REFRESH_TYPE_V8_MEMORY) == 0)
     return;
 
+  if (!uses_v8_memory_)
+    return;
+
   // The child process resources refresh is performed asynchronously, we will
   // invoke it and record the current values (which might be invalid at the
   // moment. We can safely ignore that and count on future refresh cycles
@@ -189,8 +243,12 @@ int ChildProcessTask::GetChildProcessUniqueID() const {
   return unique_child_process_id_;
 }
 
+base::string16 ChildProcessTask::GetProfileName() const {
+  return profile_name_;
+}
+
 bool ChildProcessTask::ReportsV8Memory() const {
-  return process_resources_sampler_->ReportsV8MemoryStats();
+  return uses_v8_memory_ && process_resources_sampler_->ReportsV8MemoryStats();
 }
 
 int64 ChildProcessTask::GetV8MemoryAllocated() const {