Show a notification when an ephemeral app consumes excessive disk space

Show a notification when an ephemeral app consumes excessive disk space

This patch introduces monitoring of the storage usage of extensions
and apps that are granted unlimited storage. A notification is shown
when persistent storage usage exceeds a threshold.

Monitoring is implemented for all extensions and apps, but initially
only enabled for ephemeral apps. Ephemeral apps have a lower threshold.

BUG=347801
TEST=browser_tests (ExtensionStorageMonitor.*)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=262692

[tmdiep, 2014-04-03 02:40:56]

This patch is dependent upon this patch, which introduces storage monitoring in
webkit:
https://codereview.chromium.org/218793002

I've implemented monitoring for all extensions/apps, but only enabled it for
ephemeral apps at this stage. If the feature works well, it will be rolled out
to all extensions/apps.

The notification in this patch is basic. Additional action buttons will be added
to the notification in future patches.

benwells: for extensions.
dewittj: for message_center and usage of notifications.

Thanks in advance.

[benwells, 2014-04-04 03:40:53]

sending to koz as I don't have the bandwidth right now.

[koz (OOO until 15th September), 2014-04-07 05:24:32]

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:105:
origin_state_map_.erase(it++);
Can it++ not go in the for-part?

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:139: // destruction.
nit: Replace comment with DCHECK(origin_state_map_.empty()) ?

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:209: StopMonitoringAll();
Should this also clear any notifications that are currently being shown to the
user? It might be worth considering using RAII for the currently shown
notifications. That would mean representing each currently shown notification as
an object and storing it in a map of linked_ptr<>s.

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:219:
StopMonitoringStorage(extension->id());
Same as above: when an extension gets unloaded, should we clear its
notifications?

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor_browsertest.cc (right):

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:41:
received_notifications_.end();
nit: I think this should be indented like

return received_...
    received_...;

i.e. only four spaces.

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:131:
prefs->StorageNotificationsEnabled(extension->id()));
StorageNotificationsEnabled -> GetStorageNotificationsEnabled.

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:140:
WriteDataPacket(extension, 1, false, true);
These calls might be clearer if you removed the last parameter and interned the
other bool argument like...

WriteBytesExpectingNotification(extension, kInitialusageThreshold);
WriteBytesNotExpectingNotification(extension, 1);

In the bottom test you can check that the permissions got disabled explicitly
there.

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor_factory.cc (right):

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor_factory.cc:33:
DependsOn(ExtensionPrefsFactory::GetInstance());
Does MessageCenter have a factory to DependsOn() here?

[tmdiep, 2014-04-07 08:45:00]

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:105:
origin_state_map_.erase(it++);
On 2014/04/07 05:24:32, koz wrote:
> Can it++ not go in the for-part?

I need the return value of the post-increment operator to erase the current
element, while  advancing the iterator so that it remains valid (idiom for
erasing map elements in a loop).

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:139: // destruction.
On 2014/04/07 05:24:32, koz wrote:
> nit: Replace comment with DCHECK(origin_state_map_.empty()) ?

Done.

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:209: StopMonitoringAll();
On 2014/04/07 05:24:32, koz wrote:
> Should this also clear any notifications that are currently being shown to the
> user? It might be worth considering using RAII for the currently shown
> notifications. That would mean representing each currently shown notification
as
> an object and storing it in a map of linked_ptr<>s.

Hmm, yes clearing the notifications would be a good idea. I don't think we need
to go as far as RAII for this though, can just save a list of extension ids.

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:219:
StopMonitoringStorage(extension->id());
On 2014/04/07 05:24:32, koz wrote:
> Same as above: when an extension gets unloaded, should we clear its
> notifications?

I removed the notification on uninstall. Unloading also occurs when the
extension is disabled. In future patches there will be additional actions, such
as capping quota and clearing data, which are applicable when disabled, so we
might want to keep the notification around.

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor_browsertest.cc (right):

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:41:
received_notifications_.end();
On 2014/04/07 05:24:32, koz wrote:
> nit: I think this should be indented like
> 
> return received_...
>     received_...;
> 
> i.e. only four spaces.

Done.

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:131:
prefs->StorageNotificationsEnabled(extension->id()));
On 2014/04/07 05:24:32, koz wrote:
> StorageNotificationsEnabled -> GetStorageNotificationsEnabled.

Changed to IsStorageNotificationEnabled().

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor_browsertest.cc:140:
WriteDataPacket(extension, 1, false, true);
On 2014/04/07 05:24:32, koz wrote:
> These calls might be clearer if you removed the last parameter and interned
the
> other bool argument like...
> 
> WriteBytesExpectingNotification(extension, kInitialusageThreshold);
> WriteBytesNotExpectingNotification(extension, 1);
> 
> In the bottom test you can check that the permissions got disabled explicitly
> there.

Good suggestion. Done.

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor_factory.cc (right):

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor_factory.cc:33:
DependsOn(ExtensionPrefsFactory::GetInstance());
On 2014/04/07 05:24:32, koz wrote:
> Does MessageCenter have a factory to DependsOn() here?

It's a global object, so there's no factory for it.

[koz (OOO until 15th September), 2014-04-08 00:25:59]

lgtm!

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/221933013/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:105:
origin_state_map_.erase(it++);
On 2014/04/07 08:45:01, tmdiep wrote:
> On 2014/04/07 05:24:32, koz wrote:
> > Can it++ not go in the for-part?
> 
> I need the return value of the post-increment operator to erase the current
> element, while  advancing the iterator so that it remains valid (idiom for
> erasing map elements in a loop).

Ah, I see. Very cool :-)

[dewittj, 2014-04-08 01:08:02]

Added some comments on notifications usage.

On a less code-oriented note, I'm concerned that the UX is not very compelling. 
Think of a grandma on ChromeOS (I have one) who sees this.  What are the steps
she can take?  Either close it or push the button.  Neither will solve her
problem.

Do you have a larger plan on what the final UI will look like? I suspect more
than just a notification is needed to give the proper controls (and undo
mistaken choices).

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:231: return
kNotificationIdPrefix + context_->GetPath().BaseName().MaybeAsASCII()
Can you please encode profile into the notification id?  I believe you could
have a conflict between one extension in two profiles.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:250: const int kIconSize
= extension_misc::EXTENSION_ICON_LARGE;
The icon size you need is message_center::kNotificationIconSize; (80px)

Is there such a size built-in for extensions?  I wonder if downsampling
..._EXTRA_LARGE is better quality than just ..._LARGE?

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:252: extension,
kIconSize, ExtensionIconSet::MATCH_BIGGER);
Does this handle the case where no resource matches?

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:291: base::string16() /*
display source */,
Ideally something would be in here.  If you right-click on the notification the
display source will be shown in a sentence: "Disable notifications from ___",
but this would be empty.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:295: new
message_center::HandleNotificationButtonClickDelegate(base::Bind(
I think you should just use a specialized NotificationDelegate subclass here. 
You will probably want to at least override NotificationDelegate::Close as well,
so you can set some pref or timer or something instead of just notifying the
user the next time they restart chrome (or logout/login to ChromeOS). 
Additionally you'll probably want other actions eventually too, like "disable
this app".  And maybe some UMA stats while you're at it.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:299:
notification->SetSystemPriority();
You want this to be non-auto-hide?  Is there a compelling reason?

[tmdiep, 2014-04-08 02:28:01]

The notification in this patch is not very useful, as you noted. As stated in my
initial comment, additional actions will be implemented in future patches. The
proposal is:

- Remove the extension/app.
- Revoke its unlimited storage permission and cap its usage.

This may end up being an interim solution anyway - in an email thread there has
been a proposal for smarter storage allocation to apps.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:231: return
kNotificationIdPrefix + context_->GetPath().BaseName().MaybeAsASCII()
On 2014/04/08 01:08:03, dewittj wrote:
> Can you please encode profile into the notification id?  I believe you could
> have a conflict between one extension in two profiles.

GetPath().BaseName() adds the profile name to the ID. I did some minor
refactoring here.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:250: const int kIconSize
= extension_misc::EXTENSION_ICON_LARGE;
On 2014/04/08 01:08:03, dewittj wrote:
> The icon size you need is message_center::kNotificationIconSize; (80px)
> 
> Is there such a size built-in for extensions?  I wonder if downsampling
> ..._EXTRA_LARGE is better quality than just ..._LARGE?

Done.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:252: extension,
kIconSize, ExtensionIconSet::MATCH_BIGGER);
On 2014/04/08 01:08:03, dewittj wrote:
> Does this handle the case where no resource matches?

Fixed - fallback to the default app or extension image.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:291: base::string16() /*
display source */,
On 2014/04/08 01:08:03, dewittj wrote:
> Ideally something would be in here.  If you right-click on the notification
the
> display source will be shown in a sentence: "Disable notifications from ___",
> but this would be empty.

This menu item seems to have been hidden as a result of this being empty. This
is a system notification that we don't actually want the user to disable, so I
prefer to leave as is.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:295: new
message_center::HandleNotificationButtonClickDelegate(base::Bind(
On 2014/04/08 01:08:03, dewittj wrote:
> I think you should just use a specialized NotificationDelegate subclass here. 
> You will probably want to at least override NotificationDelegate::Close as
well,
> so you can set some pref or timer or something instead of just notifying the
> user the next time they restart chrome (or logout/login to ChromeOS). 
> Additionally you'll probably want other actions eventually too, like "disable
> this app".  And maybe some UMA stats while you're at it.

Yes, once a threshold is hit, the next threshold is double (and saved to prefs)
to prevent the behavior you described. This also ensures that the user will be
notified if the usage keeps increasing.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:299:
notification->SetSystemPriority();
On 2014/04/08 01:08:03, dewittj wrote:
> You want this to be non-auto-hide?  Is there a compelling reason?

This is considered to be a system level warning of possible storage abuse by an
app or extension. We didn't want these to be auto-hidden.

[dewittj, 2014-04-08 16:52:28]

notifications changes/usage lgtm.  Thanks for the info on the ux discussions.

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_storage_monitor.cc (right):

https://codereview.chromium.org/221933013/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_storage_monitor.cc:291: base::string16() /*
display source */,
On 2014/04/08 02:28:01, tmdiep wrote:
> On 2014/04/08 01:08:03, dewittj wrote:
> > Ideally something would be in here.  If you right-click on the notification
> the
> > display source will be shown in a sentence: "Disable notifications from
___",
> > but this would be empty.
> 
> This menu item seems to have been hidden as a result of this being empty. This
> is a system notification that we don't actually want the user to disable, so I
> prefer to leave as is.

Oh okay, cool, probably that way for system notifications

[tmdiep, 2014-04-08 23:27:59]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-04-08 23:29:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/100001

[commit-bot: I haz the power, 2014-04-09 00:34:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/100001

[commit-bot: I haz the power, 2014-04-09 00:58:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/100001

[commit-bot: I haz the power, 2014-04-09 01:09:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/100001

[commit-bot: I haz the power, 2014-04-09 01:29:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/100001

[commit-bot: I haz the power, 2014-04-09 01:40:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/100001

[commit-bot: I haz the power, 2014-04-09 01:50:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/100001

[commit-bot: I haz the power, 2014-04-09 02:00:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/100001

[commit-bot: I haz the power, 2014-04-09 02:18:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/100001

[commit-bot: I haz the power, 2014-04-09 03:09:43]

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

[commit-bot: I haz the power, 2014-04-09 03:09:44]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[tmdiep, 2014-04-09 10:50:03]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-04-09 10:50:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/140001

[tmdiep, 2014-04-09 10:53:30]

The CQ bit was unchecked by tmdiep@chromium.org

[tmdiep, 2014-04-09 11:01:35]

The CQ bit was checked by tmdiep@chromium.org

[commit-bot: I haz the power, 2014-04-09 11:01:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/140001

[commit-bot: I haz the power, 2014-04-09 11:18:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/140001

[commit-bot: I haz the power, 2014-04-09 11:28:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tmdiep@chromium.org/221933013/140001

[commit-bot: I haz the power, 2014-04-09 14:10:19]

Change committed as 262692