Broken invalidation of mCachedUsage in nsDOMStoragePersistentDB

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
--
major
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: michal, Assigned: michal)

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 579272 [details] [diff] [review]
fix
Assignee: nobody → michal.novotny
Attachment #579272 - Flags: review?(jst)
Attachment #579272 - Flags: feedback?(honzab.moz)
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.
Attachment #579272 - Flags: feedback?(honzab.moz) → feedback+
(Assignee)

Comment 3

6 years ago
(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);
(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.
Blocks: 538588
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.
(Assignee)

Comment 6

5 years ago
Created attachment 581090 [details] [diff] [review]
patch v2 - added comments and simplified DomainMaybeCached method
Attachment #579272 - Attachment is obsolete: true
Attachment #579272 - Flags: review?(jst)
Attachment #581090 - Flags: review?(jst)
Comment on attachment 581090 [details] [diff] [review]
patch v2 - added comments and simplified DomainMaybeCached method

Looks good, r=jst
Attachment #581090 - Flags: review?(jst) → review+
(Assignee)

Comment 8

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/61a46d539c69
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/61a46d539c69
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.