Bug 48064 – Quota for IndexedDB should be per origin not per database

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

Quota for IndexedDB should be per origin not per database

[Jeremy Orlow]

Created attachment 71428 [details]
fix

[Jeremy Orlow]

Please review.

[Jeremy Orlow]

Created attachment 71429 [details]
oops

[Andrei Popescu]

Looks good, some small nits below:


> WebCore/manual-tests/indexeddb-persists.html
> var result = event.result.transaction([]).objectStore("test").get("key");

You don't need to pass the [] array. Not passing anything at all is equivalent.

> WebCore/storage/IDBDatabaseBackendImpl.cpp
> if (databaseQuery.prepare() != SQLResultOk)
>        ASSERT_NOT_REACHED();

why this and not (also) return false like we do in other error cases?

>   if (databaseQuery.step() == SQLResultRow)

ditto

>  if (query.prepare() != SQLResultOk)
>         ASSERT_NOT_REACHED();

ditto

>  , m_id(-1)

Make this a constant like we've done everywhere else?

[Eric Seidel (no email)]

Attachment 71429 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4718053

[Jeremy Orlow]

(In reply to comment #4)
> Looks good, some small nits below:
> 
> 
> > WebCore/manual-tests/indexeddb-persists.html
> > var result = event.result.transaction([]).objectStore("test").get("key");
> 
> You don't need to pass the [] array. Not passing anything at all is equivalent.

As specced today, I believe null implies dynamic transaction, which is not what we're looking for here.  Hopefully we get this sorted out next week at TPAC.
 
> > WebCore/storage/IDBDatabaseBackendImpl.cpp
> > if (databaseQuery.prepare() != SQLResultOk)
> >        ASSERT_NOT_REACHED();

Done.
 
> why this and not (also) return false like we do in other error cases?
> 
> >   if (databaseQuery.step() == SQLResultRow)
> 
> ditto

Because this isn't fatal.  It's hardly even worth the check.

> >  if (query.prepare() != SQLResultOk)
> >         ASSERT_NOT_REACHED();
> 
> ditto

Done.
 
> >  , m_id(-1)
> 
> Make this a constant like we've done everywhere else?

Will do.

[Jeremy Orlow]

Created attachment 71761 [details]
Patch

[Andrei Popescu]

(In reply to comment #7)
> Created an attachment (id=71761) [details]
> Patch

LGTM

[Steve Block]

Comment on attachment 71761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71761&action=review

> WebCore/ChangeLog:23
> +

Add a comment about the map by db name not origin?

> WebCore/storage/IDBFactoryBackendImpl.cpp:122
> +    String uniqueIdentifier = fileIdentifier + "_" + name;

Use @ for safety?

[Steve Block]

Comment on attachment 71761 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71761&action=review

>> WebCore/storage/IDBFactoryBackendImpl.cpp:122
>> +    String uniqueIdentifier = fileIdentifier + "_" + name;
> 
> Use @ for safety?

If we allow underscores in the domain name, I think it's unsafe to use an underscore to join the two components, since databaseIdentifier() doesn't escape underscores ...

http://www.foo.com, ".com_80_name" -> http_www.foo.com_80_.com_80_name
http://www.foo.com_80_.com, "name" -> http_www.foo.com_80_.com_80_name

Using '@' avoids the problem as the hostname can't contain an '@'.

[Jeremy Orlow]

Created attachment 71869 [details]
Patch

[Jeremy Orlow]

(In reply to comment #10)
> (From update of attachment 71761 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71761&action=review
> 
> >> WebCore/storage/IDBFactoryBackendImpl.cpp:122
> >> +    String uniqueIdentifier = fileIdentifier + "_" + name;
> > 
> > Use @ for safety?
> 
> If we allow underscores in the domain name, I think it's unsafe to use an underscore to join the two components, since databaseIdentifier() doesn't escape underscores ...
> 
> http://www.foo.com, ".com_80_name" -> http_www.foo.com_80_.com_80_name
> http://www.foo.com_80_.com, "name" -> http_www.foo.com_80_.com_80_name
> 
> Using '@' avoids the problem as the hostname can't contain an '@'.

I agree this is better.  Fixed other issue.  Please take another look?

[Steve Block]

Comment on attachment 71869 [details]
Patch

r=me

[Jeremy Orlow]

Comment on attachment 71869 [details]
Patch

Clearing flags on attachment: 71869

Committed r70522: <http://trac.webkit.org/changeset/70522>

[Jeremy Orlow]

All reviewed patches have been landed.  Closing bug.