Fix on-disk structure for persistent storage in webview tags.

Fix on-disk structure for persistent storage in webview tags.

BUG=159464
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167334

[awong, 2012-11-08 00:33:21]

Hey Nasko,

Here's the first cut at using a hash. Pretty simple change with a huge comment. 
Looking to see if there are any unittests to fix now.

-Albert

[nasko, 2012-11-08 05:03:54]

Some initial comments. Awesome job on the description of the reasoning!

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
content/browser/storage_partition_impl.h:39: BrowserContext* context, bool
in_memory,
The second parameter should be on a new line.

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
content/browser/storage_partition_impl.h:40: const FilePath& partition_path);
It will be good to put a comment that this is expected to be the absolute path
for the partition, not a relative one.

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:193: // The
kStoragePartitionDirname is contains an extensions directory which is
No need for "is" after the constant name.

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:236: // partition domain.
Awesome comment!

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
content/browser/storage_partition_impl_map.h:48: struct
StoragePartitionDescriptor {
Just FYI, I've renamed the Descriptor to Config in the latest iterations of my
CL.

[awong, 2012-11-09 02:51:22]

Addressed your comments, and started on a unittest, but stopped because I don't
believe the persist: is actually triggering the persist path in the code.

If you get a chance tomorrow morning, do you mind patching this down and seeing
if it makes sense?

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
content/browser/storage_partition_impl.h:39: BrowserContext* context, bool
in_memory,
On 2012/11/08 05:03:54, nasko wrote:
> The second parameter should be on a new line.

Done.

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
content/browser/storage_partition_impl.h:40: const FilePath& partition_path);
On 2012/11/08 05:03:54, nasko wrote:
> It will be good to put a comment that this is expected to be the absolute path
> for the partition, not a relative one.

Done.

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.h (right):

https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
content/browser/storage_partition_impl_map.h:48: struct
StoragePartitionDescriptor {
On 2012/11/08 05:03:54, nasko wrote:
> Just FYI, I've renamed the Descriptor to Config in the latest iterations of my
> CL.

Merged.

[nasko, 2012-11-09 04:19:23]

On 2012/11/09 02:51:22, awong wrote:
> Addressed your comments, and started on a unittest, but stopped because I
don't
> believe the persist: is actually triggering the persist path in the code.
> 
> If you get a chance tomorrow morning, do you mind patching this down and
seeing
> if it makes sense?
> 
>
https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
> File content/browser/storage_partition_impl.h (right):
> 
>
https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
> content/browser/storage_partition_impl.h:39: BrowserContext* context, bool
> in_memory,
> On 2012/11/08 05:03:54, nasko wrote:
> > The second parameter should be on a new line.
> 
> Done.
> 
>
https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
> content/browser/storage_partition_impl.h:40: const FilePath& partition_path);
> On 2012/11/08 05:03:54, nasko wrote:
> > It will be good to put a comment that this is expected to be the absolute
path
> > for the partition, not a relative one.
> 
> Done.
> 
>
https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
> File content/browser/storage_partition_impl_map.h (right):
> 
>
https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_pa...
> content/browser/storage_partition_impl_map.h:48: struct
> StoragePartitionDescriptor {
> On 2012/11/08 05:03:54, nasko wrote:
> > Just FYI, I've renamed the Descriptor to Config in the latest iterations of
my
> > CL.
> 
> Merged.

Yes, persistence is disabled until this change is ready. I've set it always to
be in-memory for webview tags and added a todo in this:
http://codereview.chromium.org/11234032/diff/31046/content/browser/browser_co...

[awong, 2012-11-09 06:43:09]

Blah...I should have remember that. No wonder nothing looks like it works.

On Thu, Nov 8, 2012 at 8:19 PM, <nasko@chromium.org> wrote:

> On 2012/11/09 02:51:22, awong wrote:
>
>> Addressed your comments, and started on a unittest, but stopped because I
>>
> don't
>
>> believe the persist: is actually triggering the persist path in the code.
>>
>
>  If you get a chance tomorrow morning, do you mind patching this down and
>>
> seeing
>
>> if it makes sense?
>>
>
>
> https://codereview.chromium.**org/11366140/diff/2001/**
>
content/browser/storage_**partition_impl.h<https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_partition_impl.h>
>
>> File content/browser/storage_**partition_impl.h (right):
>>
>
>
> https://codereview.chromium.**org/11366140/diff/2001/**
>
content/browser/storage_**partition_impl.h#newcode39<https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_partition_impl.h#newcode39>
>
>> content/browser/storage_**partition_impl.h:39: BrowserContext* context,
>> bool
>> in_memory,
>> On 2012/11/08 05:03:54, nasko wrote:
>> > The second parameter should be on a new line.
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.**org/11366140/diff/2001/**
>
content/browser/storage_**partition_impl.h#newcode40<https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_partition_impl.h#newcode40>
>
>> content/browser/storage_**partition_impl.h:40: const FilePath&
>> partition_path);
>> On 2012/11/08 05:03:54, nasko wrote:
>> > It will be good to put a comment that this is expected to be the
>> absolute
>>
> path
>
>> > for the partition, not a relative one.
>>
>
>  Done.
>>
>
>
> https://codereview.chromium.**org/11366140/diff/2001/**
>
content/browser/storage_**partition_impl_map.h<https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_partition_impl_map.h>
>
>> File content/browser/storage_**partition_impl_map.h (right):
>>
>
>
> https://codereview.chromium.**org/11366140/diff/2001/**
>
content/browser/storage_**partition_impl_map.h#newcode48<https://codereview.chromium.org/11366140/diff/2001/content/browser/storage_partition_impl_map.h#newcode48>
>
>> content/browser/storage_**partition_impl_map.h:48: struct
>> StoragePartitionDescriptor {
>> On 2012/11/08 05:03:54, nasko wrote:
>> > Just FYI, I've renamed the Descriptor to Config in the latest
>> iterations of
>>
> my
>
>> > CL.
>>
>
>  Merged.
>>
>
> Yes, persistence is disabled until this change is ready. I've set it
> always to
> be in-memory for webview tags and added a todo in this:
> http://codereview.chromium.**org/11234032/diff/31046/**
>
content/browser/browser_**context.cc<http://codereview.chromium.org/11234032/diff/31046/content/browser/browser_context.cc>
>
>
https://codereview.chromium.**org/11366140/<https://codereview.chromium.org/1...
>

[awong, 2012-11-10 01:02:32]

Ready for real review.

[awong, 2012-11-10 01:03:03]

On 2012/11/10 01:02:32, awong wrote:
> Ready for real review.

Note, it's missing unittests for the various storage types because I need to get
to work on Data Deletion ASAP.

[nasko, 2012-11-12 16:47:28]

A few minor things and I think it will be ready to go.

https://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/...
File chrome/browser/extensions/web_view_browsertest.cc (right):

https://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:115: 
Why the empty line?

https://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:289: // This tests that in
memory storage partitions are reset on browser restart,
"in-memory" reads better than no dash.

https://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:307:
replace_host.SetHostStr(host_str);
We don't seem to use replace_host in this function.

https://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:310: GURL
set_cookie_url("about:blank");
Why not call it "blank_url" if you aren't going to be setting cookies in the
main browser context?

https://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:383:
IN_PROC_BROWSER_TEST_F(WebViewTest, StoragePersistence) {
A comment that ties this test with the PRE_ will be useful.

https://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:391: cookie_script2.append(
We don't need the above three, right?

https://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/...
chrome/browser/extensions/web_view_browsertest.cc:396:
replace_host.SetHostStr(host_str);
Like in the previous test, this is not used in the function.

https://codereview.chromium.org/11366140/diff/1011/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/web_view_isolation/main.js
(right):

https://codereview.chromium.org/11366140/diff/1011/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/web_view_isolation/main.js:28:
"<object id='webview4' partition='partition1' src=" + url4 +
Thanks for catching this!

https://codereview.chromium.org/11366140/diff/1011/content/browser/storage_pa...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/11366140/diff/1011/content/browser/storage_pa...
content/browser/storage_partition_impl.h:42: // in_memory is true.
partition_path wouldn't be empty, would it? I think at this point we have an
absolute path, so IIRC we should be checking against the base profile path, not
empty.

https://codereview.chromium.org/11366140/diff/1011/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11366140/diff/1011/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:251:
.AppendASCII(partition_domain);
I thought we wanted to avoid the ASCII constraint? Why change that now?

[awong, 2012-11-12 21:36:16]

PTAL

http://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/w...
File chrome/browser/extensions/web_view_browsertest.cc (right):

http://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/w...
chrome/browser/extensions/web_view_browsertest.cc:115: 
On 2012/11/12 16:47:28, nasko wrote:
> Why the empty line?

Done.

http://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/w...
chrome/browser/extensions/web_view_browsertest.cc:289: // This tests that in
memory storage partitions are reset on browser restart,
On 2012/11/12 16:47:28, nasko wrote:
> "in-memory" reads better than no dash.

Done.

http://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/w...
chrome/browser/extensions/web_view_browsertest.cc:307:
replace_host.SetHostStr(host_str);
On 2012/11/12 16:47:28, nasko wrote:
> We don't seem to use replace_host in this function.

Done.

http://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/w...
chrome/browser/extensions/web_view_browsertest.cc:310: GURL
set_cookie_url("about:blank");
On 2012/11/12 16:47:28, nasko wrote:
> Why not call it "blank_url" if you aren't going to be setting cookies in the
> main browser context?

Done.

http://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/w...
chrome/browser/extensions/web_view_browsertest.cc:383:
IN_PROC_BROWSER_TEST_F(WebViewTest, StoragePersistence) {
On 2012/11/12 16:47:28, nasko wrote:
> A comment that ties this test with the PRE_ will be useful.

Done.

http://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/w...
chrome/browser/extensions/web_view_browsertest.cc:391: cookie_script2.append(
On 2012/11/12 16:47:28, nasko wrote:
> We don't need the above three, right?

Done.

http://codereview.chromium.org/11366140/diff/1011/chrome/browser/extensions/w...
chrome/browser/extensions/web_view_browsertest.cc:396:
replace_host.SetHostStr(host_str);
On 2012/11/12 16:47:28, nasko wrote:
> Like in the previous test, this is not used in the function.

Done.

http://codereview.chromium.org/11366140/diff/1011/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/web_view_isolation/main.js
(right):

http://codereview.chromium.org/11366140/diff/1011/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/web_view_isolation/main.js:28:
"<object id='webview4' partition='partition1' src=" + url4 +
On 2012/11/12 16:47:28, nasko wrote:
> Thanks for catching this!

np :)

http://codereview.chromium.org/11366140/diff/1011/content/browser/storage_par...
File content/browser/storage_partition_impl.h (right):

http://codereview.chromium.org/11366140/diff/1011/content/browser/storage_par...
content/browser/storage_partition_impl.h:42: // in_memory is true.
On 2012/11/12 16:47:28, nasko wrote:
> partition_path wouldn't be empty, would it? I think at this point we have an
> absolute path, so IIRC we should be checking against the base profile path,
not
> empty.

Reworded.

http://codereview.chromium.org/11366140/diff/1011/content/browser/storage_par...
File content/browser/storage_partition_impl_map.cc (right):

http://codereview.chromium.org/11366140/diff/1011/content/browser/storage_par...
content/browser/storage_partition_impl_map.cc:251:
.AppendASCII(partition_domain);
On 2012/11/12 16:47:28, nasko wrote:
> I thought we wanted to avoid the ASCII constraint? Why change that now?

Bad merge. Thanks for catching that.

[nasko, 2012-11-12 22:12:50]

LGTM

[awong, 2012-11-12 22:19:34]

OWNERS:
  creis -- content
  kalman -- extensions

[Charlie Reis, 2012-11-12 22:58:19]

(Adding kalman@ to the reviewers list.)

content/ OWNERs LGTM, though I didn't review the actual use of crypto to get the
directory name.  Is it worth getting a storage person's (e.g., michaeln's) eyes
on the proposed scheme?

http://codereview.chromium.org/11366140/diff/14002/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.cc (right):

http://codereview.chromium.org/11366140/diff/14002/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:227: // We the following birthday
attack approximation to caculate the max number
typo: We the
typo: caculate

[not at google - send to devlin, 2012-11-12 22:59:19]

lgtm

[awong, 2012-11-12 23:49:16]

I'm pretty confident this is all good.  We got Ben Laurie to sanity check that
truncating SHA-256 actually yielded uniformly random numbers so the crypto side
is good.  As for whether or not this is good enough for on-disk structure, the
math makes me pretty confident that this is good enough.

I propose we commit as is, and I'll get ericu to sanity check in parallel (he's
out for right now).

http://codereview.chromium.org/11366140/diff/14002/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.cc (right):

http://codereview.chromium.org/11366140/diff/14002/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:227: // We the following birthday
attack approximation to caculate the max number
On 2012/11/12 22:58:20, creis wrote:
> typo: We the
> typo: caculate

Done.

[Charlie Reis, 2012-11-13 00:03:54]

On 2012/11/12 23:49:16, awong wrote:
> I'm pretty confident this is all good.  We got Ben Laurie to sanity check that
> truncating SHA-256 actually yielded uniformly random numbers so the crypto
side
> is good.  As for whether or not this is good enough for on-disk structure, the
> math makes me pretty confident that this is good enough.
> 
> I propose we commit as is, and I'll get ericu to sanity check in parallel
(he's
> out for right now).

Works for me.  Thanks for the diligence on the crypto side.

[commit-bot: I haz the power, 2012-11-13 00:17:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/11366140/14005

[commit-bot: I haz the power, 2012-11-13 09:11:32]

Change committed as 167334

[ericu, 2012-11-14 01:55:19]

http://codereview.chromium.org/11366140/diff/14002/content/browser/storage_pa...
File content/browser/storage_partition_impl_map.cc (right):

http://codereview.chromium.org/11366140/diff/14002/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:195: // for the "default"
extension storage partition and one directory for each
Perhaps: ...directories, one for the "default" extension...

...if I'm reading that right?

http://codereview.chromium.org/11366140/diff/14002/content/browser/storage_pa...
content/browser/storage_partition_impl_map.cc:259:
crypto::SHA256HashString(partition_name, &buffer[0],
Fits on one line?

[akalin, 2012-11-14 07:05:16]

https://codereview.chromium.org/11366140/diff/14002/content/browser/storage_p...
File content/browser/storage_partition_impl_map.cc (right):

https://codereview.chromium.org/11366140/diff/14002/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:219: // We assume that all
partition names within one partition domain are
you probably should add that you're assuming that sha256 gives a
close-to-uniform distribution over its output range, as well that any hash
function which takes a prefix of a sha256 hash also gives a close-to-uniform
distribution.

https://codereview.chromium.org/11366140/diff/14002/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:225: //    p < nroot(1000000,
.99999) ~= 10^-11
i think you mean p < 1 - nroot(...)

I had to look up nroot and think a bit to figure out the notation.  I think it's
clearer as:

p < 1 - 0.99999^(1/1000000)

https://codereview.chromium.org/11366140/diff/14002/content/browser/storage_p...
content/browser/storage_partition_impl_map.cc:230: //    n(p,H) = sqrt(2*H *
ln(1/(1-p)))
from reading the wikipedia article, i think this is a lower bound, i.e.:

n(p, H) > sqrt(...)

which is nice.