implement DB maintenance and recovery for ServiceWorker Cache

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

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.
Depends on: 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.
Depends on: 1142269
Depends on: 1144214
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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.
Attachment #8623411 - Attachment is obsolete: true
Attachment #8624927 - Flags: review?(ehsan)
This performs the actual orphan detection and cleanup work when the marker is found on setup.
Attachment #8624929 - Flags: review?(ehsan)
This test forces a Cache object to be orphaned and then verifies its cleaned up on next origin setup.
Attachment #8624930 - Flags: review?(ehsan)
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.
Attachment #8624929 - Flags: review?(ehsan)
Fixed the code to find orphaned bodies.
Attachment #8624929 - Attachment is obsolete: true
Attachment #8625804 - Flags: review?(ehsan)
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.
Attachment #8624927 - Flags: review?(ehsan) → review+
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?
Attachment #8625804 - Flags: review?(ehsan) → review+
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.
Attachment #8624930 - Flags: review?(ehsan) → review+
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.
Attachment #8625805 - Flags: review?(ehsan) → review+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.