Splitting this out as discussed in bug 1163254 comment 7. This is actually the real meat of the changes, in that it creates the generalized framework by which we can easily add other app attributes. Yoshi should be able to build on this once it's green.

Given the behavior change for .origin, this may go orange: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a8678321d4f

Alright, so this should be relatively code-complete, modulo the various failures I need to look at the result from my munging of .origin. I'm out of time to look at those today, and will have a look on Monday. Meanwhile, Jonas - if you have a few minutes to give any brief feedback you'd like to make on the API, this would be a good time to do it (i.e. before I go around fixing up various consumers). You can scrape the patches here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=689a8a0292c8 I'll also upload a zip of patches in sequence that you can qimport if you prefer.

I just looked at the test_defaultStorageUpgrade.js failure. The basic problem is that it's using a packaged profile, and so it doesn't find the IDB database it expects ("1007+f+app+++system.gaiamobile.org") because it's now called "1007+f+app+++system.gaiamobile.org+appId=1007". We can obviously fix up the packaged profile, but we should think about what kind if migration scheme, if any, we'll need here.

Fixed a bug in GetAppStatusForPrincipal. This should be greener: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea6626a953d1

The current check will fail once we start munging the format of nsIPrincipal::Origin.

We also provide an opt-out for the original behavior, and use it in various consumers that look like they need fixing up. Most of the usage here is in code with persistence considerations, where we may need some sort of migration path.

Comment on attachment 8607138 [details] [diff] [review] Part 1 - Make OriginAttributes a dictionary, and make it accessible as both a jsval and a canonical string. v1 Review of attachment 8607138 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/nsIPrincipal.idl @@ +174,5 @@ > + [implicit_jscontext] > + readonly attribute jsval originAttributes; > + > + /** > + * A string of the form ?key1=value1&key2=value2, where each pair represents I feel like there are more benefits than downsides restrict ourselves to characters that can be used in filenames here. Which I think basically means switching the '?' to something else. Might even be worth simply using '&' as to not have to use different separator for the first key/value pair.

Comment on attachment 8607140 [details] [diff] [review] Part 2 - Rework the nsIScriptSecurityManager principal-minting API to be originAttributes-centric. v1 Review of attachment 8607140 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/nsIScriptSecurityManager.idl @@ +191,5 @@ > + * See nsIPrincipal.h for a description of origin attributes, and > + * SystemDictionaries.webidl for a list of origin attributes and their defaults. > + */ > + [implicit_jscontext] > + nsIPrincipal createNullPrincipal(in jsval originAttributes); Does it really make sense to set originAttributes on null principals? A null principal should never be permitted to store any data to disk, and doesn't appear to be allowed to make network requests. I'm not necessarily opposed, more curious why.

Comment on attachment 8607141 [details] [diff] [review] Part 3 - Fix up nsScriptSecurityManager::AppStatusForPrincipal to compare principals rather than origins. v1 Review of attachment 8607141 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/nsScriptSecurityManager.cpp @@ +290,5 @@ > > + // The app could contain a cross-origin iframe - make sure that the content > + // is actually same-origin with the app. > + nsCOMPtr<nsIPrincipal> appPrin; > + OriginAttributes attrs(aPrin->GetAppId(), false); might as well use |appId| here. And might be worth commenting on why you are using 'false' rather than 'inMozBrowser' as second argument. Hmm... Though shouldn't it be possible to get an OriginAttributes off of the nsIPrincipal? I thought the point was that we wanted code like this be able to just grab the OriginAttributes and forward that as an opaque object?

Comment on attachment 8607143 [details] [diff] [review] Part 5 - Serialize originSuffix into .origin. v2 Review of attachment 8607143 [details] [diff] [review]: ----------------------------------------------------------------- Is the plan here to do a followup bug to change a bunch of these from .originNoSuffix to use .origin? sr=me assuming that's the case.

(In reply to Jonas Sicking (:sicking) from comment #13) > I feel like there are more benefits than downsides restrict ourselves to > characters that can be used in filenames here. Which I think basically means > switching the '?' to something else. > > Might even be worth simply using '&' as to not have to use different > separator for the first key/value pair. Ok - I replaced it with |!|, which I think is a bit more recognizable and is easier to swap out with my existing JS parsing code. (In reply to Jonas Sicking (:sicking) from comment #14) > Does it really make sense to set originAttributes on null principals? > > A null principal should never be permitted to store any data to disk, and > doesn't appear to be allowed to make network requests. > > I'm not necessarily opposed, more curious why. I'm not sure actually, but we currently have code that explicitly handles this, so I'm assuming that there's some FirefoxOS-y reason. I added the SSM API so that I could properly test the code that we have. (In reply to Jonas Sicking (:sicking) from comment #15) > might as well use |appId| here. And might be worth commenting on why you are > using 'false' rather than 'inMozBrowser' as second argument. Ok. > Hmm... Though shouldn't it be possible to get an OriginAttributes off of the > nsIPrincipal? I thought the point was that we wanted code like this be able > to just grab the OriginAttributes and forward that as an opaque object? Which nsIPrincipal? We're comparing an nsIPrincipal with a mozIApplication here. It would be nice if mozIApplication exposed a principal instead of an origin string, but that isn't the case right now, so we need to synthesize one. (In reply to Jonas Sicking (:sicking) from comment #16) > Is the plan here to do a followup bug to change a bunch of these from > .originNoSuffix to use .origin? Yes, these are basically breadcrumbs for Yoshi to go fix up. :-)

Per today's discussion.

Comment on attachment 8607335 [details] [diff] [review] Part 7 - Add nsIPrincipal::cookieJar. v1 Review of attachment 8607335 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.cpp @@ +160,5 @@ > +BasePrincipal::GetCookieJar(nsACString& aCookieJar) > +{ > + // We just forward to .jarPrefix for now, which is a nice compact > + // stringification of the (appId, inBrowser) tuple. This will eventaully be > + // swapped out for an origin attribute - see the comment in nsIPrincipal.idl. "origin attribute" sounds wrong here, since "cookie jar" is exactly the part of .origin which isn't really an "origin". Maybe "generic attribute" instead? Or "generic mechanism"?

(In reply to Bobby Holley (:bholley) from comment #17) > > Hmm... Though shouldn't it be possible to get an OriginAttributes off of the > > nsIPrincipal? I thought the point was that we wanted code like this be able > > to just grab the OriginAttributes and forward that as an opaque object? > > Which nsIPrincipal? We're comparing an nsIPrincipal with a mozIApplication > here. It would be nice if mozIApplication exposed a principal instead of an > origin string, but that isn't the case right now, so we need to synthesize > one. You're lifting the appid+inbrowser from aPrincipal. I.e. you're lifting all the "origin suffix" from aPrincipal. Except that the test higher up ensures that inbrowser==false by the time we get down here. What this code is actually doing, is to do a same-origin check between the aPrincipal.uri and the mozIApplication.manifestURL. But it does so by creating a new nsIPrincipal where all the properties except the uri is copied from aPrincipal.

Comment on attachment 8607138 [details] [diff] [review] Part 1 - Make OriginAttributes a dictionary, and make it accessible as both a jsval and a canonical string. v1 Review of attachment 8607138 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.h @@ +56,5 @@ > + OriginAttributes() {} > + OriginAttributes(uint32_t aAppId, bool aInBrowser) > + { > + mAppId = aAppId; > + mInBrowser = aInBrowser; why did you change the usual : mAppId(aAppId) syntax here? ::: dom/webidl/SystemDictionaries.webidl @@ +17,5 @@ > + * methods. > + */ > +dictionary OriginAttributesDictionary { > + unsigned long appId = 0; > + boolean inBrowser = false; Why do we call it inBrowser here instead of the new inBrowserElement you started to use lately?

Comment on attachment 8607140 [details] [diff] [review] Part 2 - Rework the nsIScriptSecurityManager principal-minting API to be originAttributes-centric. v1 Review of attachment 8607140 [details] [diff] [review]: ----------------------------------------------------------------- I don't know about this. I see it's deprecated but I get 234 matches for getCodebasePrincipal in add-on code... Why don't we just make the second argument optional and default it to {} ?

(In reply to Jonas Sicking (:sicking) from comment #20) > "origin attribute" sounds wrong here, since "cookie jar" is exactly the part > of .origin which isn't really an "origin". It exactly fits the definition of "origin attribute" AFAICT - it's an attribute with its own semantic meaning which must be compared (along with other origin attributes) when computing same-origin-ness. > Maybe "generic attribute" instead? Or "generic mechanism"? My understand of our agreement from yesterday is that, long-term, the cookie jar would probably map exactly to a value called userContext or somesuch, which would need to live in originAttributes per the first paragraph above. We're introducing this getter so that we can seamlessly switch to it from the current appId:inBrowser scheme we have now. So the comment seems entirely accurate to me. Am I miss-remembering something? (In reply to Jonas Sicking (:sicking) from comment #21) > You're lifting the appid+inbrowser from aPrincipal. I.e. you're lifting all > the "origin suffix" from aPrincipal. Except that the test higher up ensures > that inbrowser==false by the time we get down here. > > What this code is actually doing, is to do a same-origin check between the > aPrincipal.uri and the mozIApplication.manifestURL. But it does so by > creating a new nsIPrincipal where all the properties except the uri is > copied from aPrincipal. The way I see it, this code is basically doing a principal equality check between aPrincipal and a virtual principal (with an origin, an appId, and inBrowser=false) that is deducible from mozIApplication (and would ideally be exposed by a getter on mozIApplication). It's not clear to me what semantics we want here with respect to other origin attributes, which is why I did things explicitly for now.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #22) > ::: caps/BasePrincipal.h > @@ +56,5 @@ > > + OriginAttributes() {} > > + OriginAttributes(uint32_t aAppId, bool aInBrowser) > > + { > > + mAppId = aAppId; > > + mInBrowser = aInBrowser; > > why did you change the usual : mAppId(aAppId) syntax here? To make it clear that these members exist in and are initialized by the superclass. > ::: dom/webidl/SystemDictionaries.webidl > @@ +17,5 @@ > > + * methods. > > + */ > > +dictionary OriginAttributesDictionary { > > + unsigned long appId = 0; > > + boolean inBrowser = false; > > Why do we call it inBrowser here instead of the new inBrowserElement you > started to use lately? Jonas is concerned about character length when serializing to filenames and whatnot. I'm happy to use aInBrowser everywhere going forward.

(In reply to Bobby Holley (:bholley) from comment #24) > (In reply to Jonas Sicking (:sicking) from comment #20) > > "origin attribute" sounds wrong here, since "cookie jar" is exactly the part > > of .origin which isn't really an "origin". > > It exactly fits the definition of "origin attribute" AFAICT - it's an > attribute with its own semantic meaning which must be compared (along with > other origin attributes) when computing same-origin-ness. I don't feel strongly. > (In reply to Jonas Sicking (:sicking) from comment #21) > > You're lifting the appid+inbrowser from aPrincipal. I.e. you're lifting all > > the "origin suffix" from aPrincipal. Except that the test higher up ensures > > that inbrowser==false by the time we get down here. > > > > What this code is actually doing, is to do a same-origin check between the > > aPrincipal.uri and the mozIApplication.manifestURL. But it does so by > > creating a new nsIPrincipal where all the properties except the uri is > > copied from aPrincipal. > > The way I see it, this code is basically doing a principal equality check > between aPrincipal and a virtual principal (with an origin, an appId, and > inBrowser=false) that is deducible from mozIApplication (and would ideally > be exposed by a getter on mozIApplication). It's not clear to me what > semantics we want here with respect to other origin attributes, which is why > I did things explicitly for now. What I'm saying above is the semantic meaning that this code wants to have in a generic sense. I.e. it will want to copy all parts of OriginAttributes over and only change the URI.

> > why did you change the usual : mAppId(aAppId) syntax here? > > To make it clear that these members exist in and are initialized by the > superclass. C++ doesn't allow using the initalizer-list to set members that live in the superclass. So :mAppId(aAppId) wouldn't compile.

(In reply to Jonas Sicking (:sicking) from comment #26) > What I'm saying above is the semantic meaning that this code wants to have > in a generic sense. I.e. it will want to copy all parts of OriginAttributes > over and only change the URI. Is it, though? So far, it's 50-50 in terms of which of the origin attributes it wants to copy (i.e. it explicitly doesn't want to copy mozBrowser - we could even nix the inBrowser bailout case at the beginning and just rely on the principals not comparing equal). I understand that the author originally wrote the code with the more humble intent of "just comparing origins", but I think that "compare the principal of the code with the (virtual) principal of the app in the registry" is a more useful and robust conceptual model. I don't feel strongly about it, I'm just not convinced that copying all origin attributes is clearly better. I'd be fine to switch the patch to doing that if that's what you'd prefer though.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #23) > I don't know about this. I see it's deprecated but I get 234 matches for > getCodebasePrincipal in add-on code... Oh, yuck. > Why don't we just make the second argument optional and default it to {} ? I want callers to be explicit about it, otherwise most people will probably never realize that there's a second argument. I'm going to just introduce a new method called |createCodebasePrincipal| and deprecate the others.

Comment on attachment 8608203 [details] [diff] [review] Part 0.5 - Stop recreating principals from the message manager. v1 It's not clear to me why these callers are doing it this way - Jonas, do you have any idea?

I didn't end up needing this in bug, but I think it's handy to have around.

Comment on attachment 8607833 [details] [diff] [review] Part 2 - Rework the nsIScriptSecurityManager principal-minting API to be originAttributes-centric. v3 sr=sicking Review of attachment 8607833 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.cpp @@ +203,5 @@ > + uriPrinc->GetPrincipal(getter_AddRefs(principal)); > + if (!principal) { > + return nsNullPrincipal::Create(); > + } > + nsRefPtr<BasePrincipal> concrete = dont_AddRef(Cast(principal.forget().take())); Why are you trying to avoid addreffing and releasing here? Can you just get rid of the forget and dont_AddRef here to make the code simpler?

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #36) > Why are you trying to avoid addreffing and releasing here? Can you just get > rid of the forget and dont_AddRef here to make the code simpler? Just trying to avoid unnecessary calls to addref/release, but I can buy that it's more trouble than it's worth here. I'll simplify it.

Comment on attachment 8607141 [details] [diff] [review] Part 3 - Fix up nsScriptSecurityManager::AppStatusForPrincipal to compare principals rather than origins. v1 Review of attachment 8607141 [details] [diff] [review]: ----------------------------------------------------------------- Now I can totally see why you were against putting important logic inside NS_ENSURE_SUCCESS macros... looks all correct though :)

Comment on attachment 8608203 [details] [diff] [review] Part 0.5 - Stop recreating principals from the message manager. v1 Related bugs https://bugzilla.mozilla.org/show_bug.cgi?id=916091 https://bugzilla.mozilla.org/show_bug.cgi?id=1030997 As far as I see, we do the same thing on message manager side.

Comment on attachment 8607141 [details] [diff] [review] Part 3 - Fix up nsScriptSecurityManager::AppStatusForPrincipal to compare principals rather than origins. v1 Review of attachment 8607141 [details] [diff] [review]: ----------------------------------------------------------------- Ok, lets leave this as-is.

Comment on attachment 8608202 [details] [diff] [review] Part 0.0 - Make requestsync test failures more useful. v1 Switching this to sicking, since I'll bet baku's gone to bed.

Comment on attachment 8608202 [details] [diff] [review] Part 0.0 - Make requestsync test failures more useful. v1 Ok, sicking needs to go, so I'm just going to flag various people for retroactive review on these bits and push it to inbound.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c2cd9f48962 https://treeherder.mozilla.org/#/jobs?repo=try&revision=22a99e7eeab9 https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=eabe83ede1e3

https://hg.mozilla.org/integration/mozilla-inbound/rev/c081648c06e3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a005f1452f41 https://hg.mozilla.org/integration/mozilla-inbound/rev/121b818c01a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/72f81e3ed9aa https://hg.mozilla.org/integration/mozilla-inbound/rev/5bc2395aa710 https://hg.mozilla.org/integration/mozilla-inbound/rev/61aa80fb79b6 https://hg.mozilla.org/integration/mozilla-inbound/rev/67d449223191 https://hg.mozilla.org/integration/mozilla-inbound/rev/85402fd3e3b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2c0a02352e https://hg.mozilla.org/integration/mozilla-inbound/rev/e44ba0c0744a

Comment on attachment 8607143 [details] [diff] [review] Part 5 - Serialize originSuffix into .origin. v2 Review of attachment 8607143 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp @@ +982,5 @@ > uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; > nsAutoCString appOrigin; > > doc->NodePrincipal()->GetAppStatus(&appStatus); > + doc->NodePrincipal()->GetOriginNoSuffix(getter_Copies(appOrigin)); need to rebase this for Bug 1165835 which removed getter_Copies

(In reply to Yoshi Huang[:allstars.chh] from comment #47) > Comment on attachment 8607143 [details] [diff] [review] > Part 5 - Serialize originSuffix into .origin. v2 > > Review of attachment 8607143 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/bluetooth/bluetooth2/BluetoothAdapter.cpp > @@ +982,5 @@ > > uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED; > > nsAutoCString appOrigin; > > > > doc->NodePrincipal()->GetAppStatus(&appStatus); > > + doc->NodePrincipal()->GetOriginNoSuffix(getter_Copies(appOrigin)); > > need to rebase this for Bug 1165835 which removed getter_Copies Yes, included in the version pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/85402fd3e3b4

Comment on attachment 8608205 [details] [diff] [review] Part 8 - Introduce a helper for converting from origin strings to a principal. v1 Review of attachment 8608205 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/BrowserUtils.jsm @@ +94,5 @@ > return Services.io.newURI(aCPOWURI.spec, aCPOWURI.originCharset, null); > }, > > + // Creates a codebase principal from a canonical origin string. This is the > + // the inverse operation of .origin on a codebase principal. remove 'the', you already had it from last line.

https://hg.mozilla.org/mozilla-central/rev/c081648c06e3 https://hg.mozilla.org/mozilla-central/rev/a005f1452f41 https://hg.mozilla.org/mozilla-central/rev/121b818c01a5 https://hg.mozilla.org/mozilla-central/rev/72f81e3ed9aa https://hg.mozilla.org/mozilla-central/rev/5bc2395aa710 https://hg.mozilla.org/mozilla-central/rev/61aa80fb79b6 https://hg.mozilla.org/mozilla-central/rev/67d449223191 https://hg.mozilla.org/mozilla-central/rev/85402fd3e3b4 https://hg.mozilla.org/mozilla-central/rev/cb2c0a02352e https://hg.mozilla.org/mozilla-central/rev/e44ba0c0744a https://hg.mozilla.org/mozilla-central/rev/eabe83ede1e3