Rate limit programmatic update checks for extensions

Rate limit programmatic update checks for extensions

Previously extensions could call the runtime.requestUpdateCheck method
as often as every 5 seconds. This CL introduces a more restrictive
policy of around one extra check per autoupdate period, and changes the
implementation location of the rate limiting from within the
ExtensionUpdater code itself to the ChromeRuntimeAPIDelegate, and makes
it more flexible by using net::BackoffEntry.

BUG=599310
Committed: https://crrev.com/52524d5ef1489926230064f7d40fa5dd315a1795
Cr-Commit-Position: refs/heads/master@{#388362}

[asargent_no_longer_on_chrome, 2016-04-14 22:28:27]

asargent@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-04-15 17:57:38]

mostly lg, but looks like it's failing on CrOS

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc
(right):

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:63: const
net::BackoffEntry::Policy kBackoffPolicy = {
I think this causes problems with static initializers.

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:77: //
maximum_backoff_ms
might be worth commenting that -1 == no maximum

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:212: if
(info.backoff->ShouldRejectRequest() || info.callbacks.size() >= 10) {
document what 10 is supposed to signify here.

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:356: if (it
== update_check_info_.end())
Can this happen?

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h
(right):

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h:11: #include
<vector>
nit: don't need this anymore

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h:99: private:
drive-by: remove

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc
(right):

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc:94:
scoped_ptr<ManifestFetchData> fetch_data) override {
std::unique_ptr

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc:200:
// Uses runtime_delegate_ to runs an update check for |id|, expecting
nit: to run

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc:229:
scoped_ptr<RuntimeAPIDelegate> runtime_delegate_;
unique_ptr

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc:245:
// when the ExtensionUpdater hands the new version to the ExtensionService.
For my own edification, where's this logic?

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/u...
File chrome/browser/extensions/updater/extension_updater.cc (left):

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/u...
chrome/browser/extensions/updater/extension_updater.cc:63: // Require at least 5
seconds between consecutive non-succesful extension update
heh... 5 seconds should be enough, right? ;)

[asargent_no_longer_on_chrome, 2016-04-19 17:56:53]

comments addressed - please take another look

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc
(right):

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:63: const
net::BackoffEntry::Policy kBackoffPolicy = {
On 2016/04/15 17:57:37, Devlin wrote:
> I think this causes problems with static initializers.

I think this is actually ok because net::BackoffEntry::Policy is just a struct
of PODs. (Also there are several other Policy instances instantiated like this.)


https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

"Objects with static storage duration, including global variables, static
variables, static class member variables, and function static variables, must be
Plain Old Data (POD): only ints, chars, floats, or pointers, or arrays/structs
of POD."

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:77: //
maximum_backoff_ms
On 2016/04/15 17:57:37, Devlin wrote:
> might be worth commenting that -1 == no maximum

Done.

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:212: if
(info.backoff->ShouldRejectRequest() || info.callbacks.size() >= 10) {
On 2016/04/15 17:57:37, Devlin wrote:
> document what 10 is supposed to signify here.

Done.

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:356: if (it
== update_check_info_.end())
On 2016/04/15 17:57:37, Devlin wrote:
> Can this happen?

Yep, CallUpdateCallbacks gets called from ChromeRuntimeAPIDelegate::Observe when
a regularly scheduled extension autoupdate check (ie not one triggered by the
runtime.requestUpdateCheck method) found an update.

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h
(right):

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h:11: #include
<vector>
On 2016/04/15 17:57:37, Devlin wrote:
> nit: don't need this anymore

Done.

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.h:99: private:
On 2016/04/15 17:57:37, Devlin wrote:
> drive-by: remove

Done.

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc
(right):

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc:94:
scoped_ptr<ManifestFetchData> fetch_data) override {
On 2016/04/15 17:57:38, Devlin wrote:
> std::unique_ptr

Fixed (here and in extension_downloader_delegate.h which declares the interface)

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc:200:
// Uses runtime_delegate_ to runs an update check for |id|, expecting
On 2016/04/15 17:57:37, Devlin wrote:
> nit: to run

Done.

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc:229:
scoped_ptr<RuntimeAPIDelegate> runtime_delegate_;
On 2016/04/15 17:57:37, Devlin wrote:
> unique_ptr

Done.

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate_unittest.cc:245:
// when the ExtensionUpdater hands the new version to the ExtensionService.
On 2016/04/15 17:57:37, Devlin wrote:
> For my own edification, where's this logic?

It's in ExtensionService::ShouldDelayExtensionUpdate

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/u...
File chrome/browser/extensions/updater/extension_updater.cc (left):

https://codereview.chromium.org/1887253002/diff/1/chrome/browser/extensions/u...
chrome/browser/extensions/updater/extension_updater.cc:63: // Require at least 5
seconds between consecutive non-succesful extension update
On 2016/04/15 17:57:38, Devlin wrote:
> heh... 5 seconds should be enough, right? ;)

Well, it did get us this far. =)

[Devlin, 2016-04-19 19:55:19]

lgtm

https://codereview.chromium.org/1887253002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc
(right):

https://codereview.chromium.org/1887253002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:363:
info.callbacks.clear();
nit: could we just do vector::swap() here?

[asargent_no_longer_on_chrome, 2016-04-19 20:50:50]

https://codereview.chromium.org/1887253002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc
(right):

https://codereview.chromium.org/1887253002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc:363:
info.callbacks.clear();
On 2016/04/19 19:55:19, Devlin wrote:
> nit: could we just do vector::swap() here?

Done.

[asargent_no_longer_on_chrome, 2016-04-19 20:51:04]

The CQ bit was checked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2016-04-19 20:51:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1887253002/#ps100001
(title: "use vector::swap")

[commit-bot: I haz the power, 2016-04-19 20:51:35]

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

[commit-bot: I haz the power, 2016-04-19 21:07:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-19 21:07:50]

Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[asargent_no_longer_on_chrome, 2016-04-19 22:44:00]

The CQ bit was checked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2016-04-19 22:44:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1887253002/#ps120001
(title: "fix for idleness waiting problem caused by crrev.com/388245")

[commit-bot: I haz the power, 2016-04-19 22:44:41]

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

[commit-bot: I haz the power, 2016-04-19 23:46:47]

Committed patchset #7 (id:120001)

[msramek, 2016-04-20 08:18:21]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1903713002/ by msramek@chromium.org.

The reason for reverting is: This CL is suspected by sheriff-o-matic to have
caused the perf regression in the number of static initializers.

See crbug.com/605017 for more details..

[commit-bot: I haz the power, 2016-04-22 19:18:18]

Description was changed from

==========
Rate limit programmatic update checks for extensions

Previously extensions could call the runtime.requestUpdateCheck method
as often as every 5 seconds. This CL introduces a more restrictive
policy of around one extra check per autoupdate period, and changes the
implementation location of the rate limiting from within the
ExtensionUpdater code itself to the ChromeRuntimeAPIDelegate, and makes
it more flexible by using net::BackoffEntry.

BUG=599310
==========

to

==========
Rate limit programmatic update checks for extensions

Previously extensions could call the runtime.requestUpdateCheck method
as often as every 5 seconds. This CL introduces a more restrictive
policy of around one extra check per autoupdate period, and changes the
implementation location of the rate limiting from within the
ExtensionUpdater code itself to the ChromeRuntimeAPIDelegate, and makes
it more flexible by using net::BackoffEntry.

BUG=599310

Committed: https://crrev.com/52524d5ef1489926230064f7d40fa5dd315a1795
Cr-Commit-Position: refs/heads/master@{#388362}
==========

[commit-bot: I haz the power, 2016-04-22 19:18:19]

Patchset 7 (id:??) landed as
https://crrev.com/52524d5ef1489926230064f7d40fa5dd315a1795
Cr-Commit-Position: refs/heads/master@{#388362}