New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 542668 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

InvalidAccessError when recreating old caches

Project Member Reported by jakearchibald@chromium.org, Oct 13 2015

Issue description

On Windows (confirmed in Win 7 and Win 8.1) Canary Version 48.0.2533.0:

https://blog.wanderview.com/delta-cache/

* Click "Load v1 Resources"
* Click "Load v2 Resources"
* Click "Load v3 Resources"
* Click "Load v2 Resources"

Error is InvalidAccessError.
 
Screen Shot 2015-10-13 at 14.16.26.png
95.4 KB View Download

Comment 1 by bke...@mozilla.com, Oct 13 2015

The promise rejection is coming from the cache.put() call here:

  https://github.com/wanderview/delta-cache/blob/gh-pages/main.js#L40

Note, this does not reproduce on mac.

Comment 2 by bke...@mozilla.com, Oct 13 2015

I tried creating a reduced test case by just adding an entry in the cache, deleting the cache, and then re-creating the cache.  That did not have a problem, though.  Its unclear to me exactly what the trigger conditions are here.

Comment 3 by jsb...@chromium.org, Oct 14 2015

Cc: jsb...@chromium.org
Owner: jkarlin@chromium.org
Status: Assigned
I think this is the same as what we're seeing in  issue 510002 

Also noted on StackOverflow: http://stackoverflow.com/questions/32669605/what-is-domexception-entry-already-exists

Comment 4 by jkarlin@google.com, Oct 15 2015

Thanks for the easy to reproduce bug report!

Definitely a problem with browser code. The cache deletion (clear all resources) is failing silently. It removes the cache name from the index but the cache's directory isn't being deleted from disk. Then, when the cache is created again the index gets updated and the cache opens, but when you try to add a new entry it is surprised to see that it's already on disk and returns an error.

Comment 5 by bke...@mozilla.com, Oct 15 2015

Ah.  I guess its the old "windows fails to remove files if something has a handle open" problem.  I know I spent a long time on this in gecko.  Kind of tricky since content may still hold a Cache or cache-based Response object alive when caches.delete() is executed.
Cc: jkarlin@chromium.org
Labels: -Pri-2 Pri-1
Owner: gavinp@chromium.org
#5 yes, I think that's the issue.

After https://codereview.chromium.org/1108083002/ landed the blob response to a cache match changed from being a memory copy of the file to a streamable handle to a simple cache entry (which is backed by a file handle on disk). This means that if the cache is deleted while one of these blobs is still in existence then the file handle will still be open and deleting the cache's directory on Windows will fail. This is most likely the same underlying issue that is causing windows layout test flakes: https://code.google.com/p/chromium/issues/detail?id=510002. 

The ScopedEntries need to be held by the CacheStorageCache instead of the blob entries so that they can be deleted before the cache is deleted. This leads to ref counted handles being held by the blobs. If a blob-backed Response is read after the underlying cache is deleted it makes sense that it will fail.

gavinp@ assigning to you as this is follow-on work to https://codereview.chromium.org/1108083002/. Feel free to assign back if you don't think you'll be able to get to it.

Comment 7 by bke...@mozilla.com, Oct 16 2015

> If a blob-backed Response is read after the underlying cache is deleted it makes sense that it will fail.

FWIW, we came to the conclusion these response objects should continue to work when I asked the spec editors:

https://github.com/slightlyoff/ServiceWorker/issues/538

Although I agree failing them is better than failing a later cache.put().

Comment 8 by bke...@mozilla.com, Oct 16 2015

And I guess the spec now explicitly says the Response should remain functional:

https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#cache-storage-delete
Thanks for the link to the spec update. So now I see two different changes that need to be made:

1) A Cache needs to be doomable so that when its last Request/Response/Cache reference is gone it can delete itself instead of being deleted as soon as delete is called. That brings us up to spec.

2) Every cache's directory should be unique so that two caches with the same name (an old doomed one and a newly created one) don't conflict on the filesystem. That will solve this issue.

Assigning back to me.
Cc: -jkarlin@chromium.org gavinp@chromium.org
Owner: jkarlin@chromium.org
Yep, that is indeed the issue. Reproduced with a unittest.

https://codereview.chromium.org/1410543003/
w00t!

Comment 13 by bke...@mozilla.com, Oct 20 2015

You might also want to consider testing this related, but slightly different, condition.  I'm not sure the plan you sketched out in comment 9 would cover it.

var cache;
var response;
caches.open('foo').then(function(c) {
  cache = c;
  return cache.put(url, new Response('hello world'));
}).then(function() {
  return cache.match(url);
}).then(function(r) {
  response = r;
  return cache.delete(url);
}).then(function(result) {
  // result === true
  return response.text();
}).then(function(text) {
  // text === 'hello world'
});

So, ensuring the Response is still alive after cache.delete() even if the cache itself has not been doomed yet.
We'd be fine there. The cache would be doomed (which in this case means disallow new references to it and delete it once the last reference is gone) on the call to cache.delete so the existing reference would continue to work.

Unless there is something tricky in there that I missed?

Comment 15 by bke...@mozilla.com, Oct 20 2015

How would the cache be doomed in the example code in comment 13?  It calls Cache.delete(), not CacheStorage.delete().

Or maybe I misunderstand your internal implementation terminology.
Oh sorry, that was the part that I missed. The cache entry was deleted not the cache itself. Yes, that's internally already taken care of. The cache dooms its entries on delete.

Comment 17 by bke...@mozilla.com, Oct 20 2015

Great!  Sorry for the noise.  Thanks again for looking at this.
Thank you for the super helpful report! We were running into this in cryptic layout test flakes. Your example makes it all very clear.
Project Member

Comment 19 by bugdroid1@chromium.org, Oct 26 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4519a22b4c1683f1e79543a4238bc5cdb1880b5e

commit 4519a22b4c1683f1e79543a4238bc5cdb1880b5e
Author: jkarlin <jkarlin@chromium.org>
Date: Mon Oct 26 16:03:27 2015

[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 

Review URL: https://codereview.chromium.org/1414033002

Cr-Commit-Position: refs/heads/master@{#356055}

[modify] http://crrev.com/4519a22b4c1683f1e79543a4238bc5cdb1880b5e/content/browser/cache_storage/cache_storage.cc
[modify] http://crrev.com/4519a22b4c1683f1e79543a4238bc5cdb1880b5e/content/browser/cache_storage/cache_storage.proto
[modify] http://crrev.com/4519a22b4c1683f1e79543a4238bc5cdb1880b5e/content/browser/cache_storage/cache_storage_cache.h
[modify] http://crrev.com/4519a22b4c1683f1e79543a4238bc5cdb1880b5e/content/browser/cache_storage/cache_storage_manager.h
[modify] http://crrev.com/4519a22b4c1683f1e79543a4238bc5cdb1880b5e/content/browser/cache_storage/cache_storage_manager_unittest.cc
[modify] http://crrev.com/4519a22b4c1683f1e79543a4238bc5cdb1880b5e/content/content_tests.gypi
[modify] http://crrev.com/4519a22b4c1683f1e79543a4238bc5cdb1880b5e/content/test/BUILD.gn

Resolve fixed? Also resolve  issue 510002 , or wait and see if the flakiness is resolved?
Status: Fixed
Labels: Merge-Request-47
Requesting to merge https://codereview.chromium.org/1414033002 and https://codereview.chromium.org/1428463002/ (a fix to the prior CL) into 47. They've been in trunk since Oct. 26th.

Comment 23 by tin...@google.com, Nov 2 2015

Labels: -Merge-Request-47 Merge-Approved-47 Hotlist-Merge-Approved
Congrats your change is auto-approved for M47 (branch: 2526)
Project Member

Comment 24 by bugdroid1@chromium.org, Nov 2 2015

Labels: -Merge-Approved-47 merge-merged-2526
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/26e40aa877846f7208e63860728fff703c40a122

commit 26e40aa877846f7208e63860728fff703c40a122
Author: Josh Karlin <jkarlin@chromium.org>
Date: Mon Nov 02 13:58:46 2015

[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 
TBR=jochen, jsbell

There are two CLs in this merge as the second fixes a bug in the first.

Review URL: https://codereview.chromium.org/1414033002
Review URL: https://codereview.chromium.org/1428463002

Cr-Commit-Position: refs/heads/master@{#356055}
(cherry picked from commit 4519a22b4c1683f1e79543a4238bc5cdb1880b5e)
Cr-Commit-Position: refs/heads/master@{#356107}
(cherry picked from commit b65cfea2a48ce74ac98e2b10006e9d8b868d4553)

Review URL: https://codereview.chromium.org/1422363004 .

Cr-Commit-Position: refs/branch-heads/2526@{#295}
Cr-Branched-From: cb947c0153db0ec02a8abbcb3ca086d88bf6006f-refs/heads/master@{#352221}

[modify] http://crrev.com/26e40aa877846f7208e63860728fff703c40a122/content/browser/cache_storage/cache_storage.cc
[modify] http://crrev.com/26e40aa877846f7208e63860728fff703c40a122/content/browser/cache_storage/cache_storage.proto
[modify] http://crrev.com/26e40aa877846f7208e63860728fff703c40a122/content/browser/cache_storage/cache_storage_cache.h
[modify] http://crrev.com/26e40aa877846f7208e63860728fff703c40a122/content/browser/cache_storage/cache_storage_manager.h
[modify] http://crrev.com/26e40aa877846f7208e63860728fff703c40a122/content/browser/cache_storage/cache_storage_manager_unittest.cc
[modify] http://crrev.com/26e40aa877846f7208e63860728fff703c40a122/content/content_tests.gypi
[modify] http://crrev.com/26e40aa877846f7208e63860728fff703c40a122/content/test/BUILD.gn

Project Member

Comment 25 by bugdroid1@chromium.org, Nov 2 2015

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/bling/chromium.git/+/26e40aa877846f7208e63860728fff703c40a122

commit 26e40aa877846f7208e63860728fff703c40a122
Author: Josh Karlin <jkarlin@chromium.org>
Date: Mon Nov 02 13:58:46 2015

Cc: dmu...@chromium.org asanka@chromium.org jkarlin@chromium.org
 Issue 435694  has been merged into this issue.

Sign in to add a comment