1110446 - implement DB maintenance and recovery for ServiceWorker Cache

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 1

[(no longer active)]

I'm removing the morgue directory in bug 1133763.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Create marker files when Cache API context is open. r=ehsan

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.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Create marker files when Cache API context is open. r=ehsan

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

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Cleanup stale caches/bodies if last session didn't shutdown cleanly. r=ehsan

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

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Add a test that forces a Cache object to be orphaned and reclaimed. r=ehsan

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

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Cleanup stale caches/bodies if last session didn't shutdown cleanly. r=ehsan

Fixed the code to find orphaned bodies.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Add a test that orphanes Cache API body files. r=ehsan

Test that exercises the orphaned body case.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0991db72c7fd

Comment 10

[(no longer active)]

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 11

[(no longer active)]

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 12

[(no longer active)]

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 13

[(no longer active)]

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.

Comment 14

[Pulsebot]

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

Comment 15

[Carsten Book [:Tomcat]]

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