[CacheStorage] Garbage collect unreferenced caches

[CacheStorage] Garbage collect unreferenced caches

If a cache fails to be deleted for some reason on CacheStorage.delete,
then its directory may stick around even though it can no longer be
referenced. This CL adds a garbage collection step on CacheStorage
initialization that deletes any unreferenced caches.

BUG=547110
Committed: https://crrev.com/1b8c48882a6b8e754bd10aa1e4af7f2393c96d60
Cr-Commit-Position: refs/heads/master@{#356065}

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

Description was changed from

==========
[CacheStorage] Garbage collect unreferenced directories

If a cache fails to be deleted for some reason on CacheStorage.delete,
then its directory may stick around even though it can no longer be
referenced. This CL adds a garbage collection step on CacheStorage
initialization that deletes any unreferenced caches.

BUG=547110
==========

to

==========
[CacheStorage] Garbage collect unreferenced directories

If a cache fails to be deleted for some reason on CacheStorage.delete,
then its directory may stick around even though it can no longer be
referenced. This CL adds a garbage collection step on CacheStorage
initialization that deletes any unreferenced caches.

BUG=547110
==========

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

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

[jkarlin, 2015-10-23 17:21:35]

jsbell: PTAL, many thanks!

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

Description was changed from

==========
[CacheStorage] Garbage collect unreferenced directories

If a cache fails to be deleted for some reason on CacheStorage.delete,
then its directory may stick around even though it can no longer be
referenced. This CL adds a garbage collection step on CacheStorage
initialization that deletes any unreferenced caches.

BUG=547110
==========

to

==========
[CacheStorage] Garbage collect unreferenced caches

If a cache fails to be deleted for some reason on CacheStorage.delete,
then its directory may stick around even though it can no longer be
referenced. This CL adds a garbage collection step on CacheStorage
initialization that deletes any unreferenced caches.

BUG=547110
==========

[jsbell, 2015-10-23 21:20:22]

lgtm

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

https://codereview.chromium.org/1421863002/diff/20001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:348: // TODO(jkarlin): Delete
caches that are in the directory and not returned
Remove this TODO?

https://codereview.chromium.org/1421863002/diff/20001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:362: base::FileEnumerator
file_enum(cache_base_dir, false /* recursive */,
I'm suddenly paranoid that the docs for FileEnumerator don't actually state that
it's not invalidated by changes to the directory, but we use this pattern
(FileEnumerator + DeleteFile) all over the code base, so ... I guess it must be
okay.

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

Patchset #3 (id:40001) has been deleted

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

Thanks!

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

https://codereview.chromium.org/1421863002/diff/20001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:348: // TODO(jkarlin): Delete
caches that are in the directory and not returned
On 2015/10/23 21:20:22, jsbell wrote:
> Remove this TODO?

Thanks! The whole purpose of the CL was to remove that TODO. Done.

https://codereview.chromium.org/1421863002/diff/20001/content/browser/cache_s...
content/browser/cache_storage/cache_storage.cc:362: base::FileEnumerator
file_enum(cache_base_dir, false /* recursive */,
On 2015/10/23 21:20:22, jsbell wrote:
> I'm suddenly paranoid that the docs for FileEnumerator don't actually state
that
> it's not invalidated by changes to the directory, but we use this pattern
> (FileEnumerator + DeleteFile) all over the code base, so ... I guess it must
be
> okay.

No longer deletes while iterating, just to be safe.

[jsbell, 2015-10-23 23:45:33]

still lgtm

(and sorry, didn't mean to scare you away from enum+delete, just thought I
should check. Again, the pattern is used in many places so hopefully we'd have
noticed if it wasn't safe...)

[jkarlin, 2015-10-26 16:14:11]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2015-10-26 16:14:17]

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

[commit-bot: I haz the power, 2015-10-26 17:09:49]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-10-26 17:10:30]

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