Closed
Bug 1165217
Opened 10 years ago
Closed 9 years ago
Use origin attribute in nsIUsageCallback
Categories
(Core :: Storage: IndexedDB, defect)
Core
Storage: IndexedDB
Tracking
()
RESOLVED
FIXED
mozilla43
People
(Reporter: allstars.chh, Assigned: nika)
References
Details
(Keywords: addon-compat, Whiteboard: [addon-compat mentioned in comment #1])
Attachments
(1 file, 1 obsolete file)
23.83 KB,
patch
|
janv
:
review+
bholley
:
feedback+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•10 years ago
|
Summary: Use origin in nsIUsageCallback → Use origin attribute in nsIUsageCallback
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
Assignee | ||
Comment 2•9 years ago
|
||
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)
Comment 4•9 years ago
|
||
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•9 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•9 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)
Comment 8•9 years ago
|
||
(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•9 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.
Comment 10•9 years ago
|
||
(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.
Reporter | ||
Comment 11•9 years ago
|
||
review ping?
It seems the r? has been for a month...
Comment 12•9 years ago
|
||
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•9 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).
Comment 14•9 years ago
|
||
(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•9 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
Comment 16•9 years ago
|
||
Otherwise good stuff, thanks.
Comment 17•9 years ago
|
||
Comment 18•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•9 years ago
|
QA Whiteboard: [COM=NSec]
You need to log in
before you can comment on or make changes to this bug.
Description
•