707915 - Broken invalidation of mCachedUsage in nsDOMStoragePersistentDB

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Michal Novotny [:michal]]

This is a follow up bug of #538588. The logic around mCachedUsage in nsDOMStoragePersistentDB is broken. Invalidation of mCachedUsage is missing in nsDOMStoragePersistentDB::RemoveOwners(). On other places it doesn't work properly due to wrong domain comparison. E.g. if mCachedOwner is "gro.allizom.skcah.somed.:" and we remove key in domain "gro.allizom.skcah.somed." then mCachedUsage isn't cleared.

Comment 1

[Michal Novotny [:michal]]

Attachment: fix

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 579272 [details] [diff] [review]
fix

Review of attachment 579272 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/src/storage/nsDOMStoragePersistentDB.cpp
@@ -743,5 @@
>    rv = mInsertKeyStatement->Execute();
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    if (!aStorage->GetQuotaDomainDBKey(!aExcludeOfflineFromUsage).IsEmpty()) {
> -    mCachedOwner = aStorage->GetQuotaDomainDBKey(!aExcludeOfflineFromUsage);

Why are you removing this?  I think it's a good optimization.  Also, how can you be sure mCachedOwner is the proper one here?

@@ +808,5 @@
>  
> +  if (DomainMaybeCached(
> +      aStorage->GetQuotaDomainDBKey(!aExcludeOfflineFromUsage))) {
> +    mCachedUsage = 0;
> +    mCachedOwner.Truncate(0);

So, you force to forget the usage here, why?

@@ +1141,5 @@
> +
> +  if (aDomain.Length() < len)
> +    return false;
> +
> +  if (Substring(mCachedOwner, 0, len).Equals(Substring(aDomain, 0, len)))

Use StringBeginsWith.

::: dom/src/storage/nsDOMStoragePersistentDB.h
@@ +219,5 @@
>    friend class nsDOMStorageMemoryDB;
>    nsresult
>    GetUsageInternal(const nsACString& aQuotaDomainDBKey, bool aExcludeOfflineFromUsage, PRInt32 *aUsage);
> +
> +  bool DomainMaybeCached(const nsACString& aDomain);

This needs a comment what this is doing and what is it expecting.

Comment 3

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> >    if (!aStorage->GetQuotaDomainDBKey(!aExcludeOfflineFromUsage).IsEmpty()) {
> > -    mCachedOwner = aStorage->GetQuotaDomainDBKey(!aExcludeOfflineFromUsage);
> 
> Why are you removing this?  I think it's a good optimization.  Also, how can
> you be sure mCachedOwner is the proper one here?

It is set by GetUsage() which is called at the beginning of SetKey(). But I agree that it would be good to put there at least some comment like:

// No need to set mCachedOwner since it was set by GetUsage()


> @@ +808,5 @@
> >  
> > +  if (DomainMaybeCached(
> > +      aStorage->GetQuotaDomainDBKey(!aExcludeOfflineFromUsage))) {
> > +    mCachedUsage = 0;
> > +    mCachedOwner.Truncate(0);
> 
> So, you force to forget the usage here, why?

I might be wrong here, but IMO we can't say for sure if the domain was cached just from comparison with mCachedOwner. There will be some false positives, that's why the function's name contains "Maybe". We would need to remember if the cached value excluded offline apps. And also we would need to take into account if the cached value includes subdomains. I wasn't sure if it's worth the added complexity.

Both following scenarios generate false positives:

GetUsage("gro.allizom.", true, &usage);
RemoveKey("gro.allizom.skcah.somed.", "key", false, usage);

GetUsage("gro.allizom.oof.:", false, &usage);
RemoveKey("gro.allizom.oof.rab.", "key", false, usage);

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #3)
> // No need to set mCachedOwner since it was set by GetUsage()

Yes, please add this.

> 
> 
> > @@ +808,5 @@
> > >  
> > > +  if (DomainMaybeCached(
> > > +      aStorage->GetQuotaDomainDBKey(!aExcludeOfflineFromUsage))) {
> > > +    mCachedUsage = 0;
> > > +    mCachedOwner.Truncate(0);
> > 
> > So, you force to forget the usage here, why?
> 
> I might be wrong here, but IMO we can't say for sure if the domain was
> cached just from comparison with mCachedOwner. There will be some false
> positives, that's why the function's name contains "Maybe". We would need to
> remember if the cached value excluded offline apps. And also we would need
> to take into account if the cached value includes subdomains. I wasn't sure
> if it's worth the added complexity.
> 
> Both following scenarios generate false positives:
> 
> GetUsage("gro.allizom.", true, &usage);
> RemoveKey("gro.allizom.skcah.somed.", "key", false, usage);
> 
> GetUsage("gro.allizom.oof.:", false, &usage);
> RemoveKey("gro.allizom.oof.rab.", "key", false, usage);

Understand, I was just curious why on equality of the cached owner name and GetQuotaDomainDBKey result.  I think you can freely update the cached usage.  I'm a bit worried about performance regressions.

But there is a whole story about performance and localStorage, so no need to bother about this, since this may be completely replaced with a different back end.

Comment 5

[Honza Bambas (:mayhemer)]

JST: ping on review here.  This is blocking a networking team Q4 goal, and is relatively simple.

If you cannot do it, I can take the review, also since I've already been looking at the patch.

Comment 6

[Michal Novotny [:michal]]

Attachment: patch v2 - added comments and simplified DomainMaybeCached method

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 581090 [details] [diff] [review]
patch v2 - added comments and simplified DomainMaybeCached method

Looks good, r=jst

Comment 8

[Michal Novotny [:michal]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/61a46d539c69

Comment 9

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/61a46d539c69