Last Comment Bug 707915 - Broken invalidation of mCachedUsage in nsDOMStoragePersistentDB
: Broken invalidation of mCachedUsage in nsDOMStoragePersistentDB
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- major (vote)
: mozilla11
Assigned To: Michal Novotny (:michal)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 538588
  Show dependency treegraph
 
Reported: 2011-12-06 03:02 PST by Michal Novotny (:michal)
Modified: 2013-04-04 13:53 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (4.38 KB, patch)
2011-12-06 03:04 PST, Michal Novotny (:michal)
honzab.moz: feedback+
Details | Diff | Splinter Review
patch v2 - added comments and simplified DomainMaybeCached method (4.58 KB, patch)
2011-12-12 16:30 PST, Michal Novotny (:michal)
jst: review+
Details | Diff | Splinter Review

Description Michal Novotny (:michal) 2011-12-06 03:02:23 PST
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) 2011-12-06 03:04:21 PST
Created attachment 579272 [details] [diff] [review]
fix
Comment 2 Honza Bambas (:mayhemer) 2011-12-06 10:31:46 PST
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) 2011-12-06 14:03:29 PST
(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) 2011-12-07 11:44:45 PST
(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) 2011-12-12 14:00:03 PST
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) 2011-12-12 16:30:53 PST
Created attachment 581090 [details] [diff] [review]
patch v2 - added comments and simplified DomainMaybeCached method
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2011-12-13 23:30:24 PST
Comment on attachment 581090 [details] [diff] [review]
patch v2 - added comments and simplified DomainMaybeCached method

Looks good, r=jst
Comment 8 Michal Novotny (:michal) 2011-12-16 12:36:25 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/61a46d539c69
Comment 9 Matt Brubeck (:mbrubeck) 2011-12-17 09:22:41 PST
https://hg.mozilla.org/mozilla-central/rev/61a46d539c69

Note You need to log in before you can comment on or make changes to this bug.