1348660 - Implement a method to retrieve usage data for all origins at once

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement)

Comment 1

[Jan Varga [:janv]]

Attachment: Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant

Comment 2

[Jan Varga [:janv]]

Attachment: Part 2: Rename usage related methods and structures to express the relation to a concrete origin

Comment 3

[Jan Varga [:janv]]

Attachment: Part 3: Separate the canceled state out of UsageInfo

Comment 4

[Jan Varga [:janv]]

Attachment: Part 4: Extract common code from GetOriginUsageOp to a new base class QuotaUsageRequestBase

Comment 5

[Jan Varga [:janv]]

Attachment: Part 5: Implement a method to retrieve usage data for all origins at once

Comment 6

[Jan Varga [:janv]]

Attachment: Part 6: Rename QuotaManager::IsOriginWhitelistedForPersistentStorage() to QuotaManager::IsOriginInternal()

Comment 7

[Jan Varga [:janv]]

Andrew, if you don't have time to review this, let me know.
This new method is needed for new storage preferences to get a list of all origins.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Jan Varga [:janv] from comment #7)
> Andrew, if you don't have time to review this, let me know.
> This new method is needed for new storage preferences to get a list of all
> origins.

Unfortunately, I do need to focus on multi-e10s and ServiceWorker issues for the near term (there's a face-2-face in 2 weeks).  Also, I don't think I have any exclusive-to-me mental-state on these changes, so this is probably a good opportunity to let Bevis or others hacking in IndexedDB/QuotaManager/Storage enjoy additional review fun ;)  Setting NI for you to redirect reviews.

Comment 9

[Jan Varga [:janv]]

Shawn, you attended last two meetings about new storage preferences. Can you please give Bevis some background for this bug or ask someone else involved in new storage preferences to do it ? Thanks.

Comment 10

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #8)
> Unfortunately, I do need to focus on multi-e10s and ServiceWorker issues for
> the near term (there's a face-2-face in 2 weeks).

Ok, no problem.

Comment 11

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

(In reply to Jan Varga [:janv] from comment #9)
> Shawn, you attended last two meetings about new storage preferences. Can you
> please give Bevis some background for this bug or ask someone else involved
> in new storage preferences to do it ? Thanks.

Hi Jan,
Sure, I've already involved this and explain to Bevis.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848907 [details] [diff] [review]
Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant

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

Please see my comments below.

Thanks!

I'll review the rest of parts tomorrow.

::: dom/quota/StorageManager.cpp
@@ +145,4 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
>  

NS_Free(iid);

@@ +158,2 @@
>    aStorageEstimate.mUsage.Construct() = usage;
>    aStorageEstimate.mQuota.Construct() = limit;

I think we could simplify these lines as followed:
  MOZ_ALWAYS_SUCCEEDS(
    usageResult->GetUsage(&aStorageEstimate.mUsage.Construct()));

  MOZ_ALWAYS_SUCCEEDS(
    usageResult->GetLimit(&aStorageEstimate.mQuota.Construct()));

::: dom/quota/nsIQuotaRequests.idl
@@ +21,5 @@
>  
>  [scriptable, uuid(166e28e6-cf6d-4927-a6d7-b51bca9d3469)]
>  interface nsIQuotaUsageRequest : nsIQuotaRequestBase
>  {
> +  [must_use] readonly attribute nsIVariant result;

It will be nice to explain that the *result* here represents various types (nsIQuotaUsageResult, nsIQuotaOriginUsageResult) to be defined in nsIQuotaUsageResult.idl.

Comment 13

[Jan Varga [:janv]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #12)
> ::: dom/quota/StorageManager.cpp
> @@ +145,4 @@
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> >      return rv;
> >    }
> >  
> 
> NS_Free(iid);

Ok, good catch, although NS_Free shouldn't be used outside the xpcom glue, just "free"

> 
> @@ +158,2 @@
> >    aStorageEstimate.mUsage.Construct() = usage;
> >    aStorageEstimate.mQuota.Construct() = limit;
> 
> I think we could simplify these lines as followed:
>   MOZ_ALWAYS_SUCCEEDS(
>     usageResult->GetUsage(&aStorageEstimate.mUsage.Construct()));
> 
>   MOZ_ALWAYS_SUCCEEDS(
>     usageResult->GetLimit(&aStorageEstimate.mQuota.Construct()));
> 

Ok, this is now possible since GetUsage() and GetLimit() are now infallible.

> ::: dom/quota/nsIQuotaRequests.idl
> @@ +21,5 @@
> >  
> >  [scriptable, uuid(166e28e6-cf6d-4927-a6d7-b51bca9d3469)]
> >  interface nsIQuotaUsageRequest : nsIQuotaRequestBase
> >  {
> > +  [must_use] readonly attribute nsIVariant result;
> 
> It will be nice to explain that the *result* here represents various types
> (nsIQuotaUsageResult, nsIQuotaOriginUsageResult) to be defined in
> nsIQuotaUsageResult.idl.

Ok, I'll add a comment.

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848908 [details] [diff] [review]
Part 2: Rename usage related methods and structures to express the relation to a concrete origin

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

::: dom/indexedDB/test/file.js
@@ +222,5 @@
>  {
>    testGenerator.next(request.result.fileUsage);
>  }
>  
> +function getContentUsage(usageHandler)

nit:
Is there any use case of getContentUsage() that is different from the one of getCurrentOriginUsage() in indexedDB tests?
If no, I'd prefer to rename it to GetCurrentOriginUsage() if this is the only use case for testing.

::: dom/indexedDB/test/unit/xpcshell-head-parent-process.js
@@ +518,5 @@
>  {
>    testGenerator.next(request.result.fileUsage);
>  }
>  
> +function getChromeUsage(usageHandler)

ditto:
Is there any use case of getChromeUsage() that is different from the one of getCurrentOriginUsage() in indexedDB xpcshell tests?
If no, I'd prefer to rename it to GetCurrentOriginUsage() if this is the only use case for testing.

::: dom/quota/StorageManager.cpp
@@ +155,3 @@
>  
>    uint64_t limit;
> +  MOZ_ALWAYS_SUCCEEDS(originUsageResult->GetLimit(&limit));

Same simplification as suggested in patch part 1.

Comment 15

[Jan Varga [:janv]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #14)
> ::: dom/indexedDB/test/file.js
> @@ +222,5 @@
> >  {
> >    testGenerator.next(request.result.fileUsage);
> >  }
> >  
> > +function getContentUsage(usageHandler)
> 
> nit:
> Is there any use case of getContentUsage() that is different from the one of
> getCurrentOriginUsage() in indexedDB tests?
> If no, I'd prefer to rename it to GetCurrentOriginUsage() if this is the
> only use case for testing.

I plan to add getOriginUsage(principal, usageHandler) especially for xpcshell tests
Having getOriginUsage() and getChromeUsage() sounds a bit better than
getOriginUsage() and getCurrentOriginUsage().

I have to add getCurrentOriginUsage() just because there's a test which runs as
a mochitest and also as an xpcshell test.

I'm also considering to rename getCurrentOriginUsage() to getDefaultOriginUsage().

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #15)
> I plan to add getOriginUsage(principal, usageHandler) especially for
> xpcshell tests
> Having getOriginUsage() and getChromeUsage() sounds a bit better than
> getOriginUsage() and getCurrentOriginUsage().
This makes sense to me but I thought that these tests will be added in dom/quota/test.
Will these happen in dom/indexedDB/test as well?
If yes, we need these namings for better understanding.
> I have to add getCurrentOriginUsage() just because there's a test which runs
> as a mochitest and also as an xpcshell test.
Yes, that's also my understanding. ;)
> I'm also considering to rename getCurrentOriginUsage() to
> getDefaultOriginUsage().
Ok.

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848909 [details] [diff] [review]
Part 3: Separate the canceled state out of UsageInfo

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

::: dom/indexedDB/ActorsParent.cpp
@@ +17830,5 @@
>  {
>    AssertIsOnIOThread();
>    MOZ_ASSERT(aDirectory);
>  
> +  AtomicBool canceled;

nit:
  AtomicBool dummy(false);

::: dom/quota/ActorsParent.cpp
@@ +4196,5 @@
>        UNKNOWN_FILE_WARNING(leafName);
>        return NS_ERROR_UNEXPECTED;
>      }
>  
> +    Atomic<bool> canceled;

nit:
    Atomic<bool> dummy(false);

Comment 18

[Jan Varga [:janv]]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #16)
> (In reply to Jan Varga [:janv] from comment #15)
> > I plan to add getOriginUsage(principal, usageHandler) especially for
> > xpcshell tests
> > Having getOriginUsage() and getChromeUsage() sounds a bit better than
> > getOriginUsage() and getCurrentOriginUsage().
> This makes sense to me but I thought that these tests will be added in
> dom/quota/test.
> Will these happen in dom/indexedDB/test as well?
> If yes, we need these namings for better understanding.

Yes, I'm adding new tests to dom/quota/test, but I'm also renaming getUsage() in dom/indexedDB/test
to use the same naming as much as possible.

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Jan Varga [:janv] from comment #18)
> Yes, I'm adding new tests to dom/quota/test, but I'm also renaming
> getUsage() in dom/indexedDB/test
> to use the same naming as much as possible.
OK!

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8848910 [details] [diff] [review]
Part 4: Extract common code from GetOriginUsageOp to a new base class QuotaUsageRequestBase

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

Looks great, thanks!

Comment 21

[Jan Varga [:janv]]

Attachment: Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant

Comment 22

[Jan Varga [:janv]]

Attachment: Part1 interdiff

Comment 23

[Jan Varga [:janv]]

Comment on attachment 8849574 [details] [diff] [review]
Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant

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

::: dom/quota/nsIQuotaRequests.idl
@@ +22,5 @@
>  [scriptable, uuid(166e28e6-cf6d-4927-a6d7-b51bca9d3469)]
>  interface nsIQuotaUsageRequest : nsIQuotaRequestBase
>  {
> +  // The result can contain one of these types:
> +  //   nsIQuotaUsageResult

I'll add nsIQuotaOriginUsageResult here in Part 5 patch

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8849574 [details] [diff] [review]
Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant

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

Thank you!

Comment 25

[Jan Varga [:janv]]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3c6e12b499
Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d0402b8bb0
Part 2: Rename usage related methods and structures to express the relation to a concrete origin; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f06917c82a
Part 3: Separate the canceled state out of UsageInfo; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/24244ba9a144
Part 4: Extract common code from GetOriginUsageOp to a new base class QuotaUsageRequestBase; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c3319501b59
Part 5: Implement a method to retrieve usage data for all origins at once; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/21063e15627e
Part 6: Rename QuotaManager::IsOriginWhitelistedForPersistentStorage() to QuotaManager::IsOriginInternal(); r=btseng

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/ea3c6e12b499
https://hg.mozilla.org/mozilla-central/rev/c0d0402b8bb0
https://hg.mozilla.org/mozilla-central/rev/f9f06917c82a
https://hg.mozilla.org/mozilla-central/rev/24244ba9a144
https://hg.mozilla.org/mozilla-central/rev/2c3319501b59
https://hg.mozilla.org/mozilla-central/rev/21063e15627e

Comment 28

[Jan Varga [:janv]]

Attachment: Part 1 for ESR52

Comment 29

[Jan Varga [:janv]]

Attachment: Part 2 for ESR52

Comment 30

[Jan Varga [:janv]]

Attachment: Part 3 for ESR52

Comment 31

[Jan Varga [:janv]]

Attachment: Part 4 for ESR52

Comment 32

[Jan Varga [:janv]]

Attachment: Part 5 for ESR52

Comment 33

[Jan Varga [:janv]]

Part 6 is not needed for ESR52.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift to ESR? Ryan mentioned we need this for bug 1047098. Thanks.

Comment 35

[Jan Varga [:janv]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #34)
> Can you request uplift to ESR? Ryan mentioned we need this for bug 1047098.
> Thanks.

I think bug 1047098 shouldn't land on ESR until we solve bug 1404105.

Comment 36

[Jan Varga [:janv]]

Attachment: Part 6 for ESR52

Comment 37

[Jan Varga [:janv]]

(In reply to Jan Varga [:janv] from comment #33)
> Part 6 is not needed for ESR52.

I changed my mind, Part 6 can land on ESR52 too.

Comment 38

[Jan Varga [:janv]]

Comment on attachment 8911847 [details] [diff] [review]
Part 1 for ESR52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is needed for bug 1047098 which already has ESR52 approval.
User impact if declined: See bug 1047098.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

The same approval request applies to all 6 patches "Part 1-6 for ESR52".

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8911847 [details] [diff] [review]
Part 1 for ESR52

A lot of code change for ESR but since we want to fix bug 1047098, let's risk it.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/a352bfcbaf55
https://hg.mozilla.org/releases/mozilla-esr52/rev/57f43e2ab9b5
https://hg.mozilla.org/releases/mozilla-esr52/rev/917d65bb8896
https://hg.mozilla.org/releases/mozilla-esr52/rev/28934912eede
https://hg.mozilla.org/releases/mozilla-esr52/rev/5e07bd37ac61
https://hg.mozilla.org/releases/mozilla-esr52/rev/556ff3bfb9fc