[CacheStorage] Give cache directories unique names

[CacheStorage] Give cache directories unique names

The CacheStorage class creates a directory for each cache based off of
a hash of the cache's name. This is problematic in situations where a
cache is deleted and then recreated (but not yet wiped from
disk as javascript might be referencing it).

This CL gives every new cache a unique directory and stores the directory
mapping in the cache storage index protobuf. Legacy caches are moved to
new directories on CacheStorage initialization.

BUG=542668
Committed: https://crrev.com/4519a22b4c1683f1e79543a4238bc5cdb1880b5e
Cr-Commit-Position: refs/heads/master@{#356055}

[jkarlin, 2015-10-22 14:51:46]

Description was changed from

==========
[CacheStorage] Give cache directories random names

The CacheStorage class creates a directory for each cache based off of
a hash of the cache's name. This is problematic in situations where a
cache is deleted and then recreated (but not yet wiped from
disk). Instead, give every cache a unique directory and store the
directory mapping in the cache storage index protobuf.

BUG=542668
==========

to

==========
[CacheStorage] Give cache directories random names

The CacheStorage class creates a directory for each cache based off of
a hash of the cache's name. This is problematic in situations where a
cache is deleted and then recreated (but not yet wiped from
disk as javascript might be referencing it). Instead, give every new 
cache a unique directory and store the directory mapping in the cache 
storage index protobuf. Legacy caches will continue to use the hashed
directory.

BUG=542668
==========

[jkarlin, 2015-10-22 15:08:39]

Description was changed from

==========
[CacheStorage] Give cache directories random names

The CacheStorage class creates a directory for each cache based off of
a hash of the cache's name. This is problematic in situations where a
cache is deleted and then recreated (but not yet wiped from
disk as javascript might be referencing it). Instead, give every new 
cache a unique directory and store the directory mapping in the cache 
storage index protobuf. Legacy caches will continue to use the hashed
directory.

BUG=542668
==========

to

==========
[CacheStorage] Give cache directories random names

The CacheStorage class creates a directory for each cache based off of
a hash of the cache's name. This is problematic in situations where a
cache is deleted and then recreated (but not yet wiped from
disk as javascript might be referencing it).

This CL gives every new cache a unique directory and stores the directory
mapping in the cache storage index protobuf. Legacy caches will continue
to use the hashed directory.

BUG=542668
==========

[jkarlin, 2015-10-22 15:13:50]

jkarlin@chromium.org changed reviewers:
+ jsbell@chromium.org

[jkarlin, 2015-10-22 15:13:50]

jsbell: PTAL, thanks!

[jsbell, 2015-10-22 16:38:52]

lgtm with a few minor suggestions.

What's the long term plan for removing the legacy support? Should we migrate
legacy caches to new guid-based directory names/entries on open?

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.cc (right):

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:188:
DCHECK(cache_name_to_cache_dir_.find(cache_name) !=
Could use ContainsKey() from base/stl_util

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:210: static std::string
PrepareNewCacheDirectoryInPool(
Add comment that this is run on the cache_task_runner?

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:237:
DCHECK(cache_name_to_cache_dir_.find(cache_name) !=
Could use ContainsKey()

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:269:
DCHECK(cache_name_to_cache_dir_.find(cache_names[i]) !=
ContainsKey()

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_manager_unittest.cc:769:
EXPECT_TRUE(Delete(origin1_, legacy_cache_name_));
This exercises the failure seen on Windows, right?

SetUp() implicitly opens the cache, held in callback_cache_
Delete() deletes it but callback_cache_ still keeps it alive
Open() will only overwrite callback_cache_ after it succeeds

Can you add a note or an EXPECT/ASSERT after the Delete() that callback_cache_
is still populated?

[jkarlin, 2015-10-23 17:23:09]

Thanks for your comments. PTAL at the changes.

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage.cc (right):

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:188:
DCHECK(cache_name_to_cache_dir_.find(cache_name) !=
On 2015/10/22 16:38:52, jsbell wrote:
> Could use ContainsKey() from base/stl_util

Done, thanks! Forgot about that function.

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:210: static std::string
PrepareNewCacheDirectoryInPool(
On 2015/10/22 16:38:52, jsbell wrote:
> Add comment that this is run on the cache_task_runner?

Done.

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:237:
DCHECK(cache_name_to_cache_dir_.find(cache_name) !=
On 2015/10/22 16:38:52, jsbell wrote:
> Could use ContainsKey()

Done.

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:269:
DCHECK(cache_name_to_cache_dir_.find(cache_names[i]) !=
On 2015/10/22 16:38:52, jsbell wrote:
> ContainsKey()

Done.

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
File content/browser/cache_storage/cache_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1414033002/diff/80001/content/browser/cache_s...
content/browser/cache_storage/cache_storage_manager_unittest.cc:769:
EXPECT_TRUE(Delete(origin1_, legacy_cache_name_));
On 2015/10/22 16:38:52, jsbell wrote:
> This exercises the failure seen on Windows, right?

No, but I've since added the test StorageReuseCacheName which does.
> 
> SetUp() implicitly opens the cache, held in callback_cache_
> Delete() deletes it but callback_cache_ still keeps it alive
> Open() will only overwrite callback_cache_ after it succeeds

I don't believe that callback_cache_ keeps it alive, as the simplecache doesn't
leave any handles open unless an entry is open. StorageReuseCacheName does
exactly that.

> 
> Can you add a note or an EXPECT/ASSERT after the Delete() that callback_cache_
> is still populated?

This doesn't seem necessary given the above.

[jkarlin, 2015-10-23 17:24:27]

Forgot to mention, the newest code migrates the hexed directories to UUID
directories.

[jkarlin, 2015-10-23 17:49:18]

Description was changed from

==========
[CacheStorage] Give cache directories random names

The CacheStorage class creates a directory for each cache based off of
a hash of the cache's name. This is problematic in situations where a
cache is deleted and then recreated (but not yet wiped from
disk as javascript might be referencing it).

This CL gives every new cache a unique directory and stores the directory
mapping in the cache storage index protobuf. Legacy caches will continue
to use the hashed directory.

BUG=542668
==========

to

==========
[CacheStorage] Give cache directories unique names

The CacheStorage class creates a directory for each cache based off of
a hash of the cache's name. This is problematic in situations where a
cache is deleted and then recreated (but not yet wiped from
disk as javascript might be referencing it).

This CL gives every new cache a unique directory and stores the directory
mapping in the cache storage index protobuf. Legacy caches will continue
to use the hashed directory.

BUG=542668
==========

[jsbell, 2015-10-23 21:09:59]

Awesome, lgtm

Can you update the CL description to indicate that old caches are migrated?

This will also make it much easier to fix crbug.com/432746

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
File content/browser/cache_storage/cache_storage.cc (right):

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
content/browser/cache_storage/cache_storage.cc:51: class
MigratedLegacyCacheDirectoryNameTest;
Is this actually needed? (forward decls for friends do not appear to be
necessary if same namespace)

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
content/browser/cache_storage/cache_storage.cc:366: // find a new home for the
cache.
Nit: Capitalize.

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
content/browser/cache_storage/cache_storage.cc:377: return "";
At this point, some caches may have been moved so the index is bogus. It looks
like returning a non-parsable string would then run into the "Delete caches that
are in the directory and not returned" TODO above?

Maybe add a comment here?

And/or possibly define a const std::string error(""); up above to make the
various 'return ""' more self-documenting?

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_manager.h (right):

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
content/browser/cache_storage/cache_storage_manager.h:34: class
MigratedLegacyCacheDirectoryNameTest;
Ditto - needed?

[jkarlin, 2015-10-23 22:39:40]

Description was changed from

==========
[CacheStorage] Give cache directories unique names

The CacheStorage class creates a directory for each cache based off of
a hash of the cache's name. This is problematic in situations where a
cache is deleted and then recreated (but not yet wiped from
disk as javascript might be referencing it).

This CL gives every new cache a unique directory and stores the directory
mapping in the cache storage index protobuf. Legacy caches will continue
to use the hashed directory.

BUG=542668
==========

to

==========
[CacheStorage] Give cache directories unique names

The CacheStorage class creates a directory for each cache based off of
a hash of the cache's name. This is problematic in situations where a
cache is deleted and then recreated (but not yet wiped from
disk as javascript might be referencing it).

This CL gives every new cache a unique directory and stores the directory
mapping in the cache storage index protobuf. Legacy caches are moved to
new directories on CacheStorage initialization.

BUG=542668
==========

[jkarlin, 2015-10-23 22:57:31]

Patchset #13 (id:230001) has been deleted

[jkarlin, 2015-10-23 22:57:41]

Thanks!

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
File content/browser/cache_storage/cache_storage.cc (right):

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
content/browser/cache_storage/cache_storage.cc:51: class
MigratedLegacyCacheDirectoryNameTest;
On 2015/10/23 21:09:59, jsbell wrote:
> Is this actually needed? (forward decls for friends do not appear to be
> necessary if same namespace)

Done.

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
content/browser/cache_storage/cache_storage.cc:366: // find a new home for the
cache.
On 2015/10/23 21:09:59, jsbell wrote:
> Nit: Capitalize.

Done.

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
content/browser/cache_storage/cache_storage.cc:377: return "";
On 2015/10/23 21:09:59, jsbell wrote:
> At this point, some caches may have been moved so the index is bogus. It looks
> like returning a non-parsable string would then run into the "Delete caches
that
> are in the directory and not returned" TODO above?
> 
> Maybe add a comment here?
> 
> And/or possibly define a const std::string error(""); up above to make the
> various 'return ""' more self-documenting?

Done.

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
File content/browser/cache_storage/cache_storage_manager.h (right):

https://codereview.chromium.org/1414033002/diff/210001/content/browser/cache_...
content/browser/cache_storage/cache_storage_manager.h:34: class
MigratedLegacyCacheDirectoryNameTest;
On 2015/10/23 21:09:59, jsbell wrote:
> Ditto - needed?

Done.

[jkarlin, 2015-10-23 23:10:56]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2015-10-23 23:10:56]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1414033002/#ps250001
(title: "Address comments from PS12")

[commit-bot: I haz the power, 2015-10-23 23:11:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414033002/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414033002/250001

[commit-bot: I haz the power, 2015-10-23 23:22:12]

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

[commit-bot: I haz the power, 2015-10-23 23:22:12]

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

[jkarlin, 2015-10-23 23:46:27]

jkarlin@chromium.org changed reviewers:
+ sky@chromium.org

[jkarlin, 2015-10-23 23:46:28]

sky@chromium.org: Please review changes in content/test/BUILD.gn, thanks!

[jkarlin, 2015-10-26 14:17:01]

jkarlin@chromium.org changed reviewers:
+ jochen@chromium.org
- sky@chromium.org

[jkarlin, 2015-10-26 14:18:46]

jochen: Please review changes in content/test/BUILD.gn, thanks!

[sky, 2015-10-26 15:06:50]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-10-26 15:06:50]

LGTM

[jkarlin, 2015-10-26 15:09:24]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2015-10-26 15:09:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414033002/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414033002/250001

[jochen (gone - plz use gerrit), 2015-10-26 15:26:20]

lgtm

[commit-bot: I haz the power, 2015-10-26 16:03:34]

Committed patchset #13 (id:250001)

[commit-bot: I haz the power, 2015-10-26 16:04:15]

Patchset 13 (id:??) landed as
https://crrev.com/4519a22b4c1683f1e79543a4238bc5cdb1880b5e
Cr-Commit-Position: refs/heads/master@{#356055}