Closed
Bug 1373183
Opened 7 years ago
Closed 7 years ago
Don't create directory and originInfo object for storage.estimate() if the origin has not been initialized yet.
Categories
(Core :: Storage: Quota Manager, enhancement, P2)
Core
Storage: Quota Manager
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: tt, Assigned: tt)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 5 obsolete files)
(deleted),
patch
|
tt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
tt
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
(In reply to Tom Tung [:tt] from comment #0) > Reference from bug 132116 comment #14 typo: bug 1372116 comment #14
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Assignee | ||
Comment 4•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9612c61b1e39084eadededc7f3c3885c285508d6
Updated•7 years ago
|
Priority: -- → P2
Assignee | ||
Comment 5•7 years ago
|
||
Attachment #8900156 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
Attachment #8900157 -
Attachment is obsolete: true
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
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)
Assignee | ||
Comment 9•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf2572288beda8aeabbadb86648ad70e080bf117
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
Update the patch
Attachment #8918109 -
Attachment is obsolete: true
Assignee | ||
Comment 12•7 years ago
|
||
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•7 years 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•7 years ago
|
Attachment #8918110 -
Flags: review?(jvarga) → review+
Assignee | ||
Comment 14•7 years ago
|
||
(In reply to Jan Varga [:janv] from comment #13) Thanks for the review!! I'll address the comment!
Assignee | ||
Comment 15•7 years ago
|
||
Attachment #8919109 -
Attachment is obsolete: true
Attachment #8919611 -
Flags: review+
Assignee | ||
Comment 16•7 years ago
|
||
Attachment #8918110 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8919612 -
Flags: review+
Assignee | ||
Comment 17•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25ce285c444a0e821b1728063b76890a40eb5e89
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
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
Assignee | ||
Updated•7 years ago
|
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•7 years 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
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4dbde5f59609 https://hg.mozilla.org/mozilla-central/rev/fad8f2b25e1b
Status: NEW → RESOLVED
Closed: 7 years 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.
Description
•