There is a subset of callers to createCodePrincipal that specifically deal with user permissions, feeds, accounts, bookmarks, sync, and a few other corner cases where we are not going to isolation on the user context ID. This requires us to create a new type of OriginAttribute (OA) subclass that has its own inherit functions for initialization that always slams the user context ID to 0. This new type of OA needs to always be used in the managers for these settings/prefs/values and for the about: pages that implement the user interface for managing them.

What should we call this new OA? Some ideas: PrefOriginAttribute PermissionOriginAttribute AboutColonOriginAttribute UserDataOriginAttribute I'm not sure what would be best. I'm still looking for a good name.

I think PermissionOriginAttributes would go with what we had in mind (i.e. something only used for reading/writing permissions)

Adds PermissionOriginAttributes.

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

(In reply to Jonas Sicking (:sicking) from comment #2) > I think PermissionOriginAttributes would go with what we had in mind (i.e. > something only used for reading/writing permissions) Sorry to bikeshed this. But we are planning to use this for about:feeds and about:preferences too. Hence, PermissionsOriginAttributes is more specific than our use cases. But permissions are a "preference" so PreferencesOriginAttributes might be a better name. Jonas?

Permissions are managed by the nsIPermissionManager, no? Not preferences?

Preferences aren't keyed on origins, uris, or origin-attributes, so I'm not sure how they play in here? I also don't know how about:feeds work. What storage API does it use?

Turns out that PermissionOriginAttributes is just a special case of PrincipalOriginAttributes so I adjusted the C++ accordingly. This patch also covers fixing up the permissions manager and permissions so that they use PermissionOriginAttributes instead of PrincipalOriginAttributes.

So from my audit notes: >extensions/cookie/nsPermissionManager.cpp > line 119 > nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(uri, attrs); > > line 130 > nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(aURI, attrs); > > line 141 > nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(aURI, attrs); > > line 2172 > nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(newURI, attrs); > > NOTE: fixed by switching to PermissionOriginAttributes > > extensions/cookie/nsPermission.cpp > line 171 > nsCOMPtr<nsIPrincipal> principal = mozilla::BasePrincipal::CreateCodebasePrincipal(aURI, attrs); > > NOTE: fixed by switching to PermissionOriginAttributes This patch covers these parts of my audit.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=96a6498d268a

So I was thinking about this one. It might be simpler to not add a new struct-type and instead simply make the permissionmanager code ignore the contextId in the PrincipalOriginAttribute that it is passed. Possibly we could have done this for NeckoOriginAttributes as well... If it later turns out that a new struct is better we can always implement that then. This way we get to compare both approaches.

small patch that just adds the PermissionOriginAttribute class.

patch that *just* fixes the permission and permission manager.

(In reply to Jonas Sicking (:sicking) from comment #11) > So I was thinking about this one. It might be simpler to not add a new > struct-type and instead simply make the permissionmanager code ignore the > contextId in the PrincipalOriginAttribute that it is passed. What do you mean by struct-type? AFAICT, the only reason OriginAttributes are a class is so they derive from the JS::Dictionary for easier interop with JS. Personally, IMO the Inherit*() functions are clunky. I would have just created explicit copy constructors for the desired conversions like so: > class PrincipalOriginAttribute { > explicit PrincipalOriginAttributes(const NeckoOriginAttributes& rhs) { /* copy the attr's we want */ } > } I understand that calling Inherit* is more explicit, but I think C++ conversion copy constructors are more C++-ish. But my yaks need shaving! :)

The explicitness of Inherit is pretty important, IMO - it makes the fact that a conversion is taking place more obvious to the reader, and also makes it clear _why_ that conversion needs to happen (i.e. InheritFromDocToChildDocShell). At one point we had several conversion routines with the same |this| and argument types, in which case separate names is the only way to differentiate them.

This bug now is limited to just the new DefaultContextOriginAttributes type and the associated utility functions.

(In reply to Bobby Holley (busy) from comment #16) > The explicitness of Inherit is pretty important, IMO - it makes the fact > that a conversion is taking place more obvious to the reader, and also makes > it clear _why_ that conversion needs to happen (i.e. > InheritFromDocToChildDocShell). At one point we had several conversion > routines with the same |this| and argument types, in which case separate > names is the only way to differentiate them. The problem with the Inherit* design is that the underlying type of all origin attributes is a JS::Dictionary which makes them automatically interchangeable. Also, none of the Inherit* functions are exposed to Javascript. When we ruled out { userContextID = 0; } as a manual fixup solution, then we're back to solutions similar to the recently-deprecated getNoAppPrincipal(). Utility functions that create/convert special purpose origin attributes that can then be used to create a principal from Javascript code. My patch for this bug creates a new DefaultContextOriginAttributes type and a ChromeUtils function that takes any origin attribute and returns me an origin attributes dictionary with the userContextID set to 0. That's my solution for avoiding having to manually fix up the value of userContextID everywhere and it doesn't use any of the Inherit* functions. If we had an nsIOriginAttribute interface that had functions like createNeckoOAFromDocShellOA() that I could call from Javascript, then maybe the inherit functions would work for all of the fixup I'm doing now? Thoughts?

(In reply to Dave Huseby [:huseby] from comment #18) > The problem with the Inherit* design is that the underlying type of all > origin attributes is a JS::Dictionary which makes them automatically > interchangeable. In C++ or JS? They definitely shouldn't be interchangable in C++. But yes, this doesn't really help us in JS. > Also, none of the Inherit* functions are exposed to > Javascript. We can expose those methods on ChromeUtils as appropriate if that'd be helpful. I'm not really in the loop enough to comment further, so I'll leave the rest between you and sicking.

fixed some naming issues, thanks Tanvi.

added definitions for PopulateFromSuffix and PopulateFromOrigin in the DefaultContextOriginAttributes class so that the mUserContextId gets forced to the default value after the population process happens.

The latest version of this patch doesn't compile because |mozilla::PrincipalOriginAttributes& mozilla::PrincipalOriginAttributes::operator=(const mozilla::dom::OriginAttributesDictionary&)| is not defined.

The statement "for places where we don't want context separation" only refers to user context, right? Other types of contexts, like B2G apps, or Tor contexts, will likely want to separate permissions and other things into separate "jars".

Comment on attachment 8704392 [details] [diff] [review] Bug_1229222.patch Review of attachment 8704392 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.h @@ +138,5 @@ > +{ > +public: > + DefaultContextOriginAttributes() > + { > + mUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID; Why is this needed? Shouldn't the OriginAttributes baseclass default ctor set this? @@ +145,5 @@ > + DefaultContextOriginAttributes(uint32_t aAppId, bool aInBrowser) > + { > + mAppId = aAppId; > + mInBrowser = aInBrowser; > + mUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID; Is this ctor really needed? If so, why? @@ +156,5 @@ > + > + DefaultContextOriginAttributes& operator=(const OriginAttributesDictionary& aOther) > + { > + PrincipalOriginAttributes::operator=(aOther); > + mUserContextId = nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID; In general I don't understand the design here. It feels both non-generic (special treatment to some attributes over others) and surprising (why does the assignment operator not copy all attributes but the copy ctor does?) I guess I don't really understand how this struct is intended to be used.

To be clear, I'm not saying that a new struct is wrong. I just don't understand the current approach.

rebased.

tests.

Alright, after fixing up a bunch of patches, I think this is the minimal set of changes needed to accomplish what we want.

Full coverage tests for the changes.

full coverage tests for the changes.

Comment on attachment 8713777 [details] [diff] [review] Bug_1229222.patch Review of attachment 8713777 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/ChromeUtils.cpp @@ +98,5 @@ > + const dom::OriginAttributesDictionary& aAttrs, > + dom::OriginAttributesDictionary& aNewAttrs) > +{ > + GenericOriginAttributes attrs(aAttrs); > + aNewAttrs = attrs; Do you even need the temporary? Wouldn't simply aNewAttrs = aAttrs work? @@ +135,5 @@ > + > +/* static */ void > +ChromeUtils::SetDefaultUserContextId(dom::GlobalObject& aGlobal, > + const dom::OriginAttributesDictionary& aAttrs, > + dom::OriginAttributesDictionary& aNewAttrs) Why is this function needed? Why not just have the JS code do myDict.userContextId = 0; Or, if you want to initialize all non-set members to default values, and also set the userContextId to default value, then: myDict = chromeUtils.createOriginAttributesFromDict(myDict); myDict.userContextId = 0; ? ::: dom/webidl/ChromeUtils.webidl @@ +75,5 @@ > + * both, including defaults. > + */ > + static OriginAttributesDictionary > + setOriginAttributes(optional OriginAttributesDictionary originAttrs, > + optional OriginAttributesPatternDictionary pattern); I don't understand what this function is intended to do, or when it's supposed to be used. It appears that the documentation above has typos in it because there is no setAttrs parameter, and there is no description of the 'pattern' parameter. Also, OriginAttributesPatternDictionary was intended to be a way to test if given OA-struct matches a given wild-card enabled pattern. It was not meant as a way to initialize OA-structs. I'm curious to understand what you are doing that needs this function.

Comment on attachment 8713777 [details] [diff] [review] Bug_1229222.patch Review of attachment 8713777 [details] [diff] [review]: ----------------------------------------------------------------- For what it's worth, the review comments here and in bug 1233904 is holding up my other UserContextId reviews as well since those patches seem very similar.

(In reply to Jonas Sicking (:sicking) from comment #33) > Comment on attachment 8713777 [details] [diff] [review] > Bug_1229222.patch > > Review of attachment 8713777 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/base/ChromeUtils.cpp > @@ +98,5 @@ > > + const dom::OriginAttributesDictionary& aAttrs, > > + dom::OriginAttributesDictionary& aNewAttrs) > > +{ > > + GenericOriginAttributes attrs(aAttrs); > > + aNewAttrs = attrs; > > Do you even need the temporary? Wouldn't simply > > aNewAttrs = aAttrs > > work? I think you're right. That's a vestige of when we were creating a DefaultContextOriginAttributes instance from the OriginAttributesDictionary. Let me test it to make sure. > @@ +135,5 @@ > > + > > +/* static */ void > > +ChromeUtils::SetDefaultUserContextId(dom::GlobalObject& aGlobal, > > + const dom::OriginAttributesDictionary& aAttrs, > > + dom::OriginAttributesDictionary& aNewAttrs) > > Why is this function needed? Why not just have the JS code do > > myDict.userContextId = 0; > > Or, if you want to initialize all non-set members to default values, and > also set the userContextId to default value, then: > > myDict = chromeUtils.createOriginAttributesFromDict(myDict); > myDict.userContextId = 0; > > ? I did it this way for two reasons: #1 We wanted to avoid setting origin attributes by hand in this way because of maintenance reasons. If we later decide to choose a different default value, change the name of the attribute, and/or make the default some result of a function, I don't want to have to go back through the code grep'ing for assignments like this. With it this way, we would only have to change the body of this function. #2 This also gives us a chance to make sure that all of the default attributes are defined and have default values as well. By forcing a JS -> C++ -> JS transition, we will end up with a JS dictionary will all of the members. > ::: dom/webidl/ChromeUtils.webidl > @@ +75,5 @@ > > + * both, including defaults. > > + */ > > + static OriginAttributesDictionary > > + setOriginAttributes(optional OriginAttributesDictionary originAttrs, > > + optional OriginAttributesPatternDictionary pattern); > > I don't understand what this function is intended to do, or when it's > supposed to be used. > > It appears that the documentation above has typos in it because there is no > setAttrs parameter, and there is no description of the 'pattern' parameter. > > Also, OriginAttributesPatternDictionary was intended to be a way to test if > given OA-struct matches a given wild-card enabled pattern. It was not meant > as a way to initialize OA-structs. > > I'm curious to understand what you are doing that needs this function. You're right about the typo, good catch. I'll fix it. The OriginAttributesPatternDictionary is handy for testing if an attributes was explicitly set and not just the default value. That makes it possible to do: > attrs = ChromeUtils.setAttributes(attrs, {userContextId: 0, inBrowser: true}); Without knowing if a member is manually set, you wind up overwriting all members of attrs and not just the ones defined in the second attribute.

> #1 We wanted to avoid setting origin attributes by hand in this way because > of maintenance reasons. If we later decide to choose a different default > value, change the name of the attribute, and/or make the default some result > of a function, I don't want to have to go back through the code grep'ing for > assignments like this. With it this way, we would only have to change the > body of this function. > > #2 This also gives us a chance to make sure that all of the default > attributes are defined and have default values as well. By forcing a JS -> > C++ -> JS transition, we will end up with a JS dictionary will all of the > members. Why not delete myDict.userContextId; myDict = chromeUtils.createOriginAttributesFromDict(myDict); or myDict.userContextId = undefined; myDict = chromeUtils.createOriginAttributesFromDict(myDict); > > attrs = ChromeUtils.setAttributes(attrs, {userContextId: 0, inBrowser: true}); > > Without knowing if a member is manually set, you wind up overwriting all > members of attrs and not just the ones defined in the second attribute. Why not attrs.userContextId = 0; attrs.inBrowser = true; attrs = chromeUtils.createOriginAttributesFromDict(attrs); (where the latter is only needed if you want to make sure that all attributes have explicit default values)

(In reply to Jonas Sicking (:sicking) from comment #36) > > #1 We wanted to avoid setting origin attributes by hand in this way because > > of maintenance reasons. If we later decide to choose a different default > > value, change the name of the attribute, and/or make the default some result > > of a function, I don't want to have to go back through the code grep'ing for > > assignments like this. With it this way, we would only have to change the > > body of this function. > > > > #2 This also gives us a chance to make sure that all of the default > > attributes are defined and have default values as well. By forcing a JS -> > > C++ -> JS transition, we will end up with a JS dictionary will all of the > > members. > > Why not > > delete myDict.userContextId; > myDict = chromeUtils.createOriginAttributesFromDict(myDict); > > or > > myDict.userContextId = undefined; > myDict = chromeUtils.createOriginAttributesFromDict(myDict); You're referencing the attributes by name in those cases. Those would work, but it makes the code more brittle. > > > > attrs = ChromeUtils.setAttributes(attrs, {userContextId: 0, inBrowser: true}); > > > > Without knowing if a member is manually set, you wind up overwriting all > > members of attrs and not just the ones defined in the second attribute. > > Why not > > attrs.userContextId = 0; > attrs.inBrowser = true; > attrs = chromeUtils.createOriginAttributesFromDict(attrs); > > (where the latter is only needed if you want to make sure that all > attributes have explicit default values) This works too and I can't claim it is more brittle because I referenced the attributes by name in my example too.

You are referencing userContextId just as much in ChromeUtils.setDefaultUserContextId(...) If we decide to change the name of userContextId, or remove it, in either way we'd need to search for references to setDefaultUserContextId/userContextId and figure out how to change the code appropriately.

(In reply to Jonas Sicking PTO until Feb 16th (:sicking) from comment #38) > You are referencing userContextId just as much in > ChromeUtils.setDefaultUserContextId(...) > > If we decide to change the name of userContextId, or remove it, in either > way we'd need to search for references to > setDefaultUserContextId/userContextId and figure out how to change the code > appropriately. Fair enough. So do you want me to get rid of setDefaultUserContextId and just use direct assignments?

(In reply to Jonas Sicking PTO until Feb 16th (:sicking) from comment #38) > You are referencing userContextId just as much in > ChromeUtils.setDefaultUserContextId(...) > > If we decide to change the name of userContextId, or remove it, in either > way we'd need to search for references to > setDefaultUserContextId/userContextId and figure out how to change the code > appropriately. You're right about the name. But I did it this way so that the C++ could refer to nsIScriptSecurityManager::DEFAULT_USER_CONTEXT_ID. If all of our code calls this function when overwriting the user context id with the default, then if we ever change the default, we only have to change that constant in nsIScriptSecurityManager.

This patch reflects the API changes we agreed to.

updated tests for the new API.

Comment on attachment 8720422 [details] [diff] [review] Bug_1229222.patch Thanks Dave for the updated patches! >diff --git a/dom/webidl/ChromeUtils.webidl b/dom/webidl/ChromeUtils.webidl >index 623b5cd..b067abb 100644 >--- a/dom/webidl/ChromeUtils.webidl >+++ b/dom/webidl/ChromeUtils.webidl >@@ -25,23 +25,48 @@ interface ChromeUtils : ThreadSafeChromeUtils { > * @param originAttrs The originAttributes under consideration. > * @param pattern The pattern to use for matching. > */ > static boolean > originAttributesMatchPattern(optional OriginAttributesDictionary originAttrs, > optional OriginAttributesPatternDictionary pattern); > > /** >- * Returns an OriginAttributes dictionary using the origin URI but forcing >- * the passed userContextId. >+ * Returns an OriginAttributesDictionary with all default members specified >+ * with default values. What do you mean by default members specified with default values? Do you mean all members specified with default values? >+ * >+ * @returns An OriginAttributesDictionary populated with the >+ * default members with default values. >+ */ >+ static OriginAttributesDictionary >+ createDefaultOriginAttributes(); >+ >+ /** >+ * Returns an OriginAttributesDictionary created from |origin| with default >+ * members specified with default values. >+ * >+ * @param origin The origin URI to create from. >+ * @returns An OriginAttributesDictionary with values from >+ * the origin suffix and unspecified default members >+ * with default values. > */ > [Throws] > static OriginAttributesDictionary >- createOriginAttributesWithUserContextId(DOMString origin, >- unsigned long userContextId); >+ createOriginAttributesFromOrigin(DOMString origin); >+ For this one, we are populating the origin attributes with things taken from the passed in origin. Any unspecified values will get their default values. So not sure if "default members specified with default values" is correct here. Insetad maybe it should be "unspecified members are given default values" >+ /** >+ * Returns an OriginAttributesDictionary that is a copy of |originAttrs| with >+ * any uspecified members added with default values. unspecified >+ * >+ * @param originAttrs The origin attributes to copy. >+ * @returns An OriginAttributesDictionary copy of |originAttrs| >+ * and default members specified with default values. >+ */ >+ static OriginAttributesDictionary >+ createOriginAttributesFromDict(optional OriginAttributesDictionary originAttrs); > }; >

woohoo!!! \o/

Hi Dave, Please fix the "uspecified" typo referenced in comment 43, along with the other comments. Removing checkin-needed for now. Thanks!

Reworded descriptions and fixed typo. Thanks Tanvi!

Tanvi, If you approve, please flag it for checkin-needed. Thanks!

Thanks Dave!

https://hg.mozilla.org/integration/mozilla-inbound/rev/736daf4b4a56 https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b8bb93313b

backed this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d00d189cd28b since test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=22016049&repo=mozilla-inbound when this changes landed

r+ already by :sicking, backed out due to test failure. this fixes the failure. try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fb3ba6513ad

r+ already by :sicking, backed out due to test failures. this fixes those failures. try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fb3ba6513ad

the push results are all green. there's 5 oranges that don't look related. setting for checkin-needed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d7b620ac5ed https://hg.mozilla.org/integration/mozilla-inbound/rev/13f5670eac32

https://hg.mozilla.org/mozilla-central/rev/6d7b620ac5ed https://hg.mozilla.org/mozilla-central/rev/13f5670eac32

Backout: https://hg.mozilla.org/integration/b2g-inbound/rev/c4207d6e57d0

Reopened so I can track the landing (hope this is ok, correct me if this is wrong).

(In reply to Paul Theriault [:pauljt] from comment #57) > Reopened so I can track the landing (hope this is ok, correct me if this is > wrong). The patch landed to central on March 1st. I'm not sure what the b2g-inbound backout in comment 56 is about. Or if it is supposed to be relanded on b2g-inbound.

(In reply to Tanvi Vyas [:tanvi] from comment #58) > (In reply to Paul Theriault [:pauljt] from comment #57) > > Reopened so I can track the landing (hope this is ok, correct me if this is > > wrong). > > The patch landed to central on March 1st. I'm not sure what the b2g-inbound > backout in comment 56 is about. Or if it is supposed to be relanded on > b2g-inbound. Ah ok, you are right [1], I thought the backout was from moz central. [1] https://hg.mozilla.org/mozilla-central/log/tip/browser/components/sessionstore/SessionStorage.jsm

(In reply to Paul Theriault [:pauljt] from comment #59) > (In reply to Tanvi Vyas [:tanvi] from comment #58) > > (In reply to Paul Theriault [:pauljt] from comment #57) > > > Reopened so I can track the landing (hope this is ok, correct me if this is > > > wrong). > > > > The patch landed to central on March 1st. I'm not sure what the b2g-inbound > > backout in comment 56 is about. Or if it is supposed to be relanded on > > b2g-inbound. > > Ah ok, you are right [1], I thought the backout was from moz central. > > [1] > https://hg.mozilla.org/mozilla-central/log/tip/browser/components/ > sessionstore/SessionStorage.jsm Do we need to reland in b2g-inbound? Or is it okay if b2g doesn't have this code? Is b2g code all being deprecated?

> Do we need to reland in b2g-inbound? Or is it okay if b2g doesn't have this > code? Is b2g code all being deprecated? No we don't - b2g is working in Pine now, which takes code from mozilla central. I double checked and this code is in Pine: https://hg.mozilla.org/projects/pine/rev/6d7b620ac5ed