Expire appcaches that fail to update for "too long".

Expire appcaches that fail to update for "too long".

BUG=413706
Committed: https://crrev.com/657a07754224a29a72461e11067b4ab2558d31ce
Cr-Commit-Position: refs/heads/master@{#338196}

[michaeln, 2015-03-11 22:47:23]

michaeln@chromium.org changed reviewers:
+ jsbell@chromium.org, palmer@chromium.org

[palmer, 2015-04-21 19:56:34]

Sorry; somehow I didn't see this in my queue. Seems a reasonable approach.

https://codereview.chromium.org/879393002/diff/1/content/browser/appcache/app...
File content/browser/appcache/appcache.h (right):

https://codereview.chromium.org/879393002/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache.h:110: base::Time
newest_successful_unconditional_update_check_time() { return base::Time(); }
In the real implementation, these wouldn't really be base::Time(), right? You'd
store them in fields and persist them to disk?

https://codereview.chromium.org/879393002/diff/1/content/browser/appcache/app...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/879393002/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache_update_job.cc:493: // maybe "too long" for
http is less than it is for https?
Good idea.

[michaeln, 2015-04-21 20:20:16]

https://codereview.chromium.org/879393002/diff/1/content/browser/appcache/app...
File content/browser/appcache/appcache.h (right):

https://codereview.chromium.org/879393002/diff/1/content/browser/appcache/app...
content/browser/appcache/appcache.h:110: base::Time
newest_successful_unconditional_update_check_time() { return base::Time(); }
On 2015/04/21 19:56:34, palmer wrote:
> In the real implementation, these wouldn't really be base::Time(), right?
You'd
> store them in fields and persist them to disk?

Just like update_time, the value would stored on disk and the in-memory
representation would be base::Time.

[palmer, 2015-05-08 18:23:47]

Friendly ping. :)

[michaeln, 2015-05-23 00:29:16]

still WIP but getting fleshed out

[michaeln, 2015-05-27 20:23:56]

ptal, i haven't added tests yet but i'd like to get feedback on the production
code changes first, wdyt?

[jsbell, 2015-05-27 22:38:19]

looking good so far

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
File content/browser/appcache/appcache_database.cc (right):

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
content/browser/appcache/appcache_database.cc:428: sql::Statement
statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
Does this Statement need to be explicitly Run()?

(I'm a bit ignorant of the API here)

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
content/browser/appcache/appcache_database.cc:1249: if
(meta_table_->GetVersionNumber() == 6) {
Could this just be < 7 in both SimpleCache and other cases?

(This looks fine/correct, just thinking of ways to simplify)

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
File content/browser/appcache/appcache_storage.h (right):

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
content/browser/appcache/appcache_storage.h:154: // Schedules a tast to update
persistent storage with the times of the first
typo: tast->task

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
content/browser/appcache/appcache_update_job.cc:559: return;
Much nicer code flow here!

[michaeln, 2015-05-27 23:21:51]

thnx, i'll look at tests next

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
File content/browser/appcache/appcache_database.cc (right):

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
content/browser/appcache/appcache_database.cc:428: sql::Statement
statement(db_->GetCachedStatement(SQL_FROM_HERE, kSql));
On 2015/05/27 22:38:18, jsbell wrote:
> Does this Statement need to be explicitly Run()?
> 
> (I'm a bit ignorant of the API here)

Ooops, sure does need a call to Run.

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
content/browser/appcache/appcache_database.cc:1249: if
(meta_table_->GetVersionNumber() == 6) {
On 2015/05/27 22:38:18, jsbell wrote:
> Could this just be < 7 in both SimpleCache and other cases?
> 
> (This looks fine/correct, just thinking of ways to simplify)

That could work with an early return up top for < 2. Either way, I'd want to
leave the comment in about how the version upgrade mechanism was used (or
abused) on android for the version we're upgrading out of.

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
File content/browser/appcache/appcache_update_job.cc (right):

https://codereview.chromium.org/879393002/diff/60001/content/browser/appcache...
content/browser/appcache/appcache_update_job.cc:559: return;
On 2015/05/27 22:38:18, jsbell wrote:
> Much nicer code flow here!

yup, i put in some early returns elsewhere too for that reason

[michaeln, 2015-05-29 01:24:51]

Added dbclass tests, but still need to add jobclass tests.

Also, as coded, this will probably do the wrong thing at the very worst of times
(nuke a manifest that is in use while the user is behind a restrictive firewall
or stranded on an island w/o internet access). Probably needs some refining?

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
File content/browser/appcache/appcache_database_unittest.cc (left):

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_database_unittest.cc:748:
TEST(AppCacheDatabaseTest, UpgradeSchema3to5) {
The diffs here are confusing. I've removed the UpgradeSchema3to5 altoghter. Made
minimal mods to UpgradeSchema4to5 to turn it into UpgradeSchema4to7. And added
UpgradeSchema5or6to7 to check the lastest upgrade path.

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
File content/browser/appcache/appcache_database_unittest.cc (right):

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_database_unittest.cc:1031: EXPECT_EQ(7,
db.meta_table_->GetCompatibleVersionNumber());
The four expectations ^^^ are what has changed in this existing test.

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_database_unittest.cc:1065:
TEST(AppCacheDatabaseTest, UpgradeSchema5or6to7) {
This entire test is brand new.

[palmer, 2015-06-01 21:09:02]

Thanks for working on this! Some minor nits for the moment. Let me know when
you're ready and I'll take another look.

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
File content/browser/appcache/appcache_database.cc (right):

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_database.cc:401: bool
AppCacheDatabase::InsertGroup(const GroupRecord* record) {
Nit: I normally think to use a const reference, rather than pointer, for uses
like this.

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_database.cc:1248: // version [5,6] was pre May
2015
Nit: Punctuation, et c.

// Versions 5 and 6 were pre-May 2015. Version 7 adds support for expiring
// caches that are failing to update.

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
File content/browser/appcache/appcache_database_unittest.cc (right):

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_database_unittest.cc:493: // Commit the lazy
value and verify is sticks.
Typo: "...it sticks."

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
File content/browser/appcache/appcache_group.h (right):

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_group.h:150: // resource, but we bust the http
cache for a "full" update check after 24
Nit: What does "bust" mean here? Invalidate?

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
File content/browser/appcache/appcache_storage.h (right):

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_storage.h:156: virtual void
StoreEvictionTimes(AppCacheGroup* group) {}
Nit: = 0 seems to be the idiom in this file. Probably better to stick with it.

[michaeln, 2015-06-19 21:38:20]

Ptal, added tests for the job class too.

What should the two times values be? Should they be any different for http vs
https?

I've got kFullUpdateIntervale set to 24 hours which i think is reasonable. I've
got kMaxEvictableErrorDuration set to 2 weeks which may be controversial, i
picked that number based on the length of a vacation which might put you in odd
networking circumstances.

I'm pretty certain this logic has the capacity to do the wrong thing in some
cases.
- infrequently used app gets used twice at just the wrong time
- app only used on behind a prohibitive proxy goes away just when you want to
use it

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
File content/browser/appcache/appcache_database.cc (right):

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_database.cc:401: bool
AppCacheDatabase::InsertGroup(const GroupRecord* record) {
On 2015/06/01 21:09:02, palmer wrote:
> Nit: I normally think to use a const reference, rather than pointer, for uses
> like this.

You're right!?

This is an existing method (it only shows up here as fully green because I
rearranged the order of the method bodies to match the order in the .h file) and
I'd rather not change the method signature in this CL.

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_database.cc:1248: // version [5,6] was pre May
2015
On 2015/06/01 21:09:02, palmer wrote:
> Nit: Punctuation, et c.
> 
> // Versions 5 and 6 were pre-May 2015. Version 7 adds support for expiring
> // caches that are failing to update.

Done.

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
File content/browser/appcache/appcache_group.h (right):

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_group.h:150: // resource, but we bust the http
cache for a "full" update check after 24
On 2015/06/01 21:09:02, palmer wrote:
> Nit: What does "bust" mean here? Invalidate?

as in cache-busting, i used the term "bypass" instead

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
File content/browser/appcache/appcache_storage.h (right):

https://codereview.chromium.org/879393002/diff/80001/content/browser/appcache...
content/browser/appcache/appcache_storage.h:156: virtual void
StoreEvictionTimes(AppCacheGroup* group) {}
On 2015/06/01 21:09:02, palmer wrote:
> Nit: = 0 seems to be the idiom in this file. Probably better to stick with it.

Done.

https://codereview.chromium.org/879393002/diff/120001/content/browser/appcach...
File content/browser/appcache/appcache_update_job_unittest.cc (right):

https://codereview.chromium.org/879393002/diff/120001/content/browser/appcach...
content/browser/appcache/appcache_update_job_unittest.cc:3105:
EXPECT_EQ(expect_group_is_being_deleted_ || expect_eviction_,
There are a lot of existing update job tests that force the job class thru
various scenarios. This method is used to verify expectations for the lot of
them. I've added new expectations related to the new eviction logic to check for
whether new times are stored or eviction is triggered, and slightly modified a
couple of the existing tests to induce particular eviction related behavior.

[michaeln, 2015-06-22 23:39:47]

https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
File content/browser/appcache/appcache_update_job_unittest.cc (right):

https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
content/browser/appcache/appcache_update_job_unittest.cc:2825:
group_->set_last_full_update_check_time(base::Time::Now());
i don't think i need this line, if not i'll remove it

[palmer, 2015-06-26 18:23:32]

https://codereview.chromium.org/879393002/diff/120001/content/browser/appcach...
File content/browser/appcache/appcache_database.h (right):

https://codereview.chromium.org/879393002/diff/120001/content/browser/appcach...
content/browser/appcache/appcache_database.h:127: // given invalid groupids. The
return value only indicates whether
Nit: I would say "group IDs".

[palmer, 2015-06-26 20:41:27]

LGTM with some nits.

https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
File content/browser/appcache/appcache_database.cc (right):

https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
content/browser/appcache/appcache_database.cc:405: const char* kSql =
Nit: const char kSql[] =

https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
content/browser/appcache/appcache_database.cc:425: const char* kSql =
Nit: const char kSql[] = 

(and throughout)

https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
File content/browser/appcache/appcache_update_job_unittest.cc (right):

https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
content/browser/appcache/appcache_update_job_unittest.cc:2825:
group_->set_last_full_update_check_time(base::Time::Now());
On 2015/06/22 23:39:47, michaeln wrote:
> i don't think i need this line, if not i'll remove it

You're a better judge of that than I.

[michaeln, 2015-06-29 23:51:43]

On 2015/06/26 20:41:27, palmer wrote:
> LGTM with some nits.
> 
>
https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
> File content/browser/appcache/appcache_database.cc (right):
> 
>
https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
> content/browser/appcache/appcache_database.cc:405: const char* kSql =
> Nit: const char kSql[] =
> 
>
https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
> content/browser/appcache/appcache_database.cc:425: const char* kSql =
> Nit: const char kSql[] = 
> 
> (and throughout)
> 
>
https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
> File content/browser/appcache/appcache_update_job_unittest.cc (right):
> 
>
https://codereview.chromium.org/879393002/diff/140001/content/browser/appcach...
> content/browser/appcache/appcache_update_job_unittest.cc:2825:
> group_->set_last_full_update_check_time(base::Time::Now());
> On 2015/06/22 23:39:47, michaeln wrote:
> > i don't think i need this line, if not i'll remove it
> 
> You're a better judge of that than I.

done, done, and done

[palmer, 2015-07-06 23:48:21]

> done, done, and done

Thanks! LGTM.

[michaeln, 2015-07-07 00:02:17]

@jsbell, ptal

[jsbell, 2015-07-07 00:18:59]

lgtm with comment nits

https://codereview.chromium.org/879393002/diff/160001/content/browser/appcach...
File content/browser/appcache/appcache_database.cc (right):

https://codereview.chromium.org/879393002/diff/160001/content/browser/appcach...
content/browser/appcache/appcache_database.cc:402: if (!LazyOpen(true))
Would be nice to have /* create_if_needed */ comments here (and elsewhere)

https://codereview.chromium.org/879393002/diff/160001/content/browser/appcach...
content/browser/appcache/appcache_database.cc:1248: // Versions 5 and 6 were
pre-May 2015.
Bump to July ?

https://codereview.chromium.org/879393002/diff/160001/content/browser/appcach...
File content/browser/appcache/appcache_database.h (right):

https://codereview.chromium.org/879393002/diff/160001/content/browser/appcach...
content/browser/appcache/appcache_database.h:127: // given invalid groupids. The
return value only indicates whether
nit: group_ids

[michaeln, 2015-07-09 23:31:57]

The CQ bit was checked by michaeln@chromium.org

[michaeln, 2015-07-09 23:31:57]

The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org,
jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/879393002/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-07-09 23:33:43]

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

[commit-bot: I haz the power, 2015-07-10 00:26:39]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-07-10 00:28:00]

Patchset 10 (id:??) landed as
https://crrev.com/657a07754224a29a72461e11067b4ab2558d31ce
Cr-Commit-Position: refs/heads/master@{#338196}