ServiceWorker: Use SimpleCache for the script cache

ServiceWorker: Use SimpleCache for the script cache

This CL migrates the backend of ServiceWorkerDiskCache from Default to
SimpleCache on all platforms.

If old DiskCache images exist,
(1) DiskCache backend fails to open an pre-existing data due to incompatible
format (in both Default->SimpleCache and SimpleCache->Default cases),
(2) ServiceWorkerStorage wipes out all cached scripts and registrations via
DeleteAndStartOver, and
(3) navigation falls back to network.

BUG=487482
TEST=manual (use DiskCache and SimpleCache alternately and make sure the system can recover)

Committed: https://crrev.com/a00e5be5a06e626de1b62cbfc7c350bef08647ea
Cr-Commit-Position: refs/heads/master@{#330274}

[nhiroki, 2015-05-13 06:29:55]

Patchset #2 (id:20001) has been deleted

[nhiroki, 2015-05-13 06:30:53]

Patchset #2 (id:40001) has been deleted

[nhiroki, 2015-05-13 06:41:00]

nhiroki@chromium.org changed reviewers:
+ kinuko@chromium.org

[nhiroki, 2015-05-13 06:41:12]

nhiroki@chromium.org changed reviewers:
+ michaeln@chromium.org

[nhiroki, 2015-05-13 06:41:25]

PTAL, thanks!

[nhiroki, 2015-05-13 06:42:35]

(+gavinp@ to CC)

[kinuko, 2015-05-13 07:19:37]

Looks good, but looks like I should defer this to michaeln.

[nhiroki, 2015-05-13 09:03:23]

Hmmm... ServiceWorkerBrowserTest is failing in AppCacheDiskCache on win bots.
I'm now investigating...

[nhiroki, 2015-05-13 09:42:15]

On 2015/05/13 09:03:23, nhiroki wrote:
> Hmmm... ServiceWorkerBrowserTest is failing in AppCacheDiskCache on win bots.
> I'm now investigating...

I maybe found the reason of this error.

AFAICS, AppCacheDiskCache can be destroyed even though there are inflight
operations (AppCacheDiskCache::ActiveCall). ActiveCall creates a callback with
"base::Unretained(this)" and manages its own lifetime, but AppCacheDiskCache
could delete them on its dtor before operations are completed. I guess that
switching the backend delays completion of operations and causes an obscure
error.

I created a patch to replace a rawptr with a weakptr in ActiveCall. It seems to
be working well:
https://codereview.chromium.org/1139573003/

[nhiroki, 2015-05-13 11:33:31]

On 2015/05/13 09:42:15, nhiroki wrote:
> On 2015/05/13 09:03:23, nhiroki wrote:
> > Hmmm... ServiceWorkerBrowserTest is failing in AppCacheDiskCache on win
bots.
> > I'm now investigating...
> 
> I maybe found the reason of this error.
> 
> AFAICS, AppCacheDiskCache can be destroyed even though there are inflight
> operations (AppCacheDiskCache::ActiveCall). ActiveCall creates a callback with
> "base::Unretained(this)" and manages its own lifetime, but AppCacheDiskCache
> could delete them on its dtor before operations are completed. I guess that
> switching the backend delays completion of operations and causes an obscure
> error.
> 
> I created a patch to replace a rawptr with a weakptr in ActiveCall. It seems
to
> be working well:
> https://codereview.chromium.org/1139573003/

FYI: Opened a separate review for this
https://codereview.chromium.org/1136373003/

[nhiroki, 2015-05-14 05:40:09]

Patchset #4 (id:50005) has been deleted

[nhiroki, 2015-05-14 06:26:08]

Patchset #5 (id:130001) has been deleted

[nhiroki, 2015-05-14 06:30:16]

Patchset #4 (id:110001) has been deleted

[michaeln, 2015-05-14 23:35:27]

once the other cl is worked out, this one lgtm

https://codereview.chromium.org/1140853002/diff/80001/content/browser/appcach...
File content/browser/appcache/appcache_disk_cache.cc (right):

https://codereview.chromium.org/1140853002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_disk_cache.cc:182: use_simple_cache_ = true;
nit: initializer list instead of the method body

https://codereview.chromium.org/1140853002/diff/80001/content/browser/appcach...
File content/browser/appcache/appcache_disk_cache.h (right):

https://codereview.chromium.org/1140853002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_disk_cache.h:28: AppCacheDiskCache();
might be cleaner to provide a second protected ctor that takes the param instead
of exposing the datamember as protected and expecting derivatives to set it in
their ctor

[nhiroki, 2015-05-15 01:48:48]

Patchset #4 (id:150001) has been deleted

[nhiroki, 2015-05-15 02:12:52]

Thank you!

https://codereview.chromium.org/1140853002/diff/80001/content/browser/appcach...
File content/browser/appcache/appcache_disk_cache.cc (right):

https://codereview.chromium.org/1140853002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_disk_cache.cc:182: use_simple_cache_ = true;
On 2015/05/14 23:35:26, michaeln wrote:
> nit: initializer list instead of the method body

Done.

https://codereview.chromium.org/1140853002/diff/80001/content/browser/appcach...
File content/browser/appcache/appcache_disk_cache.h (right):

https://codereview.chromium.org/1140853002/diff/80001/content/browser/appcach...
content/browser/appcache/appcache_disk_cache.h:28: AppCacheDiskCache();
On 2015/05/14 23:35:26, michaeln wrote:
> might be cleaner to provide a second protected ctor that takes the param
instead
> of exposing the datamember as protected and expecting derivatives to set it in
> their ctor

Done.

[kinuko, 2015-05-15 02:28:48]

lgtm + a few nits

https://codereview.chromium.org/1140853002/diff/170001/content/browser/appcac...
File content/browser/appcache/appcache_disk_cache.cc (right):

https://codereview.chromium.org/1140853002/diff/170001/content/browser/appcac...
content/browser/appcache/appcache_disk_cache.cc:181: : use_simple_cache_(true)
nit: we can use delegated ctor too

https://codereview.chromium.org/1140853002/diff/170001/content/browser/appcac...
File content/browser/appcache/appcache_disk_cache.h (right):

https://codereview.chromium.org/1140853002/diff/170001/content/browser/appcac...
content/browser/appcache/appcache_disk_cache.h:107: bool use_simple_cache_ =
false;
nit: we initialize this differently in ctors, it's probably not the best use of
in-class member initialization.

nit: and if that's the case probably we can just leave is_disabled_
initialization in ctors too (it's optional, but having all initialization in one
place is preferred in general)

[nhiroki, 2015-05-15 03:02:47]

Thanks! Updated.

https://codereview.chromium.org/1140853002/diff/170001/content/browser/appcac...
File content/browser/appcache/appcache_disk_cache.cc (right):

https://codereview.chromium.org/1140853002/diff/170001/content/browser/appcac...
content/browser/appcache/appcache_disk_cache.cc:181: : use_simple_cache_(true)
On 2015/05/15 02:28:48, kinuko wrote:
> nit: we can use delegated ctor too

Done (I thought delegated ctor was still under discussion).

https://codereview.chromium.org/1140853002/diff/170001/content/browser/appcac...
File content/browser/appcache/appcache_disk_cache.h (right):

https://codereview.chromium.org/1140853002/diff/170001/content/browser/appcac...
content/browser/appcache/appcache_disk_cache.h:107: bool use_simple_cache_ =
false;
On 2015/05/15 02:28:48, kinuko wrote:
> nit: we initialize this differently in ctors, it's probably not the best use
of
> in-class member initialization.
> 
> nit: and if that's the case probably we can just leave is_disabled_
> initialization in ctors too (it's optional, but having all initialization in
one
> place is preferred in general)

Done.

[michaeln, 2015-05-15 23:54:24]

What's the plan for existing data, Migrate or DeleteAndStartOver? Probably
should have a plan of attack for that prior to committing this.

[michaeln, 2015-05-15 23:58:44]

On 2015/05/15 23:54:24, michaeln wrote:
> What's the plan for existing data, Migrate or DeleteAndStartOver? Probably
> should have a plan of attack for that prior to committing this.

Ah, just read the TEST annotation, sounds like InitWithDiskBackend fails if the
pre-existing data is in the wrong format. Is that right or do we discover a
failure further downstream from there?

[nhiroki, 2015-05-16 00:52:24]

On 2015/05/15 23:58:44, michaeln wrote:
> On 2015/05/15 23:54:24, michaeln wrote:
> > What's the plan for existing data, Migrate or DeleteAndStartOver? Probably
> > should have a plan of attack for that prior to committing this.

kinuko@ and I think that DeleteAndStartOver would be preferable because
DiskCache migration looks so tough and could cause further corruption.

> Ah, just read the TEST annotation, sounds like InitWithDiskBackend fails if
the
> pre-existing data is in the wrong format. Is that right or do we discover a
> failure further downstream from there?

Right. When navigation happens...

(1) InitWithDiskBackend fails to open an pre-existing data due to incompatible
format (in both default->simple and simple->default cases),
(2) ServiceWorkerStorage detects it and schedules DeleteAndStartOver, and
(3) navigation falls back to network.

[michaeln, 2015-05-16 01:03:56]

> Right. When navigation happens...
> 
> (1) InitWithDiskBackend fails to open an pre-existing data due to incompatible
> format (in both default->simple and simple->default cases),
> (2) ServiceWorkerStorage detects it and schedules DeleteAndStartOver, and
> (3) navigation falls back to network.

sgtm!

[nhiroki, 2015-05-16 01:23:47]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2015-05-16 01:23:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1140853002/#ps230001
(title: "rebase")

[commit-bot: I haz the power, 2015-05-16 01:25:08]

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

[commit-bot: I haz the power, 2015-05-16 03:12:54]

Committed patchset #7 (id:230001)

[commit-bot: I haz the power, 2015-05-18 11:31:15]

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

[nhiroki, 2015-05-19 09:47:13]

A revert of this CL (patchset #7 id:230001) has been created in
https://codereview.chromium.org/1142193002/ by nhiroki@chromium.org.

The reason for reverting is: We'll reland this CL with DiskCache migration
code..