Use "database identifier" rather than raw origin as directory hash input

Use "database identifier" rather than raw origin as directory hash input

When we allow non-Service Workers to have access to the Cache Storage
API we will want to match other storage systems and use a database
identifier generated from a Blink SecurityOrigin object, rather than
just the origin URL, as the key for per-origin storage.

This change changes the computation of the directory name on disk (a
hash) from using the origin to a database identifier, in anticipation
of an identifier being passed in after some future CLs.

Existing data is preserved by preceding the uses of the directory name
(open, delete, or enumeration) with a directory rename from the
"legacy" name computed from the origin to the "new" name computed from
the directory identifier. The rename is done by posting a task to the
cache task runner just before the actual task. After a few releases we
remove the migration code.

R=michaeln,jkarlin
BUG=439389
Committed: https://crrev.com/e82f0728287f968b27d9fc9e54153df145a9fe1c
Cr-Commit-Position: refs/heads/master@{#322055}

[jsbell, 2015-03-20 22:03:22]

jkarlin, michaeln - what do you think about this?

I'm thinking: land this, then tweak https://codereview.chromium.org/992353003/
to pass identifiers through instead. (To compute the "legacy" path, the
identifier can be converted back to an origin.)

[jsbell, 2015-03-20 22:32:46]

Actually, maybe I'll leave the other CL alone for now so I don't touch
CacheStorageManager (which still reasons about origins) at all. Instead I'll do
a follow-up, even though it touches messages the intent of the change will be
much clearer.

[jkarlin, 2015-03-23 15:06:24]

Please add tests.

[michaeln, 2015-03-23 19:38:53]

> as the key for per-origin storage

Although we probably should, I don't think we consistently use that as the key
for everything.

https://codereview.chromium.org/1020413002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_cache_storage_manager.cc
(right):

https://codereview.chromium.org/1020413002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_cache_storage_manager.cc:376:
base::Move(old_path, new_path);
An odd error condition might be able to arise?

Suppose Move() fails and the program continues to generate/poplulate a directory
for the new path. Next time migrate gets called, the oldstuff would overwrite
the newstuff (assuming Move doesn't fail that time around). Idk?

 if (oldExists)
  if (!newExists)
    Move(old, new);
  Delete(old);

[jsbell, 2015-03-23 22:39:59]

https://codereview.chromium.org/1020413002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_cache_storage_manager.cc
(right):

https://codereview.chromium.org/1020413002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_cache_storage_manager.cc:376:
base::Move(old_path, new_path);
On 2015/03/23 19:38:53, michaeln wrote:
> An odd error condition might be able to arise?
> 
> Suppose Move() fails and the program continues to generate/poplulate a
directory
> for the new path. Next time migrate gets called, the oldstuff would overwrite
> the newstuff (assuming Move doesn't fail that time around). Idk?
> 
>  if (oldExists)
>   if (!newExists)
>     Move(old, new);
>   Delete(old);
> 

Good idea. Done and added test.

[jsbell, 2015-03-23 22:40:16]

Added tests for migration cases. PTAL?

[michaeln, 2015-03-23 23:53:33]

lgtm!

https://codereview.chromium.org/1020413002/diff/40001/content/browser/service...
File
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc
(right):

https://codereview.chromium.org/1020413002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc:580:
// Populate a cache, the move it to the "legacy" location
spelling "the" s/b "then"

[jsbell, 2015-03-24 00:18:37]

Thanks! jkarlin@ - I'll wait for your go-ahead.

https://codereview.chromium.org/1020413002/diff/40001/content/browser/service...
File
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc
(right):

https://codereview.chromium.org/1020413002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc:580:
// Populate a cache, the move it to the "legacy" location
On 2015/03/23 23:53:33, michaeln wrote:
> spelling "the" s/b "then"

Fixed!

[jkarlin, 2015-03-24 18:58:54]

lgtm but note the comment below

https://codereview.chromium.org/1020413002/diff/60001/content/browser/service...
File
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc
(right):

https://codereview.chromium.org/1020413002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc:590:
ASSERT_TRUE(Open(origin1_, cache2_));
FYI callback_cache_ holds a refptr to cache2_'s cache after this. So you're
moving a live cache around. I don't think that's intended, and I don't believe
you can run into this in operation?

[jsbell, 2015-03-24 19:03:07]

https://codereview.chromium.org/1020413002/diff/60001/content/browser/service...
File
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc
(right):

https://codereview.chromium.org/1020413002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_cache_storage_manager_unittest.cc:590:
ASSERT_TRUE(Open(origin1_, cache2_));
On 2015/03/24 18:58:54, jkarlin wrote:
> FYI callback_cache_ holds a refptr to cache2_'s cache after this. So you're
> moving a live cache around. I don't think that's intended, and I don't believe
> you can run into this in operation?

Ack, good point. I'll clear out callback_cache_ here since there's no need to
keep it open.

[jsbell, 2015-03-24 19:03:52]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2015-03-24 19:03:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1020413002/#ps80001
(title: "Make sure we're not keeping an object reference around")

[commit-bot: I haz the power, 2015-03-24 19:04:48]

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

[commit-bot: I haz the power, 2015-03-24 20:01:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-24 20:03:31]

Patchset 5 (id:??) landed as
https://crrev.com/e82f0728287f968b27d9fc9e54153df145a9fe1c
Cr-Commit-Position: refs/heads/master@{#322055}