Use origin attribute in nsIUsageCallback

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: allstars.chh, Assigned: Nika)

Tracking

({addon-compat})

Trunk
mozilla43
addon-compat
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox41 affected, firefox43 fixed)

Details

(Whiteboard: [addon-compat mentioned in comment #1])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Summary: Use origin in nsIUsageCallback → Use origin attribute in nsIUsageCallback
No longer depends on: 1163254
Blocks: 1179985
(Assignee)

Comment 1

4 years ago
Created attachment 8629513 [details] [diff] [review]
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.
Attachment #8629513 - Flags: review?(bobbyholley)
(Assignee)

Updated

4 years ago
Assignee: nobody → michael
Keywords: addon-compat
Whiteboard: [addon-compat mentioned in comment #1]
(Assignee)

Comment 2

4 years ago
Created attachment 8629531 [details] [diff] [review]
Use nsIPrincipal instead of nsIURI/appId/inBrowser for nsIQuotaManager

Fixed a typo.
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=88ae39d25a24
Attachment #8629513 - Attachment is obsolete: true
Attachment #8629513 - Flags: review?(bobbyholley)
Attachment #8629531 - Flags: review?(bobbyholley)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1165224
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.
Attachment #8629531 - Flags: review?(bobbyholley)
Attachment #8629531 - Flags: review?(Jan.Varga)
Attachment #8629531 - Flags: feedback+

Comment 5

4 years ago
(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 ?
Flags: needinfo?(bobbyholley)

Comment 6

4 years ago
Jonas, maybe you would know more, see the comment above.
Flags: needinfo?(jonas)
I'll let Bobby cover this one unless my input is really needed.
Flags: needinfo?(jonas)
(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!
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 9

4 years ago
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.
(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.
review ping?
It seems the r? has been for a month...
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 ?
Attachment #8629531 - Flags: review?(Jan.Varga) → review+
(Assignee)

Comment 13

3 years ago
(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).
(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.
(Assignee)

Comment 15

3 years ago
(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
Otherwise good stuff, thanks.
(Assignee)

Updated

3 years ago
Blocks: 1195930
https://hg.mozilla.org/mozilla-central/rev/a460f643b912
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43

Updated

3 years ago
QA Whiteboard: [COM=NSec]
You need to log in before you can comment on or make changes to this bug.