1165217 - Use origin attribute in nsIUsageCallback

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

We implemented indexedDB for apps on B2G back in Bug 756645 and Bug 786295.
Now for our new security model we will update nsIPrincipal.origin in Bug 1163254.
I noticed that in nsIUsaseCallback it uses appId/mozBrowser as arguments.
We should try to use the new attribures added in nsIPrincial.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Use nsIPrincipal instead of nsIURI/appId/inBrowser for nsIQuotaManager

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

This dramatically changes the API for nsIQuotaManager to expose an nsIPrincipal, rather than the triple of nsIURI/appId/inBrowser.

I have checked, and only 2 addons I was able to find uses this API: https://addons.mozilla.org/en-US/firefox/addon/data-manager/ and the b2g emulator so I think it should be OK to make such a big change.

Comment 2

[Nika Layzell [:nika] (ni? for response)]

Attachment: Use nsIPrincipal instead of nsIURI/appId/inBrowser for nsIQuotaManager

Fixed a typo.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ae39d25a24

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8629531 [details] [diff] [review]
Use nsIPrincipal instead of nsIURI/appId/inBrowser for nsIQuotaManager

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

This looks like the right thing in general, though it should get review by somebody who knows this code.

This patch is incomplete though in the sense that it doesn't actually deal with including all origin attributes in the serialized data, and therefore the migration issue, right? Are you planning on doing that in this bug, or is there another bug on file for that work?

::: browser/base/content/pageinfo/permissions.js
@@ +190,5 @@
>    var quotaManager = Components.classes["@mozilla.org/dom/quota/manager;1"]
>                                 .getService(nsIQuotaManager);
> +  let principal = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
> +                            .getService(Components.interfaces.nsIScriptSecurityManager)
> +                            .getNoAppCodebasePrincipal(gPermURI);

You can just do .createCodebasePrincipal(gPermURI, {}). I think we should deprecate the *App* variants, since their naming makes less sense as we introduce new origin attributes.

Comment 5

[Jan Varga [:janv]]

(In reply to Bobby Holley (:bholley) from comment #4)
> Comment on attachment 8629531 [details] [diff] [review]
> Use nsIPrincipal instead of nsIURI/appId/inBrowser for nsIQuotaManager
> 
> Review of attachment 8629531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like the right thing in general, though it should get review by
> somebody who knows this code.
> 
> This patch is incomplete though in the sense that it doesn't actually deal
> with including all origin attributes in the serialized data, and therefore
> the migration issue, right? Are you planning on doing that in this bug, or
> is there another bug on file for that work?
> 

Ok, I can review this, but I need more information...
QuotaManager currently handles simple origins, for example: http+++www.mozilla.org
and origins with jar prefix, for example: 1007+f+app+++system.gaiamobile.org
The order (1007, f, app://system.gaiamobile.orig) is very important. When an app is uninstalled, we can remove all directories which begin with the jar prefix.
When you say "this patch is incomplete", do you mean that we need to include origin attributes in our "extended" origin string which is currently calculated ad jarPrefix + origin ?

Comment 6

[Jan Varga [:janv]]

Jonas, maybe you would know more, see the comment above.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll let Bobby cover this one unless my input is really needed.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Jan Varga [:janv] from comment #5)
> Ok, I can review this, but I need more information...

Hi Jan,

Origin Attributes are designed to replace jarPrefix. You can read more about them here:

https://hg.mozilla.org/mozilla-central/annotate/cef11c3e86c3/caps/nsIPrincipal.idl#l158

> QuotaManager currently handles simple origins, for example:
> http+++www.mozilla.org
> and origins with jar prefix, for example: 1007+f+app+++system.gaiamobile.org
> The order (1007, f, app://system.gaiamobile.orig) is very important. When an
> app is uninstalled, we can remove all directories which begin with the jar
> prefix.

The idea is that we want to use the "cookieJar" property on nsIPrincipal for any situation where we need to clear persistent data associated with an app/etc. See here: https://hg.mozilla.org/mozilla-central/annotate/cef11c3e86c3/caps/nsIPrincipal.idl#l207

The cookie jar itself is not guaranteed to encode all the information that we may need in order to properly separate origins in different buckets. For that, .origin (or .originSuffix) should be used.

> When you say "this patch is incomplete", do you mean that we need to include
> origin attributes in our "extended" origin string which is currently
> calculated ad jarPrefix + origin 

Right - his patch doesn't yet change any of the persistent stuff. Naively, the new scheme should be:

prin.cookieJar SOME_UNIQUE_SEPARATOR prin.origin

Where prin.origin will contain all of the relevant OriginAttributes.

Please let me know if you have any other questions or confusion about the new setup. I'm happy to explain anything in more detail!

Comment 9

[Nika Layzell [:nika] (ni? for response)]

I don't really understand enough just yet to quickly fix the serialization backend for QuotaManager to use origins.  I can look into it, but it may make sense to land the API change first, such that new consumers don't use an unsupported API, and then change the backend and serialization after.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Michael Layzell [:mystor] from comment #9)
> I don't really understand enough just yet to quickly fix the serialization
> backend for QuotaManager to use origins.  I can look into it, but it may
> make sense to land the API change first, such that new consumers don't use
> an unsupported API, and then change the backend and serialization after.

Sure. Just make sure to file a followup bug, and have it block bug 1179985? janv might be able to help out.

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

review ping?
It seems the r? has been for a month...

Comment 12

[Jan Varga [:janv]]

Comment on attachment 8629531 [details] [diff] [review]
Use nsIPrincipal instead of nsIURI/appId/inBrowser for nsIQuotaManager

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

::: browser/base/content/pageinfo/permissions.js
@@ +190,5 @@
>    var quotaManager = Components.classes["@mozilla.org/dom/quota/manager;1"]
>                                 .getService(nsIQuotaManager);
> +  let principal = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
> +                            .getService(Components.interfaces.nsIScriptSecurityManager)
> +                            .getNoAppCodebasePrincipal(gPermURI);

What bholley said, here and elsewhere.

::: testing/specialpowers/content/SpecialPowersObserverAPI.js
@@ -504,5 @@
>          if (op != 'clear' && op != 'getUsage' && op != 'reset') {
>            throw new SpecialPowersError('Invalid operation for SPQuotaManager');
>          }
>  
> -        let uri = this._getURI(msg.uri);

Why is this removed ?

Comment 13

[Nika Layzell [:nika] (ni? for response)]

(In reply to Jan Varga [:janv] from comment #12)
> > -        let uri = this._getURI(msg.uri);
> 
> Why is this removed ?

I appear to call it in each of the branches of the following if statement, so it wasn't really removed, just moved. the uri variable is no longer used in the function. I could leave the variable binding in, but it doesn't really make a definition. (I wrote this long enough ago that I don't remember why I would have moved the call into the if statement branches).

Comment 14

[Jan Varga [:janv]]

(In reply to Michael Layzell [:mystor] from comment #13)
> (In reply to Jan Varga [:janv] from comment #12)
> > > -        let uri = this._getURI(msg.uri);
> > 
> > Why is this removed ?
> 
> I appear to call it in each of the branches of the following if statement,
> so it wasn't really removed, just moved. the uri variable is no longer used
> in the function. I could leave the variable binding in, but it doesn't
> really make a definition. (I wrote this long enough ago that I don't
> remember why I would have moved the call into the if statement branches).

Err, I meant why you removed the variable, I think it still makes sense to keep it.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

(In reply to Jan Varga [:janv] from comment #14)
> Err, I meant why you removed the variable, I think it still makes sense to
> keep it.

Alright, sounds good to me

Comment 16

[Jan Varga [:janv]]

Otherwise good stuff, thanks.

Comment 17

[Pulsebot]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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