Support GetUsageForGroup in QuotaManagerService.

I would like to try on this bug.

Hi Jan, I create this patch to have a method to get group usage and group quota. I am not sure about whether we should calculate the "Persistent" type of usage in gecko, so I send a feedback request to make sure I am on the right direction. Could you take a look at this patch? Thanks! In this patch, I implement a "get group usage and quota" function in QuotaManager. To get the group usage and quota, this function need a input argument to find correct GroupInfoPair. Thus, I choose aGroup (it should be get from GetInfoFromPrincinpal) for input argument. Then, I use the GroupInfoPair to calculate the correct group usage and quota. Besides, I recompute the quota when the global available space is less than the group limit - group usage. The real available space should be that in this case.

Hi Jan, I'm working on Storage API v1 with Shawn. Could you help me to review this patch? Thanks! I've already done the patch, so I remove the feedback request and resend a review request. Sorry for that. This patch is mainly to provide a function to get group quota and group usage(non-persistent type). In this patch, 1. add a function GetGroupUsageAndQuota() in QuotaManager. 2. add a function GetGroupUsageAndQuotaForPrincipal() in QuotaManagerService and expose to XPCOM. 3. utilize existing ipc UsageRequest/UsageRespose to add another params and pass group usage/quota. For 1, I initialize temporary storage at the first time. Then get "GroupInfo" from hash table and use this "GroupInfo" to get the group usage. For the group quota, return this value from GetGroupLimit() unless the total available space is less then group available space.

I just found I sent wrong version of patch. Sorry for disturbance .

Comment on attachment 8766199 [details] [diff] [review] Create a function to get groupUsage and groupQuota Review of attachment 8766199 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsChild.cpp @@ +148,5 @@ > mRequest->SetResult(aResponse.usage(), aResponse.fileUsage()); > } > > void > +QuotaUsageRequestChild::HandleResponse(const GroupUsageAndQuotaResponse& aResponse) Not needed @@ +184,5 @@ > case UsageRequestResponse::TUsageResponse: > HandleResponse(aResponse.get_UsageResponse()); > break; > > + case UsageRequestResponse::TGroupUsageAndQuotaResponse: Not needed ::: dom/quota/ActorsChild.h @@ +100,5 @@ > void > HandleResponse(const UsageResponse& aResponse); > > + void > + HandleResponse(const GroupUsageAndQuotaResponse& aResponse); Not needed ::: dom/quota/ActorsParent.cpp @@ +1048,5 @@ > virtual bool > RecvCancel() override; > }; > > +class GetGroupUsageQuotaOp final I believe we don't need a new subclass for this. @@ +4514,5 @@ > } > > +nsresult > +QuotaManager::InitializeTemporaryStorage(PersistenceType aPersistenceType) > +{ Assert the thread at least. @@ +4615,5 @@ > +void > +QuotaManager::GetGroupUsageAndQuota(const nsACString& aGroup, > + uint64_t* aGroupUsage, > + uint64_t* aGroupQuota) > +{ Assert the thread at least. @@ +5756,5 @@ > > PQuotaUsageRequestParent* > Quota::AllocPQuotaUsageRequestParent(const UsageRequestParams& aParams) > { > + RefPtr<GetUsageOp> actor; Not needed. @@ +6162,5 @@ > > return true; > } > > +GetGroupUsageQuotaOp::GetGroupUsageQuotaOp(const UsageRequestParams& aParams) Not needed. @@ +6178,5 @@ > + PROFILER_LABEL("Quota", "GetGroupUsageQuotaOp::DoDirectoryWork", > + js::ProfileEntry::Category::OTHER); > + > + // Add all the temporary/default storage files we care about. > + aQuotaManager->GetGroupUsageAndQuota(mGroup, &mGroupUsage, &mGroupQuota); This can be in a |if (mGroupUsage)| branch. Also, the assumption for |getUsageForPrincipal()| is to return usage/groupUsage for all storage types (default/temporary/persistent). We need to figure out something to do this cleaner. I'll comment later about it. @@ +6183,5 @@ > + return NS_OK; > +} > + > +void > +GetGroupUsageQuotaOp::SendResults() We can just do |if (mGroupUsage)| here. ::: dom/quota/PQuota.ipdl @@ +21,5 @@ > { > PrincipalInfo principalInfo; > }; > > +struct GroupUsageAndQuotaParams Not needed @@ +30,4 @@ > union UsageRequestParams > { > UsageParams; > + GroupUsageAndQuotaParams; Not needed ::: dom/quota/PQuotaUsageRequest.ipdl @@ +13,5 @@ > uint64_t usage; > uint64_t fileUsage; > }; > > +struct GroupUsageAndQuotaResponse Not needed, just add |limit| to UsageResponse @@ +23,5 @@ > union UsageRequestResponse > { > nsresult; > UsageResponse; > + GroupUsageAndQuotaResponse; Not needed ::: dom/quota/QuotaManagerService.cpp @@ +496,5 @@ > > NS_IMETHODIMP > +QuotaManagerService::GetGroupUsageAndQuotaForPrincipal(nsIPrincipal* aPrincipal, > + nsIQuotaUsageCallback* aCallback, > + nsIQuotaUsageRequest** _retval) Just add the "bool groupUsage" to the existing method. ::: dom/quota/QuotaRequests.cpp @@ +97,5 @@ > , mCallback(aCallback) > , mUsage(0) > , mFileUsage(0) > + , mGroupUsage(0) > + , mGroupQuota(0) just mLimit(0) @@ +139,5 @@ > FireCallback(); > } > > +void > +UsageRequest::SetGroupUsageAndQuota(uint64_t aGroupUsage, uint64_t aGroupQuota) Not needed IMO @@ +188,5 @@ > return NS_OK; > } > > NS_IMETHODIMP > +UsageRequest::GetGroupUsage(uint64_t* aGroupUsage) Not needed. @@ +202,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +UsageRequest::GetGroupQuota(uint64_t* aGroupQuota) Renamed to GetLimit() ::: dom/quota/QuotaRequests.h @@ +73,5 @@ > > uint64_t mUsage; > uint64_t mFileUsage; > > + uint64_t mGroupUsage; this can go away @@ +74,5 @@ > uint64_t mUsage; > uint64_t mFileUsage; > > + uint64_t mGroupUsage; > + uint64_t mGroupQuota; mLimit @@ +99,5 @@ > void > SetResult(uint64_t aUsage, uint64_t aFileUsage); > > + void > + SetGroupUsageAndQuota(uint64_t aGroupUsage, uint64_t aGroupQuota); This can go away too if you just add |aLimit| to SetResult() ::: dom/quota/nsIQuotaManagerService.idl @@ +15,5 @@ > interface nsIQuotaManagerService : nsISupports > { > + nsIQuotaUsageRequest > + getGroupUsageAndQuotaForPrincipal(in nsIPrincipal aPrincipal, > + in nsIQuotaUsageCallback aCallback); I would put this after |getUsageForPrincipal| Actually, what about not creating a new method here and rather add a new argument to |getUsageForPrincipal| for example |in boolean groupUsage| ? It seems to me that code sharing can be done better this way. ::: dom/quota/nsIQuotaRequests.idl @@ +24,5 @@ > readonly attribute unsigned long long usage; > > readonly attribute unsigned long long fileUsage; > > + readonly attribute unsigned long long groupUsage; I'm not sure we need this extra attribute, we can just use |usage|. We don't need to return origin usage and group usage at the same time, right ? @@ +26,5 @@ > readonly attribute unsigned long long fileUsage; > > + readonly attribute unsigned long long groupUsage; > + > + readonly attribute unsigned long long groupQuota; I would call this |limit|. It better matches GetGroupLimit().

Almost done, but leaving few nits and related tests(call getUsageForPrincipal()) is needed changes. I'll complete this patch and send it to review later.

An interdiff patch would be great too.

Comment on attachment 8770499 [details] [diff] [review] WIP patch Review of attachment 8770499 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +1016,5 @@ > nsCString mGroup; > bool mIsApp; > > + uint64_t mGroupUsage; > + uint64_t mLimit; maybe we could extend UsageInfo instead of adding redundant mGroupUsage integer ::: dom/quota/nsIQuotaManagerService.idl @@ +27,5 @@ > * The callback that will be called when the usage is available. > */ > nsIQuotaUsageRequest > getUsageForPrincipal(in nsIPrincipal aPrincipal, > + in boolean aGetGroupUsageAndLimit, consider to make this an optional attribute, you won't have to update all the callers

Hi Jan, Thanks for the review and feedback! I learn a lot from them. I addressed comment #6 & #9 in this patch. Could you help me to review this patch? Thanks!

Comment on attachment 8770609 [details] [diff] [review] Bug 1281103 - v2.1 - Add a method to get group usage and limit in QuotaManagerService. Review of attachment 8770609 [details] [diff] [review]: ----------------------------------------------------------------- Mostly ok, I will have to take a look at GetGroupUsageAndLimit() one more time. ::: dom/quota/ActorsChild.cpp @@ +144,5 @@ > { > AssertIsOnOwningThread(); > MOZ_ASSERT(mRequest); > > + mRequest->SetResult(aResponse.usage(), aResponse.fileUsage(), aResponse.limit()); wrap to 80 chars per line: mRequest->SetResult(aResponse.usage(), aResponse.fileUsage(), aResponse.limit()); ::: dom/quota/ActorsParent.cpp @@ +4602,5 @@ > + AssertIsOnIOThread(); > + > + if (!mTemporaryStorageInitialized) { > + nsresult rv; > + rv = InitializeTemporaryStorage(PERSISTENCE_TYPE_TEMPORARY); I don't think factoring out just temporary storage initialization is enough here. In theory, estimate() can be called very early when <profile>/storage hasn't been upgraded yet. So we would end up initializing old version of storage directory tree. I think you can just call EnsureOriginIsInitialized(), you can get arguments for it by calling GteInfoFromPrincipal() @@ +4644,5 @@ > + groupQuota = groupUsage + totalAvailable; > + } > + > + > + if (groupQuota * .85 >= groupUsage) { Why do we need this ? @@ +6070,5 @@ > PROFILER_LABEL("Quota", "GetUsageOp::DoDirectoryWork", > js::ProfileEntry::Category::OTHER); > > + if (mGetGroupUsageAndLimit) { > + MOZ_ASSERT(mUsageInfo.TotalUsage() == 0.0); 0.0 ? Why not just 0? ::: dom/quota/PQuota.ipdl @@ +20,5 @@ > struct UsageParams > { > PrincipalInfo principalInfo; > + > + bool getGroupUsageAndLimit; just getGroupUsage ::: dom/quota/QuotaManager.h @@ +352,5 @@ > > + void > + GetGroupUsageAndQuota(const nsACString& aGroup, > + uint64_t* aGroupUsage, > + uint64_t* aGroupQuota); Rename GetGroupUsageAndQuota to GetGroupUsageAndLimit and aGroupQuota to aGroupLimit. ::: dom/quota/QuotaManagerService.cpp @@ +509,5 @@ > > UsageParams params; > > PrincipalInfo& principalInfo = params.principalInfo(); > + params.getGroupUsageAndLimit() = aGetGroupUsageAndLimit; this doesn't logically belong here, I would place it before: |nsAutoPtr<PendingRequestInfo> info(new UsageRequestInfo(request, params));| ::: dom/quota/nsIQuotaManagerService.idl @@ +21,5 @@ > * @param aPrincipal > * A principal for the origin whose usage is being queried. > * @param aCallback > * The callback that will be called when the usage is available. > + * @param aGetGroupUsageAndLimit aGetGroupUsage is enough I think @@ +22,5 @@ > * A principal for the origin whose usage is being queried. > * @param aCallback > * The callback that will be called when the usage is available. > + * @param aGetGroupUsageAndLimit > + * An optional flag to indicate whether getting group usage/limit "group usage and limit or origin usage and file usage" @@ +24,5 @@ > * The callback that will be called when the usage is available. > + * @param aGetGroupUsageAndLimit > + * An optional flag to indicate whether getting group usage/limit > + * or origin usage and file usage. The default value is false. > + * Note: Origin usage here represent total usage of an origin. However, represents ?

(In reply to Jan Varga [:janv] from comment #12) Hi Jan, Thanks for your review and feedback! I'll address the comment in next patch but I have a question about one of them. I leave it with few responses below inline. > Comment on attachment 8770609 [details] [diff] [review] > Bug 1281103 - v2.1 - Add a method to get group usage and limit in > QuotaManagerService. > > Review of attachment 8770609 [details] [diff] [review]: > ----------------------------------------------------------------- > > Mostly ok, I will have to take a look at GetGroupUsageAndLimit() one more > time. > > ::: dom/quota/ActorsParent.cpp > @@ +4602,5 @@ > > + AssertIsOnIOThread(); > > + > > + if (!mTemporaryStorageInitialized) { > > + nsresult rv; > > + rv = InitializeTemporaryStorage(PERSISTENCE_TYPE_TEMPORARY); > > I don't think factoring out just temporary storage initialization is enough > here. In theory, estimate() can be called very early when <profile>/storage > hasn't been upgraded yet. So we would end up initializing old version of > storage directory tree. > I think you can just call EnsureOriginIsInitialized(), you can get arguments > for it by calling GteInfoFromPrincipal() Yeah, it cover more situations but it makes me think about another thing. I'm afraid that only is "ensuring an 'origin' is initialized" enough to cover all the origin under a group? So, I think I may need to ensure a group is initialized? > @@ +4644,5 @@ > > + groupQuota = groupUsage + totalAvailable; > > + } > > + > > + > > + if (groupQuota * .85 >= groupUsage) { > > Why do we need this ? I'll remove this one. I wanted to hind the real quota to avoid some issue. It seems that I should do it on another bug. > @@ +6070,5 @@ > > PROFILER_LABEL("Quota", "GetUsageOp::DoDirectoryWork", > > js::ProfileEntry::Category::OTHER); > > > > + if (mGetGroupUsageAndLimit) { > > + MOZ_ASSERT(mUsageInfo.TotalUsage() == 0.0); > > 0.0 ? Why not just 0? Oh, it should be 0 rather than 0.0. > @@ +24,5 @@ > > * The callback that will be called when the usage is available. > > + * @param aGetGroupUsageAndLimit > > + * An optional flag to indicate whether getting group usage/limit > > + * or origin usage and file usage. The default value is false. > > + * Note: Origin usage here represent total usage of an origin. However, > > represents ? Yeah, sorry for that.

(In reply to Tom Tung [:tt] from comment #13) > Yeah, it cover more situations but it makes me think about another thing. > I'm afraid that only is "ensuring an 'origin' is initialized" enough to > cover all the origin under a group? So, I think I may need to ensure a group > is initialized? No, I don't think so we need that. I agree that the name of the method isn't 100% correct in this case, but it will scan all origins for temporary storage anyway, so you should be fine. You can add a comment why ensuring just origin is enough.

Hi Jan, I addressed the comment #12 but I am not sure about putting as right arguments of EnsureOriginIsInitialized(..) as you meant. I get them from GetInfoFromPrincipal(..) but I not so sure about what "directory object" should I use to call it. Could you help me to review this? Thanks!

interdiff

(In reply to Tom Tung [:tt] from comment #15) > I addressed the comment #12 but I am not sure about putting as right > arguments of EnsureOriginIsInitialized(..) as you meant. I get them from > GetInfoFromPrincipal(..) but I not so sure about what "directory object" > should I use to call it. what "directory object" ? That's an output argument (not input), right ?

(In reply to Jan Varga [:janv] from comment #17) > what "directory object" ? That's an output argument (not input), right ? Oh, I see. So, I guess I did the right thing to pass a referenced empty |nsCOMPtr<nsIFile>| into it. Thanks!

Right. I'm going to take a look at the patch one more time.

Comment on attachment 8771951 [details] [diff] [review] Bug 1281103 - v3 - Add a method to get group usage and limit in QuotaManagerService. Review of attachment 8771951 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/ActorsParent.cpp @@ +1008,5 @@ > : public NormalOriginOperationBase > , public PQuotaUsageRequestParent > { > + // If mGetGroupUsage is false, use mUsageInfo to record origin > + // usage. Otherwise, record group usage. This comment needs to be slightly updated if you move mLimit to UsageInfo as I'm suggesting below. @@ +1013,4 @@ > UsageInfo mUsageInfo; > > const UsageParams mParams; > + bool mGetGroupUsage; mGetGroupUsage should be coupled with other bools (like mIsApp) to save memory (the one byte bool here will be aligned to 4/8 bytes by compiler I think) @@ +1019,4 @@ > nsCString mGroup; > bool mIsApp; > > + uint64_t mLimit; mLimit here can go to UsageInfo class too. @@ +4586,5 @@ > +QuotaManager::GetGroupUsageAndLimit(const nsACString& aGroup, > + uint64_t* aGroupUsage, > + uint64_t* aGroupLimit) > +{ > + AssertIsOnIOThread(); Add a new line here. @@ +4612,5 @@ > + if (defaultGroupInfo) { > + *aGroupUsage += defaultGroupInfo->mUsage; > + } > + > + // Adjust the limit if we have less available space in global. I don't understand this check at all, what is it trying to achieve ? Anyway, I think we should return exactly the same usage and limit which is used in MaybeUpdateSize() check: AssertNoOverflow(groupUsage, delta); if (groupUsage + delta > quotaManager->GetGroupLimit()) { return false; } That will guarantee that the check which JS will be doing (see the example in storage spec) will be doing the same thing: navigator.storage.estimate().then(info => { if(info.quota - info.usage > metroidPrimeLevel.size) return fetch(metroidPrimeLevel.url) throw new Error("no space") }).then( /* … */ ) There's one thing and that's the global limit, but we can't do |mTemporaryStorageLimit - mTemporaryStorageUsage| here. MaybeUpdateSize() will try to free some space before returning "false" - no free space. So that info is rather dynamic and can't be incorporated here. Well, we could estimate how much space would get freed by running the origin eviction, but that can be done in a followup bug since it's not trivial stuff. @@ +6038,5 @@ > PROFILER_LABEL("Quota", "GetUsageOp::DoDirectoryWork", > js::ProfileEntry::Category::OTHER); > > + if (mGetGroupUsage) { > + MOZ_ASSERT(mUsageInfo.TotalUsage() == 0); This assertion is useful for the other case too I think, you can move it after the thread assertion. @@ +6042,5 @@ > + MOZ_ASSERT(mUsageInfo.TotalUsage() == 0); > + > + nsCOMPtr<nsIFile> directory; > + // Ensure origin is initialized first. It will all origins for temporary > + // storage. "It will ..." what ? initialize ? Maybe: It will initialize all origins in temporary storage including origins belonging to our group. @@ +6043,5 @@ > + > + nsCOMPtr<nsIFile> directory; > + // Ensure origin is initialized first. It will all origins for temporary > + // storage. > + nsresult rv = You can move "nsresult rv; " before "if (mGetGroupUsage)" to share it by both code paths. @@ +6048,5 @@ > + aQuotaManager->EnsureOriginIsInitialized(PERSISTENCE_TYPE_TEMPORARY, > + mSuffix, mGroup, > + mOriginScope.GetOrigin(), mIsApp, > + getter_AddRefs(directory)); > + A new line is not needed before the call and result code check. @@ +6053,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + > + // Add all the temporary/default storage files we care about. We don't count files here. Maybe just: "Get cached usage and limit (the method doesn't have to stat any files)." @@ +6054,5 @@ > + return rv; > + } > + > + // Add all the temporary/default storage files we care about. > + uint64_t usage; GetGroupUsageAndLimit() can take UsageInfo* instead of usage and limit. @@ +6093,5 @@ > if (NS_SUCCEEDED(mResultCode)) { > UsageResponse usageResponse; > + > + // Note: we'll get group usage when mGetGroupUsageAndLimit is true > + // get origin usage otherwise. mGetGroupUsageAndLimit changed to mGetGroupUsage. And I think the "Note: " prefix is useless, we put comments here and there without it. If there's something very important we use the "XXX" sign :) ::: dom/quota/PQuota.ipdl @@ +19,5 @@ > > struct UsageParams > { > PrincipalInfo principalInfo; > + I think we don't need an extra empty line here.

(In reply to Jan Varga [:janv] from comment #20) Thanks for your review and feedback! It helps me a lot! > Comment on attachment 8771951 [details] [diff] [review] > Bug 1281103 - v3 - Add a method to get group usage and limit in > QuotaManagerService. > > Review of attachment 8771951 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/quota/ActorsParent.cpp > @@ +4612,5 @@ > > + if (defaultGroupInfo) { > > + *aGroupUsage += defaultGroupInfo->mUsage; > > + } > > + > > + // Adjust the limit if we have less available space in global. > > I don't understand this check at all, what is it trying to achieve ? Actually, I just wanted to cover more situations but you are right. As you mention, we should ensure that estimate() can show the same behavior as MaybeUpdateSize(). > Anyway, I think we should return exactly the same usage and limit which is > used in MaybeUpdateSize() check: > AssertNoOverflow(groupUsage, delta); > if (groupUsage + delta > quotaManager->GetGroupLimit()) { > return false; > } > > That will guarantee that the check which JS will be doing (see the example > in storage spec) will be doing the same thing: > navigator.storage.estimate().then(info => { > if(info.quota - info.usage > metroidPrimeLevel.size) > return fetch(metroidPrimeLevel.url) > throw new Error("no space") > }).then( /* … */ ) > > There's one thing and that's the global limit, but we can't do > |mTemporaryStorageLimit - mTemporaryStorageUsage| here. MaybeUpdateSize() > will try to free some space before returning "false" - no free space. > So that info is rather dynamic and can't be incorporated here. > > Well, we could estimate how much space would get freed by running the origin > eviction, but that can be done in a followup bug since it's not trivial > stuff. Sure. I'll file a follow-up bug for it.

Hi Janv, I address comment #20 in this patch. Could you help me to review it again? Thanks!

interdiff

(In reply to Tom Tung [:tt] from comment #21) > Sure. I'll file a follow-up bug for it. Bug 1288032

Comment on attachment 8772725 [details] [diff] [review] Bug 1281103 - v4 - Add a method to get group usage and limit in QuotaManagerService. Review of attachment 8772725 [details] [diff] [review]: ----------------------------------------------------------------- Nice! r=me ::: dom/quota/ActorsParent.cpp @@ +4584,5 @@ > +void > +QuotaManager::GetGroupUsageAndLimit(const nsACString& aGroup, > + UsageInfo* aUsageInfo) > +{ > + AssertIsOnIOThread(); aUsageInfo needs to be asserted here @@ +6031,5 @@ > > PROFILER_LABEL("Quota", "GetUsageOp::DoDirectoryWork", > js::ProfileEntry::Category::OTHER); > > + nsresult rv; Nit: I would add an empty line here. @@ +6047,5 @@ > + return rv; > + } > + > + // Get cached usage and limit (the method doesn't have to stat any files). > + aQuotaManager->GetGroupUsageAndLimit(mGroup, &mUsageInfo); Nit: I would add an empty line here.

(In reply to Jan Varga [:janv] from comment #25) Hi Jan, Thanks for your review and your time! try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6100ae456615

Pushed by ihsiao@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0a5aab8c13 Support getting the group usage and the limit in QuotaManagerService. r=janv.