|
|
DescriptionThis 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
Messages
Total messages: 61 (42 generated)
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== This creates a task manager provider that tracks the life and death of RenderProcessHosts. It does this because this task manager row can continue to live after a tab has navigated away but a service worker is left alive in the previous process (NTP does this). TM showing all RPHs changed to use child process task changed to use child process task fixed failures after killing a tab and reloading it first commit with TM tracking all rph BUG= ========== to ========== This creates a task manager provider that tracks the life and death of RenderProcessHosts. It does this because this task manager row can continue to live after a tab has navigated away but a service worker is left alive in the previous process (NTP does this). BUG=716609 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== This creates a task manager provider that tracks the life and death of RenderProcessHosts. It does this because this task manager row can continue to live after a tab has navigated away but a service worker is left alive in the previous process (NTP does this). BUG=716609 ========== to ========== This creates a task manager provider that tracks the life and death of RenderProcessHosts. It does this because this task manager row can continue to live after a tab has navigated away but a service worker is left alive in the previous process (NTP does this). It will be expanded to track service workers that are in a Render Process that has a web contents. BUG=716609 ==========
cburn@google.com changed reviewers: + nick@chromium.org
ptal
The CQ bit was checked by nick@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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... 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_man... File chrome/browser/task_manager/providers/child_process_task.cc (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/child_process_task.cc:104: case content::PROCESS_TYPE_BROWSER: The fallthrough behavior of 'case' labels means that we'll return "Render Process Host" for PROCESS_TYPE_BROWSER. Is that intentional? https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/child_process_task.cc:107: switches::kEnableAggressiveProcessTracking)) { Is this code actually reachable without the command line switch? If so, then I don't think we actually want to return "Render Process Host" in the cases where it was reachable before. If not, then I don't think we need to check for the presence of the command line switch. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/child_process_task.cc:108: return base::ASCIIToUTF16("Render Process Host"); Users will see this, so we will need to get this string translated ( http://dev.chromium.org/developers/design-documents/ui-localization ) so that it appears correctly in other locales. This can be done now, or left as a TODO, but it'll be a requirement before we can remove the flag. Render Process Host is probably too jargony to ask the translators to do something with. Let's talk face-to-face about what to do here. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... File chrome/browser/task_manager/providers/render_process_host_task_provider.cc (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:46: return nullptr; 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_. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:61: CreateTask(data); 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. } https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:78: const content::ChildProcessData& data) { 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. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:84: DeleteTask(data.id); 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. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:99: // completed at |ChildProcessDataCollected()|. Does this comment make sense? https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:104: // NOTREACHED(); 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. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:127: data.handle = host->GetHandle(); 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? https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... File chrome/browser/task_manager/providers/render_process_host_task_provider.h (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.h:29: // that are active, this can track interstitial pages and tracks RPHs that have "active, this" -> "active. This" https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.h:58: void DeleteTask(const int render_process_host_ID); "host_ID" -> "host_id". We lowercase acronyms & initialisms in variable names. https://codereview.chromium.org/2988453002/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:101: extern const char kEnableAddToShelf[]; This looks like a merge error. https://codereview.chromium.org/2988453002/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:102: extern const char kEnableAggressiveProcessTracking[]; This flag should mention the task manager somehow, like maybe: --task-manager-show-extra-renderers
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_androi...)
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== This creates a task manager provider that tracks the life and death of RenderProcessHosts. It does this because this task manager row can continue to live after a tab has navigated away but a service worker is left alive in the previous process (NTP does this). It will be expanded to track service workers that are in a Render Process that has a web contents. BUG=716609 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
Description was changed from ========== 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 ========== to ========== 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 ==========
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... chrome/browser/BUILD.gn:3427: "task_manager/providers/render_process_host_provider.h", On 2017/07/26 21:22:33, ncarter (slow) wrote: > Should be *task_* provider, right? Done. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... File chrome/browser/task_manager/providers/child_process_task.cc (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/child_process_task.cc:104: case content::PROCESS_TYPE_BROWSER: On 2017/07/26 21:22:34, ncarter (slow) wrote: > The fallthrough behavior of 'case' labels means that we'll return "Render > Process Host" for PROCESS_TYPE_BROWSER. Is that intentional? It was not, fixed now. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/child_process_task.cc:107: switches::kEnableAggressiveProcessTracking)) { On 2017/07/26 21:22:34, ncarter (slow) wrote: > Is this code actually reachable without the command line switch? > > If so, then I don't think we actually want to return "Render Process Host" in > the cases where it was reachable before. > > If not, then I don't think we need to check for the presence of the command line > switch. As far as I can tell I don't think it is hit and is one of the types that doesn't need a display name. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/child_process_task.cc:108: return base::ASCIIToUTF16("Render Process Host"); On 2017/07/26 21:22:34, ncarter (slow) wrote: > Users will see this, so we will need to get this string translated ( > http://dev.chromium.org/developers/design-documents/ui-localization ) so that it > appears correctly in other locales. This can be done now, or left as a TODO, but > it'll be a requirement before we can remove the flag. > > Render Process Host is probably too jargony to ask the translators to do > something with. Let's talk face-to-face about what to do here. Sounds good. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... File chrome/browser/task_manager/providers/render_process_host_task_provider.cc (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:46: return nullptr; On 2017/07/26 21:22:34, ncarter (slow) wrote: > 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_. Done. This was the only reason tasks_by_pid_. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:61: CreateTask(data); On 2017/07/26 21:22:34, ncarter (slow) wrote: > 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. > } I like the second option. It looks cleaner to me and easier to understand when the next person comes along and reads this class. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:78: const content::ChildProcessData& data) { On 2017/07/26 21:22:34, ncarter (slow) wrote: > 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. I agree. I went with rph_id so it is parallel with the |DeleteTask| signature. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:84: DeleteTask(data.id); On 2017/07/26 21:22:34, ncarter (slow) wrote: > 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. Done. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:99: // completed at |ChildProcessDataCollected()|. On 2017/07/26 21:22:34, ncarter (slow) wrote: > Does this comment make sense? It does not. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:104: // NOTREACHED(); On 2017/07/26 21:22:34, ncarter (slow) wrote: > 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. Done. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:127: data.handle = host->GetHandle(); On 2017/07/26 21:22:34, ncarter (slow) wrote: > 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? I don't think we need to do the PostTaskWhenProcessIsReady reading over this again. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... File chrome/browser/task_manager/providers/render_process_host_task_provider.h (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.h:29: // that are active, this can track interstitial pages and tracks RPHs that have On 2017/07/26 21:22:34, ncarter (slow) wrote: > "active, this" -> "active. This" Done. https://codereview.chromium.org/2988453002/diff/60001/chrome/browser/task_man... chrome/browser/task_manager/providers/render_process_host_task_provider.h:58: void DeleteTask(const int render_process_host_ID); On 2017/07/26 21:22:35, ncarter (slow) wrote: > "host_ID" -> "host_id". > > We lowercase acronyms & initialisms in variable names. Done. https://codereview.chromium.org/2988453002/diff/60001/chrome/common/chrome_sw... File chrome/common/chrome_switches.h (right): https://codereview.chromium.org/2988453002/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:101: extern const char kEnableAddToShelf[]; On 2017/07/26 21:22:35, ncarter (slow) wrote: > This looks like a merge error. It is. https://codereview.chromium.org/2988453002/diff/60001/chrome/common/chrome_sw... chrome/common/chrome_switches.h:102: extern const char kEnableAggressiveProcessTracking[]; On 2017/07/26 21:22:35, ncarter (slow) wrote: > This flag should mention the task manager somehow, like maybe: > > --task-manager-show-extra-renderers Done.
lgtm with 3 fixes https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/render_process_host_task_provider.cc (right): https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_ma... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2017 https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_ma... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:69: // ChildProcessDataCollected() should never be called after this, and hence This comment looks like copy/paste cruft https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_ma... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:71: weak_ptr_factory_.InvalidateWeakPtrs(); It looks like we can remove the weak_ptr_factory_ member, right? We don't call GetWeakPtr() anymore.
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/render_process_host_task_provider.cc (right): https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_ma... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2017/07/28 17:46:29, ncarter (slow) wrote: > 2017 Done. https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_ma... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:69: // ChildProcessDataCollected() should never be called after this, and hence On 2017/07/28 17:46:29, ncarter (slow) wrote: > This comment looks like copy/paste cruft It is. Done https://codereview.chromium.org/2988453002/diff/120001/chrome/browser/task_ma... chrome/browser/task_manager/providers/render_process_host_task_provider.cc:71: weak_ptr_factory_.InvalidateWeakPtrs(); On 2017/07/28 17:46:29, ncarter (slow) wrote: > It looks like we can remove the weak_ptr_factory_ member, right? We don't call > GetWeakPtr() anymore. Yes, removed now.
The CQ bit was unchecked by nick@chromium.org
The CQ bit was checked by nick@chromium.org
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/2988453002/#ps140001 (title: "fixed copy past cruft")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by cburn@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by cburn@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by nick@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by cburn@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_androi...)
The CQ bit was checked by cburn@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1501691692010170, "parent_rev": "ce899142efad247aa2d7e38f9251220bb031797e", "commit_rev": "d154ad87e7a64c56f49cf745c898af1bcfc3eb30"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/d154ad87e7a64c56f49cf745c898... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/d154ad87e7a64c56f49cf745c898...
Message was sent while issue was closed.
creis@chromium.org changed reviewers: + creis@chromium.org
Message was sent while issue was closed.
Nice! I looked over this after the fact and it looks good, but one request below to add more to the class-level comment to give context to others. (A few nits along the way.) Probably best to just write a separate small CL to update these. Thanks! 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 nit: Slight syntax fix: TODO(cburn): Also, let's move the TODO inside the conditional, since it only matters when the flag is present. 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. 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? 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 nit: Update "ID" here as well.
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. |