[Durable] Clear durable permission when clearing site data.

[Durable] Clear durable permission when clearing site data.

The Durable permission allows sites to gait preferential
treatment during our auto-eviction process for all storage
(indexeddb, local storage, cache storage, etc). If we're
clearing all of this data per user request, we want to
remove the permission, and then the site can ask for that
permission again.

BUG=629951
Committed: https://crrev.com/27ca374c96f1b58190e64520e2ae65d9e7a28ccc
Cr-Commit-Position: refs/heads/master@{#421936}

[dmurph, 2016-09-20 20:54:54]

dmurph@chromium.org changed reviewers:
+ msramek@chromium.org

[dmurph, 2016-09-20 20:54:55]

Hey Martin, can you PTAL at this change?

[dmurph, 2016-09-20 20:55:01]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 20:55:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 21:27:02]

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

[commit-bot: I haz the power, 2016-09-20 21:27:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmurph, 2016-09-20 22:53:04]

Description was changed from

==========
[Durable] Clear durable permission when clearing site data.

BUG=629951
==========

to

==========
[Durable] Clear durable permission when clearing site data.

BUG=629951
==========

[msramek, 2016-09-21 13:46:11]

Looks generally good, but I left some comments on the test.

Can you remind me what signals we use to grant the durable storage permission? I
want to make sure that adding it to "SITE_DATA" makes sense.

https://codereview.chromium.org/2354843006/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2354843006/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2655: // Verify
we only have allow
I don't think that's what we're verifying. Both added exceptions are ALLOW, so
even if the deletion failed, we would only have ALLOW.

https://codereview.chromium.org/2354843006/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2660:
ASSERT_TRUE(host_settings.size() >= 1u);
This seems unnecessary given the next line.

https://codereview.chromium.org/2354843006/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2662: // The
second origin should be deleted.
nit: I would instead say "only the first origin should have been left", as
that's what you're testing.

https://codereview.chromium.org/2354843006/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2667: <<
host_settings[0].primary_pattern.ToString();
This output seems out of place - should that be |host_settings[0].setting|
instead? Also below.

[dmurph, 2016-09-21 20:37:51]

We currently grant durable storage only if the user has a bookmark w/ the
requesting origin. We want to expand this in the future to use our 'Important'
metric - which combines site engagement, bookmarks, add to homescreen, and
notification permission.

It's considered a bug that clearing the sites data doesn't remove the permission
- the site should need to re-request the permission (which in this case would be
granted again as long as the user didn't remove their bookmarks).

Durable permission lets sites store more data.

https://codereview.chromium.org/2354843006/diff/1/chrome/browser/browsing_dat...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/2354843006/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2660:
ASSERT_TRUE(host_settings.size() >= 1u);
On 2016/09/21 13:46:11, msramek wrote:
> This seems unnecessary given the next line.

Done.

https://codereview.chromium.org/2354843006/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2662: // The
second origin should be deleted.
On 2016/09/21 13:46:11, msramek wrote:
> nit: I would instead say "only the first origin should have been left", as
> that's what you're testing.

Done.

https://codereview.chromium.org/2354843006/diff/1/chrome/browser/browsing_dat...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:2667: <<
host_settings[0].primary_pattern.ToString();
On 2016/09/21 13:46:11, msramek wrote:
> This output seems out of place - should that be |host_settings[0].setting|
> instead? Also below.

Done.

[dmurph, 2016-09-21 20:38:23]

edit: it doesn't let sites store more data, it removes them from the auto-evict
list if we get storage pressure.

[dmurph, 2016-09-21 20:48:42]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 20:49:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 21:43:41]

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

[commit-bot: I haz the power, 2016-09-21 21:43:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[msramek, 2016-09-22 16:23:52]

Ok, that makes sense. I was considering whether history would be a better
option, and in some sense that's already the case - as deleting history (->
clearing site engagement) and bookmarks will make a website ineligible for
durable storage.

Clearing site data revokes the actually granted permission, but the website can
ask for it again and will probably get it again if no other conditions changed.

So this LGTM.

[msramek, 2016-09-22 16:30:59]

Hmm, one more question - does this only affect local storage or other backends
as well? Because in that case we should simply delete it together with local
storage.

[dmurph, 2016-09-23 18:31:16]

On 2016/09/22 16:30:59, msramek wrote:
> Hmm, one more question - does this only affect local storage or other backends
> as well? Because in that case we should simply delete it together with local
> storage.

It effects all storage backends (indexeddb, localstorage, cache storage, etc)

[dmurph, 2016-09-23 18:31:25]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2016-09-23 18:32:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2016-09-23 18:33:09]

Description was changed from

==========
[Durable] Clear durable permission when clearing site data.

BUG=629951
==========

to

==========
[Durable] Clear durable permission when clearing site data.

The Durable permission allows sites to gait preferential
treatment during our auto-eviction process for all storage
(indexeddb, local storage, cache storage, etc). If we're
clearing all of this data per user request, we want to
remove the permission, and then the site can ask for that
permission again.

BUG=629951
==========

[commit-bot: I haz the power, 2016-09-23 19:13:46]

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

[commit-bot: I haz the power, 2016-09-23 19:13:47]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msramek, 2016-09-23 19:57:40]

OK. Those failing tests are exactly the reason I was asking - if we could lump
durable storage with an existing datatype, we wouldn't have to deal with them.

What's failing is that there's no concept of SITE_DATA in extensions'
browsingData API, only individual types of storage. You'll probably want to
define a "durable_storage" type there, or just fix the tests if you think
extensions should not have that capability.

[dmurph, 2016-09-23 21:32:47]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 21:33:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2016-09-23 21:34:47]

Thanks for the info, I fixed the tests but added a bug to support it. It looks
like we might want this to happen when the extension clears "protectedWeb". Is
there an easy way to do that?

I added a bug to add this feature to the browsing data api:
https://bugs.chromium.org/p/chromium/issues/detail?id=649817.

Can you PTAL one more time to verify my logic?

Thanks,
DAniel

[commit-bot: I haz the power, 2016-09-23 22:11:34]

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

[commit-bot: I haz the power, 2016-09-23 22:11:35]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[msramek, 2016-09-26 12:58:59]

msramek@chromium.org changed reviewers:
+ mkwst@chromium.org

[msramek, 2016-09-26 12:59:00]

A TODO and a bug to add the extensions API later SGTM.

However, when we implement it, it should have its own bucket. I can see how you
came to the idea of lumping it with "protectedWeb", since both concepts
represent some kind of privileged websites, but it doesn't work that way.
"protectedWeb" is an orthogonal category - it means hosted apps' data. And I
assume whether something is a hosted app is independent of whether it has a
durable storage?

I left some comments, but note that I'm not an owner of the API, so let me add
+Mike.

https://codereview.chromium.org/2354843006/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/browsing_data/browsing_data_test.cc (right):

https://codereview.chromium.org/2354843006/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/browsing_data/browsing_data_test.cc:501: int
site_data_no_plugins = site_data_no_usage & site_data_no_durable &
nit: These constants exclude more than they claim. Can we rename them?

https://codereview.chromium.org/2354843006/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/browsing_data/browsing_data_test.cc:520:
site_data_no_usage);
This should probably be |site_data_no_durable| in the current setup.

https://codereview.chromium.org/2354843006/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/browsing_data/browsing_data_test.cc:525: int
site_data_no_plugins = BrowsingDataRemover::REMOVE_SITE_DATA &
Ditto here. I know that it was already wrong before, but please rename to e.g.
|site_data_supported_types|.

[dmurph, 2016-09-28 18:12:46]

The CQ bit was checked by dmurph@chromium.org to run a CQ dry run

[dmurph, 2016-09-28 18:12:52]

https://codereview.chromium.org/2354843006/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/browsing_data/browsing_data_test.cc (right):

https://codereview.chromium.org/2354843006/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/browsing_data/browsing_data_test.cc:501: int
site_data_no_plugins = site_data_no_usage & site_data_no_durable &
On 2016/09/26 12:59:00, msramek wrote:
> nit: These constants exclude more than they claim. Can we rename them?

Done.

https://codereview.chromium.org/2354843006/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/browsing_data/browsing_data_test.cc:520:
site_data_no_usage);
On 2016/09/26 12:59:00, msramek wrote:
> This should probably be |site_data_no_durable| in the current setup.

Done.

[commit-bot: I haz the power, 2016-09-28 18:13:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmurph, 2016-09-28 18:14:56]

For the browsing data api question:

A hosted app is different than durable, but the effort is to make any webapp
able to request the durable permission and then have it's storage a bit more
protected like a hosted app. So it's sorted of the protected web in that way,
where we don't want to default it to being cleared. But maybe the API isn't
supposed to be set up that way.

[commit-bot: I haz the power, 2016-09-28 19:39:42]

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

[commit-bot: I haz the power, 2016-09-28 19:39:43]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-09-28 19:48:33]

The changes to the extension test LGTM. Deferring to msramek@ on the rest.

[dmurph, 2016-09-29 20:41:05]

Thanks! The discussion between martin and I after his LGTM was just the test and
the followup with Browsing Data API, so this patch should be good.

[dmurph, 2016-09-29 20:41:33]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-09-29 20:41:34]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2354843006/#ps60001
(title: "fix test")

[commit-bot: I haz the power, 2016-09-29 20:42:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 21:20:32]

Description was changed from

==========
[Durable] Clear durable permission when clearing site data.

The Durable permission allows sites to gait preferential
treatment during our auto-eviction process for all storage
(indexeddb, local storage, cache storage, etc). If we're
clearing all of this data per user request, we want to
remove the permission, and then the site can ask for that
permission again.

BUG=629951
==========

to

==========
[Durable] Clear durable permission when clearing site data.

The Durable permission allows sites to gait preferential
treatment during our auto-eviction process for all storage
(indexeddb, local storage, cache storage, etc). If we're
clearing all of this data per user request, we want to
remove the permission, and then the site can ask for that
permission again.

BUG=629951
==========

[commit-bot: I haz the power, 2016-09-29 21:20:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-29 21:23:15]

Description was changed from

==========
[Durable] Clear durable permission when clearing site data.

The Durable permission allows sites to gait preferential
treatment during our auto-eviction process for all storage
(indexeddb, local storage, cache storage, etc). If we're
clearing all of this data per user request, we want to
remove the permission, and then the site can ask for that
permission again.

BUG=629951
==========

to

==========
[Durable] Clear durable permission when clearing site data.

The Durable permission allows sites to gait preferential
treatment during our auto-eviction process for all storage
(indexeddb, local storage, cache storage, etc). If we're
clearing all of this data per user request, we want to
remove the permission, and then the site can ask for that
permission again.

BUG=629951

Committed: https://crrev.com/27ca374c96f1b58190e64520e2ae65d9e7a28ccc
Cr-Commit-Position: refs/heads/master@{#421936}
==========

[commit-bot: I haz the power, 2016-09-29 21:23:16]

Patchset 4 (id:??) landed as
https://crrev.com/27ca374c96f1b58190e64520e2ae65d9e7a28ccc
Cr-Commit-Position: refs/heads/master@{#421936}