Don't create directory and originInfo object for storage.estimate() if the origin has not been initialized yet.

RESOLVED FIXED in Firefox 58

Status

()

Core
DOM: Quota Manager
P2
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: tt, Assigned: tt)

Tracking

(Blocks: 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Reference from bug 132116 comment #14

The implementation for Storage.estimate() in the QuotaManager initializes origin by PERSISTENCE_TYPE_TEMPORARY [1]. This makes the QuotaManager create the directory and memory object for it.

It's not correct. it should just initialize repositories like [2].

[1] http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/dom/quota/ActorsParent.cpp#6969-6972
[2] http://searchfox.org/mozilla-central/rev/c49a70b53f67dd5550eec8a08793805f2aca8d42/dom/quota/ActorsParent.cpp#5095-5135
(In reply to Tom Tung [:tt] from comment #0)
> Reference from bug 132116 comment #14
typo: bug 1372116 comment #14
Created attachment 8900156 [details] [diff] [review]
P1: only initialize temporary storage for estimate.
Created attachment 8900157 [details] [diff] [review]
P1.1: remove unnessary argument.
Priority: -- → P2
Created attachment 8918109 [details] [diff] [review]
Bug 1373183 - P1: Only initialize the temporary storage for estimate(). r?janv
Attachment #8900156 - Attachment is obsolete: true
Created attachment 8918110 [details] [diff] [review]
Bug 1373183 - P2: Remove the argument for EnsureTemporaryStorageIsInitialized() since it's unnecessary. r?janv
Attachment #8900157 - Attachment is obsolete: true
Comment on attachment 8918109 [details] [diff] [review]
Bug 1373183 - P1: Only initialize the temporary storage for estimate(). r?janv

Hi Jan,

Storage.estimate() shouldn't create an originInfo object with temporary type, it only need to ensure the runtime memory objects (originInfo/groupInfo/groupInfoPair) are created. Thus, this patch extract the code for initializing temporary storage out from EnsureOriginIsInitializedInternal().

Could you help to me review this? Thanks!
Attachment #8918109 - Flags: review?(jvarga)
Comment on attachment 8918110 [details] [diff] [review]
Bug 1373183 - P2: Remove the argument for EnsureTemporaryStorageIsInitialized() since it's unnecessary. r?janv

This one is just remove the argument "aPersistenceType" in the reuse function.
Attachment #8918110 - Flags: review?(jvarga)
Comment on attachment 8918109 [details] [diff] [review]
Bug 1373183 - P1: Only initialize the temporary storage for estimate(). r?janv

After discussing with Shawn, I find out that I forget to call EnsureStorageIsInitialized() before calling the new function EnsureTemporaryStorageIsInitialized() in GetOriginUsageOp::DoDirectoryWork().

I should also assert the IsStorageIsInitialized()/mStroageIsInitialized flag to be true in the EnsureTemporaryStorageIsInitialized();

I'll update the patch and re-send it to review tomorrow.
Attachment #8918109 - Flags: review?(jvarga)
Created attachment 8919109 [details] [diff] [review]
Bug 1373183 - P1 (v1): Only initialize the temporary storage for estimate(). r?janv

Update the patch
Attachment #8918109 - Attachment is obsolete: true
Comment on attachment 8919109 [details] [diff] [review]
Bug 1373183 - P1 (v1): Only initialize the temporary storage for estimate(). r?janv

Re-send the review
Attachment #8919109 - Flags: review?(jvarga)

Comment 13

9 months ago
Comment on attachment 8919109 [details] [diff] [review]
Bug 1373183 - P1 (v1): Only initialize the temporary storage for estimate(). r?janv

Review of attachment 8919109 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/quota/ActorsParent.cpp
@@ +5315,5 @@
> +    }
> +
> +    rv = GetTemporaryStorageLimit(storageDir, mTemporaryStorageUsage,
> +                                  &mTemporaryStorageLimit);
> +    NS_ENSURE_SUCCESS(rv, rv);

You can convert this NS_ENSURE_SUCCESS(rv, rv) to:
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

@@ +7137,5 @@
>  
>    if (mGetGroupUsage) {
> +    // Ensure temporary storage is initialized first. It will initialize all
> +    // origins for temporary storage including origins belonging to our group by
> +    // traversing the repositories. Thus, ensuring storage is iniitialized is

iniitialized -> initialized

Maybe replace:
Thus, ensuring storage is iniitialized is needed before ensuring temporary storage is initialized.
with:
EnsureStorageIsInitialized is needed before EnsureTemporaryStorageIsInitialized
Attachment #8919109 - Flags: review?(jvarga) → review+

Updated

9 months ago
Attachment #8918110 - Flags: review?(jvarga) → review+
(In reply to Jan Varga [:janv] from comment #13)
Thanks for the review!! I'll address the comment!
Created attachment 8919611 [details] [diff] [review]
[Final] Bug 1373183 - P1: Only initialize the temporary storage for estimate(). r=janv
Attachment #8919109 - Attachment is obsolete: true
Attachment #8919611 - Flags: review+
Created attachment 8919612 [details] [diff] [review]
[Final] Bug 1373183 - P2: Remove the argument for EnsureTemporaryStorageIsInitialized() since it's unnecessary. r=janv
Attachment #8918110 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8919611 - Attachment description: Bug 1373183 - P1: Only initialize the temporary storage for estimate(). r=janv → [Final] Bug 1373183 - P1: Only initialize the temporary storage for estimate(). r=janv
Attachment #8919612 - Attachment description: Bug 1373183 - P2: Remove the argument for EnsureTemporaryStorageIsInitialized() since it's unnecessary. r=janv → [Final] Bug 1373183 - P2: Remove the argument for EnsureTemporaryStorageIsInitialized() since it's unnecessary. r=janv

Comment 18

9 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dbde5f59609
Part 1: Only initialize the temporary storage for estimate(). r=janv
https://hg.mozilla.org/integration/mozilla-inbound/rev/fad8f2b25e1b
Part 2: Remove the argument for EnsureTemporaryStorageIsInitialized() since it's unnecessary. r=janv
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4dbde5f59609
https://hg.mozilla.org/mozilla-central/rev/fad8f2b25e1b
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.