Fix various TaskManager bugs and add new enhancements

Fix various TaskManager bugs and add new enhancements

The following items are included in this CL.

1- Javascipt column for child processes that don't use V8 memory should always show "N/A".
2- Idle wakeups should show up only once per process.
3- NaCl debug ports column shows "Disabled" when the enable-nacl-debug flag is disabled.
4- NaCl modules should show the name of the app/extension instead of the URL.
5- Network usage can now be reported per process.

R=nick@chromium.org
BUG=551580
Committed: https://crrev.com/46afb30171cfbf9e2e0431decc67e9ced81c7716
Cr-Commit-Position: refs/heads/master@{#361152}

[afakhry, 2015-11-14 01:39:14]

Description was changed from

==========
Fix various TaskManager bugs

BUG=551580
==========

to

==========
Fix various TaskManager bugs and add new enhancements

The following items are included in this CL.

1- Profile name should show up for non shared child processes.
2- Javascipt column for child processes that don't use V8 memory should always
show "N/A".
3- Idle wakeups should show up only once per process.
4- NaCl debug ports column shows "Disabled" when the enable-nacl-debug flag is
disabled.
5- NaCl modules should show the name of the app/extension instead of the URL.
6- Network usage can now be reported per process.                               
    

R=nick@chromium.org
BUG=551580
==========

[afakhry, 2015-11-14 01:39:37]

afakhry@chromium.org changed reviewers:
+ nick@chromium.org

[afakhry, 2015-11-14 01:41:50]

Hey Nick, please review this CL. Thank you!

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

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
File chrome/browser/task_management/providers/child_process_task.cc (right):

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/providers/child_process_task.cc:163:
ProfileManager::GetActiveUserProfile());
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:11:55]

Description was changed from

==========
Fix various TaskManager bugs and add new enhancements

The following items are included in this CL.

1- Profile name should show up for non shared child processes.
2- Javascipt column for child processes that don't use V8 memory should always
show "N/A".
3- Idle wakeups should show up only once per process.
4- NaCl debug ports column shows "Disabled" when the enable-nacl-debug flag is
disabled.
5- NaCl modules should show the name of the app/extension instead of the URL.
6- Network usage can now be reported per process.                               
    

R=nick@chromium.org
BUG=551580
==========

to

==========
Fix various TaskManager bugs and add new enhancements

The following items are included in this CL.

1- Javascipt column for child processes that don't use V8 memory should always
show "N/A".
2- Idle wakeups should show up only once per process.
3- NaCl debug ports column shows "Disabled" when the enable-nacl-debug flag is
disabled.
4- NaCl modules should show the name of the app/extension instead of the URL.
5- Network usage can now be reported per process.                               
    

R=nick@chromium.org
BUG=551580
==========

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

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
File chrome/browser/task_management/providers/child_process_task.cc (right):

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/providers/child_process_task.cc:84:
ProfileManager::GetActiveUserProfile())->enabled_extensions();
What about this? We still need a profile or a browser context to get the
extension registry.

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/providers/child_process_task.cc:163:
ProfileManager::GetActiveUserProfile());
On 2015/11/18 18:39:56, ncarter wrote:
> 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, http://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?

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.

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
File chrome/browser/task_management/providers/task.h (right):

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/providers/task.h:49: static base::string16
GetProfileNameFromProfile(Profile* profile);
I'm keeping this here. It can be useful for future use.

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

ps6 looks good with the exception of the remaining call to GetActiveUserProfile.

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
File chrome/browser/task_management/providers/child_process_task.cc (right):

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/providers/child_process_task.cc:84:
ProfileManager::GetActiveUserProfile())->enabled_extensions();
On 2015/11/19 01:12:01, afakhry wrote:
> What about this? We still need a profile or a browser context to get the
> extension registry.

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?

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/providers/child_process_task.cc:163:
ProfileManager::GetActiveUserProfile());
On 2015/11/19 01:12:01, afakhry wrote:
> On 2015/11/18 18:39:56, ncarter wrote:
> > 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, http://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?
> 
> 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.

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.

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

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
File chrome/browser/task_management/providers/child_process_task.cc (right):

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/providers/child_process_task.cc:84:
ProfileManager::GetActiveUserProfile())->enabled_extensions();
On 2015/11/19 20:06:44, ncarter wrote:
> On 2015/11/19 01:12:01, afakhry wrote:
> > What about this? We still need a profile or a browser context to get the
> > extension registry.
> 
> 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?

Done.

[ncarter (slow), 2015-11-20 21:44:37]

lgtm

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
File chrome/browser/task_management/providers/task.h (right):

https://codereview.chromium.org/1439213004/diff/80001/chrome/browser/task_man...
chrome/browser/task_management/providers/task.h:49: static base::string16
GetProfileNameFromProfile(Profile* profile);
On 2015/11/19 01:12:01, afakhry wrote:
> I'm keeping this here. It can be useful for future use.

I agree.

https://codereview.chromium.org/1439213004/diff/120001/chrome/browser/task_ma...
File chrome/browser/task_management/providers/child_process_task.cc (right):

https://codereview.chromium.org/1439213004/diff/120001/chrome/browser/task_ma...
chrome/browser/task_management/providers/child_process_task.cc:86: for (auto*&
profile : loaded_profiles) {
ref to a pointer looks unusual to me -- auto* should be fine, right?

[afakhry, 2015-11-23 18:06:20]

https://codereview.chromium.org/1439213004/diff/120001/chrome/browser/task_ma...
File chrome/browser/task_management/providers/child_process_task.cc (right):

https://codereview.chromium.org/1439213004/diff/120001/chrome/browser/task_ma...
chrome/browser/task_management/providers/child_process_task.cc:86: for (auto*&
profile : loaded_profiles) {
On 2015/11/20 21:44:37, ncarter wrote:
> ref to a pointer looks unusual to me -- auto* should be fine, right?

Done.

[afakhry, 2015-11-23 18:06:35]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-11-23 18:06:35]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org
Link to the patchset: https://codereview.chromium.org/1439213004/#ps140001
(title: "auto*& --> auto*")

[commit-bot: I haz the power, 2015-11-23 18:07:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1439213004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1439213004/140001

[commit-bot: I haz the power, 2015-11-23 19:26:19]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-11-23 19:26:57]

Patchset 8 (id:??) landed as
https://crrev.com/46afb30171cfbf9e2e0431decc67e9ced81c7716
Cr-Commit-Position: refs/heads/master@{#361152}