Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2281323002:
Use exponential backoff for component extension reloads. (Closed)
DescriptionUse exponential backoff for component extension reloads.
Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details.
This is a short term fix, which will mitigate crash loops for component extensions. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it.
This patch adds tracking for BackoffEntry objects corresponding to each component extensions that attempt to reload. The backoff policy values are similar to the extension check update's backoff policy.
BUG=639104
Committed: https://crrev.com/fb3b9fae7ff5afefa0275f1c8d30351fbfde81fc
Cr-Commit-Position: refs/heads/master@{#415555}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Changes per atwilson@'s comments. #
Total comments: 6
Patch Set 3 : Changes per atwilson@'s comments. #
Total comments: 4
Messages
Total messages: 50 (36 generated)
Description was changed from ========== comment BUG= ========== to ========== ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Description was changed from ========== ========== to ========== Use exponential backoff for some extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. BUG=639104 ==========
apacible@chromium.org changed reviewers: + atwilson@chromium.org
Description was changed from ========== Use exponential backoff for some extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. BUG=639104 ========== to ========== Use exponential backoff for some extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. This patch adds tracking for BackoffEntry objects corresponding to each non-policy, component extensions that attempt to reload. BUG=639104 ==========
Description was changed from ========== Use exponential backoff for some extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. This patch adds tracking for BackoffEntry objects corresponding to each non-policy, component extensions that attempt to reload. BUG=639104 ========== to ========== Use exponential backoff for non-policy, component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. This patch adds tracking for BackoffEntry objects corresponding to each non-policy, component extensions that attempt to reload. BUG=639104 ==========
Description was changed from ========== Use exponential backoff for non-policy, component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. This patch adds tracking for BackoffEntry objects corresponding to each non-policy, component extensions that attempt to reload. BUG=639104 ========== to ========== Use exponential backoff for non-policy, component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it. This patch adds tracking for BackoffEntry objects corresponding to each non-policy, component extensions that attempt to reload. BUG=639104 ==========
The CQ bit was checked by apacible@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_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 apacible@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...
The CQ bit was checked by apacible@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...
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Use exponential backoff for non-policy, component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it. This patch adds tracking for BackoffEntry objects corresponding to each non-policy, component extensions that attempt to reload. BUG=639104 ========== to ========== Use exponential backoff for non-policy, component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it. This patch adds tracking for BackoffEntry objects corresponding to each non-policy, component extensions that attempt to reload. The backoff policy values are similar to the extension check update's backoff policy. BUG=639104 ==========
apacible@chromium.org changed reviewers: - atwilson@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
apacible@chromium.org changed reviewers: + atwilson@chromium.org
PTAL, thanks!
https://codereview.chromium.org/2281323002/diff/80001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/2281323002/diff/80001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:540: // If the extension was a non-policy installed, component extension, use I'm not convinced this is the right approach (adding exponential backoff and related complexity to component extension restarts). First of all, I don't understand why we'd treat component extensions differently from other extensions. Secondly, the main motivation seems to be to not have this crash crap up our crash statistics from a few clients. So, I'd either suggest making this code apply to all extensions, or get rid of the complexity and don't reload component extensions. https://codereview.chromium.org/2281323002/diff/80001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:543: !extensions::Manifest::IsPolicyLocation(extension->location())) { I don't understand this - what extensions are *both* policy loaded and component extensions?
Description was changed from ========== Use exponential backoff for non-policy, component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions that are NOT policy-installed. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it. This patch adds tracking for BackoffEntry objects corresponding to each non-policy, component extensions that attempt to reload. The backoff policy values are similar to the extension check update's backoff policy. BUG=639104 ========== to ========== Use exponential backoff for component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it. This patch adds tracking for BackoffEntry objects corresponding to each component extensions that attempt to reload. The backoff policy values are similar to the extension check update's backoff policy. BUG=639104 ==========
Patchset #2 (id:100001) has been deleted
Thanks for your comments! +rdcronin (extensions) to weigh in on the suggestions. https://codereview.chromium.org/2281323002/diff/80001/chrome/browser/backgrou... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/2281323002/diff/80001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:540: // If the extension was a non-policy installed, component extension, use > First of all, I don't understand why we'd treat component extensions differently > from other extensions. Component extensions are loaded automatically (and retried automatically), whereas other extensions would require user action to try to reinstall. > Secondly, the main motivation seems to be to not have this crash crap up our > crash statistics from a few clients. The other motivation is to avoid an endless reload-attempt loop while the browser is running (regardless of whether this affects our stats). > So, I'd either suggest making this code apply to all extensions, or get rid of > the complexity and don't reload component extensions. If we don't try to reload component extensions, the user just won't be able to use the feature during the browser session. It could just be a one off crash. https://codereview.chromium.org/2281323002/diff/80001/chrome/browser/backgrou... chrome/browser/background/background_contents_service.cc:543: !extensions::Manifest::IsPolicyLocation(extension->location())) { On 2016/08/29 07:53:47, Andrew T Wilson (Slow) wrote: > I don't understand this - what extensions are *both* policy loaded and component > extensions? Good catch, it's just one or the other (as extensions only have one 'source location'). Updated.
On 2016/08/29 16:40:11, apacible wrote: > +rdcronin (extensions) to weigh in on the suggestions. I think apacible@ summed this up pretty nicely. Non-"forced" extensions aren't automatically reloaded, so we don't need any kind of back off associated with them - the user can only click the balloon so many times and so fast. :) We also should continue to reload component extensions because, occasionally, they *do* crash (or, if the user kills it in task manager, it should come back). Having them just not reload at all really isn't a viable option and gets the user into weirdness very quickly. It just so happens that some users are already subjected to that weirdness - hence the backoff.
rdevlin.cronin@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:447: RestartForceInstalledExtensionOnCrash(extension, profile); I'm not an OWNER here, but since I'm on the review, I'll add my $0.02. :) To me, it makes more sense to handle the component case here, rather than in RestartForceInstalledExtensionOnCrash(). Before, the restart code was the same for forced/not-forced, so it made sense. Now, I'd rather we just put all of it in one place (possibly in a new function) rather than needing the "force installed" logic duplicated.
https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:447: RestartForceInstalledExtensionOnCrash(extension, profile); On 2016/08/29 20:22:04, Devlin wrote: > I'm not an OWNER here, but since I'm on the review, I'll add my $0.02. :) To > me, it makes more sense to handle the component case here, rather than in > RestartForceInstalledExtensionOnCrash(). Before, the restart code was the same > for forced/not-forced, so it made sense. Now, I'd rather we just put all of it > in one place (possibly in a new function) rather than needing the "force > installed" logic duplicated. Acknowledged. Makes sense; I'll wait for atwilson@'s feedback before making changes. Thanks!
Thanks for clarifying - agreed that the auto-restart behavior for component extensions deserves the special handling. I would only do this for component extensions (not policy extensions) - so your code is correct as it stands. LGTM with one question about a possible shutdown leak. https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:447: RestartForceInstalledExtensionOnCrash(extension, profile); On 2016/08/29 23:26:34, apacible wrote: > On 2016/08/29 20:22:04, Devlin wrote: > > I'm not an OWNER here, but since I'm on the review, I'll add my $0.02. :) To > > me, it makes more sense to handle the component case here, rather than in > > RestartForceInstalledExtensionOnCrash(). Before, the restart code was the > same > > for forced/not-forced, so it made sense. Now, I'd rather we just put all of > it > > in one place (possibly in a new function) rather than needing the "force > > installed" logic duplicated. > > Acknowledged. Makes sense; I'll wait for atwilson@'s feedback before making > changes. Thanks! I want to make sure we only do the backoff logic for component extensions, not for policy-force-installed extensions, because I don't want people to be able to effectively disable force-installed extensions by killing them repeatedly and forcing a long exponential timeout. So in the end, I think the logic is slightly different, so I'm OK with leaving it down in the Restart() function. https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:550: backoff_entry = new net::BackoffEntry(&kExtensionReloadBackoffPolicy); I think these BackoffEntry() items leak on shutdown - should we store a unique_ptr instead?
Thanks for clarifying - agreed that the auto-restart behavior for component extensions deserves the special handling. I would only do this for component extensions (not policy extensions) - so your code is correct as it stands. LGTM with one question about a possible shutdown leak. https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:447: RestartForceInstalledExtensionOnCrash(extension, profile); On 2016/08/29 23:26:34, apacible wrote: > On 2016/08/29 20:22:04, Devlin wrote: > > I'm not an OWNER here, but since I'm on the review, I'll add my $0.02. :) To > > me, it makes more sense to handle the component case here, rather than in > > RestartForceInstalledExtensionOnCrash(). Before, the restart code was the > same > > for forced/not-forced, so it made sense. Now, I'd rather we just put all of > it > > in one place (possibly in a new function) rather than needing the "force > > installed" logic duplicated. > > Acknowledged. Makes sense; I'll wait for atwilson@'s feedback before making > changes. Thanks! I want to make sure we only do the backoff logic for component extensions, not for policy-force-installed extensions, because I don't want people to be able to effectively disable force-installed extensions by killing them repeatedly and forcing a long exponential timeout. So in the end, I think the logic is slightly different, so I'm OK with leaving it down in the Restart() function. https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:550: backoff_entry = new net::BackoffEntry(&kExtensionReloadBackoffPolicy); I think these BackoffEntry() items leak on shutdown - should we store a unique_ptr instead?
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by apacible@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/2281323002/diff/120001/chrome/browser/backgro... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:447: RestartForceInstalledExtensionOnCrash(extension, profile); On 2016/08/30 08:24:57, Andrew T Wilson (Slow) wrote: > On 2016/08/29 23:26:34, apacible wrote: > > On 2016/08/29 20:22:04, Devlin wrote: > > > I'm not an OWNER here, but since I'm on the review, I'll add my $0.02. :) > To > > > me, it makes more sense to handle the component case here, rather than in > > > RestartForceInstalledExtensionOnCrash(). Before, the restart code was the > > same > > > for forced/not-forced, so it made sense. Now, I'd rather we just put all of > > it > > > in one place (possibly in a new function) rather than needing the "force > > > installed" logic duplicated. > > > > Acknowledged. Makes sense; I'll wait for atwilson@'s feedback before making > > changes. Thanks! > I want to make sure we only do the backoff logic for component extensions, not > for policy-force-installed extensions, because I don't want people to be able to > effectively disable force-installed extensions by killing them repeatedly and > forcing a long exponential timeout. > > So in the end, I think the logic is slightly different, so I'm OK with leaving > it down in the Restart() function. Agree with both comments -- I made some changes to use unique_ptr below and decided to just keep it in the Restart() function. https://codereview.chromium.org/2281323002/diff/120001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:550: backoff_entry = new net::BackoffEntry(&kExtensionReloadBackoffPolicy); On 2016/08/30 08:24:57, Andrew T Wilson (Slow) wrote: > I think these BackoffEntry() items leak on shutdown - should we store a > unique_ptr instead? Done. Used get() to return a pointer to the BackoffEntry to avoid releasing ownership.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/2281323002/#ps160001 (title: "Changes per atwilson@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use exponential backoff for component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it. This patch adds tracking for BackoffEntry objects corresponding to each component extensions that attempt to reload. The backoff policy values are similar to the extension check update's backoff policy. BUG=639104 ========== to ========== Use exponential backoff for component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it. This patch adds tracking for BackoffEntry objects corresponding to each component extensions that attempt to reload. The backoff policy values are similar to the extension check update's backoff policy. BUG=639104 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Use exponential backoff for component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it. This patch adds tracking for BackoffEntry objects corresponding to each component extensions that attempt to reload. The backoff policy values are similar to the extension check update's backoff policy. BUG=639104 ========== to ========== Use exponential backoff for component extension reloads. Currently, any force installed extension attempts to reload after 3s. We're currently seeing an uptick in extension process crashes -- please see BUG for details. This is a short term fix, which will mitigate crash loops for component extensions. Any extensions that are policy-enabled will still be reloaded after 3s. We avoid using exponential backoff for policy-installed extensions since this can allow users to kill the extension multiple times, effectively disabling it. This patch adds tracking for BackoffEntry objects corresponding to each component extensions that attempt to reload. The backoff policy values are similar to the extension check update's backoff policy. BUG=639104 Committed: https://crrev.com/fb3b9fae7ff5afefa0275f1c8d30351fbfde81fc Cr-Commit-Position: refs/heads/master@{#415555} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fb3b9fae7ff5afefa0275f1c8d30351fbfde81fc Cr-Commit-Position: refs/heads/master@{#415555}
Message was sent while issue was closed.
wez@chromium.org changed reviewers: + wez@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2281323002/diff/160001/chrome/browser/backgro... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/2281323002/diff/160001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:551: backoff_map_.insert( You're inserting a new entry into backoff_map_ here but never erase()ing it - should it be cleaned-up similarly to content_map_, so that we can have a destructor DCHECK(empty) as we do for that? https://codereview.chromium.org/2281323002/diff/160001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:559: restart_delay = entry->GetTimeUntilRelease().InMilliseconds(); When do we re-set the backoff for an extension, if ever? e.g. if an extension happens to hit a crash every so often then won't the restart delay still ramp up, over time, to some arbitrary level? e.g. if WebRTC sometimes crashes the MR extension then after a dozen crashes the Cast feature will seem to be completely broken?
Message was sent while issue was closed.
https://codereview.chromium.org/2281323002/diff/160001/chrome/browser/backgro... File chrome/browser/background/background_contents_service.cc (right): https://codereview.chromium.org/2281323002/diff/160001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:551: backoff_map_.insert( On 2016/09/01 21:47:58, Wez wrote: > You're inserting a new entry into backoff_map_ here but never erase()ing it - > should it be cleaned-up similarly to content_map_, so that we can have a > destructor DCHECK(empty) as we do for that? Good catch. I'll address this in a follow up CL. https://codereview.chromium.org/2281323002/diff/160001/chrome/browser/backgro... chrome/browser/background/background_contents_service.cc:559: restart_delay = entry->GetTimeUntilRelease().InMilliseconds(); On 2016/09/01 21:47:58, Wez wrote: > When do we re-set the backoff for an extension, if ever? > > e.g. if an extension happens to hit a crash every so often then won't the > restart delay still ramp up, over time, to some arbitrary level? > > e.g. if WebRTC sometimes crashes the MR extension then after a dozen crashes the > Cast feature will seem to be completely broken? We currently do not reset the backoff within the same browser session. Chatted offline - it would make sense reset the backoff entry after the background contents normally shuts down or after has been running continuously for a fixed time (e.g. one minute). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
