1123637 - Remove all code relevant to quota prompts

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

Attachment: patch

Comment 1

[Jan Varga [:janv]]

Comment on attachment 8551745 [details] [diff] [review]
patch

Ben, this patch is not small, but it shouldn't be so hard to review. Most of it is code and test removal. However, since persistent storage now gets unlimited quota from the beginning, we can adjust/simplify some structures that are used to track quota.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=5cf1bac76cdf

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Jan Varga [:janv] from comment #1)
> However, since persistent storage now gets
> unlimited quota from the beginning, we can adjust/simplify some structures
> that are used to track quota.

This sounds like it would have been great as a separate patch...

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8551745 [details] [diff] [review]
patch

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

Thanks! This looks good to me, with these addressed:

::: browser/modules/SitePermissions.jsm
@@ -87,5 @@
>      Services.perms.add(aURI, aPermissionID, aState);
> -
> -    if (aPermissionID in gPermissionObject &&
> -        gPermissionObject[aPermissionID].onChange)
> -      gPermissionObject[aPermissionID].onChange(aURI, aState);

I'm not sure that we want to remove this... Could addons be using this feature? Please get this change reviewed by a browser peer.

::: dom/apps/PermissionsTable.jsm
@@ -311,5 @@
>                               trusted: ALLOW_ACTION,
>                               privileged: ALLOW_ACTION,
> -                             certified: ALLOW_ACTION,
> -                             substitute: [
> -                               "indexedDB-unlimited"

This whole "storage" permission can now be removed. It isn't used for anything after this.

::: dom/quota/QuotaManager.cpp
@@ +1458,5 @@
>                                       int64_t aSize)
>  {
>    MOZ_ASSERT(!NS_IsMainThread());
>  
> +  if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT) {

This can't be asserted as it is in most of the other places?

::: dom/quota/QuotaObject.cpp
@@ +308,5 @@
>    AssertCurrentThreadOwnsQuotaMutex();
>  
>    for (uint32_t index = 0; index < mOriginInfos.Length(); index++) {
>      if (mOriginInfos[index]->mOrigin == aOrigin) {
>        mUsage -= mOriginInfos[index]->mUsage;

Can you assert that this won't overflow?

  MOZ_ASSERT(mUsage >= mOriginInfos[index]->mUsage);
  ... -= ...;

@@ +311,5 @@
>      if (mOriginInfos[index]->mOrigin == aOrigin) {
>        mUsage -= mOriginInfos[index]->mUsage;
>  
> +      QuotaManager* quotaManager = QuotaManager::Get();
> +      MOZ_ASSERT(quotaManager);

Nit: Hoist this out of the loop

@@ +316,2 @@
>  
> +      quotaManager->mTemporaryStorageUsage -= mOriginInfos[index]->mUsage;

Can you assert that this won't overflow?

  MOZ_ASSERT(quotaManager->mTemporaryStorageUsage >= mOriginInfos[index]->mUsage);
  ... -= ...;

Comment 4

[Jan Varga [:janv]]

Comment on attachment 8551745 [details] [diff] [review]
patch

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #3)
> ::: browser/modules/SitePermissions.jsm
> @@ -87,5 @@
> >      Services.perms.add(aURI, aPermissionID, aState);
> > -
> > -    if (aPermissionID in gPermissionObject &&
> > -        gPermissionObject[aPermissionID].onChange)
> > -      gPermissionObject[aPermissionID].onChange(aURI, aState);
> 
> I'm not sure that we want to remove this... Could addons be using this
> feature? Please get this change reviewed by a browser peer.

Ehsan, can you take a look ?

Comment 5

[Jan Varga [:janv]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #3)
> ::: dom/apps/PermissionsTable.jsm
> @@ -311,5 @@
> >                               trusted: ALLOW_ACTION,
> >                               privileged: ALLOW_ACTION,
> > -                             certified: ALLOW_ACTION,
> > -                             substitute: [
> > -                               "indexedDB-unlimited"
> 
> This whole "storage" permission can now be removed. It isn't used for
> anything after this.

Oh, you're right.

> 
> ::: dom/quota/QuotaManager.cpp
> @@ +1458,5 @@
> >                                       int64_t aSize)
> >  {
> >    MOZ_ASSERT(!NS_IsMainThread());
> >  
> > +  if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT) {
> 
> This can't be asserted as it is in most of the other places?

The original idea was to just do an early return for public QM methods, but now I see that all the callers check mQuotaEnforced before calling this specific method, so I think we're ok with the assert here.

> 
> ::: dom/quota/QuotaObject.cpp
> @@ +308,5 @@
> >    AssertCurrentThreadOwnsQuotaMutex();
> >  
> >    for (uint32_t index = 0; index < mOriginInfos.Length(); index++) {
> >      if (mOriginInfos[index]->mOrigin == aOrigin) {
> >        mUsage -= mOriginInfos[index]->mUsage;
> 
> Can you assert that this won't overflow?
> 
>   MOZ_ASSERT(mUsage >= mOriginInfos[index]->mUsage);
>   ... -= ...;

Ok

> 
> @@ +311,5 @@
> >      if (mOriginInfos[index]->mOrigin == aOrigin) {
> >        mUsage -= mOriginInfos[index]->mUsage;
> >  
> > +      QuotaManager* quotaManager = QuotaManager::Get();
> > +      MOZ_ASSERT(quotaManager);
> 
> Nit: Hoist this out of the loop

Actually, this case is a bit different, since we call return in that branch.

> 
> @@ +316,2 @@
> >  
> > +      quotaManager->mTemporaryStorageUsage -= mOriginInfos[index]->mUsage;
> 
> Can you assert that this won't overflow?
> 
>   MOZ_ASSERT(quotaManager->mTemporaryStorageUsage >=
> mOriginInfos[index]->mUsage);
>   ... -= ...;

Ok

Comment 6

[(no longer active)]

Comment on attachment 8551745 [details] [diff] [review]
patch

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

::: browser/modules/SitePermissions.jsm
@@ -87,5 @@
>      Services.perms.add(aURI, aPermissionID, aState);
> -
> -    if (aPermissionID in gPermissionObject &&
> -        gPermissionObject[aPermissionID].onChange)
> -      gPermissionObject[aPermissionID].onChange(aURI, aState);

<https://mxr.mozilla.org/addons/search?string=gPermissionObject> seems to suggest that add-ons do not use this.

Comment 7

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4823baf754

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/ef4823baf754