New issue
Advanced search Search tips

Issue 618540 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

alarms api can cause alarms to fire too frequently (0s period) and cause heavy browser memory/cpu usage

Project Member Reported by lazyboy@chromium.org, Jun 9 2016

Issue description

1. In a published extension (crx), alarms.create shows a warning if the alarm period is less than a minute, saying alarm won't fire more frequently than a minute.

We store the alarms in StateStore through AlarmManager::WriteToStorage. The 1 minute check is only performed during the creation of the alarms. On subsequent browser run, when alarms are pulled off of StateStore, the 1 minute limit isn't ever checked. Therefore, the alarm will go off every whatever minutes the creation parameter specified "periodInMinutes".

This is easy to check:
a. Pack a crx platform app with
chrome.alarms.create(null, {"periodInMinutes": -2.0})
b. Load the packed extension.
c. Quit chromium
d. Run chromium
The browser memory will soon rise > 10GB

2. In dev mode with unpacked extensions, we allow the minimum period to be set to 0. This is a foot-gun thing, though not severe because it won't happen on published extensions. I think the minimum period for this should be bumped up too, sth like 1s.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 11 2016

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

commit 65f4c296239324342751425ce8f16dbe4a07034b
Author: lazyboy <lazyboy@chromium.org>
Date: Sat Jun 11 03:30:36 2016

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.

Review-Url: https://codereview.chromium.org/2051573003
Cr-Commit-Position: refs/heads/master@{#399360}

[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarm_manager.cc
[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarm_manager.h
[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarms_api.cc
[add] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarms_api_constants.cc
[add] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarms_api_constants.h
[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarms_api_unittest.cc
[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/extensions.gypi

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 15 2016

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

commit 65f4c296239324342751425ce8f16dbe4a07034b
Author: lazyboy <lazyboy@chromium.org>
Date: Sat Jun 11 03:30:36 2016

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.

Review-Url: https://codereview.chromium.org/2051573003
Cr-Commit-Position: refs/heads/master@{#399360}

[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarm_manager.cc
[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarm_manager.h
[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarms_api.cc
[add] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarms_api_constants.cc
[add] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarms_api_constants.h
[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/browser/api/alarms/alarms_api_unittest.cc
[modify] https://crrev.com/65f4c296239324342751425ce8f16dbe4a07034b/extensions/extensions.gypi

Status: Fixed (was: Started)

Sign in to add a comment