1298329 - Implement persist()/persisted() attributes in QuotaManager and expose it to QuotaManagerService.

Tom Tung [:tt, :ttung]

Assignee

Description

•

8 years ago

I did persisted() attribute alone in bug 1287701 but I would like to do them(persisted/persist) here together.

Hsin-Yi Tsai (she/her) [:hsinyi]

Updated

•

8 years ago

Blocks: 1254428

Tom Tung [:tt, :ttung]

Assignee

Comment 1

•

8 years ago

Attached patch Bug-1298329-v0-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Implement a mechanism to persist the origin, a 'isPersistent' flag to distinguish whether is persist or not , a hash table to track the DEFAULT type storage, modify isTreatedAsPersistent() function to enure it run in IO thread and check the hash table rather than isApp flag.

Attachment #8787077 - Flags: review?(jvarga)

Jan Varga [:janv]

Comment 2

•

8 years ago

Tom, do you want me to do full review of this patch or just a feedback ?

Tom Tung [:tt, :ttung]

Assignee

Comment 3

•

8 years ago

(In reply to Jan Varga [:janv] from comment #2) > Tom, do you want me to do full review of this patch or just a feedback ? Just a feedback will be great! Thanks! I sent a review request without considering limiting total quota of persistent storage.

Tom Tung [:tt, :ttung]

Assignee

Comment 4

•

8 years ago

Comment on attachment 8787077 [details] [diff] [review] Bug-1298329-v0-Implement persist/persisted in QuotaManager/QuotaManagerService. Changing review request to feedback since I need to rethink about tracking usage for persistent storage.

Attachment #8787077 - Flags: review?(jvarga) → feedback?(jvarga)

Jan Varga [:janv]

Comment 5

•

8 years ago

I applied the patch locally, I'll send you what I find ...

Jan Varga [:janv]

Comment 6

•

8 years ago

Comment on attachment 8787077 [details] [diff] [review] Bug-1298329-v0-Implement persist/persisted in QuotaManager/QuotaManagerService. Review of attachment 8787077 [details] [diff] [review]: ----------------------------------------------------------------- Some initial comments. ::: dom/quota/ActorsChild.h @@ +138,5 @@ > void > HandleResponse(); > > + void > + HandleResponse(const PersistResponse& aResponse); Replace |const PersistResponse& aResponse| with |bool aResponse|. ::: dom/quota/QuotaRequests.cpp @@ +246,5 @@ > > Request::Request(nsIPrincipal* aPrincipal) > : RequestBase(aPrincipal) > { > AssertIsOnOwningThread(); not your fault, but we can add an asserttion for aPrincipal here too @@ +255,5 @@ > + : RequestBase(aPrincipal) > + , mCallback(aCallback) > + , mIsPersistent(false) > +{ > + AssertIsOnOwningThread(); assert aPrincipal and aCallback @@ +288,5 @@ > + FireCallback(); > +} > + > +NS_IMETHODIMP > +Request::GetIsPersistent(bool* aIsPersistent) This should be |GetResult(nsIVariant** aResult)| @@ +305,1 @@ > NS_IMPL_CYCLE_COLLECTION_INHERITED(Request, RequestBase, mCallback) You need to add mVariant here, I think. ::: dom/quota/QuotaRequests.h @@ +117,5 @@ > { > nsCOMPtr<nsIQuotaCallback> mCallback; > > + // Persist() & Persisted() > + bool mIsPersistent; nsIVariant @@ +125,5 @@ > > explicit Request(nsIPrincipal* aPrincipal); > > + explicit Request(nsIPrincipal* aPrincipal, > + nsIQuotaCallback* aCallback); |explicit| is not needed if there is more than one argument @@ +131,5 @@ > void > SetResult(); > > + void > + SetResult(bool aIsPersistent); I think we don't need to SetResult methods, merge them to just one and use nsIVariant as an argument. For the case when a request returns nothing, just call variant->SetAsVoid() ::: dom/quota/nsIQuotaRequests.idl @@ +35,5 @@ > > [scriptable, uuid(22890e3e-ff25-4372-9684-d901060e2f6c)] > interface nsIQuotaRequest : nsIQuotaRequestBase > { > + readonly attribute bool isPersistent; nsIQuotaRequest interface is intended for generic requests, so adding a concrete type here is not suitable. How about |readonly attribute nsIVariant result| ?

Jan Varga [:janv]

Comment 7

•

8 years ago

Comment on attachment 8787077 [details] [diff] [review] Bug-1298329-v0-Implement persist/persisted in QuotaManager/QuotaManagerService. Review of attachment 8787077 [details] [diff] [review]: ----------------------------------------------------------------- I'm a bit confused, if this is just another WIP patch or a complete patch. I think I gave enough feedback and now it's time to finish the patch, do your best and don't forget to add a test, so I'll be able check all code paths. Thanks. ::: dom/quota/ActorsChild.cpp @@ +266,5 @@ > case RequestResponse::TResetAllResponse: > HandleResponse(); > break; > > + case RequestResponse::TPersistResponse: What about TPersistedResponse response, it's not handled here at all. Do you actually have a test for persist() and isPersisted() ? It would be probably better to write a simple xpcshell test to call the new methods in nsIQuotaManagerService(). I see too many not handled cases in this patch. ::: dom/quota/ActorsParent.cpp @@ +1295,5 @@ > } > > namespace { > +// A Hash table and it's used to record PERSISTENCE_TYPE_DEFAULT origins. > +static nsDataHashtable<nsCStringHashKey, bool> gPersistentBoxes; I don't think it's a good idea to declare it this way. I think this can be actually a member of QuotaManager class. @@ +1586,2 @@ > return true; > + } else if (aPersistenceType == PERSISTENCE_TYPE_TEMPORARY) { no need for |else if| if you have |return true| above @@ +1975,5 @@ > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > > + MOZ_ASSERT_IF(aIsApp, aIsPersistent); The assertion should be placed in the beginning of this method @@ -3222,5 @@ > uint64_t aUsageBytes, > int64_t aAccessTime) > { > AssertIsOnIOThread(); > - MOZ_ASSERT(IsTreatedAsTemporary(aPersistenceType, aIsApp)); Why did you remove this assertion. Do we need to create memory objects for persistent origins ? @@ +3349,5 @@ > > void > QuotaManager::RemoveQuota() > { > + AssertIsOnIOThread(); add new line here @@ +6679,5 @@ > + return NS_OK; > +} > + > +void > +PersistOp::SendResults() Why did you override this method ? It looks almost identical to QuotaRequestBase::SendResults() And at the same time you also overrode GetResponse() which is called by QuotaRequestBase::SendResults() Doesn't make sense to me. This class is used by both requests persist() and isPersisted(), but doesn't look finished to me. I'm actually not sure if it's a good idea to share the same class for both requests, especially when we know that persist() might involve showing the prompt, etc. ::: dom/quota/PQuota.ipdl @@ +47,5 @@ > struct ResetAllParams > { > }; > > +struct PersistedParams maybe GetPersistedParams @@ +52,5 @@ > +{ > + PrincipalInfo principalInfo; > +}; > + > +struct PersistParams this sounds too terse to me, what about PersistOriginParams ? ::: dom/quota/PQuotaRequest.ipdl @@ +23,5 @@ > struct ResetAllResponse > { > }; > > +struct PersistResponse thow separate PersistRequest and PersistedRequest, but only one response type ? ::: dom/quota/QuotaManagerService.cpp @@ +634,5 @@ > > NS_IMETHODIMP > +QuotaManagerService::CheckIsPersistentForPrincipal(nsIPrincipal* aPrincipal, > + nsIQuotaCallback* aCallback, > + nsIQuotaRequest** _retval) I see a lot of code duplication here, we might be able to factor out some parts and share it even with existing methods.

Attachment #8787077 - Flags: feedback?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 8

•

8 years ago

(In reply to Jan Varga [:janv] from comment #7) > Comment on attachment 8787077 [details] [diff] [review] > Bug-1298329-v0-Implement persist/persisted in > QuotaManager/QuotaManagerService. > > Review of attachment 8787077 [details] [diff] [review]: > ----------------------------------------------------------------- > > I'm a bit confused, if this is just another WIP patch or a complete patch. > I think I gave enough feedback and now it's time to finish the patch, do > your best and don't forget to add a test, so I'll be able check all code > paths. > Thanks. Thanks for your feedback! I'll look at bug 1296591 first to ensure whether we need to reuse OriginInfo even for persistent storage first. > ::: dom/quota/ActorsChild.cpp > @@ +266,5 @@ > > case RequestResponse::TResetAllResponse: > > HandleResponse(); > > break; > > > > + case RequestResponse::TPersistResponse: > > What about TPersistedResponse response, it's not handled here at all. > Do you actually have a test for persist() and isPersisted() ? > It would be probably better to write a simple xpcshell test to call the new > methods in nsIQuotaManagerService(). > I see too many not handled cases in this patch. I plan to shared a response for both since both of them return a bool(success/fail, persistent/not-persistent) and error code. It's okay to separate them. I did test it but I only write a simple test and do the test in local. I'll provide a test in next patch. > ::: dom/quota/ActorsParent.cpp > @@ +1295,5 @@ > > } > > > > namespace { > > +// A Hash table and it's used to record PERSISTENCE_TYPE_DEFAULT origins. > > +static nsDataHashtable<nsCStringHashKey, bool> gPersistentBoxes; > > I don't think it's a good idea to declare it this way. > I think this can be actually a member of QuotaManager class. I was thinking that it can be called in the static function(IsTreatedAsPersistent()) and PersistOp so I declared it as a global variable. It's okay to declare it as a static variable in QuotaManger. > @@ +1586,2 @@ > > return true; > > + } else if (aPersistenceType == PERSISTENCE_TYPE_TEMPORARY) { > > no need for |else if| if you have |return true| above > I forget to change that. Sorry for this. :( > @@ +1975,5 @@ > > if (NS_WARN_IF(NS_FAILED(rv))) { > > return rv; > > } > > > > + MOZ_ASSERT_IF(aIsApp, aIsPersistent); > > The assertion should be placed in the beginning of this method > Sure > @@ -3222,5 @@ > > uint64_t aUsageBytes, > > int64_t aAccessTime) > > { > > AssertIsOnIOThread(); > > - MOZ_ASSERT(IsTreatedAsTemporary(aPersistenceType, aIsApp)); > > Why did you remove this assertion. Do we need to create memory objects for > persistent origins ? No, sorry for that. > @@ +3349,5 @@ > > > > void > > QuotaManager::RemoveQuota() > > { > > + AssertIsOnIOThread(); > > add new line here Sure. > @@ +6679,5 @@ > > + return NS_OK; > > +} > > + > > +void > > +PersistOp::SendResults() > > Why did you override this method ? > It looks almost identical to QuotaRequestBase::SendResults() > And at the same time you also overrode GetResponse() which is called by > QuotaRequestBase::SendResults() > Doesn't make sense to me. I should not override the GetResponse(). But, I need to pass the result(mIsPeristent) so I need to override that. > This class is used by both requests persist() and isPersisted(), but doesn't > look finished to me. I'm actually not sure if it's a good idea to share the > same class for both requests, especially when we know that persist() might > involve showing the prompt, etc. Hmm, I'll redesign the class. I did not consider the case about prompt. I shared a same class for both requests because both of them need to approach hash table. > ::: dom/quota/PQuota.ipdl > @@ +47,5 @@ > > struct ResetAllParams > > { > > }; > > > > +struct PersistedParams > > maybe GetPersistedParams OK > @@ +52,5 @@ > > +{ > > + PrincipalInfo principalInfo; > > +}; > > + > > +struct PersistParams > > this sounds too terse to me, what about PersistOriginParams ? Sure. It's much better! > > ::: dom/quota/PQuotaRequest.ipdl > @@ +23,5 @@ > > struct ResetAllResponse > > { > > }; > > > > +struct PersistResponse > > thow separate PersistRequest and PersistedRequest, but only one response > type ? Yeah, but it's okay to separate their resoponse. > > ::: dom/quota/QuotaManagerService.cpp > @@ +634,5 @@ > > > > NS_IMETHODIMP > > +QuotaManagerService::CheckIsPersistentForPrincipal(nsIPrincipal* aPrincipal, > > + nsIQuotaCallback* aCallback, > > + nsIQuotaRequest** _retval) > > I see a lot of code duplication here, we might be able to factor out some > parts and share it even with existing methods. Sure, I'll try to do that in next patch!

Jan Varga [:janv]

Comment 9

•

8 years ago

(In reply to Tom Tung [:tt] from comment #8) > > @@ +6679,5 @@ > > > + return NS_OK; > > > +} > > > + > > > +void > > > +PersistOp::SendResults() > > > > Why did you override this method ? > > It looks almost identical to QuotaRequestBase::SendResults() > > And at the same time you also overrode GetResponse() which is called by > > QuotaRequestBase::SendResults() > > Doesn't make sense to me. > > I should not override the GetResponse(). But, I need to pass the > result(mIsPeristent) so I need to override that. Actually, you just need to override GetResponse() and QuotaRequestBase::SendResults() should do everything else for you.

Tom Tung [:tt, :ttung]

Assignee

Comment 10

•

8 years ago

Attached patch Bug-1298329-v1-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Hi Janv, I address the comment and also provide a xpcshell test for eviction which is similar to |temporary_storage.js|. Besides, I modify the callers for calling QM::IsQuotaEnforced() in IndexedDB and ASMJSCache to ensure the callers is in IOthread. However, I'm not sure the way I did in IndexedDB is good or not. Could you help me to review this patch? The interdiff for patch will be provided. Thanks!

Attachment #8787077 - Attachment is obsolete: true

Attachment #8793696 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

•

8 years ago

Attached patch [WIP] Bug-1296591-P0-v0- Bound persistent storage by total quota. (obsolete) — Details — Splinter Review

Hi Jan, Could you give me some feedback for this patch? Thanks! I want to ensure the direction is correct, although there still are few things (e.g. test) needs to be done. This patch is mainly for bounding persistent storage by total quota. Besides, I change the name for |mTemporaryStorage*| to |mGlobalStorage*| since they are used by both persistent and non-persistent storage.

Attachment #8797562 - Flags: feedback?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 15

•

8 years ago

Attached patch [WIP] Bug-1298329-P1-v0-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Rebase the patch for implementing persist()/persisted(). In this patch, I remove the hashtable for PersistenceTypeDefault's origin since we can track them by originInfo. Currently, I have a problem for setting global limit. For example, we have 2G persistent storage usage and somehow a user set his global limit to 1G. I think we can pend or prevent the request/promise for setting limit and pop up a prompt to ask the user to delete his persistent storage.

Attachment #8793696 - Attachment is obsolete: true

Attachment #8793697 - Attachment is obsolete: true

Jan Varga [:janv]

•

8 years ago

Attached patch [WIP] Bug-1296591-P0-v0.1- Bound persistent storage by total quota. (obsolete) — Details — Splinter Review

(In reply to Tom Tung [:tt] from comment #18) The new patch remove the renaming and add a list for recording persistent origin. Jan, If you have not read the patch #8797562 yet, please see this one.

Flags: needinfo?(jvarga)

Jan Varga [:janv]

Comment 20

•

8 years ago

Comment on attachment 8798357 [details] [diff] [review] [WIP] Bug-1296591-P0-v0.1- Bound persistent storage by total quota. Review of attachment 8798357 [details] [diff] [review]: ----------------------------------------------------------------- The direction of this patch mostly looks ok. ::: dom/quota/ActorsParent.cpp @@ +2601,5 @@ > return false; > } > > AssertNoOverflow(quotaManager->mTemporaryStorageUsage, delta); > + uint64_t newStorageUsage = quotaManager->mTemporaryStorageUsage + delta; this should be part of the renamed patch @@ +2612,5 @@ > uint64_t sizeToBeFreed = > quotaManager->LockedCollectOriginsForEviction(delta, locks); > > if (!sizeToBeFreed) { > + if (quotaManager->HasPersistentStorage()) { Isn't this out of scope of this bug ? We don't have such a prompt yet. @@ +2686,5 @@ > return false; > } > > AssertNoOverflow(quotaManager->mTemporaryStorageUsage, delta); > + newStorageUsage = quotaManager->mTemporaryStorageUsage + delta; renaming patch @@ +5179,5 @@ > +} > + > +void > +OriginInfo::LockedIncreaseGroupUsage(QuotaManager* aQuotaManager) > +{ It seems to me that this method can share some code with CheckTemporaryStorageLimits(). Anyway, I see bigger problems here. For example you call OriginClearCompleted() here, but that can only run on the quota manager IO thread and this method can run on any IO thread right ? CheckTemporaryStorageLimits() can be a bit simpler because we know that the origins we touch can't be used in the browser yet, but here you actually can touch origins that are in use. So you need something like we have in MaybeUpdateSize() which collects a list of inactive origins. On the other hand, we could end up in a situation when origins are all active, so we can't delete them, but we need to delete them to satisfy the group limit check. We can actually abort operations for active origins which should remove them from the active origin list. All this is quite complex and I don't see why it needs to be part of this patch. When do we need to revert the persisted flag ? I guess, that will be needed for the new preferences UI, right ? You either need to fix this method or just remove it for now and do it once it's actually needed.

Attachment #8798357 - Flags: feedback+

Jan Varga [:janv]

Updated

•

8 years ago

Attachment #8797562 - Flags: feedback?(jvarga)

Jan Varga [:janv]

Comment 21

•

8 years ago

(In reply to Tom Tung [:tt] from comment #15) > Created attachment 8797565 [details] [diff] [review] > [WIP] Bug-1298329-P1-v0-Implement persist/persisted in > QuotaManager/QuotaManagerService. > > Rebase the patch for implementing persist()/persisted(). > In this patch, I remove the hashtable for PersistenceTypeDefault's origin > since we can track them by originInfo. > > Currently, I have a problem for setting global limit. For example, we have > 2G persistent storage usage and somehow a user set his global limit to 1G. I > think we can pend or prevent the request/promise for setting limit and pop > up a prompt to ask the user to delete his persistent storage. A user sets the global limit ... you mean once quota manager is already running and all initialization of storage is done, the user changes the pref for global limit. How can that happen ? We don't react to the pref changes once we are initialized as far ask I know.

Flags: needinfo?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 22

•

8 years ago

(In reply to Jan Varga [:janv] from comment #20) Thanks for your feedback! > Comment on attachment 8798357 [details] [diff] [review] > [WIP] Bug-1296591-P0-v0.1- Bound persistent storage by total quota. > > Review of attachment 8798357 [details] [diff] [review]: > ----------------------------------------------------------------- > > The direction of this patch mostly looks ok. > > @@ +2612,5 @@ > > uint64_t sizeToBeFreed = > > quotaManager->LockedCollectOriginsForEviction(delta, locks); > > > > if (!sizeToBeFreed) { > > + if (quotaManager->HasPersistentStorage()) { > > Isn't this out of scope of this bug ? > We don't have such a prompt yet. Yep, I just what to mark that we should do the check and maybe pop up a prompt here. > @@ +5179,5 @@ > > +} > > + > > +void > > +OriginInfo::LockedIncreaseGroupUsage(QuotaManager* aQuotaManager) > > +{ > > It seems to me that this method can share some code with > CheckTemporaryStorageLimits(). > > Anyway, I see bigger problems here. For example you call > OriginClearCompleted() here, but that can only run on the quota manager IO > thread and this method can run on any IO thread right ? Sorry, I cannot get it. Can you tell me more about that? I thought this is only called by kind of RevertPersist() function though the QuotaManagerService to QuotaManager and that function should inherit OriginOperationBase like FinalizeOriginEvictionOp. Besides, originInfo only be used in QuotaManager. Hence, I couldn't realize why this method can run on any IO thread. > CheckTemporaryStorageLimits() can be a bit simpler because we know that the > origins we touch can't be used in the browser yet, but here you actually can > touch origins that are in use. > So you need something like we have in MaybeUpdateSize() which collects a > list of > inactive origins. > On the other hand, we could end up in a situation when origins are all > active, so we can't delete them, but we need to delete them to satisfy the > group limit check. > We can actually abort operations for active origins which should remove them > from the active origin list. > All this is quite complex and I don't see why it needs to be part of this > patch. When do we need to revert the persisted flag ? > I guess, that will be needed for the new preferences UI, right ? Yes, you are right. > You either need to fix this method or just remove it for now and do it once > it's actually needed. Maybe remove it and do it until we need it would be better. I'll file a bug for implementing that. (In reply to Jan Varga [:janv] from comment #21) > > Currently, I have a problem for setting global limit. For example, we have > > 2G persistent storage usage and somehow a user set his global limit to 1G. I > > think we can pend or prevent the request/promise for setting limit and pop > > up a prompt to ask the user to delete his persistent storage. > > A user sets the global limit ... you mean once quota manager is already > running > and all initialization of storage is done, the user changes the pref for > global limit. > How can that happen ? > We don't react to the pref changes once we are initialized as far ask I know. So we cannot actually do the same scenario as [1]? I found out that possible problem when I was writing the test. [1] http://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_temporary_storage.js#198-200

Tom Tung [:tt, :ttung]

Assignee

Comment 23

•

8 years ago

(In reply to Tom Tung [:tt] from comment #22) > Maybe remove it and do it until we need it would be better. I'll file a bug > for implementing that. Oops, Shawn have already filed bug 1301276 for it.

Tom Tung [:tt, :ttung]

Assignee

Updated

•

8 years ago

Blocks: 1301276

Tom Tung [:tt, :ttung]

Assignee

Updated

•

8 years ago

Attachment #8797562 - Attachment is obsolete: true

Jan Varga [:janv]

Comment 24

•

8 years ago

(In reply to Tom Tung [:tt] from comment #22) > > Isn't this out of scope of this bug ? > > We don't have such a prompt yet. > > Yep, I just what to mark that we should do the check and maybe pop up a > prompt here. But you need to add HasPersistenBoxes() for that, and I don't see any other caller for that. > > Anyway, I see bigger problems here. For example you call > > OriginClearCompleted() here, but that can only run on the quota manager IO > > thread and this method can run on any IO thread right ? > > Sorry, I cannot get it. Can you tell me more about that? > > I thought this is only called by kind of RevertPersist() function though the > QuotaManagerService to QuotaManager and that function should inherit > OriginOperationBase like FinalizeOriginEvictionOp. Besides, originInfo only > be used in QuotaManager. Hence, I couldn't realize why this method can run > on any IO thread. Ok, but then why you call it LockedSomething and call AssertCurrentThreadOwnsQuotaMutex() ? All the LockedSomething methods are intended for background IO threads (like IndexedD transaction pool) and they use the quota mutex to protect access to shared structures. > > A user sets the global limit ... you mean once quota manager is already > > running > > and all initialization of storage is done, the user changes the pref for > > global limit. > > How can that happen ? > > We don't react to the pref changes once we are initialized as far ask I know. > > So we cannot actually do the same scenario as [1]? I found out that possible > problem when I was writing the test. But that's a special scenario intended just for testing, it's an xpcshell test, not a browser. And after we set the pref, we call resetAllDatabases() which re-initializes entire storage.

Tom Tung [:tt, :ttung]

Assignee

Comment 25

•

8 years ago

(In reply to Jan Varga [:janv] from comment #24) > But you need to add HasPersistenBoxes() for that, and I don't see any other > caller for that. You're right. I'll remove that function. Should I also remove the list for recording persistent storage? > Ok, but then why you call it LockedSomething and call > AssertCurrentThreadOwnsQuotaMutex() ? > All the LockedSomething methods are intended for background IO threads (like > IndexedD > transaction pool) and they use the quota mutex to protect access to shared > structures. > Sorry for misleading, I didn't notice that. I did that because we often lock with quota mutex and then access originInfo/groupInfos so I added that assertion and name it with LockedXXX. > But that's a special scenario intended just for testing, it's an xpcshell > test, > not a browser. And after we set the pref, we call resetAllDatabases() which > re-initializes entire storage. Oh, I see. Thank you!

Jan Varga [:janv]

•

8 years ago

Attached patch [WIP] Bug-1296591-P0-v1- Bound persistent storage by total quota. (obsolete) — Details — Splinter Review

Patch after addressing the feedback.

Attachment #8798357 - Attachment is obsolete: true

Jan Varga [:janv]

Comment 30

•

8 years ago

(In reply to Tom Tung [:tt] from comment #28) > The similar situation will happen when I persist() an origin as a Quota > Client hold the directory lock (e.g. indexedDB db.open, then persist , then > db.close). Sorry, I don't understand. persist() should just modify the metadata file. And I believe we only ever touch that file on the quota manager IO thread. Every time we touch it, we open the file and close immediately. We need to get a directory lock and abort operations only when we need to delete *all* files, not just the metadata file. But maybe I overlooked something, please do tell me why you think we need to do that for persist() too.

Tom Tung [:tt, :ttung]

Assignee

Comment 31

•

8 years ago

(In reply to Jan Varga [:janv] from comment #30) > Sorry, I don't understand. persist() should just modify the metadata file. > And I believe we only ever touch that file on the quota manager IO thread. > Every time we touch it, we open the file and close immediately. > > We need to get a directory lock and abort operations only when we need to > delete > *all* files, not just the metadata file. > > But maybe I overlooked something, please do tell me why you think we need to > do that > for persist() too. You are right. I wrote a test for testing persistent storage should not bound by group limit. During testing, I found some failure and misunderstood them to be locking problem. Finally, I found out the reason. The test is similar to test_quotaExceeded_recovery but I persist that origin after getting abort event(reaching limit) [1]. Then, I do objectStore to the same transaction again but I got failed. The reason is that we set the mQuotaExceed when previous objectStore [2] which triggers onabort event. It is related to bug 1182987 and we do that because we don't have enough space to store. However, we do have enough space after calling persist(). I believe we may have same issue for different quota clients. Should we check the |quota - usage| and cancel that kinds of flag after calling persist? Currently, I close that db, reset, open it again and then do rest of test to avoid this problem. [1] http://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_quotaExceeded_recovery.js#100 [2] http://searchfox.org/mozilla-central/source/dom/indexedDB/IDBTransaction.cpp#796

Jan Varga [:janv]

Comment 32

•

8 years ago

Yes, it's related to bug 1182987. Persist() adds a new corner case for the quota exceeded recovery. We need to handle that somehow.

Tom Tung [:tt, :ttung]

Assignee

Comment 33

•

8 years ago

Attached patch Bug-1296591-P0-v1.1- Bound persistent storage by total quota. (obsolete) — Details — Splinter Review

Hi Jan, I think I have done the everything for bound persistent storage by total quota. Could you help me to review this patch? In this patch, - Remove two arguments(Origin, isApp) for IsQuotaEnforced and modify logic inside since we just track every type of storage except the PERSISTENCE_TYPE_PERSISTENT. - Modify OriginInfo to make it know itself is persistent or not and add function to the changes for calling persist() - Modify MaybeUpdateSize() by adding condition for not changing groupUsage when originInfo is persistent. - Modify CollectOriginsForEviction() to prevent persistent from eviction. - Add a list(mPersistentOriginBoxes) to record all the persistent origin I'll do the following things in other patch: - Rename the mTemporaryStorageLimit/mTemporaryStorageUsage/.. since the naming is confused after this patch - Modify mInitializedOrigins since we only store PERSISTENCE_TYPE_PERSISTENT in this list.

Attachment #8798832 - Attachment is obsolete: true

Attachment #8799679 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 34

•

8 years ago

Attached patch [WIP] Bug-1298329-P1-v1-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Almost done except the factoring duplication in QuotaManagerService.cpp and modify the IsFirstPromptRequired().

Attachment #8797565 - Attachment is obsolete: true

Jan Varga [:janv]

Comment 35

•

8 years ago

Ok, I'll take a look ASAP.

Tom Tung [:tt, :ttung]

Assignee

Comment 36

•

8 years ago

Attached patch [WIP] Test for persisted() and persist() (obsolete) — Details — Splinter Review

Basically, this patch is done but I would like to add more tests for it. Jan, could give me some feedback about these two tests since I am not familiar with indexedDB? I wrote these two tests refer to test_temporary_storage.js and test_quotaExceeded_recovery.js. Thanks!

Attachment #8799700 - Flags: feedback?(jvarga)

Tom Tung [:tt, :ttung]

•

8 years ago

(In reply to Jan Varga [:janv] from comment #39) > Comment on attachment 8800562 [details] [diff] [review] > Bug-1298329-P1-v1.1-Implement persist/persisted in > QuotaManager/QuotaManagerService. > > Review of attachment 8800562 [details] [diff] [review]: > ----------------------------------------------------------------- > > Just went quickly through the patch, I remember I suggested to use > nsIVariant in QuotaRequests.h/.cpp > > ::: dom/quota/QuotaRequests.h > @@ +117,5 @@ > > { > > nsCOMPtr<nsIQuotaCallback> mCallback; > > > > + // Persist() & Persisted() > > + bool mIsPersistent; > > I suggested to use nsIVariant for this in comment 6. Is there a reason to > not use it ? > > @@ +131,5 @@ > > void > > SetResult(); > > > > + void > > + SetResult(bool aIsPersistent); > > dtto Sorry for that, I'll recheck the patch. :(

Tom Tung [:tt, :ttung]

Assignee

Comment 42

•

8 years ago

Comment on attachment 8800562 [details] [diff] [review] Bug-1298329-P1-v1.1-Implement persist/persisted in QuotaManager/QuotaManagerService. I find out that I somehow miss the comment 6 so removing the review request. I'll address them and then send the patch for review later.

Attachment #8800562 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 43

•

8 years ago

Attached patch Bug-1298329-P1-v1.2-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Sorry for overlooking the comment 6. I address them in this patch. Could you help me to review this patch, Jan? Thanks!

Attachment #8800562 - Attachment is obsolete: true

Attachment #8801020 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 44

•

8 years ago

Attached patch Bug-1298329-P2-v0.2-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Sorry for disturbance! Rebase the patch due to modify the P1. Basically, this patch do the same thing as I put in comment 40. Change the attachment 8800564 [details] [diff] [review]'s feedback request to this one.

Attachment #8799700 - Attachment is obsolete: true

Attachment #8800564 - Attachment is obsolete: true

Attachment #8800564 - Flags: feedback?(jvarga)

Attachment #8801022 - Flags: feedback?(jvarga)

Jan Varga [:janv]

Comment 45

•

8 years ago

Ok, I'll start reviewing it.

Hsin-Yi Tsai (she/her) [:hsinyi]

•

8 years ago

How often someone would go to the storage preferences ? I think it's ok to generate the array on demand.

Jan Varga [:janv]

Comment 52

•

•

8 years ago

Fabrice, maybe it's obvious, but I didn't follow discussions about this much. Please see comment 57, what is the current status and plan for near future regarding installed apps ? Basically, we do this in quota manager: isApp = principal->GetAppStatus() != nsIPrincipal::APP_STATUS_NOT_INSTALLED; and we treat such principals/origins differently, we don't track quota for them Thanks

Flags: needinfo?(fabrice)

Jan Varga [:janv]

Comment 59

•

8 years ago

Ok there are two bugs for dom/apps removal bug 1261019 and bug 1291291. Ehsan removed mozApps in bug 1261019. Fabrice commented in bug 1291291: > jst, do you have someone that can tackle that? That will lead to a lot of > code removal all over the place, since we won't need the appId and appStatus > on the principals. So it doesn't make sense for quota manager to support special handling for installed apps. I'm going to remove it.

Flags: needinfo?(fabrice)

Tom Tung [:tt, :ttung]

Assignee

•

8 years ago

Attached patch [WIP] Bug-1296591-P0-v2- Bound persistent storage by total quota. (obsolete) — Details — Splinter Review

WIP patch for P0, addressed comment #49 #54 #55 #56 and #60. I'll wait the appId's removal based on comment #61 and re-base this patch again at that time.

Tom Tung [:tt, :ttung]

Assignee

Comment 64

•

8 years ago

Attached patch [WIP] Bug-1298329-P1-v1.3-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

WIP patch, re-base on P0's changes. (In reply to Jan Varga [:janv] from comment #61) > The P0 patch shouldn't depend on changes in P1 (I'm not 100% sure if there > are any dependencies), I'd like to make it a standalone patch since the > changes we are doing here are quite risky, we should be very careful. You are right, P0 should not depend on changes in P1 and it should be standalone patch. I meant P1 depend on P0 so I need to re-base P1 after addressing the comment to P0. Sorry for making misunderstanding.

•

8 years ago

Attached patch [WIP] Bug-1298329-P2-v1.1-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Re-based after applying initial patches in bug 1311057. (In reply to Jan Varga [:janv] from comment #65) > Tom, I submitted some initial patches in bug 1311057, please apply them and > re-base your patches. Thanks. Sure, I've re-based them. Thanks for your patches!

Tom Tung [:tt, :ttung]

Assignee

Comment 69

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P2-v1.2-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Re-base P2 again after applying part 4 in bug 1311057.

Attachment #8809722 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 70

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Update the patch for checking whether the origin has already been persisted. If it is, we should not persist it again.

Attachment #8809721 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 71

•

8 years ago

Comment on attachment 8809720 [details] [diff] [review] [Rebased Patch] Bug-1296591-P0-v2.1- Bound persistent storage by total quota. remove WIP title

Attachment #8809720 - Attachment description: [WIP] Bug-1296591-P0-v2.1- Bound persistent storage by total quota. → Bug-1296591-P0-v2.1- Bound persistent storage by total quota.

Tom Tung [:tt, :ttung]

Assignee

Updated

•

8 years ago

Attachment #8809773 - Attachment description: [WIP] Bug-1298329-P2-v1.2-Test for persisted() and persist(). → [Rebased Patch] Bug-1298329-P2-v1.2-Test for persisted() and persist().

Tom Tung [:tt, :ttung]

Assignee

Updated

•

8 years ago

Attachment #8810336 - Attachment description: [WIP] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService. → [Rebased Patch] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService.

Tom Tung [:tt, :ttung]

Assignee

Updated

•

8 years ago

Attachment #8809720 - Attachment description: Bug-1296591-P0-v2.1- Bound persistent storage by total quota. → [Rebased Patch] Bug-1296591-P0-v2.1- Bound persistent storage by total quota.

Jan Varga [:janv]

Comment 72

•

8 years ago

Comment on attachment 8809720 [details] [diff] [review] [Rebased Patch] Bug-1296591-P0-v2.1- Bound persistent storage by total quota. Review of attachment 8809720 [details] [diff] [review]: ----------------------------------------------------------------- Since, isApp is gone, all storage/default subdirs are quota initialized. So "bound persistent storage by total quota" is not right name for this patch anymore. It's more like "add persisted attribute ..." I'm a bit worried about changes in QuotaObject::MaybeUpdateSize(), this is very sensitive stuff, the method can be called from multiple threads, there's locking, waiting, relocking, etc. I have to take a look at it one more time before I give final r+. ::: dom/quota/ActorsParent.cpp @@ +4998,5 @@ > + uint64_t aUsage, int64_t aAccessTime, bool aPersisted) > + : mGroupInfo(aGroupInfo), mOrigin(aOrigin), mUsage(aUsage), > + mAccessTime(aAccessTime), mPersisted(aPersisted) > +{ > + MOZ_COUNT_CTOR(OriginInfo); assertions first, then an empty line and then MOZ_COUNT_CTOR

Tom Tung [:tt, :ttung]

Assignee

Comment 73

•

8 years ago

Attached patch [Rebased Patch] Bug-1296591-P0-v3- Add persisted attribute to originInfo and bound origins with this attribute only by total quota (not by group quota). (obsolete) — Details — Splinter Review

Addressed comment #72. Also, to reduce unnecessary group usage computations when the origin has persisted attribute, I rewrite few if-conditions in MaybeUpdateSize(). I will provide interdiff patch between this patch(v3) and previous patch(v2.1)

Attachment #8809720 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

•

8 years ago

Attached patch [Rebased Patch] Bug-1296591-P0-v4- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). (obsolete) — Details — Splinter Review

Updated patch per f2f discussion this afternoon. In this patch, I mainly do the following things: - Add assertion about holding mutex in accesstime and persist's getter function. - Integrate if-conditions for checking persisted in MaybeUpdateSize(). - Add ifdef to hold a QuotaMutex when we remove doomorigins because we add an assert for getter function. I'll also update the interdiff.

Attachment #8815584 - Attachment is obsolete: true

Attachment #8817118 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 76

•

8 years ago

Attached patch interdiff-P0-v3-v4.patch (obsolete) — Details — Splinter Review

interdiff patch.

Attachment #8815585 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 77

•

8 years ago

Comment on attachment 8810336 [details] [diff] [review] [Rebased Patch] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService. Change the review flag to the newest patch.

Attachment #8810336 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 78

•

8 years ago

Comment on attachment 8809773 [details] [diff] [review] [Rebased Patch] Bug-1298329-P2-v1.2-Test for persisted() and persist(). Change the review flag to the newest patch.

Attachment #8809773 - Flags: review?(jvarga)

Jan Varga [:janv]

Comment 79

•

8 years ago

Comment on attachment 8817118 [details] [diff] [review] [Rebased Patch] Bug-1296591-P0-v4- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). Review of attachment 8817118 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +5158,5 @@ > +{ > + AssertCurrentThreadOwnsQuotaMutex(); > + MOZ_ASSERT(mGroupInfo->mPersistenceType == PERSISTENCE_TYPE_DEFAULT); > + > + mPersisted = true; Nit: add an empty line here @@ +5235,5 @@ > > for (uint32_t index = mOriginInfos.Length(); index > 0; index--) { > OriginInfo* originInfo = mOriginInfos[index - 1]; > > + if (!originInfo->Persisted()) { I think you forgot to add this check to LockedRemoveOriginInfo(aOrigin).

Attachment #8817118 - Flags: review?(jvarga) → review+

Tom Tung [:tt, :ttung]

Assignee

Comment 80

•

8 years ago

Attached patch [Rebased Patch] Bug-1296591-P0-v5- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). r=janv. (obsolete) — Details — Splinter Review

(In reply to Jan Varga [:janv] from comment #79) Thanks for the review! I learn a lot from that. > Comment on attachment 8817118 [details] [diff] [review] > [Rebased Patch] Bug-1296591-P0-v4- Add persisted attribute to originInfo and > bound persisted origins only by total quota (not by group quota). > > Review of attachment 8817118 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/quota/ActorsParent.cpp > @@ +5235,5 @@ > > > > for (uint32_t index = mOriginInfos.Length(); index > 0; index--) { > > OriginInfo* originInfo = mOriginInfos[index - 1]; > > > > + if (!originInfo->Persisted()) { > > I think you forgot to add this check to LockedRemoveOriginInfo(aOrigin). Sorry for forgetting to add this check. I should find it.

Attachment #8817118 - Attachment is obsolete: true

Attachment #8817119 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 81

•

8 years ago

Add dependency since all the patches in these bug are based on Bug 1311057.

Depends on: 1311057

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

•

8 years ago

(In reply to Jan Varga [:janv] from comment #88) > Are you sure we don't have it ? I think I was mixing it up with my previous proposal... Really sorry for that :( > We call InitializeRepository() for origins in default and temporary storage > if the initialization didn't happen yet. So if there's an origin directory > for the given *persisted* origin, OriginInfo is created and mPersisted flag > is set to true. > > If the directory doesn't exist we just create a new metadata file with the > flag set to false. That's it. Yeah, you are right. So, we do create it even its "persisted" is set to be true, and we'll create a new metadata with setting "persisted" to be false if we don't have one. > > IsTreatedAsPersistent(). If we only check |aPersistenceType == > > PERSISTENCE_TYPE_PERSISTENT|, we cannot distinguish from whether the origin > > is persisted or not. I mean "persisted" is like "isApp", but sometimes we > > need to check the directory. > > Again, why do we need to distinguish it here ? Sorry again, be the same as above, I mix it up with previous proposal which make "persisted" not have a originInfo. I will remove this. (In reply to Jan Varga [:janv] from comment #91) > Yes, persisted is like isApp, but the goal of your P0 patch is to create > OriginInfo objects for all origins, even for those with isApp/persisted set > to true, no ? Yes, you are right. Sorry for confusing... (In reply to Jan Varga [:janv] from comment #93) > Comment on attachment 8809773 [details] [diff] [review] > [Rebased Patch] Bug-1298329-P2-v1.2-Test for persisted() and persist(). > > Review of attachment 8809773 [details] [diff] [review]: > ----------------------------------------------------------------- > > I quickly looked over these tests and my first impression is that it's way > too complicated. > It might help if you describe each test a bit. I'll rewrite each of them to make they easier to understand. > ::: dom/indexedDB/test/mochitest.ini > @@ +82,5 @@ > > unit/test_open_objectStore.js > > unit/test_optionalArguments.js > > unit/test_overlapping_transactions.js > > unit/test_persistenceType.js > > + unit/test_persistent_storage.js > > I believe you just forgot to remove this line. Yep...

Jan Varga [:janv]

Comment 95

•

8 years ago

(In reply to Tom Tung [:tt] from comment #94) > > I quickly looked over these tests and my first impression is that it's way > > too complicated. > > It might help if you describe each test a bit. > > I'll rewrite each of them to make they easier to understand. It might also help if you say which tests you used as a base/template for your new tests.

Tom Tung [:tt, :ttung]

Assignee

Comment 96

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v2-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Hi Jan, Thanks for giving so much feedback, I addressed them in this patch. Could you help me to review it again? Thanks!

Attachment #8810336 - Attachment is obsolete: true

Attachment #8826914 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 97

•

8 years ago

Attached patch interdiff-P1-v1.5-2.patch (obsolete) — Details — Splinter Review

interdiff patch for P1.

Tom Tung [:tt, :ttung]

Assignee

Comment 98

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P2-v2-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Hi Jan, Sorry for implementing testcase too complex but I try to do the same procedures to the persisted origins to ensure they have hit different events as expected. I test the persisted origins under pressure of group limit (test_groupLimit.js) and global limit (test_persistent_storage). Moreover, I test the basic function of persist()/persisted() APIs as well (test_persitent_basic.js). In this patch, I implement three testcases and they are listed as following: test_persistent_basic.js: In this testcase, I intended to test basic persist()/persisted() functionalities. - Test1: Persist an origin before storing anything and check the persisted() before and after persist(). - Test2: Store an object into the origin and check if it is still persisted - Test3: Clean the origin and check if it is still persisted as well. test_persistent_storage.js: This test is based on [1] and it is mainly test the global limit and eviction. - What I'm going to do here is to keep create database and make it hit the global limit . (Stage 1) - Then, I close all of database, persist() them and then I create one more. It should be hit the onerror event since they shared the same global limit. However, there are no any evictions happened since they are all persisted. (Stage 2) - Next, cut the limit into half but the QuotaManger should not evict any of storage since they all are persisted. (Stage 3) [1] http://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_temporary_storage.js#139 test_groupLimit.js: I write this test based on [2]. I create this testcase to make sure QuotaManager won't delete the persisted origin and the group limit should not bound persisted origin. I only create a database and keep filling an object into it until it cannot be written which means reaching group limit. Then I persist() it and write one more object into it again. And, I get oncomplete event as expected. [2] http://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_quotaExceeded_recovery.js

Attachment #8809773 - Attachment is obsolete: true

Attachment #8809773 - Flags: review?(jvarga)

Attachment #8826918 - Flags: review?(jvarga)

Jan Varga [:janv]

Comment 99

•

8 years ago

Comment on attachment 8826914 [details] [diff] [review] [Rebased Patch] Bug-1298329-P1-v2-Implement persist/persisted in QuotaManager/QuotaManagerService. Review of attachment 8826914 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +4753,5 @@ > } > } > > +nsresult > +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup, A typo in the method name. @@ +4755,5 @@ > > +nsresult > +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup, > + const nsACString& aOrigin) > +{ Which thread this method can be called on ? @@ +4756,5 @@ > +nsresult > +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup, > + const nsACString& aOrigin) > +{ > + MOZ_ASSERT(mTemporaryStorageInitialized); Is it safe to access this member variable on this thread ? @@ +4758,5 @@ > + const nsACString& aOrigin) > +{ > + MOZ_ASSERT(mTemporaryStorageInitialized); > + > + MutexAutoLock lock(mQuotaMutex); So, if you look at all Locked* methods, they don't acquire the lock, they just check that it's been already acquired. Either removed the Locked prefix, or rework the method body. @@ +4762,5 @@ > + MutexAutoLock lock(mQuotaMutex); > + > + GroupInfoPair* pair; > + if (!mGroupInfoPairs.Get(aGroup, &pair)) { > + return NS_ERROR_UNEXPECTED; I don't like how you return NS_ERROR_UNEXPECTED here. We need to improve this in next iteration. @@ +4788,5 @@ > +nsresult > +QuotaManager::LoackedGetPersistedForOrigin(const nsACString& aGroup, > + const nsACString& aOrigin, > + bool* aResult) > +{ The same comments apply here. @@ +6656,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + // Figure out which origin This sounds like an unfinished sentence. @@ +6677,5 @@ > + js::ProfileEntry::Category::OTHER); > + > + // Ensure origin is initialized first. > + nsCOMPtr<nsIFile> directory; > + nsresult rv = aQuotaManager->EnsureOriginIsInitialized( So now persisted() creates the directory and metadata if the directory doesn't exist ? Is it what you want ? @@ +6734,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + rv = aQuotaManager->LoackedPersistForOrigin(mGroup, mOrigin); This method should be called after we successfully updated metadata on disk. Ideally the method would be infallible, so it doesn't return any errors (See my comment about NS_ERROR_UNEXPECTED. The problem is that you update objects in memory first and then something fails during on disk update. The memory objects should be reverted, but that's quite complicated. It would be better to execute stuff that can fail first and then call infallible methods. @@ +7875,5 @@ > return rv; > } > } > > + // We don't have any approach to restore aPersisted, so assume it is false. ... , so reset it to false. ::: dom/quota/nsIQuotaManagerService.idl @@ +75,5 @@ > [must_use] nsIQuotaRequest > reset(); > + > + /** > + * Check the storages stored in given origin is persisted or not. Check if given origin is persisted. @@ +79,5 @@ > + * Check the storages stored in given origin is persisted or not. > + * > + * @param aPrincipal > + * A principal for the origin which we want to check is perisitent or > + * not. A principal for the origin which we want to check.

Attachment #8826914 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 100

•

8 years ago

(In reply to Jan Varga [:janv] from comment #99) Thanks for giving feedback so quickly! > Comment on attachment 8826914 [details] [diff] [review] > [Rebased Patch] Bug-1298329-P1-v2-Implement persist/persisted in > QuotaManager/QuotaManagerService. > > Review of attachment 8826914 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/quota/ActorsParent.cpp > @@ +4755,5 @@ > > > > +nsresult > > +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup, > > + const nsACString& aOrigin) > > +{ > > Which thread this method can be called on ? IO thread I'll add assertion for it. > @@ +4756,5 @@ > > +nsresult > > +QuotaManager::LoackedPersistForOrigin(const nsACString& aGroup, > > + const nsACString& aOrigin) > > +{ > > + MOZ_ASSERT(mTemporaryStorageInitialized); > > Is it safe to access this member variable on this thread ? It's in IO thread so it should be safe. > @@ +4758,5 @@ > > + const nsACString& aOrigin) > > +{ > > + MOZ_ASSERT(mTemporaryStorageInitialized); > > + > > + MutexAutoLock lock(mQuotaMutex); > > So, if you look at all Locked* methods, they don't acquire the lock, they > just check that it's been already acquired. Either removed the Locked > prefix, or rework the method body. Sure, I will remove Lock* prefix! > @@ +4762,5 @@ > > + MutexAutoLock lock(mQuotaMutex); > > + > > + GroupInfoPair* pair; > > + if (!mGroupInfoPairs.Get(aGroup, &pair)) { > > + return NS_ERROR_UNEXPECTED; > > I don't like how you return NS_ERROR_UNEXPECTED here. We need to improve > this in next iteration. Sure, I'll try to improve it. > @@ +6656,5 @@ > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + return rv; > > + } > > + > > + // Figure out which origin > > This sounds like an unfinished sentence. I thought it might be clearer... I'll recover it to "Figure out which origin we're dealing with". > @@ +6677,5 @@ > > + js::ProfileEntry::Category::OTHER); > > + > > + // Ensure origin is initialized first. > > + nsCOMPtr<nsIFile> directory; > > + nsresult rv = aQuotaManager->EnsureOriginIsInitialized( > > So now persisted() creates the directory and metadata if the directory > doesn't exist ? > Is it what you want ? Sorry, I forget it. I will rewrite it. > @@ +6734,5 @@ > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > + return rv; > > + } > > + > > + rv = aQuotaManager->LoackedPersistForOrigin(mGroup, mOrigin); > > This method should be called after we successfully updated metadata on disk. > Ideally the method would be infallible, so it doesn't return any errors (See > my comment about NS_ERROR_UNEXPECTED. I think this method is fallable, and thus the returning promise can be either true(persisted) or false(best-effort). [1] https://storage.spec.whatwg.org/#dfnReturnLink-0 > The problem is that you update objects in memory first and then something > fails during on disk update. The memory objects should be reverted, but > that's quite complicated. It would be better to execute stuff that can fail > first and then call infallible methods. Sorry, I'm not sure how to call infallible methods here. Could you explain more? Thanks! ><

Tom Tung [:tt, :ttung]

Assignee

Comment 101

•

8 years ago

(In reply to Tom Tung [:tt] from comment #100) > > This method should be called after we successfully updated metadata on disk. > > Ideally the method would be infallible, so it doesn't return any errors (See > > my comment about NS_ERROR_UNEXPECTED. > > I think this method is fallable, and thus the returning promise can be > either true(persisted) or false(best-effort). > [1] https://storage.spec.whatwg.org/#dfnReturnLink-0 I just realize that this happen because user can deny the prompt. Sorry for that!

Jan Varga [:janv]

Comment 102

•

8 years ago

(In reply to Tom Tung [:tt] from comment #100) > > @@ +6734,5 @@ > > > + if (NS_WARN_IF(NS_FAILED(rv))) { > > > + return rv; > > > + } > > > + > > > + rv = aQuotaManager->LoackedPersistForOrigin(mGroup, mOrigin); > > > > This method should be called after we successfully updated metadata on disk. > > Ideally the method would be infallible, so it doesn't return any errors (See > > my comment about NS_ERROR_UNEXPECTED. > > I think this method is fallable, and thus the returning promise can be > either true(persisted) or false(best-effort). > [1] https://storage.spec.whatwg.org/#dfnReturnLink-0 I meant that LoackedPersistForOrigin() should be infallible. > > > The problem is that you update objects in memory first and then something > > fails during on disk update. The memory objects should be reverted, but > > that's quite complicated. It would be better to execute stuff that can fail > > first and then call infallible methods. > > Sorry, I'm not sure how to call infallible methods here. Could you explain > more? Thanks! >< infallible == returning "void" in this context

Jan Varga [:janv]

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v3-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Addressed comment except few of them that are uncertain for me. And, I listed these in comment #105

Attachment #8826914 - Attachment is obsolete: true

Jan Varga [:janv]

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v3.1-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Hi Jan, In this patch, I addressed the comment. Besides, I change return type for PersistOrigin() from nsresult to void. The reason is that I think even if we cannot get originInfo, we can correct the "persisted" metadata and the originInfo will be initialize with correct value from correct metadata in next time. Moreover, I change return type for OriginPersisted() from nsresult to bool. The reason is that I just need to check the metadata once we cannot get originInfo. Could you help me to review this patch? Thanks!

Attachment #8827058 - Attachment is obsolete: true

Attachment #8827091 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 111

•

8 years ago

Attached patch interdiff-P1-v2-3.1.patch (obsolete) — Details — Splinter Review

interdiff patch for P1

Attachment #8826915 - Attachment is obsolete: true

Jan Varga [:janv]

Comment 112

•

8 years ago

(In reply to Tom Tung [:tt] from comment #110) > Besides, I change return type for PersistOrigin() from nsresult to void. > The reason is that I think even if we cannot get originInfo, we can correct > the "persisted" metadata and the originInfo will be initialize with correct > value from correct metadata in next time. EnsureOriginIsInitialized() is called first which is expected to create OriginInfo, right ? So it would be an error if originIfo didn't exist after that call. However, I just realized that EnsureOriginIsInitialized() is not necessary. It's sufficient that EnsureStorageIsInitialized() is called in OriginOperationBase::DirectoryWork(). > > Moreover, I change return type for OriginPersisted() from nsresult to bool. > The reason is that I just need to check the metadata once we cannot get > originInfo. It's not bad, but I think we can do better. I'll attach a patch.

Jan Varga [:janv]

Comment 113

•

8 years ago

Comment on attachment 8817250 [details] [diff] [review] [Rebased Patch] Bug-1296591-P0-v5- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). r=janv. Review of attachment 8817250 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +5156,5 @@ > +void > +OriginInfo::LockedPersist() > +{ > + AssertCurrentThreadOwnsQuotaMutex(); > + MOZ_ASSERT(mGroupInfo->mPersistenceType == PERSISTENCE_TYPE_DEFAULT); We should also assert here that mPersisted is false.

Jan Varga [:janv]

Comment 114

•

8 years ago

Comment on attachment 8817250 [details] [diff] [review] [Rebased Patch] Bug-1296591-P0-v5- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). r=janv. Review of attachment 8817250 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +456,5 @@ > return mAccessTime; > } > > + bool > + Persisted() const This should be LockedPersisted(). Let's fix AccessTime() elsewhere.

Jan Varga [:janv]

Comment 115

•

8 years ago

Comment on attachment 8827091 [details] [diff] [review] [Rebased Patch] Bug-1298329-P1-v3.1-Implement persist/persisted in QuotaManager/QuotaManagerService. Review of attachment 8827091 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +1135,5 @@ > + bool mPersisted; > + > +protected: > + const RequestParams mParams; > + nsCString mSuffix; mSuffix won't be needed once we remove EnsureOriginIsInitialized() call. @@ +3451,5 @@ > return GetQuotaObject(aPersistenceType, aGroup, aOrigin, file); > } > > +bool > +QuotaManager::OriginPersisted(const nsACString& aGroup, This method along with PersistOrigin contains very similar code. The code should be shared somehow. @@ +6629,5 @@ > + MOZ_ASSERT(aQuota); > + > + mNeedsMainThreadInit = true; > + > + mPersistenceType.SetValue(PERSISTENCE_TYPE_DEFAULT); I would put this above |mNeedsMainThreadInit = true;| @@ +6670,5 @@ > +nsresult > +PersistedOp::DoDirectoryWork(QuotaManager* aQuotaManager) > +{ > + AssertIsOnIOThread(); > + MOZ_ASSERT(mParams.type() == RequestParams::TPersistedParams); I don't think we have to assert this, mParams.type() is not accessed in this method at all. @@ +6671,5 @@ > +PersistedOp::DoDirectoryWork(QuotaManager* aQuotaManager) > +{ > + AssertIsOnIOThread(); > + MOZ_ASSERT(mParams.type() == RequestParams::TPersistedParams); > + MOZ_ASSERT(mPersistenceType.Value() == PERSISTENCE_TYPE_DEFAULT); But I do think that we need assert !mPersistebceType.IsNull(). We should also assert mOriginScope.IsOrigin() @@ +6677,5 @@ > + PROFILER_LABEL("Quota", "PersistedOp::DoDirectoryWork", > + js::ProfileEntry::Category::OTHER); > + > + // Get "persisted" from directory if we cannot find it in originInfo. > + if (!aQuotaManager->OriginPersisted(mGroup, I understand your intention here, but this doesn't read well. If origin is not persisted, then ... but we want: If persisted flag/info is not cached yet, then ... @@ +6695,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + if(!exists) { What about switching these branches ? You don't have to negate "exists" then. @@ +6696,5 @@ > + return rv; > + } > + > + if(!exists) { > + // Metadata have not been created yet. has @@ +6703,5 @@ > + // Get persisted flag > + int64_t dummyTimestamp; > + nsCString dummySuffix; > + nsCString dummyGroup; > + nsCString dummyOrigin; Too many dummies here. We should simplify this somehow. @@ +6704,5 @@ > + int64_t dummyTimestamp; > + nsCString dummySuffix; > + nsCString dummyGroup; > + nsCString dummyOrigin; > + rv = aQuotaManager->GetDirectoryMetadata2(directory, &dummyTimestamp, So, what if the metadata file is missing or broken ? The whole process of getting the persisted flag will fail, right ? I think we can happily use the *WithRestore method here. @@ +6755,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + aQuotaManager->PersistOrigin(mGroup, mOriginScope.GetOrigin()); Like I said in one of my previous comments, calling PersistOrigin() first and then updating stuff on disk can lead to inconsistency. If we fail somewhere in the code that updates stuff on disk, for example GetBinaryOutputStream() fails, then you return from this method - PersistOp::DoDirectoryWork(), but PersistOrigin() already updated OriginInfo. You could add code that reverts changes done by PersistOrigin() if GetBinaryOutputStream() failed, but it's just easier to execute stuff that can fail and call PersistOrigin() after that. @@ +7963,5 @@ > MOZ_ASSERT(mOriginProps.Length() == 1); > > OriginProps& originProps = mOriginProps[0]; > > + // We don't have any approach to restore aPersisted, so reset it is false. , so reset it *to* false.

Attachment #8827091 - Flags: review?(jvarga)

Jan Varga [:janv]

Comment 116

•

8 years ago

Attached patch patch (obsolete) — Details — Splinter Review

Here are my changes. It builds, but I haven't done any testing, you need to do it yourself.

Tom Tung [:tt, :ttung]

Assignee

Comment 117

•

8 years ago

(In reply to Jan Varga [:janv] from comment #116) > Created attachment 8827207 [details] [diff] [review] > patch > > Here are my changes. It builds, but I haven't done any testing, you need to > do it yourself. Hi Jan, I think we maybe still need EnsureOriginIsInitialize(..) to create directory for metadata if the metadata's directory has not been created yet. I tried to persist() origin before storing an object into it and then I got fail while setting stream [1]. The reason is the directory is not exist and make the stream null [2]. I guess the reason is because EnsureStorageIsInitialized() won't create the directory for origin if it is the first time to access. But, I'm not sure about that... If I'm correct, I have two options for this problem: 1. Use EnsureOriginIsInitialize() and don't reset mNeedsQuotaManagerInit so that we can avoid calling EnsureStorageIsInitialized() twice. 2. Calling EnsureDirectory(), CreateDirectoryMetadata() and CreateDirectoryMetadata2() in PersistOp::DoDirectoryWork() if the directory has not been created yet. Could you give me some suggests for it and correct me if I'm wrong? Thanks! [1] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#1838 [2] http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#1778

Jan Varga [:janv]

Comment 118

•

8 years ago

(In reply to Tom Tung [:tt] from comment #117) > If I'm correct, I have two options for this problem: > 1. Use EnsureOriginIsInitialize() and don't reset mNeedsQuotaManagerInit so > that we can avoid calling EnsureStorageIsInitialized() twice. I'm not worried about calling it twice, actually that's not the case. if mNeedsQuotaManagerInit is set then EnsureStorageIsInitialized() is called not EnsureOriginIsInitialized(). The problem is that EnsureOriginIsInitialized() will call InitializedRepository() and we don't need that in this case. We just need to store the flag. > 2. Calling EnsureDirectory(), CreateDirectoryMetadata() and > CreateDirectoryMetadata2() in PersistOp::DoDirectoryWork() if the directory > has not been created yet. Yes, these methods need to be called if the origin directory doesn't exist. However you should try to share code as usual, so maybe you need to create a new helper method that will be called by EnsureOriginIsInitialized() and PersistOp::DoDirectoryWork()

Tom Tung [:tt, :ttung]

Assignee

Comment 119

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v4-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Hi Jan, In this patch, I addressed the comment and did a little changes, thus I listed my changes as following: - Rewrite function OriginPersisted(), PersistOrigin(), GetDirectoryMetadata2(), GetDirectoryMetadata2WithRestore() as your suggest. - Implement LockedGetOriginInfo() as your comment - Improve code in PersistedOp::DoDirectoryWork() as your comment but doing a little changes on last condition. - Remove assertion for mTemporaryStorageInitialized in PersistOrigin() since we don't call EnsureOriginIsInitialized(). (Maybe add a flag into new function to ensure we initialized origin before calling this function?) - Extract EnsureDirectory(), CreateDirectoryMetadata(), CreateDirectoryMetadata2() and InitializeOrigin() from EnsureOriginIsInitialized() and name it as InitializeDirectoryAndOrigin(). - Add Setting mNeedsQuotaManagerInit as true back to make sure calling EnsureStorageIsInitialized() since QuotaRequestBase::Init() is overrided by PersistedOp::Init(). - Keep mSuffix in class PersistedOp since we may need it when creating directory metadata. - Rewrite class PersistOp and PersistedOp since PersistOp don’t need mPersist flag and PersistedOp don’t need mSuffix string Could you help me to review this patch? Thanks!

Attachment #8827091 - Attachment is obsolete: true

Attachment #8827791 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 120

•

8 years ago

Attached patch interdiff-P1-v3-4.patch (obsolete) — Details — Splinter Review

Attachment #8827092 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 121

•

8 years ago

Attached patch [Rebased Patch] Bug-1296591-P0-v6- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). r=janv. (obsolete) — Details — Splinter Review

Update this patch since addressing comment #114.

Attachment #8817250 - Attachment is obsolete: true

Attachment #8827793 - Flags: review+

Jan Varga [:janv]

Comment 122

•

8 years ago

Tom, can you rebase your tree, P1 doesn't apply well to current m-c.

Tom Tung [:tt, :ttung]

Assignee

Comment 123

•

8 years ago

(In reply to Jan Varga [:janv] from comment #122) > Tom, can you rebase your tree, P1 doesn't apply well to current m-c. I implement it base on your four patches in Bug 1311057. While I tried to update them, I cannot build it because PrincipalOriginAttributes is an unknown type name. I cannot find this type in current m-c but I guess I need to change it to OriginAttributes. So, I will give it a try!

Tom Tung [:tt, :ttung]

Assignee

Comment 124

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v4.1-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Rebase the patch to current m-c and the newest patch in RemoveIsApp.

Attachment #8827791 - Attachment is obsolete: true

Attachment #8827791 - Flags: review?(jvarga)

Attachment #8827900 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 125

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P2-v2.1-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Rebased patch as well.

Attachment #8826918 - Attachment is obsolete: true

Attachment #8826918 - Flags: review?(jvarga)

Attachment #8827901 - Flags: review?(jvarga)

Jan Varga [:janv]

Comment 126

•

8 years ago

Comment on attachment 8827900 [details] [diff] [review] [Rebased Patch] Bug-1298329-P1-v4.1-Implement persist/persisted in QuotaManager/QuotaManagerService. Review of attachment 8827900 [details] [diff] [review]: ----------------------------------------------------------------- Tom, as I said, dig deeper into the code and do more self reviews :) I'll send comments and my patch which addresses them later.

Attachment #8827900 - Flags: review?(jvarga)

Jan Varga [:janv]

Comment 127

•

8 years ago

Attached patch patch (obsolete) — Details — Splinter Review

Apply this patch on top of your P1 patch. This time I also ran xpcshell tests, including your new tests from patch P2. Anyway, please test my patch on your side too.

Attachment #8827207 - Attachment is obsolete: true

Jan Varga [:janv]

Comment 128

•

8 years ago

Comment on attachment 8827900 [details] [diff] [review] [Rebased Patch] Bug-1298329-P1-v4.1-Implement persist/persisted in QuotaManager/QuotaManagerService. Review of attachment 8827900 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +1179,5 @@ > +public: > + explicit PersistedOp(const RequestParams& aParams); > + > + bool > + Init(Quota* aQuota) override; Ok, so you saved mSuffix, on the other hand you introduced a new Init() method which is exactly same as the one in PersistOp(). Then you created a static method for both DoInitOnMainThread() methods. All in all it looks like a mix of C and C++ code. Can't you just create a new base class that will do initialization for PersistedOp and PersistOp ? @@ +1227,5 @@ > + static nsresult > + PersistInitOnMainThread(const RequestParams& aParams, > + nsACString* aSuffix, > + nsACString* aGroup, > + OriginScope& aOriginScope); As I said, why a static method ? @@ +3515,5 @@ > + > + RefPtr<OriginInfo> originInfo = LockedGetOriginInfo(PERSISTENCE_TYPE_DEFAULT, > + aGroup, > + aOrigin); > + if (!originInfo) { So, we do all these |if (! ...) { return ... }| to keep the indentation sane. If there's just one condition, you can rather do |if (something)| without the negation. @@ +3533,5 @@ > + > + RefPtr<OriginInfo> originInfo = LockedGetOriginInfo(PERSISTENCE_TYPE_DEFAULT, > + aGroup, > + aOrigin); > + if (!originInfo) { Again, no need for the negation. @@ +4654,5 @@ > } > } > > nsresult > +QuotaManager::InitializeDirectoryAndOrigin(PersistenceType aPersistenceType, As I said, this method doesn't help much with code sharing, efficiency and correctness. @@ +4688,5 @@ > + aSuffix, > + aGroup, > + aOrigin); > + NS_ENSURE_SUCCESS(rv, rv); > + } else { What about this indentation? It looks wrong. @@ +4827,5 @@ > } > } > #endif > > + rv = InitializeDirectoryAndOrigin(aPersistenceType, directory, aSuffix, So the first problem here is that you didn't move/use the |#ifndef RELEASE_OR_BETA| code, I'll let you figure out why it's important. @@ +5135,5 @@ > +QuotaManager::LockedGetOriginInfo(PersistenceType aPersistenceType, > + const nsACString& aGroup, > + const nsACString& aOrigin) > +{ > + mQuotaMutex.AssertCurrentThreadOwns(); Nit: It's nicer to have all assertions in one section here (no need for the empty line) @@ +6763,5 @@ > + > + mPersistenceType.SetValue(PERSISTENCE_TYPE_DEFAULT); > + > + mNeedsMainThreadInit = true; > + mNeedsQuotaManagerInit = true; So you added |mNeedsQuotaManagerInit = true;| again, despite the fact that it's already set in QuotaRequestBase::Init(). The problem is that you forgot to call QuotaRequestBase::Init() here. @@ +6776,5 @@ > + MOZ_ASSERT(GetState() == State_Initializing); > + MOZ_ASSERT(mNeedsMainThreadInit); > + > + nsresult rv = PersistOp::PersistInitOnMainThread(mParams, nullptr, &mGroup, > + mOriginScope); Again, this is C++ not C. @@ +6821,5 @@ > + > + if (!exists) { > + // The directory has not been created yet. > + mPersisted = false; > + return NS_OK; extra space there @@ +6829,5 @@ > + int64_t dummyTimestamp; > + rv = aQuotaManager->GetDirectoryMetadata2WithRestore(directory, > + /* aPersistent */ false, > + &dummyTimestamp, > + &persisted.SetValue()); Not sure why you insist on using the Nullable here. @@ +6942,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + rv = aQuotaManager->InitializeDirectoryAndOrigin(mPersistenceType.Value(), So here, if the directory doesn't exist yet, you create a metadata file with persisted flag set to true and then you open the same file again and rewrite the flag to true. Can't we just create the file with the flag set to true ? ::: dom/quota/QuotaManager.h @@ +281,5 @@ > + InitializeDirectoryAndOrigin(PersistenceType aPersistenceType, > + nsIFile* aDirectory, > + const nsACString& aSuffix, > + const nsACString& aGroup, > + const nsACString& aOrigin); Extracting the whole block in EnsureOriginIsInitialized to this new method doesn't help much.

Jan Varga [:janv]

Comment 129

•

8 years ago

(In reply to Jan Varga [:janv] from comment #128) > So here, if the directory doesn't exist yet, you create a metadata file with > persisted flag set to true and then you open the same file again and rewrite > the flag to true. Can't we just create the file with the flag set to true ? Correction: So here, if the directory doesn't exist yet, you create a metadata file with persisted flag set to *false* and then you open the same file again and rewrite the flag to true. Can't we just create the file with the flag set to true ?

Tom Tung [:tt, :ttung]

Assignee

Comment 130

•

8 years ago

(In reply to Jan Varga [:janv] from comment #128) > ::: dom/quota/ActorsParent.cpp > @@ +1179,5 @@ > Ok, so you saved mSuffix, on the other hand you introduced a new Init() > method which is exactly same as the one in PersistOp(). Then you created a > static method for both DoInitOnMainThread() methods. All in all it looks > like a mix of C and C++ code. > Can't you just create a new base class that will do initialization for > PersistedOp and PersistOp ? Ok, I'll implement a base class next time. I have considered about creating a new base class, but I thought it is better to implement a helper function to extract out the same code. And I was wrong. > @@ +1227,5 @@ > > + static nsresult > > + PersistInitOnMainThread(const RequestParams& aParams, > > + nsACString* aSuffix, > > + nsACString* aGroup, > > + OriginScope& aOriginScope); > > As I said, why a static method ? As I mentioned above, I was wrong. :( I was thinking to have a static helper function so that I don't need to care about class's instance. > @@ +3515,5 @@ > > + > > + RefPtr<OriginInfo> originInfo = LockedGetOriginInfo(PERSISTENCE_TYPE_DEFAULT, > > + aGroup, > > + aOrigin); > > + if (!originInfo) { > > So, we do all these |if (! ...) { return ... }| to keep the indentation sane. > If there's just one condition, you can rather do |if (something)| without > the negation. Thanks, I'll keep it in mind. > @@ +4688,5 @@ > > + aSuffix, > > + aGroup, > > + aOrigin); > > + NS_ENSURE_SUCCESS(rv, rv); > > + } else { > > What about this indentation? It looks wrong. Sorry, it should not happen. > @@ +4827,5 @@ > > } > > } > > #endif > > > > + rv = InitializeDirectoryAndOrigin(aPersistenceType, directory, aSuffix, > > So the first problem here is that you didn't move/use the |#ifndef > RELEASE_OR_BETA| code, I'll let you figure out why it's important. I thought it is a check for preventing from an origin which cannot be parsed. The failure happens only in test cases (fake origin). However, I was wrong, and it really happens in real origin. > @@ +6763,5 @@ > > + > > + mPersistenceType.SetValue(PERSISTENCE_TYPE_DEFAULT); > > + > > + mNeedsMainThreadInit = true; > > + mNeedsQuotaManagerInit = true; > > So you added |mNeedsQuotaManagerInit = true;| again, despite the fact that > it's already set in QuotaRequestBase::Init(). The problem is that you forgot > to call QuotaRequestBase::Init() here. Sorry for not explain clearly or misunderstanding. :( I tried to express it in comment #119. I override QuotaRequestBase::Init() by Persisted::Init() so I need to reset it to true again. > > @@ +6776,5 @@ > > + MOZ_ASSERT(GetState() == State_Initializing); > > + MOZ_ASSERT(mNeedsMainThreadInit); > > + > > + nsresult rv = PersistOp::PersistInitOnMainThread(mParams, nullptr, &mGroup, > > + mOriginScope); > > Again, this is C++ not C. I'll address base-class method next time > @@ +6829,5 @@ > > + int64_t dummyTimestamp; > > + rv = aQuotaManager->GetDirectoryMetadata2WithRestore(directory, > > + /* aPersistent */ false, > > + &dummyTimestamp, > > + &persisted.SetValue()); > > Not sure why you insist on using the Nullable here. I thought we don't to set "mPersisted" as failure happens, so we need a temporary variable to store it. And it's not necessary to create another boolean since we've already haven one. Please correct me if there is any problem here! Thanks! > ::: dom/quota/QuotaManager.h > @@ +281,5 @@ > > + InitializeDirectoryAndOrigin(PersistenceType aPersistenceType, > > + nsIFile* aDirectory, > > + const nsACString& aSuffix, > > + const nsACString& aGroup, > > + const nsACString& aOrigin); > > Extracting the whole block in EnsureOriginIsInitialized to this new method > doesn't help much. I'll think about a better approach next time! (In reply to Jan Varga [:janv] from comment #129) > Correction: > So here, if the directory doesn't exist yet, you create a metadata file with > persisted flag set to *false* and then you open the same file again and > rewrite > the flag to true. Can't we just create the file with the flag set to true ? That's a good idea! I'll do it in next patch!

Jan Varga [:janv]

Comment 131

•

8 years ago

(In reply to Tom Tung [:tt] from comment #130) > > So the first problem here is that you didn't move/use the |#ifndef > > RELEASE_OR_BETA| code, I'll let you figure out why it's important. > > I thought it is a check for preventing from an origin which cannot be > parsed. The failure happens only in test cases (fake origin). However, I was > wrong, and it really happens in real origin. It's a guard. Before we create a new origin directory we make sure that it can be parsed by our origin parser. PersistOp needs to check it too. > > So you added |mNeedsQuotaManagerInit = true;| again, despite the fact that > > it's already set in QuotaRequestBase::Init(). The problem is that you forgot > > to call QuotaRequestBase::Init() here. > > Sorry for not explain clearly or misunderstanding. :( > I tried to express it in comment #119. > I override QuotaRequestBase::Init() by Persisted::Init() so I need to reset > it to true again. Overridden methods can call methods which they are overriding, right ? So if we add something to QuotaRequestBase::Init() it will be automatically executed by a derived class. > > Not sure why you insist on using the Nullable here. > > I thought we don't to set "mPersisted" as failure happens, so we need a > temporary variable to store it. And it's not necessary to create another > boolean since we've already haven one. > > Please correct me if there is any problem here! Thanks! Local variables are quite cheap, they are on the stack. Having a new 1-byte variable is not a big deal, easier than passing |&persisted.SetValue()| > > So here, if the directory doesn't exist yet, you create a metadata file with > > persisted flag set to *false* and then you open the same file again and > > rewrite > > the flag to true. Can't we just create the file with the flag set to true ? > > That's a good idea! I'll do it in next patch! Just to be sure, you just need to apply my patch and that's it (if you don't find anything problematic during your testing).

Jan Varga [:janv]

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P2-v3-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Update patch for testing (P2). - I rewrite the "test_persistent_basic.js" and rename it as "test_persist_basic.js". - In "test_persist_basic.js", I test both persist() and persisted() before and after EnsureOriginIsInitialized(). Besides, I also test them when the origin cannot be parsed. Next: I'll rename "test_persistent_storage.js" later.

Tom Tung [:tt, :ttung]

Assignee

Comment 137

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P2-v3-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Updated patch again!

Attachment #8828695 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 138

•

8 years ago

Comment on attachment 8827901 [details] [diff] [review] [Rebased Patch] Bug-1298329-P2-v2.1-Test for persisted() and persist(). Remove the r? request since I found some similar problems in the other two test cases. I'll fix them and send a review request later. Sorry for that if you are reviewing them.

Attachment #8827901 - Flags: review?(jvarga)

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

Comment 139

•

8 years ago

I'm a bit worried about we add callback into Request instead of registering callback in the beginning. Right now we call persist() and then get nsIQuotaRequest instance, then setCallback(). What if we got callback (ex: onComplete) before we setCallback()? Since we removed callback parameter, do we guarantee callback can't be fired before we register callback? RefPtr<nsIQuotaRequest> request; rv = CheckIsPersistentForPrincipal(principal, getter_AddRefs(request)); if (NS_SUCCEEDED(rv)) { request->SetCallback(resolver); }

Jan Varga [:janv]

Comment 140

•

8 years ago

You are still on the same thread, right ? So nothing else can touch the callback. And all the methods in nsIQuotaManagerService shouldn't synchronously fire any callbacks when they are being executed. Even when a result is ready, a new runnable should be dispatched to current thread to fire the callback asynchronously. You can add comments for this if you want.

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

Comment 141

•

8 years ago

(In reply to Jan Varga [:janv] from comment #140) > You are still on the same thread, right ? So nothing else can touch the > callback. > And all the methods in nsIQuotaManagerService shouldn't synchronously fire > any callbacks when they are being executed. Even when a result is ready, a > new runnable should be dispatched to current thread to fire the callback > asynchronously. You can add comments for this if you want. I see, thanks for the explanation.

Fischer [:Fischer]

Updated

•

8 years ago

Blocks: 1312375

Tom Tung [:tt, :ttung]

Assignee

Comment 142

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P2-v3.1-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Update patch.

Attachment #8827901 - Attachment is obsolete: true

Attachment #8828697 - Attachment is obsolete: true

Jan Varga [:janv]

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P2-v3.2-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Hi Jan, In this patch, I rewrite the test cases. Could you help me to review it? Thanks! The test_persist.js test the persist() and persisted() before and after calling EnsureOriginIsInitialized(). Besides, it also verifies the persist() will fail when the origin cannot be parsed. In test_persist_quotaExceeded.js, I test the quota exceeded situations before calling persist() and after calling persist(). First, I test two different origins in the same group when the quata is exceeded. Before persisting one of them, they shared the same group limit & usage. After that, the best-effort one can store more data since the persisted origin doesn't share the same group usage. Moreover, it can store more because it doesn't be bounded by group limit but by the global limit. In the end of the test, I use the other origin which is not in the same group to test eviction won't happen on the persisted origin.

Attachment #8829808 - Attachment is obsolete: true

Attachment #8829825 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 147

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v5-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Update the patch but I will check it again tomorrow!

Attachment #8827900 - Attachment is obsolete: true

Attachment #8828253 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 148

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v5.1-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Hi Jan, Except apply your comments & patch, I re-write few function and I list them below inline: - Make function QuotaManager::GetDirectoryMetadata2(..) accept passing 'nullptr' to aTimestamp as well for removing dummyTimestamp in PersistedOp::DoDirectoryWork() and PersistOp::DoDirectoryWork. However, I modified the assertion to MOZ_ASSERT(aTimestamp || aPersisted) and I'm not sure about whether it is ok or not. Please help me check this part! - Add comments before passing 'nullptr' to make it more readable. - Rewrite QuotaManager::LockedGetOriginInfo() to remove negation condition but I'm not sure about this as well. Please help me check this part as well. Would you mind reviewing this patch again? Thanks! I'll upload the inter-diff patch (between applying your patch and this patch) later.

Attachment #8829827 - Attachment is obsolete: true

Attachment #8830134 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 149

•

8 years ago

Attached patch interdiff-P1-v4-5.patch (obsolete) — Details — Splinter Review

inter-diff patch for P1 between my previous r? patch + :janv's patch and current r? patch.

Attachment #8827792 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 150

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v5.2-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

Update patch to make all the Persist* things (e.g. PersistParams) are sorted alphabetically. Sorry for that if you are reviewing it, Jan. :(

Attachment #8830134 - Attachment is obsolete: true

Attachment #8830134 - Flags: review?(jvarga)

Attachment #8832809 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 151

•

8 years ago

Attached patch interdiff-P1-v4-5.2.patch (obsolete) — Details — Splinter Review

inter-diff patch for P1 between my previous reviewed patch & Jan's patch and current r? patch.

Attachment #8830135 - Attachment is obsolete: true

Jan Varga [:janv]

Comment 152

•

8 years ago

(In reply to Tom Tung [:tt] from comment #150) > Created attachment 8832809 [details] [diff] [review] > [Rebased Patch] Bug-1298329-P1-v5.2-Implement persist/persisted in > QuotaManager/QuotaManagerService. > > Update patch to make all the Persist* things (e.g. PersistParams) are sorted > alphabetically. Sorry for that if you are reviewing it, Jan. :( That union mostly follows the order of methods in nsIQuotaManagerService, I don't think we need to sort them alphabetically here.

Tom Tung [:tt, :ttung]

Assignee

Comment 153

•

8 years ago

(In reply to Jan Varga [:janv] from comment #152) > That union mostly follows the order of methods in nsIQuotaManagerService, I > don't think we need to sort them alphabetically here. Oh, I see. I will correct them back in next patch! Thanks!

Tom Tung [:tt, :ttung]

Assignee

Comment 154

•

8 years ago

Attached patch [Rebased Patch] Bug-1298329-P1-v5.3-Implement persist/persisted in QuotaManager/QuotaManagerService. (obsolete) — Details — Splinter Review

I changed my mind. I decided to update patch for following the order in nsIQutaManagerService.

Attachment #8832809 - Attachment is obsolete: true

Attachment #8832809 - Flags: review?(jvarga)

Attachment #8833142 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 155

•

8 years ago

Attached patch interdiff-P1-v4-5.3.patch (obsolete) — Details — Splinter Review

Update inter-diff patch as well

Attachment #8832810 - Attachment is obsolete: true

Jan Varga [:janv]

Comment 156

•

8 years ago

Comment on attachment 8833142 [details] [diff] [review] [Rebased Patch] Bug-1298329-P1-v5.3-Implement persist/persisted in QuotaManager/QuotaManagerService. Review of attachment 8833142 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Attachment #8833142 - Flags: review?(jvarga) → review+

Francis Lee [:frlee]

Updated

•

8 years ago

Whiteboard: storage-v1 → [storage-v1]

Jan Varga [:janv]

•

8 years ago

Comment on attachment 8827793 [details] [diff] [review] [Rebased Patch] Bug-1296591-P0-v6- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). r=janv. Remove the patches to avoid confusing since :janv have already updated them.

Attachment #8827793 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 165

•

8 years ago

Comment on attachment 8833142 [details] [diff] [review] [Rebased Patch] Bug-1298329-P1-v5.3-Implement persist/persisted in QuotaManager/QuotaManagerService. Remove it because it have been updated in comment 158.

Attachment #8833142 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 166

•

8 years ago

Attached patch Bug-1298329-P2-v4-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Attachment #8829825 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 167

•

8 years ago

Attached patch Bug-1298329-P2-v4.1-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Attachment #8842700 - Attachment is obsolete: true

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

Comment 168

•

8 years ago

Comment on attachment 8842705 [details] [diff] [review] Bug-1298329-P2-v4.1-Test for persisted() and persist(). Review of attachment 8842705 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/test/unit/head.js @@ +264,5 @@ > } > > +function persist(principal, callback) { > + let request = SpecialPowers._getQuotaManager().persist(principal); > + nit: Based on the style of head.js, this line can be removed. @@ +272,5 @@ > +} > + > +function persisted(principal, callback) { > + let request = SpecialPowers._getQuotaManager().persisted(principal); > + ditto.

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

Comment 169

•

8 years ago

Comment on attachment 8842705 [details] [diff] [review] Bug-1298329-P2-v4.1-Test for persisted() and persist(). Review of attachment 8842705 [details] [diff] [review]: ----------------------------------------------------------------- test_persist.js looks good to me.

Tom Tung [:tt, :ttung]

Assignee

Comment 170

•

8 years ago

Attached patch Bug-1298329-P2-v4.2-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Address Shawn's feedback! Hi Jan, I rewrite the test_persist.js to follow the test format in dom/quota/test/unit. Could you help me to review it? Thanks!

Attachment #8842705 - Attachment is obsolete: true

Attachment #8842839 - Flags: review?(jvarga)

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

Updated

•

8 years ago

Component: DOM → DOM: Quota Manager

Jan Varga [:janv]

Comment 171

•

8 years ago

Attached patch P1 - rebased on top of bug 1339081 (obsolete) — Details — Splinter Review

rebased again

Attachment #8841919 - Attachment is obsolete: true

Attachment #8843955 - Flags: review+

Jan Varga [:janv]

Updated

•

8 years ago

Attachment #8841918 - Flags: review+

Jan Varga [:janv]

Comment 172

•

8 years ago

Comment on attachment 8842839 [details] [diff] [review] Bug-1298329-P2-v4.2-Test for persisted() and persist(). Review of attachment 8842839 [details] [diff] [review]: ----------------------------------------------------------------- See my comments for test_persist.js below If you want to be really sure about persist() actually updating the metadata file, you can use FileReader to verify that. See dom/indexedDB/test/unit/test_wasm_recompile.js how it's done. Or test_obsoleteOriginAttributesUpgrade.js in https://bug1339081.bmoattachments.org/attachment.cgi?id=8843718 ::: dom/quota/test/unit/head.js @@ +262,5 @@ > .getService(Ci.nsIScriptSecurityManager); > return ssm.createCodebasePrincipal(uri, {}); > } > > +function persist(principal, callback) { Please move these two new methods after reset() ::: dom/quota/test/unit/test_persist.js @@ +11,5 @@ > + url: "http://default.testPersist", > + persistence: "default" > + }; > + > + const principal = getPrincipal(origin.url); I think "let" is more suitable here. @@ +13,5 @@ > + }; > + > + const principal = getPrincipal(origin.url); > + > + info("Persist an origin before it is initialized"); The convention in these tests is to use present continuous tense here. "Persisting an uninitialized origin" sounds better. @@ +17,5 @@ > + info("Persist an origin before it is initialized"); > + > + let request = persisted(principal, continueToNextStepSync); > + yield undefined; > + We should check request.resultCode for persisted() too @@ +18,5 @@ > + > + let request = persisted(principal, continueToNextStepSync); > + yield undefined; > + > + ok(!request.result, "The origin shouldn't be persisted by default"); The convention in these tests is to use something like this: "The origin is not persisted" @@ +23,5 @@ > + > + request = persist(principal, continueToNextStepSync); > + yield undefined; > + > + ok(request.resultCode === Components.results.NS_OK, NS_OK is now defined as a const in head.js @@ +24,5 @@ > + request = persist(principal, continueToNextStepSync); > + yield undefined; > + > + ok(request.resultCode === Components.results.NS_OK, > + "The persist() operation shouldn't fail in this case"); "Persist succeeded" sounds better. @@ +29,5 @@ > + > + request = persisted(principal, continueToNextStepSync); > + yield undefined; > + > + ok(request.result, "The origin should be persisted after calling persist()"); "The origin is persisted" @@ +34,5 @@ > + > + info("Clearing the origin") > + > + // Clear the origin since we'll test the same directory again under different > + // situation. "under different circumstances" @@ +60,5 @@ > + yield undefined; > + > + ok(request.result, "The origin should be persisted after calling persist()"); > + > + info("Persist an unparsable origin"); "invalid" sounds better @@ +62,5 @@ > + ok(request.result, "The origin should be persisted after calling persist()"); > + > + info("Persist an unparsable origin"); > + > + const badPrincipal = getPrincipal("ftp://ftp.example.com"); I think "let" is more suitable here. "ftp://ftp.example.com" can be a const, defined after "const origin".

Attachment #8842839 - Flags: review?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

•

8 years ago

(In reply to Jan Varga [:janv] from comment #175) Thanks, again! > Btw, the name - test_persist_quotaExceeded.js is not exactly what we want, > I'll try to figure out a better name Sure, thank you! > ::: dom/indexedDB/test/unit/test_persist_quotaExceeded.js > @@ +25,5 @@ > Nit: just Database ? Yes > @@ +60,5 @@ > > + } > > + > > + info("Got abort event."); > > + > > + ok(success === expectedWriteSuccess, > > Ok, I understand that "===" is sometimes needed to avoid conversion before > the comparison happens, but is it needed for simple bools like we have here ? > Is there maybe a performance advantage for using "===" instead of "==" ? No, I was thinking to make it as strict as possible. > @@ +90,5 @@ > > + const groupLimitMB = 8 * 1024; > > + // The group limit is calculated as 20% of the global temporary storage limit. > > + const globalStorageLimitKB = groupLimitMB * 5; > > + > > + setTemporaryStorageLimit(globalStorageLimitKB); > > You should call clearAllDatabase() or resetAllDatabases() after calling > SetTemporaryStorageLimit() to force quota manager to use the new pref value. > In this special case quota manager doesn't exist yet, but it's still better > to explicitly call one of those methods. Ok, I see! > @@ +124,5 @@ > > + > > + info("Step 2: Fill up the databases under the same group to reach group " + > > + "limit."); > > + > > + yield* fillUpDataBase(db[1], true /* expectedWriteSuccess */); > > Ok, so this fills the db until we get quota exceeded. > > @@ +128,5 @@ > > + yield* fillUpDataBase(db[1], true /* expectedWriteSuccess */); > > + > > + // We cannot write more object into the other db since the two dbs share > > + // the same group limit. > > + yield* fillUpDataBase(db[0], false /* expectedWriteSuccess */); > > Here, we actually don't try to fill the database, we expect that any add/put > should result in quota exceeded error. > expectedWriteSuccess is a bit confusing/misleading name. > If I understand it correctly, it means that at least one add succeeded. Yes, you got it. I used to name it as somethings like "atLeastOneSuccess", but I thought "expectedWriteSuccess" is better. I'll sort it out!

Tom Tung [:tt, :ttung]

Assignee

Comment 177

•

8 years ago

Attached patch Bug-1298329-P2-v5-Test for persisted() and persist(). (obsolete) — Details — Splinter Review

Sorry for obsoleting reviewing patch. I didn't notice it is still reviewed. Update the patch for addressing current comment

Attachment #8844366 - Attachment is obsolete: true

Tom Tung [:tt, :ttung]

Assignee

Comment 178

•

Attachment #8845309 - Flags: review+

Tom Tung [:tt, :ttung]

Assignee

Comment 186

•

8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6da47efdf966

Pulsebot

Comment 187

•

8 years ago

Pushed by shuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7c15b51fce Part 0: Add persisted attribute to OriginInfo; r=janv

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

Updated

•

8 years ago

Keywords: leave-open

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 188

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/1b7c15b51fce

Tom Tung [:tt, :ttung]

•

8 years ago

Attached patch Bug-1298329 - Part 2: Add a test to verify persist()/persisted(); r=janv (obsolete) — Details — Splinter Review

Address comment, split eviction test to another patch and update commit message.

Attachment #8848344 - Attachment is obsolete: true

Attachment #8848344 - Flags: review?(jvarga)

Jan Varga [:janv]

Comment 196

•

8 years ago

(In reply to Tom Tung [:tt] from comment #195) > Created attachment 8848420 [details] [diff] [review] > Bug-1298329 - Part 2: Add a test to verify persist()/persisted(); r=janv > > Address comment, split eviction test to another patch and update commit > message. Sounds good, just remove the "-"

Jan Varga [:janv]

Updated

•

8 years ago

Attachment #8848420 - Flags: review+

•

8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2111587fa9a13b0c082d31eb611b8fb46bc1f13

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

Comment 201

•

8 years ago

(In reply to Tom Tung [:tt] from comment #200) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=b2111587fa9a13b0c082d31eb611b8fb46bc1f13 I can reproduce this on my linux machine. 0:00.55 TEST_STATUS: Thread-1 testSteps PASS [testSteps : 34] Persisted succeeded - true == true 0:00.55 TEST_STATUS: Thread-1 testSteps PASS [testSteps : 35] The origin is not persisted - true == true 0:00.55 LOG: Thread-1 INFO "Verifying persist() does update the metadata" 0:00.56 TEST_STATUS: Thread-1 testSteps PASS [testSteps : 42] Persist() succeeded - true == true 0:00.56 TEST_STATUS: Thread-1 testSteps FAIL [testSteps : 46] Origin directory does exist - false == true

Tom Tung [:tt, :ttung]

Assignee

Comment 202

•

8 years ago

(In reply to Shawn Huang [:shawnjohnjr] from comment #201) > (In reply to Tom Tung [:tt] from comment #200) > > https://treeherder.mozilla.org/#/ > > jobs?repo=try&revision=b2111587fa9a13b0c082d31eb611b8fb46bc1f13 > > I can reproduce this on my linux machine. > 0:00.55 LOG: Thread-1 INFO "Verifying persist() does update the metadata" > 0:00.56 TEST_STATUS: Thread-1 testSteps PASS [testSteps : 42] Persist() > succeeded - true == true > 0:00.56 TEST_STATUS: Thread-1 testSteps FAIL [testSteps : 46] Origin > directory does exist - false == true Hi Jan, We found failure[1] happens in linux machine, but do not in mac (my local machine). In "test_persist.js", I persist an uninitialized origin, and it should create the metadata on its directory. Thus, I expceted fileReader can read the directory (dir should exist). However, I got failed when checking for this. I guess the reason is that we parse the same origin to different origin path on Linux and Mac. Could you give me some suggestions for this? Thanks! [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=98c4759d99a80d9e90f17f63ddb78d30c84b5ffa&selectedJob=84581132&filter-searchStr=Linux%20debug%20Xpcshell%20tests%20executed%20by%20TaskCluster%20test-linux32%2Fdebug-xpcshell-3%20tc-X(X3)

Flags: needinfo?(jvarga)

Tom Tung [:tt, :ttung]

Assignee

Comment 203

•

8 years ago

(In reply to Tom Tung [:tt] from comment #202) > for this. I guess the reason is that we parse the same origin to different > origin path on Linux and Mac. I meant I wonder the formats of path string in Linux and Mac are different for the same origin, so we can get "storage/default/http+++default.testPersist" in Mac, but cannot get it in Linux.

Jan Varga [:janv]

Comment 204

•

8 years ago

The path should be the same on both systems.

Flags: needinfo?(jvarga)

Jan Varga [:janv]

Comment 205

•

8 years ago

If you can reproduce it easily on your linux machine then it should be easy to debug the problem.

Jan Varga [:janv]

Comment 206

•

8 years ago

Could it be related to lowercase vs uppercase ? http://default.testPersist vs http://default.testpersist

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

Comment 207

•

8 years ago

(In reply to Jan Varga [:janv] from comment #206) > Could it be related to lowercase vs uppercase ? > http://default.testPersist vs http://default.testpersist Hi Tom, For your reference, on my Linux machine, the folder is just lowercase. ./mach xpcshell-test --debugger gdb --debugger-interactive dom/quota/test/unit/test_persist.js (gdb) b QuotaManagerService.cpp:752 Then enable breakpoint (gdb) b nsDirectoryService.cpp:344 Breakpoint 2 at 0x7ffff25e2308: file /code/mozilla-inbound/xpcom/io/nsDirectoryService.cpp, line 344. Checking /tmp/firefox/xpcshellprofile/storage/default /tmp/firefox/xpcshellprofile/storage/default$ ls http+++default.testpersist

Tom Tung [:tt, :ttung]

Assignee

•

8 years ago

Attached patch Bug 1298329 - Part 4: Lowercase the url and path string in test_persist.js to fix the case sensitive issue on Linux; (obsolete) — Details — Splinter Review

Hi Jan, I lowercase the path and url strings in test_persist.js to fix the problem on the test. Could you help me to review it? Thanks!

Attachment #8848964 - Flags: review?(jvarga)

Jan Varga [:janv]

Comment 214

•

8 years ago

Comment on attachment 8848964 [details] [diff] [review] Bug 1298329 - Part 4: Lowercase the url and path string in test_persist.js to fix the case sensitive issue on Linux; Review of attachment 8848964 [details] [diff] [review]: ----------------------------------------------------------------- Ok, but I don't think this should be a separate patch.

Attachment #8848964 - Flags: review?(jvarga) → review+

Tom Tung [:tt, :ttung]

Assignee

Comment 215

•

8 years ago

Attached patch Bug 1298329 - Part 2: Add a test to verify persist()/persisted(); r=janv (obsolete) — Details — Splinter Review

(In reply to Jan Varga [:janv] from comment #214) > Ok, but I don't think this should be a separate patch. Thanks! I merge it with Part 2.

Attachment #8848427 - Attachment is obsolete: true

Attachment #8848964 - Attachment is obsolete: true

Attachment #8848998 - Flags: review+

Tom Tung [:tt, :ttung]

Assignee

Comment 216

•

8 years ago

Attached patch Bug 1298329 - Part 2: Add a test to verify persist()/persisted(); r=janv — Details — Splinter Review

Sorry for disturbance! I found that I updated the wrong patch, so updated it again!

Attachment #8848998 - Attachment is obsolete: true

Attachment #8849000 - Flags: review+

Pulsebot

Comment 217

•

8 years ago

Pushed by shuang@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d7e8cd0ad3c0 Part 1: Implement persist/persisted in QuotaManager and expose them to QuotaManagerService; r=janv https://hg.mozilla.org/integration/mozilla-inbound/rev/4217af9ee902 Part 2: Add a test to verify persist()/persisted(); r=janv

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 218

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/d7e8cd0ad3c0 https://hg.mozilla.org/mozilla-central/rev/4217af9ee902

Tom Tung [:tt, :ttung]

Assignee

Comment 219

•

8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4ba73c5430f4251cf8f6f7138edbec6c002ce46

Jan Varga [:janv]

•

7 years ago

(In reply to Jan Varga [:janv] from comment #221) > However, other part of the test related to eviction is not clear to me ... I tried to ensure the persisted origin isn't evicted by the QM. I reference from test_temporary_storage.js [1]. It makes sure the eviction happens by storing objects into some databases and reaching the global limit, then close these databases (make them become inactive). Then open databases in other different origins and ensure store objects into them successfully (eviction is triggered). > ::: dom/indexedDB/test/unit/test_persist_quotaExceeded.js > @@ +193,5 @@ > > + resetAllDatabases(continueToNextStepSync); > > + > > + yield undefined; > > + > > + info("Step 5: Ensure the eviction won't happen on persisted db"); > > "Step 5: Ensure the eviction won't happen on persisted db" - I don't see how > is this checked here. > > The rest of this test doesn't makes sense to me. > Tom, can you describe what you wanted to check here ? > Do you just want to make sure that persisted origin won't be ever evicted ? Yes, that's what I want to check. > @@ +214,5 @@ > > + info("Fill the persisted db"); > > + > > + // The eviction should be triggered since the space is full and QM evicts the > > + // databases[0]. Thus, we could store more objects into databases[1] after > > + // eviction. > > How do you know that eviction is triggered ? So, I tried to ensure it in the opposite way (eviction isn't triggered). I fill up the persisted database first (make sure we write at least one object into db) and close it, after that ensure we cannot write any object into the other best-effort db. And, it indicates the eviction isn't triggered. If we fill up the best-effort db (origin A) first, then close it and fill up another best-effort db (origin B). It should success because the QM evict the inactive origin A when we want to write somethings into origin B. > @@ +224,5 @@ > > + info("Fill the best-effort db under the different groups"); > > + > > + // The eviction is not triggered since the persisted db shouldn't be evicted > > + // by QM to free up more space. > > + yield* fillUpDatabase(databases[2], false /* expectedAtLeastOneAdd */); > > How do you know that eviction is not triggered ? (QM is empty) First, I make the persisted origin keep storing objects until reaching the global limit. (QM only have one persisted origin and its usage is the same as global limit) Then, I ensure the persisted origin becomes in-active by close the database. (QM only have one persisted origin, its usage is the same as global limit, and it is inactive) Finally, We cannot write anything into the other best-effort origin. It means the QM cannot find any inactive non-persisted origins. [1] http://searchfox.org/mozilla-central/source/dom/indexedDB/test/unit/test_temporary_storage.js#130-132

Jan Varga [:janv]

Comment 223

•

7 years ago

Well, this just confirms that it's very complicated to write these tests using IndexedDB. I'm not convinced that your tests actually tests eviction after persist thoroughly. test_temporary_storage.js was rewritten when we switched to WAL. The test is not so precise as it was before, so it uses some tricks to work around IndexedDB behavior (compression, delayed closing of files, etc). I'll submit a patch for simpledb soon, so you can try to write a test for the eviction using the new simple quota client.

Tom Tung [:tt, :ttung]

Assignee

Comment 224

•

7 years ago

(In reply to Jan Varga [:janv] from comment #223) > I'll submit a patch for simpledb soon, so you can try to write a test for > the eviction using the new simple quota client. Sure, I'll try to re-write it after that. Thanks!

Tom Tung [:tt, :ttung]

Assignee

Comment 225

•

7 years ago

Attached patch Test to verify eviction (obsolete) — Details — Splinter Review

Rewrite the test of eviction part. The test for group limit before/after persist is covered by bug 1361330.

Attachment #8848429 - Attachment is obsolete: true

Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)

Updated

•

7 years ago

Blocks: 1389380

Tom Tung [:tt, :ttung]

Assignee

•

7 years ago

Close this bug, since all the patches were landed.

Status: NEW → RESOLVED

Closed: 7 years ago

Resolution: --- → FIXED

Firefox Bug Husbandry Bot

Comment 229

•

7 years ago

Removing leave-open keyword from resolved bugs, per :sylvestre.

Keywords: leave-open

Bug-1298329-v0-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 49.20 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-v1-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 66.16 KB, patch		Details \| Diff \| Splinter Review
interdiff-0-1.txt 8 years ago Tom Tung [:tt, :ttung] 33.52 KB, patch		Details \| Diff \| Splinter Review
[WIP] Bug-1296591-P0-v0- Bound persistent storage by total quota. 8 years ago Tom Tung [:tt, :ttung] 36.83 KB, patch		Details \| Diff \| Splinter Review
[WIP] Bug-1298329-P1-v0-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 57.15 KB, patch		Details \| Diff \| Splinter Review
[WIP] Bug-1296591-P0-v0.1- Bound persistent storage by total quota. 8 years ago Tom Tung [:tt, :ttung] 33.20 KB, patch	janv : feedback+	Details \| Diff \| Splinter Review
[WIP] Bug-1296591-P0-v1- Bound persistent storage by total quota. 8 years ago Tom Tung [:tt, :ttung] 29.25 KB, patch		Details \| Diff \| Splinter Review
Bug-1296591-P0-v1.1- Bound persistent storage by total quota. 8 years ago Tom Tung [:tt, :ttung] 28.77 KB, patch		Details \| Diff \| Splinter Review
[WIP] Bug-1298329-P1-v1-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 40.94 KB, patch		Details \| Diff \| Splinter Review
[WIP] Test for persisted() and persist() 8 years ago Tom Tung [:tt, :ttung] 15.41 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-P1-v1.1-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 43.21 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-P2-v0.1-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 18.69 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-P1-v1.2-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 44.19 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-P2-v0.2-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 18.61 KB, patch		Details \| Diff \| Splinter Review
[WIP] Bug-1296591-P0-v2- Bound persistent storage by total quota. 8 years ago Tom Tung [:tt, :ttung] 39.36 KB, patch		Details \| Diff \| Splinter Review
[WIP] Bug-1298329-P1-v1.3-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 42.59 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1296591-P0-v2.1- Bound persistent storage by total quota. 8 years ago Tom Tung [:tt, :ttung] 18.02 KB, patch		Details \| Diff \| Splinter Review
[WIP] Bug-1298329-P1-v1.4-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 39.94 KB, patch		Details \| Diff \| Splinter Review
[WIP] Bug-1298329-P2-v1.1-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 18.59 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P2-v1.2-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 18.59 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v1.5-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 40.07 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1296591-P0-v3- Add persisted attribute to originInfo and bound origins with this attribute only by total quota (not by group quota). 8 years ago Tom Tung [:tt, :ttung] 18.57 KB, patch		Details \| Diff \| Splinter Review
interdiff-P0-v2.1-v3.patch 8 years ago Tom Tung [:tt, :ttung] 1.76 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1296591-P0-v4- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). 8 years ago Tom Tung [:tt, :ttung] 19.56 KB, patch	janv : review+	Details \| Diff \| Splinter Review
interdiff-P0-v3-v4.patch 8 years ago Tom Tung [:tt, :ttung] 3.74 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1296591-P0-v5- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). r=janv. 8 years ago Tom Tung [:tt, :ttung] 20.37 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v2-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 34.34 KB, patch		Details \| Diff \| Splinter Review
interdiff-P1-v1.5-2.patch 8 years ago Tom Tung [:tt, :ttung] 32.04 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P2-v2-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 18.41 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v3-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 33.68 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v3.1-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 34.02 KB, patch		Details \| Diff \| Splinter Review
interdiff-P1-v2-3.1.patch 8 years ago Tom Tung [:tt, :ttung] 11.48 KB, patch		Details \| Diff \| Splinter Review
patch 8 years ago Jan Varga [:janv] 14.96 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v4-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 46.95 KB, patch		Details \| Diff \| Splinter Review
interdiff-P1-v3-4.patch 8 years ago Tom Tung [:tt, :ttung] 21.76 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1296591-P0-v6- Add persisted attribute to originInfo and bound persisted origins only by total quota (not by group quota). r=janv. 8 years ago Tom Tung [:tt, :ttung] 21.11 KB, patch	tt : review+	Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v4.1-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 45.50 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P2-v2.1-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 18.42 KB, patch		Details \| Diff \| Splinter Review
patch 8 years ago Jan Varga [:janv] 24.13 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P2-v3-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 18.17 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P2-v3-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 18.17 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P2-v3.1-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 10.85 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P2-v3.2-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 11.57 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v5-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 44.70 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v5.1-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 44.90 KB, patch		Details \| Diff \| Splinter Review
interdiff-P1-v4-5.patch 8 years ago Tom Tung [:tt, :ttung] 2.96 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v5.2-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 44.94 KB, patch		Details \| Diff \| Splinter Review
interdiff-P1-v4-5.2.patch 8 years ago Tom Tung [:tt, :ttung] 5.02 KB, patch		Details \| Diff \| Splinter Review
[Rebased Patch] Bug-1298329-P1-v5.3-Implement persist/persisted in QuotaManager/QuotaManagerService. 8 years ago Tom Tung [:tt, :ttung] 44.90 KB, patch	janv : review+	Details \| Diff \| Splinter Review
interdiff-P1-v4-5.3.patch 8 years ago Tom Tung [:tt, :ttung] 3.40 KB, patch		Details \| Diff \| Splinter Review
P0 - rebased on top of bug 1339081 8 years ago Jan Varga [:janv] 21.03 KB, patch	janv : review+	Details \| Diff \| Splinter Review
P1 - rebased on top of bug 1339081 8 years ago Jan Varga [:janv] 42.40 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-P2-v4-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 13.08 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-P2-v4.1-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 12.94 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-P2-v4.2-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 12.94 KB, patch		Details \| Diff \| Splinter Review
P1 - rebased on top of bug 1339081 8 years ago Jan Varga [:janv] 42.28 KB, patch	janv : review+	Details \| Diff \| Splinter Review
Bug-1298329-P2-v5-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 16.03 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-P2-v5-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 16.08 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329-P2-v5-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 16.31 KB, patch		Details \| Diff \| Splinter Review
Bug 1298329 - Part 0: Add persisted attribute to OriginInfo; r=janv 8 years ago Tom Tung [:tt, :ttung] 20.98 KB, patch	tt : review+	Details \| Diff \| Splinter Review
Bug-1298329-P2-v5.1-Test for persisted() and persist(). 8 years ago Tom Tung [:tt, :ttung] 16.37 KB, patch		Details \| Diff \| Splinter Review
Bug-1298329 - Part 1: Implement persist/persisted in QuotaManager and expose them to QuotaManagerService; r=janv 8 years ago Tom Tung [:tt, :ttung] 42.33 KB, patch	janv : review+	Details \| Diff \| Splinter Review
Bug-1298329 - Part 2: Add a test to verify persist()/persisted(); r=janv 8 years ago Tom Tung [:tt, :ttung] 7.41 KB, patch	janv : review+	Details \| Diff \| Splinter Review
Bug 1298329 - Part 1: Implement persist/persisted in QuotaManager and expose them to QuotaManagerService; r=janv 8 years ago Tom Tung [:tt, :ttung] 42.33 KB, patch	tt : review+	Details \| Diff \| Splinter Review
Bug 1298329 - Part 2: Add a test to verify persist()/persisted(); r=janv 8 years ago Tom Tung [:tt, :ttung] 7.41 KB, patch	tt : review+	Details \| Diff \| Splinter Review
Bug 1298329 - Part3: Add a test to verify persisted origins won't be evicted when reaching global limit. 8 years ago Tom Tung [:tt, :ttung] 9.22 KB, patch		Details \| Diff \| Splinter Review
Bug 1298329 - Part 4: Lowercase the url and path string in test_persist.js to fix the case sensitive issue on Linux; 8 years ago Tom Tung [:tt, :ttung] 1013 bytes, patch	janv : review+	Details \| Diff \| Splinter Review
Bug 1298329 - Part 2: Add a test to verify persist()/persisted(); r=janv 8 years ago Tom Tung [:tt, :ttung] 7.41 KB, patch	tt : review+	Details \| Diff \| Splinter Review
Bug 1298329 - Part 2: Add a test to verify persist()/persisted(); r=janv 8 years ago Tom Tung [:tt, :ttung] 7.42 KB, patch	tt : review+	Details \| Diff \| Splinter Review
Test to verify eviction 7 years ago Tom Tung [:tt, :ttung] 5.41 KB, patch		Details \| Diff \| Splinter Review