Closed
Bug 707915
Opened 13 years ago
Closed 13 years ago
Broken invalidation of mCachedUsage in nsDOMStoragePersistentDB
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: michal, Assigned: michal)
References
Details
Attachments
(1 file, 1 obsolete file)
4.58 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Assignee: nobody → michal.novotny
Attachment #579272 -
Flags: review?(jst)
Attachment #579272 -
Flags: feedback?(honzab.moz)
Comment 2•13 years ago
|
||
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•13 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);
Comment 4•13 years ago
|
||
(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
Comment 5•13 years ago
|
||
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•13 years ago
|
||
Attachment #579272 -
Attachment is obsolete: true
Attachment #579272 -
Flags: review?(jst)
Attachment #581090 -
Flags: review?(jst)
Comment 7•13 years ago
|
||
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•13 years ago
|
||
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
Comment 9•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Updated•12 years ago
|
Component: DOM: Mozilla Extensions → DOM
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•