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

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement, P2)

Description

[Tom Tung [:tt, :ttung]]

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

Comment 1

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #0)
> Reference from bug 132116 comment #14
typo: bug 1372116 comment #14

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: P1: only initialize temporary storage for estimate.

Comment 3

[Tom Tung [:tt, :ttung]]

Attachment: P1.1: remove unnessary argument.

Comment 4

[Tom Tung [:tt, :ttung]]

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

Comment 5

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1373183 - P1: Only initialize the temporary storage for estimate(). r?janv

Comment 6

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1373183 - P2: Remove the argument for EnsureTemporaryStorageIsInitialized() since it's unnecessary. r?janv

Comment 7

[Tom Tung [:tt, :ttung]]

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!

Comment 8

[Tom Tung [:tt, :ttung]]

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.

Comment 9

[Tom Tung [:tt, :ttung]]

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

Comment 10

[Tom Tung [:tt, :ttung]]

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.

Comment 11

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1373183 - P1 (v1): Only initialize the temporary storage for estimate(). r?janv

Update the patch

Comment 12

[Tom Tung [:tt, :ttung]]

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

Re-send the review

Comment 13

[Jan Varga [:janv]]

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

Comment 14

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #13)
Thanks for the review!! I'll address the comment!

Comment 15

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1373183 - P1: Only initialize the temporary storage for estimate(). r=janv

Comment 16

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1373183 - P2: Remove the argument for EnsureTemporaryStorageIsInitialized() since it's unnecessary. r=janv

Comment 17

[Tom Tung [:tt, :ttung]]

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4dbde5f59609
https://hg.mozilla.org/mozilla-central/rev/fad8f2b25e1b