Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(450)

Issue 2988453002: Task manager tracking RenderProcessHosts processes (Closed)

Created:
3 years, 5 months ago by cburn
Modified:
3 years, 4 months ago
CC:
chromium-reviews, jam, darin-cc_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

This creates a task manager provider that tracks the life and death of RenderProcessHosts. This is useful because it will track processes that are otherwise not included in the current task manager. An example of this is the NTP which has a service worker that lives beyond navigating away from the NTP. With this you can accurately see when the termination of the RenderProcessHost that is running this service worker. This version is requires the commandline flag of "--task-manager-show-extra-renderers" to be used. BUG=716609 Review-Url: https://codereview.chromium.org/2988453002 Cr-Commit-Position: refs/heads/master@{#491420} Committed: https://chromium.googlesource.com/chromium/src/+/d154ad87e7a64c56f49cf745c898af1bcfc3eb30

Patch Set 1 #

Patch Set 2 : TM showing all RPHs #

Patch Set 3 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into tmallproc #

Patch Set 4 : formatting fixes #

Total comments: 30

Patch Set 5 : formatting #

Patch Set 6 : removed command line guard from the RenderProcessHost returning in the child process task #

Patch Set 7 : added l10n reminder #

Total comments: 6

Patch Set 8 : fixed copy past cruft #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+224 lines, -4 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/task_manager/providers/child_process_task.cc View 1 2 3 4 5 6 4 chunks +12 lines, -1 line 2 comments Download
A chrome/browser/task_manager/providers/render_process_host_task_provider.h View 1 2 3 4 1 chunk +69 lines, -0 lines 4 comments Download
A chrome/browser/task_manager/providers/render_process_host_task_provider.cc View 1 2 3 4 5 6 7 1 chunk +126 lines, -0 lines 0 comments Download
M chrome/browser/task_manager/providers/web_contents/renderer_task.cc View 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/task_manager/sampling/task_manager_impl.cc View 1 2 3 4 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M content/public/browser/render_process_host.h View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 61 (42 generated)
cburn
ptal
3 years, 5 months ago (2017-07-21 17:37:30 UTC) #14
ncarter (slow)
https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/BUILD.gn#newcode3427 chrome/browser/BUILD.gn:3427: "task_manager/providers/render_process_host_provider.h", Should be *task_* provider, right? https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_manager/providers/child_process_task.cc File chrome/browser/task_manager/providers/child_process_task.cc ...
3 years, 4 months ago (2017-07-26 21:22:35 UTC) #17
cburn
ptal https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/BUILD.gn#newcode3427 chrome/browser/BUILD.gn:3427: "task_manager/providers/render_process_host_provider.h", On 2017/07/26 21:22:33, ncarter (slow) wrote: > ...
3 years, 4 months ago (2017-07-27 19:28:30 UTC) #27
ncarter (slow)
lgtm with 3 fixes https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_manager/providers/render_process_host_task_provider.cc File chrome/browser/task_manager/providers/render_process_host_task_provider.cc (right): https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_manager/providers/render_process_host_task_provider.cc#newcode1 chrome/browser/task_manager/providers/render_process_host_task_provider.cc:1: // Copyright 2015 The Chromium ...
3 years, 4 months ago (2017-07-28 17:46:29 UTC) #28
cburn
https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_manager/providers/render_process_host_task_provider.cc File chrome/browser/task_manager/providers/render_process_host_task_provider.cc (right): https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_manager/providers/render_process_host_task_provider.cc#newcode1 chrome/browser/task_manager/providers/render_process_host_task_provider.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. ...
3 years, 4 months ago (2017-07-28 18:08:21 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2988453002/140001
3 years, 4 months ago (2017-07-28 20:58:37 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/232030)
3 years, 4 months ago (2017-07-29 03:08:20 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2988453002/140001
3 years, 4 months ago (2017-07-31 16:35:21 UTC) #39
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/423191)
3 years, 4 months ago (2017-07-31 18:03:20 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2988453002/140001
3 years, 4 months ago (2017-07-31 19:54:57 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_asan_rel_ng/builds/423421)
3 years, 4 months ago (2017-07-31 21:53:35 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2988453002/140001
3 years, 4 months ago (2017-07-31 22:20:19 UTC) #47
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/514029)
3 years, 4 months ago (2017-08-01 00:18:39 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2988453002/140001
3 years, 4 months ago (2017-08-01 21:49:39 UTC) #51
commit-bot: I haz the power
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/353058)
3 years, 4 months ago (2017-08-01 23:58:39 UTC) #53
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2988453002/140001
3 years, 4 months ago (2017-08-02 16:35:03 UTC) #55
commit-bot: I haz the power
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/d154ad87e7a64c56f49cf745c898af1bcfc3eb30
3 years, 4 months ago (2017-08-02 17:25:54 UTC) #58
Charlie Reis
Nice! I looked over this after the fact and it looks good, but one request ...
3 years, 4 months ago (2017-08-02 18:39:15 UTC) #60
cburn
3 years, 4 months ago (2017-08-04 16:39:53 UTC) #61
Message was sent while issue was closed.
I am putting all of these changes from the comments into the next CL on this
issue.

https://codereview.chromium.org/2988453002/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_manager/providers/child_process_task.cc (right):

https://codereview.chromium.org/2988453002/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/providers/child_process_task.cc:104: // TODO:
(cburn) Start the UI Localization process for this. Currently the
On 2017/08/02 18:39:14, Charlie Reis (slow) wrote:
> nit: Slight syntax fix:
> 
> TODO(cburn): 
> 
> Also, let's move the TODO inside the conditional, since it only matters when
the
> flag is present.

Done.

https://codereview.chromium.org/2988453002/diff/140001/chrome/browser/task_ma...
File chrome/browser/task_manager/providers/render_process_host_task_provider.h
(right):

https://codereview.chromium.org/2988453002/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/providers/render_process_host_task_provider.h:25: //
service workers still alive in them.
On 2017/08/02 18:39:14, Charlie Reis (slow) wrote:
> I think this is a little unclear to people who aren't familiar with the
project.
>  Maybe we can rephrase it to offer a bit more context?
> 
> My understanding is that this is currently a fallback provider that shows a
row
> for renderer processes when they exist but don't otherwise have tasks in them.

> For now, this is useful for showing things like interstitials and service
> workers, but eventually those should get task providers of their own and this
> provider won't provide anything shown to users.  (I could be wrong?)
> 
> It's also worth noting that this is only enabled if the
> --task-manager-show-extra-renderers flag is passed.
> 
> I'm also unclear whether this currently shows anything if a process already
has
> tasks in it (e.g., if a tab and a service worker are in the same process, do
we
> get a line here for the service worker?  or only if there's no tabs?).  The
> "possibly redundant" part made that unclear to me.  Can you clarify in the
> comment?

I have fixed this in the follow on CL, where the behavior of this changes from
so that it will only show the "Renderer: " task row when there isn't a web
contents task that has the same Pid.

https://codereview.chromium.org/2988453002/diff/140001/chrome/browser/task_ma...
chrome/browser/task_manager/providers/render_process_host_task_provider.h:51: //
Deletes a RenderProcessHostTask whose |render_process_host_ID| is provided
On 2017/08/02 18:39:14, Charlie Reis (slow) wrote:
> nit: Update "ID" here as well.

Done.

Powered by Google App Engine
This is Rietveld 408576698