Perform alarm's period limit check while reading alarm info from StateStore.

Perform alarm's period limit check while reading alarm info from StateStore.

We used to only check for the minimum allowable period length in
chrome.alarms.create(). This CL also adds that check when alarms are
read through StateStore. This CL stores alarm.minimum_granularity to make
sure we do not poll more frequently than that granularity.

Because already stored (before this CL) alarms in StateStore won't have
minimum_granularity set, fall back to a relaxed minimum_granularity
in those cases (kDevDelayMinimum).

This CL also bumps up the minimum allowed period (kDevDelayMinimum)
for unpacked extensions from 0s to ~1s. The rationale is that using
a 0s period can result in very high browser memory usage very quickly.
For example, in test, I could make the browser/ process take up to
10GB of memory within 30s of browser run.

BUG=618540
Test=See bug description for repro case.

Committed: https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b
Cr-Commit-Position: refs/heads/master@{#399360}

[lazyboy, 2016-06-09 02:43:48]

lazyboy@chromium.org changed reviewers:
+ asargent@chromium.org

[lazyboy, 2016-06-09 02:43:49]

Sending the CL out, it didn't turn out as trivial as I have hoped.

/cc Devlin in case you want to take a look.

[Devlin, 2016-06-09 17:05:00]

On 2016/06/09 02:43:49, lazyboy wrote:
> Sending the CL out, it didn't turn out as trivial as I have hoped.
> 
> /cc Devlin in case you want to take a look.

Does this fully address the concern?  What happens if I register 10k alarms to
go off in 1s or 1m (in which case we still have 10k alarms firing within rapid
succession)?

[lazyboy, 2016-06-09 17:53:30]

On 2016/06/09 17:05:00, Devlin wrote:
> On 2016/06/09 02:43:49, lazyboy wrote:
> > Sending the CL out, it didn't turn out as trivial as I have hoped.
> > 
> > /cc Devlin in case you want to take a look.
> 
> Does this fully address the concern?  
This CL is about fixing a flaw of alarms API:
A single alarm can bypass the limit we thought we impose on alarms through our
API.

What happens if I register 10k alarms to
> go off in 1s or 1m (in which case we still have 10k alarms firing within rapid
> succession)?

It is kind of a (valid) API concern.
One solution could be to simply limit the number of active alarms an extension
can register.
Is there a reason one extension would ever need more than ~100 alarms?

[asargent_no_longer_on_chrome, 2016-06-09 19:44:57]

Devlin's question about having very large numbers of registered alarms seems
like a good additional thing we should defend against. 

Would it help to base the limit on a minimum number of seconds between wake-ups
(60), instead of (or perhaps in addition to) a maximum number of alarms? Having
a limit of 100 is probably reasonable, but that potentially lets the
extension/app wake itself up more than once per second (using the when value)
unless we somehow already defend against this.

https://codereview.chromium.org/2051573003/diff/40001/extensions/browser/api/...
File extensions/browser/api/alarms/alarm_manager.cc (right):

https://codereview.chromium.org/2051573003/diff/40001/extensions/browser/api/...
extensions/browser/api/alarms/alarm_manager.cc:87: // know whether this alarm's
extension was loaded from a release (.crx)
Can you just look up the extension by id, and call
Extension::IsUnpackedLocation(extension->location()) ?

[lazyboy, 2016-06-09 22:00:21]

Antony: Devlin's question about having very large numbers of registered alarms
....

Let's tackle that separately, however much we want to tackle because it seems
unclear about what exactly we'd do there.

https://codereview.chromium.org/2051573003/diff/40001/extensions/browser/api/...
File extensions/browser/api/alarms/alarm_manager.cc (right):

https://codereview.chromium.org/2051573003/diff/40001/extensions/browser/api/...
extensions/browser/api/alarms/alarm_manager.cc:87: // know whether this alarm's
extension was loaded from a release (.crx)
On 2016/06/09 19:44:56, Antony Sargent wrote:
> Can you just look up the extension by id, and call
> Extension::IsUnpackedLocation(extension->location()) ?
> 

deleted kAlarmMinimumGranularity, thanks.
Done.

[asargent_no_longer_on_chrome, 2016-06-10 00:07:33]

lgtm

https://codereview.chromium.org/2051573003/diff/80001/extensions/browser/api/...
File extensions/browser/api/alarms/alarms_api.cc (left):

https://codereview.chromium.org/2051573003/diff/80001/extensions/browser/api/...
extensions/browser/api/alarms/alarms_api.cc:56: "warning message must be
updated");
optional: I see you moved these to a single assert in alarms_api_constants.cc.
It seems like that is slightly more likely to lead to someone blindly just
updating the value in the static_assert test without reading the error message
and realizing that it means the strings here need to be updated. My personal
preference would be to leave these as-is, but it isn't that big of a deal either
way. If you want to keep the change as you have it in the CL that's ok.

[lazyboy, 2016-06-10 20:01:32]

https://codereview.chromium.org/2051573003/diff/80001/extensions/browser/api/...
File extensions/browser/api/alarms/alarms_api.cc (left):

https://codereview.chromium.org/2051573003/diff/80001/extensions/browser/api/...
extensions/browser/api/alarms/alarms_api.cc:56: "warning message must be
updated");
On 2016/06/10 00:07:33, Antony Sargent wrote:
> optional: I see you moved these to a single assert in alarms_api_constants.cc.
> It seems like that is slightly more likely to lead to someone blindly just
> updating the value in the static_assert test without reading the error message
> and realizing that it means the strings here need to be updated. My personal
> preference would be to leave these as-is, but it isn't that big of a deal
either
> way. If you want to keep the change as you have it in the CL that's ok. 

I couldn't use static assert with variable declared/defined in
alarms_api_constants.h/cc
I've moved the warning messages too to alarms_api_constants.

[lazyboy, 2016-06-11 02:47:03]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2016-06-11 02:47:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/2051573003/#ps120001
(title: "rebase @tott")

[commit-bot: I haz the power, 2016-06-11 02:47:09]

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

[commit-bot: I haz the power, 2016-06-11 03:32:11]

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

[commit-bot: I haz the power, 2016-06-11 03:32:13]

Failed to apply the patch.

[commit-bot: I haz the power, 2016-06-11 03:33:13]

Description was changed from

==========
Perform alarm's period limit check while reading alarm info from StateStore.

We used to only check for the minimum allowable period length in
chrome.alarms.create(). This CL also adds that check when alarms are
read through StateStore. This CL stores alarm.minimum_granularity to make
sure we do not poll more frequently than that granularity.

Because already stored (before this CL) alarms in StateStore won't have
minimum_granularity set, fall back to a relaxed minimum_granularity
in those cases (kDevDelayMinimum).

This CL also bumps up the minimum allowed period (kDevDelayMinimum)
for unpacked extensions from 0s to ~1s. The rationale is that using
a 0s period can result in very high browser memory usage very quickly.
For example, in test, I could make the browser/ process take up to
10GB of memory within 30s of browser run.

BUG=618540
Test=See bug description for repro case.
==========

to

==========
Perform alarm's period limit check while reading alarm info from StateStore.

We used to only check for the minimum allowable period length in
chrome.alarms.create(). This CL also adds that check when alarms are
read through StateStore. This CL stores alarm.minimum_granularity to make
sure we do not poll more frequently than that granularity.

Because already stored (before this CL) alarms in StateStore won't have
minimum_granularity set, fall back to a relaxed minimum_granularity
in those cases (kDevDelayMinimum).

This CL also bumps up the minimum allowed period (kDevDelayMinimum)
for unpacked extensions from 0s to ~1s. The rationale is that using
a 0s period can result in very high browser memory usage very quickly.
For example, in test, I could make the browser/ process take up to
10GB of memory within 30s of browser run.

BUG=618540
Test=See bug description for repro case.

Committed: https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b
Cr-Commit-Position: refs/heads/master@{#399360}
==========

[commit-bot: I haz the power, 2016-06-11 03:33:15]

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