Closed Bug 707915 Opened 13 years ago Closed 13 years ago

Broken invalidation of mCachedUsage in nsDOMStoragePersistentDB

Categories

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

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: michal, Assigned: michal)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch fix (obsolete) — Splinter Review
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+
(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.
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+
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: