After the Cache is moved to mozilla-central from maple, we need to implement the maintenance and recovery bits. This means: * executing VACUUM when appropriate * detecting orphaned cache objects and body files after crashes

I'm removing the morgue directory in bug 1133763.

For v1 I think we should probably punt on a fancy VACUUM system and just use auto-vacuum. It will fragment the disk more, but should be adequate for the initial version. We should write a follow-up bug for examining VACUUM performance in Q2 if we go this way.

Step one is placing a marker in the cache directory while the context is open. If the marker is present on first access, then we know a crash occurred during the last usage of Cache. The marker is also left if a forced shutdown on the quota directory causes us to orphan data.

Ehsan, I believe you indicated you were willing to review the patches for this bug. We can talk next week in person about it.

This performs the actual orphan detection and cleanup work when the marker is found on setup.

This test forces a Cache object to be orphaned and then verifies its cleaned up on next origin setup.

Comment on attachment 8624929 [details] [diff] [review] P2 Cleanup stale caches/bodies if last session didn't shutdown cleanly. r=ehsan I found some bugs in the patch that I need to fix. Basically the code to find the orphaned bodies in the db was just wrong.

Fixed the code to find orphaned bodies.

Test that exercises the orphaned body case. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0991db72c7fd

Comment on attachment 8624927 [details] [diff] [review] P1 Create marker files when Cache API context is open. r=ehsan Review of attachment 8624927 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/Manager.cpp @@ +1487,5 @@ > + // Before forgetting the Context, check to see if we have any outstanding > + // cache or body objects waiting for deletion. If so, note that we've > + // orphaned data so it will be cleaned up on the next open. > + for (uint32_t i = 0; i < mCacheIdRefs.Length(); ++i) { > + if(mCacheIdRefs[i].mOrphaned) { Nit: whitespace, same in a few places below.

Comment on attachment 8625804 [details] [diff] [review] P2 Cleanup stale caches/bodies if last session didn't shutdown cleanly. r=ehsan Review of attachment 8625804 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/FileUtils.cpp @@ +341,5 @@ > + rv = dir->GetDirectoryEntries(getter_AddRefs(entries)); > + if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > + > + // Iterate over all the intermediate morgue subdirs > + bool hasMore; Nit: please initialize all out arguments on the stack. @@ +355,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > + > + // If a file got in here somehow, just ignore it > + if (NS_WARN_IF(!isDir)) { > + continue; Should we perhaps delete such files? I don't feel strongly either way... @@ +377,5 @@ > + rv = file->GetNativeLeafName(leafName); > + if (NS_WARN_IF(NS_FAILED(rv))) { return rv; } > + > + // Delete all tmp files regardless of known bodies. These are > + // all considered orphans. Similar to the above, we may be dealing with a "foo.tmp" directory here. Should we not care and remove recursively? Or check to see if this is a directory and leave it untouched in that case? @@ +401,5 @@ > + continue; > + } > + > + if (!aKnownBodyIdList.Contains(id)) { > + rv = file->Remove(false /* recursive */); Ditto. (These are obviously all pathological edge cases!) @@ +475,5 @@ > + rv = marker->Append(NS_LITERAL_STRING("cache")); > + if (NS_WARN_IF(NS_FAILED(rv))) { return false; } > + > + rv = marker->Append(NS_LITERAL_STRING("context_open.marker")); > + if (NS_WARN_IF(NS_FAILED(rv))) { return false; } Should we refactor these bits into a helper?

Comment on attachment 8624930 [details] [diff] [review] P3 Add a test that forces a Cache object to be orphaned and reclaimed. r=ehsan Review of attachment 8624930 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: dom/cache/test/mochitest/test_cache_orphaned_cache.html @@ +94,5 @@ > + }).then(function(usage) { > + is(0, usage, 'disk usage should be zero to start'); > + > + // Initialize and populate an initial cache to get the base sqlite pages > + // and directory structure allocated. Nit: indentation of comments in this file is wrong.

Comment on attachment 8625805 [details] [diff] [review] P4 Add a test that orphanes Cache API body files. r=ehsan Review of attachment 8625805 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/cache/test/mochitest/test_cache_orphaned_body.html @@ +95,5 @@ > + }).then(function(usage) { > + is(0, usage, 'disk usage should be zero to start'); > + > + // Initialize and populate an initial cache to get the base sqlite pages > + // and directory structure allocated. Ditto.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f5b3cfa8649f https://hg.mozilla.org/integration/mozilla-inbound/rev/b89856b358ea https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f10e13ec41 https://hg.mozilla.org/integration/mozilla-inbound/rev/331d60f2c42e

https://hg.mozilla.org/mozilla-central/rev/f5b3cfa8649f https://hg.mozilla.org/mozilla-central/rev/b89856b358ea https://hg.mozilla.org/mozilla-central/rev/a9f10e13ec41 https://hg.mozilla.org/mozilla-central/rev/331d60f2c42e