New issue
Advanced search Search tips

Issue 695711 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature



Sign in to add a comment

Add a const char* parameter to ProcessManager::Increment/DecrementKeepaliveCount() and track in Debug builds

Project Member Reported by w...@chromium.org, Feb 24 2017

Issue description

To simplify tracking of the cause of lazy extension processes unexpectedly lingering, we could add a parameter to the Increment/Decrement APIs which describes the caller (e.g. the name of the async Extension API that wants to keep the Extension alive). 

Debug builds can maintain a table of ref-counts per description/name, so that we can expose the information e.g. via chrome://extensions, or Inspect.

Alternatively, identifying a small set of "buckets" describing keepalive reasons (e.g. AsyncApiPending, PluginImpulse, PpapiPlugin, etc) could be used to present similar information in some abbreviated form in Task Manager even in Release builds.
 
In theory, this sounds great to me - I'd love to be able to track down what's doing the keepalive.

In practice, where we actually surface this data might be a bit tricky - the extensions page is in pretty heavy flux, and finding a good place for it will be difficult.  I also don't know exactly where we'd surface it in the task manager.  But if you can get the requisite folks on board, I'd love to see it happen!

Comment 2 by w...@chromium.org, Feb 24 2017

If we took the enum-based approach (i.e. a small number of buckets) then we
could, e.g replace the keep-alive count column with some sort of coded
indication of the reasons, e.g. if a page has one or more tabs using it
it'd have a "T", if it is waiting on an API call it'd also have an "A", if
it has a plugin active then "P" and so on.

It wouldn't be pretty by any means, but useful for us and developers.

The other option would be to expose the information via an
extension-accessible API - when things are alive it should always be
possible to go into Developer Tools, run a command to query the keepalive
reasons - though we'd need a way to exclude the API itself from the reasons
;)
So with the enum-based approach, we could expect to see, e.g.
TTAAAAAP in the keepalive?  I guess it's reasonably descriptive, if somewhat unorthodox.

Re the extension API approach, that's also an interesting thought, and I could see something like a chrome.runtime.getKeepalive(), but I worry about whether there's valid use cases outside of debugging.

Comment 4 by w...@chromium.org, Feb 24 2017

No, I think I'd limit it to one instance of each "reason", so it doesn't
become unweildy, e.g. you'd get a single T no matter how many tabs are
using the extension. The most interesting cases we'll want to diagnose are,
I suspect, at the 0->1 reference boundary.

Re the API, yes, it'd be preferable to have an API that actually means
something useful in a more general context. e.g. if there were a standard
way to ask "what pages are currently relying on this ServiceWorker" then we
could perhaps crowbar Extension keep-alive reasons into that?
> The most interesting cases we'll want to diagnose are, I suspect, at the 0->1 reference boundary.

I'm not sure I agree - it could be, e.g., that a number of API functions are never finishing, and thus the keepalive is never being decremented.  The indication of that would be a ton of AAAAAAs that never go away.

Comment 6 by w...@chromium.org, Feb 24 2017

I'm thinking of any info in Task Manager as the indicator that there's an
unexpected problem, with some other means (e.g. chrome://inspect,
chrome://extensions, DevTools) as the means to find out the detail.
Status: Available (was: Untriaged)
Ah, okay.  In that case, it might be best to start with just the detailed form, since the keepalive count in the task manager is a good indication of whether or not more investigation is warranted.

This probably isn't something I'll be able to get to soon, so marking as available, but if you wanna jump on it, I'm happy to help review. :)

Comment 8 by w...@chromium.org, Feb 24 2017

If you have an idea of where to surface this then I'd be happy to - my
suggestion would be in chrome://extensions, as a "Used-by:" line below the
links to the active WebContents for each.
Project Member

Comment 9 by bugdroid1@chromium.org, Mar 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/adhd/+/ab692f0b051c25a94e30d25c906df321b3fc443d

commit ab692f0b051c25a94e30d25c906df321b3fc443d
Author: Cheng-Yi Chiang <cychiang@chromium.org>
Date: Thu Mar 02 02:42:15 2017

CRAS: iodev_list - Fix argument of cras_iodev_list_dev_is_enabled

This function queries whether a device is enabled, so it should take a
pointer to const iodev.

BUG=chromium:695711
TEST=make check

Change-Id: I14f3460089b66059fd0b9315bc0bf79672769827
Reviewed-on: https://chromium-review.googlesource.com/448241
Commit-Ready: Cheng-Yi Chiang <cychiang@chromium.org>
Tested-by: Cheng-Yi Chiang <cychiang@chromium.org>
Reviewed-by: Dylan Reid <dgreid@chromium.org>

[modify] https://crrev.com/ab692f0b051c25a94e30d25c906df321b3fc443d/cras/src/server/cras_iodev_list.c
[modify] https://crrev.com/ab692f0b051c25a94e30d25c906df321b3fc443d/cras/src/server/cras_iodev_list.h
[modify] https://crrev.com/ab692f0b051c25a94e30d25c906df321b3fc443d/cras/src/tests/bt_device_unittest.cc

Please ignore CL in #9, the bug number was a typo for  issue 697511 
Project Member

Comment 11 by sheriffbot@chromium.org, Mar 6 2018

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: dbertoni@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 13 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c063c77087274c4f13b0694a42ebcf3f4627d62e

commit c063c77087274c4f13b0694a42ebcf3f4627d62e
Author: David Bertoni <dbertoni@chromium.org>
Date: Mon Aug 27 16:12:56 2018

[Extensions] Rearranged code so we only ACK events that are expected.

Bug: 695711
Change-Id: Ic286b58b0d9a9984428e1db62ce82bd6f9c0ec0e
Reviewed-on: https://chromium-review.googlesource.com/1187826
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586275}
[modify] https://crrev.com/c063c77087274c4f13b0694a42ebcf3f4627d62e/extensions/browser/extension_host.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/79661573b41734cdf7fcb2f2e637ef1023d9cb57

commit 79661573b41734cdf7fcb2f2e637ef1023d9cb57
Author: David Bertoni <dbertoni@chromium.org>
Date: Tue Aug 28 23:51:14 2018

[Extensions] Initial implementation of the chrome://extensions-internals page.

Design doc is here: https://docs.google.com/document/d/1wYrk_EVzWF_df9OXl2Th0eHlCFlX5CpNqK-8lHg6Z7A/edit?usp=sharing

The extension fields included in the current implementation are:
ID
Keepalive count
Location
Manifest version
Name
Path
Type
Version

Bug: 695711
Change-Id: I7b1d61383b2107024e4b56cbd5389ab8fe20bee7
Reviewed-on: https://chromium-review.googlesource.com/1168119
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586939}
[modify] https://crrev.com/79661573b41734cdf7fcb2f2e637ef1023d9cb57/chrome/browser/extensions/extension_system_impl.cc
[modify] https://crrev.com/79661573b41734cdf7fcb2f2e637ef1023d9cb57/chrome/browser/ui/BUILD.gn
[add] https://crrev.com/79661573b41734cdf7fcb2f2e637ef1023d9cb57/chrome/browser/ui/webui/extensions/extensions_internals_browsertest.cc
[add] https://crrev.com/79661573b41734cdf7fcb2f2e637ef1023d9cb57/chrome/browser/ui/webui/extensions/extensions_internals_source.cc
[add] https://crrev.com/79661573b41734cdf7fcb2f2e637ef1023d9cb57/chrome/browser/ui/webui/extensions/extensions_internals_source.h
[modify] https://crrev.com/79661573b41734cdf7fcb2f2e637ef1023d9cb57/chrome/common/webui_url_constants.cc
[modify] https://crrev.com/79661573b41734cdf7fcb2f2e637ef1023d9cb57/chrome/common/webui_url_constants.h
[modify] https://crrev.com/79661573b41734cdf7fcb2f2e637ef1023d9cb57/chrome/test/BUILD.gn

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f

commit 3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f
Author: David Bertoni <dbertoni@chromium.org>
Date: Wed Aug 29 20:39:30 2018

[Extensions] Initial implementation of enhanced keepalive data.

The CL adds additional data when incrementing and decrementing the keepalive count, to allow for easier debugging of the counts.

Bug: 695711
Change-Id: Ibd078259c34cc11f786b3f895dc63c6452025604
Reviewed-on: https://chromium-review.googlesource.com/1168139
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587285}
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/chrome/browser/extensions/app_background_page_apitest.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/chrome/browser/extensions/extension_loading_browsertest.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/chrome/browser/extensions/lazy_background_page_apitest.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/chrome/browser/extensions/process_manager_browsertest.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/chrome/browser/extensions/service_worker_apitest.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/BUILD.gn
[add] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/activity.cc
[add] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/activity.h
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/api/messaging/extension_message_port.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/event_router.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/event_router.h
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/extension_function.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/extension_function_dispatcher.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/extension_function_dispatcher.h
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/extension_host.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/extension_host.h
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/extension_registrar.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/extension_web_contents_observer.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/lazy_background_task_queue.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/mojo/keep_alive_impl.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/mojo/keep_alive_impl_unittest.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/process_manager.cc
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/browser/process_manager.h
[modify] https://crrev.com/3e1e9fac1331b2dedab5d611e6cc9ed08fb0fc0f/extensions/components/javascript_dialog_extensions_client/javascript_dialog_extension_client_impl.cc

Project Member

Comment 16 by bugdroid1@chromium.org, Aug 30

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2cb53ba1f4b64078b4394237ffd74ee09ff2c62c

commit 2cb53ba1f4b64078b4394237ffd74ee09ff2c62c
Author: David Bertoni <dbertoni@chromium.org>
Date: Thu Aug 30 21:08:33 2018

[Extensions] Add the keepalive data to the extensions-internals page.

This CL integrates the new enhanced keepalive data into the JSON output for the chrome://extensions-internals page.

Bug: 695711
Change-Id: I7cc3b09dbccd2fef9379f2cb68939f4e029136cf
Reviewed-on: https://chromium-review.googlesource.com/1196169
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587763}
[modify] https://crrev.com/2cb53ba1f4b64078b4394237ffd74ee09ff2c62c/chrome/browser/ui/webui/extensions/extensions_internals_source.cc
[modify] https://crrev.com/2cb53ba1f4b64078b4394237ffd74ee09ff2c62c/extensions/browser/activity.cc
[modify] https://crrev.com/2cb53ba1f4b64078b4394237ffd74ee09ff2c62c/extensions/browser/activity.h

Sign in to add a comment