This is some code that will need to be generalized before we add new attributes in bug 1163254.

Eventually nsILoadContext (which generally lives on the loadgroup) should just go away and be replaced by nsILoadInfo (which lives on the channel). But if this is blocking bug 1163254 we should definitely not wait for nsILoadContext's death.

Yoshi, can either you or Ethan take this one?

I can, thanks

Hi Bobby, Is this bug about removing getLoadContextCodebasePrincipal and getDocShellCodebasePrincipal and also fix the callers to use createCodebasePrincipal? If not, can you help to explain more detail of this bug? Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #4) > Hi Bobby, > Is this bug about removing getLoadContextCodebasePrincipal and > getDocShellCodebasePrincipal and also fix the callers to use > createCodebasePrincipal? > > If not, can you help to explain more detail of this bug? Hi Yoshi, So the issue here is that we currently have hard-coded logic that assigns a principal to a document based on the document's URI and the appId/inBrowser properties of the docshell. We need to generalize that code a bit - docshell probably needs to store an OriginAttributes, and we need a method called OriginAttributes::Inherit(OriginAttributes&) that copies over the inheritable OriginAttributes (like appId and inBrowser) but not the non-inheritable ones (like signedPkg). We spoke about this with the security/privacy team today, and they're interested in this too.

Comment on attachment 8629322 [details] [diff] [review] WIP. Patch Review of attachment 8629322 [details] [diff] [review]: ----------------------------------------------------------------- WIP patch. ask feedback? from Bobby first.

Comment on attachment 8629322 [details] [diff] [review] WIP. Patch Review of attachment 8629322 [details] [diff] [review]: ----------------------------------------------------------------- I think the algorithm for computing the OriginAttributes should look like this: nsDocShell::GetOriginAttributes() const { // Start with the default attributes. OriginAttributes attrs; // Copy inheritable attributes from our parent. if (parent) { attrs.Inherit(parent->GetOriginAttributes()); } // // Apply any modifications that make this docshell different from its parent. // if (mOwnOrContainingAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID) { attrs.appId = mOwnOrContainingAppId; } if (mFrameType == eFrameTypeBrowser) { attrs.inBrowser = true; } return attrs; } ::: caps/BasePrincipal.h @@ +30,5 @@ > } > explicit OriginAttributes(const OriginAttributesDictionary& aOther) > : OriginAttributesDictionary(aOther) {} > > + void Inherit(const OriginAttributes& aOther) We need a comment explaining that this should copy over only attributes that we want to be inherited from parent docshells. ::: caps/nsScriptSecurityManager.cpp @@ +1063,5 @@ > nsIPrincipal** aPrincipal) > { > + mozilla::AutoSafeJSContext cx; > + JS::RootedValue val(cx); > + aDocShell->GetOriginAttributes(cx, &val); Why isn't this calling the argument-less version? ::: docshell/base/nsDocShell.cpp @@ +13842,5 @@ > + nsCOMPtr<nsIDocShell> parent; > + GetSameTypeParentIgnoreBrowserAndAppBoundaries(getter_AddRefs(parent)); > + if (!parent) { > + return OriginAttributes(GetAppId(), GetIsInBrowserElement()); > + } This is wrong - it means that we only use mOwnOrContainingAppId in the case where we have no parent, which is different than what the current code does. @@ +13846,5 @@ > + } > + > + OriginAttributes attrs; > + attrs.Inherit(static_cast<nsDocShell*>(parent.get())->GetOriginAttributes()); > + return attrs; return parent->GetOriginAttributes().

Comment on attachment 8629322 [details] [diff] [review] WIP. Patch Review of attachment 8629322 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/nsScriptSecurityManager.cpp @@ +1063,5 @@ > nsIPrincipal** aPrincipal) > { > + mozilla::AutoSafeJSContext cx; > + JS::RootedValue val(cx); > + aDocShell->GetOriginAttributes(cx, &val); The arg-less version is defined in nsDocShell instead of nsIDocShell. Also if I downcast to nsDocShell, the GetOriginAttributes has to be 'public' instead of 'protected' now. Or did I misunderstand your comment? ::: docshell/base/nsDocShell.cpp @@ +13846,5 @@ > + } > + > + OriginAttributes attrs; > + attrs.Inherit(static_cast<nsDocShell*>(parent.get())->GetOriginAttributes()); > + return attrs; But from your review comments you said return attrs instead of returning parent's oringinAttributes. I'll skip this comment for now.

(In reply to Yoshi Huang[:allstars.chh] from comment #9) > The arg-less version is defined in nsDocShell instead of nsIDocShell. > Also if I downcast to nsDocShell, the GetOriginAttributes has to be 'public' > instead of 'protected' now. I think we should downcast and make the member public - no need to round-trip through the JS engine and do a bunch of object allocations. I'd also be for introducing a static method called nsDocShell::Cast that performs the static_cast from nsIDocShell, similar to BasePrincipal::Cast.

addressed bobby's comment.

Comment on attachment 8639811 [details] [diff] [review] Part 2: fix LoadContext Review of attachment 8639811 [details] [diff] [review]: ----------------------------------------------------------------- Hi Bobby I didn't update nsILoadContext.idl as it will also require changes in app_cache part, that should be done in Bug 1165256. I am not sure if I have to modify SerializedLoadContext here as I thought that should be done in IPC bug. But feel free to comment if I have to. Thanks.

Comment on attachment 8639810 [details] [diff] [review] Part 1: fix up docshell in ssm Review of attachment 8639810 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. Please make extra sure to fix the eFrameTypeBrowser thing :-) ::: caps/BasePrincipal.h @@ +31,5 @@ > explicit OriginAttributes(const OriginAttributesDictionary& aOther) > : OriginAttributesDictionary(aOther) {} > > + // Call this method to copy over attributes we want to be inherited from > + // parent docshell. Please add an additional comment above OriginAttributesDictionary (in ChromeUtils.webidl) saying that Inherit() should be updated as well in the case that the given attribute should be inherited from a parent docshell. ::: docshell/base/nsDocShell.cpp @@ +13849,5 @@ > + if (mOwnOrContainingAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID) { > + attrs.mAppId = mOwnOrContainingAppId; > + } > + > + attrs.mInBrowser = mFrameType == eFrameTypeBrowser; This is wrong - it will incorrectly report that an iframe inside a mozBrowser has inBrowser=false, because it unconditionally overrides the inherited value, rather than only doing so at the boundary. Please use the code from comment 8 here. @@ +13857,5 @@ > +NS_IMETHODIMP > +nsDocShell::GetOriginAttributes(JSContext* aCx, JS::MutableHandle<JS::Value> aVal) > +{ > + OriginAttributes attrs = GetOriginAttributes(); > + return NS_WARN_IF(!ToJSValue(aCx, attrs, aVal)) ? NS_ERROR_FAILURE : NS_OK; This pattern is a little unconventional. I'd do: bool ok = ToJSValue(aCx, attrs, aVal); NS_ENSURE_TRUE(ok, NS_ERROR_FAILURE); return NS_OK;

Comment on attachment 8639811 [details] [diff] [review] Part 2: fix LoadContext Review of attachment 8639811 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/LoadContext.h @@ +115,5 @@ > nsILoadContext* aOptionalBase = nullptr); > > + mozilla::OriginAttributes GetOriginAttributes(); > + > + static LoadContext* Cast(nsILoadContext* aLoadContext) You can't do this trick here - there are multiple concrete classes that implement nsILoadContext. Moreover, nsILoadContext is not marked |builtinclass|, which means that it can also be implemented by JS. Instead, you can do the following in nsILoadContext.idl: at the top: [ref] native OriginAttributesRef(mozilla::OriginAttributes); then mark the interface |builtinclass|, then inside: [noscript, notxpcom, nostdcall] OriginAttributesRef GetOriginAttributes(); And then implement that in all of the things that implement

Bobby, are the orignAttributes supposed to be stored on the docShell, loadContext, or both?

(In reply to Tanvi Vyas [:tanvi] from comment #17) > Bobby, are the orignAttributes supposed to be stored on the docShell, > loadContext, or both? Inheritable stuff (like your stuff) should definitely on the docshell. I'm not sure about whether there are any cases where we'd need it on the LoadContext, or whether that's just for attributes that are deducible from the URI. Jonas?

I don't actually think it's inheritable vs. non-inheritable that should determine if something lives in the docshell or not. I'd rather say that if something is constant for the lifetime of the docshell, then it should live in the docshell. So for example the appid should since we always open a different app in a different tab. And the thing Tanvi is building should since we remove and recreate the tab (and thus the docshell) if the user switches context. The Tor identifier would probably not live in the docshell since if the user navigates to a different domain, then we'd change the Tor identifier. I don't feel strongly about how we store this information. I.e. if we store an actual OriginAttributes, and just ignore the pieces that we decide should not live in the docshell, or if we store separate members. Either way, we need to change nsIDocShell and nsILoadContext. nsILoadContext basically is a miniature version of nsIDocShell that only contains the pieces that the network stack needs available and that we can send over IPC.

(In reply to Jonas Sicking (:sicking) from comment #19) > I don't actually think it's inheritable vs. non-inheritable that should > determine if something lives in the docshell or not. I guess there's really two meanings of 'inheritable': (1) "things that child docshells should inherit from parent docshells", which is currently implemented in OriginAttributes::Inherit in Yoshi's patch. (2) "things that a global inherits from its docshell", which at present is implicit in GetDocShellCodebasePrincipal. (1) is a subset of (2), which is a subset of OriginAttributes. The current approach (in which we store an OriginAttributes on each DocShell) works fine so long as we make sure to leave all non-(2) attributes in their default value. Still, we may want to make this more explicit with two separate methods on OriginAttributes (InheritFromDocShell and InheritFromDocShellParent). Yoshi, could you do that? > I don't feel strongly about how we store this information. I.e. if we store > an actual OriginAttributes, and just ignore the pieces that we decide should > not live in the docshell, or if we store separate members. I think we should definitely store OriginAttributes, to minimize the number of code changes required to add a new OriginAttribute. > Either way, we need to change nsIDocShell and nsILoadContext. nsILoadContext > basically is a miniature version of nsIDocShell that only contains the > pieces that the network stack needs available and that we can send over IPC. Ok, sounds like docshell and loadcontext need to mirror each other then.

(In reply to Bobby Holley (:bholley) from comment #20) > (In reply to Jonas Sicking (:sicking) from comment #19) > > I don't actually think it's inheritable vs. non-inheritable that should > > determine if something lives in the docshell or not. > > I guess there's really two meanings of 'inheritable': > > (1) "things that child docshells should inherit from parent docshells", > which is currently implemented in OriginAttributes::Inherit in Yoshi's patch. > (2) "things that a global inherits from its docshell", which at present is > implicit in GetDocShellCodebasePrincipal. > > (1) is a subset of (2), which is a subset of OriginAttributes. For (2), what I'd actually love to have is a function which takes a docshell and an nsIChannel (which has reached OnStartRequest) and which produces an nsIPrincipal or an OriginAttributes. This way we can let the principal used for a global be a function of both static information in the docshell, as well as dynamic information from the loaded URI. This would endable things like switching cookie storage when the user browses from one website to another. For (1), I'd then have a function which takes a docshell and the nsIPrincipal of the global, and creates an OriginAttributes which is stored in the child docshell. How does that sound? > > I don't feel strongly about how we store this information. I.e. if we store > > an actual OriginAttributes, and just ignore the pieces that we decide should > > not live in the docshell, or if we store separate members. > > I think we should definitely store OriginAttributes, to minimize the number > of code changes required to add a new OriginAttribute. Cool.

(In reply to Bobby Holley (:bholley) from comment #20) > Still, we may want to make this more explicit with > two separate methods on OriginAttributes (InheritFromDocShell and > InheritFromDocShellParent). Yoshi, could you do that? > Hi Bobby, So for (1) I just rename my patch 'Inherit' to 'InheritFromDocShellParent'? And when will InheritFromDocShell be used? And the two methods are basically doing the same thing for now, right? both will assign appId and inBrowser.

(In reply to Yoshi Huang[:allstars.chh] from comment #22) > So for (1) I just rename my patch 'Inherit' to 'InheritFromDocShellParent'? Correct. > And when will InheritFromDocShell be used? in nsScriptSecurityManager::Get{LoadContext,DocShell}CodebasePrincipal > And the two methods are basically doing the same thing for now, right? No. InheritFromDocShell will copy everything (for now), and InheritFromDocShellParent does what |Inherit| does now.

(In reply to Jonas Sicking (:sicking) from comment #21) > For (2), what I'd actually love to have is a function which takes a docshell > and an nsIChannel (which has reached OnStartRequest) and which produces an > nsIPrincipal or an OriginAttributes. Is this conceptually different from: OriginAttributes attrs; attrs.InheritFromDocShell(aDocShell); attrs.InheritFromChannel(aChannel); // Or from aURI? > This way we can let the principal used for a global be a function of both > static information in the docshell, as well as dynamic information from the > loaded URI. This would endable things like switching cookie storage when the > user browses from one website to another. Sure. Seems like would could just add an additional inheritable thing where the use-case arises. > For (1), I'd then have a function which takes a docshell and the > nsIPrincipal of the global, and creates an OriginAttributes which is stored > in the child docshell. That doesn't make sense to me - why would it depend on the nsIPrincipal of the global?

- rename Inherit to InheritFromDocShellParent - Add InheritFromDocShell - Call InheritFromDocShell from ssm.

Comment on attachment 8643599 [details] [diff] [review] Part 2: fix LoadContext v2. Review of attachment 8643599 [details] [diff] [review]: ----------------------------------------------------------------- Hi Bobby I've addressed your comment 16. I changed the naming to 'GetOriginAttributesRef'(return OriginAttributes&) as it will conflict with the original GetOriginAttributes(return OriginAttributes) in Part 1. I am not sure when to initialize the mOriginAttributes in nsDocShell, do you have better suggestion? Thanks

ni? Jonas for Bobby's comment 24.

Comment on attachment 8643599 [details] [diff] [review] Part 2: fix LoadContext v2. Review of attachment 8643599 [details] [diff] [review]: ----------------------------------------------------------------- We also need to modify SerializedLoadContext and all the IPC pieces, right? I'm also a bit confused as to how the docshell changes here fit in. Part 1 added "GetOriginAttributes" to nsDocShell.h, and this patch adds "GetOriginAttributesRef". Please make sure to merge any changes into the appropriate patch, otherwise it's really hard for me to figure out what's going on. :-) ::: docshell/base/LoadContext.h @@ +119,4 @@ > > nsWeakPtr mTopFrameElement; > uint64_t mNestedFrameId; > uint32_t mAppId; We should get rid of mAppId and mIsInBrowserElement in this patch, otherwise we have redundant information. ::: docshell/base/nsDocShell.h @@ +226,5 @@ > NS_IMETHOD SetUsePrivateBrowsing(bool) override; > NS_IMETHOD SetPrivateBrowsing(bool) override; > NS_IMETHOD GetUseRemoteTabs(bool*) override; > NS_IMETHOD SetRemoteTabs(bool) override; > + mozilla::OriginAttributes& GetOriginAttributesRef() override Given that it's infallible, let's call this method "OriginAttributesRef", both on docshell and loadcontext. ::: uriloader/prefetch/OfflineCacheUpdateParent.h @@ +51,5 @@ > > bool mIPCClosed; > > bool mIsInBrowserElement; > uint32_t mAppId; We need to remove these as well.

(In reply to Bobby Holley (:bholley) from comment #24) > (In reply to Jonas Sicking (:sicking) from comment #21) > > For (2), what I'd actually love to have is a function which takes a docshell > > and an nsIChannel (which has reached OnStartRequest) and which produces an > > nsIPrincipal or an OriginAttributes. > > > Is this conceptually different from: > OriginAttributes attrs; > attrs.InheritFromDocShell(aDocShell); > attrs.InheritFromChannel(aChannel); // Or from aURI? Yes. I want to be able to call out to addons in this function and provide both a docshell into which a document is going to be loaded, and the channel of the document which is about to be created, and ask the addon "what OriginAttributes should the resulting document use". I don't think that's possible with the approach above. > > For (1), I'd then have a function which takes a docshell and the > > nsIPrincipal of the global, and creates an OriginAttributes which is stored > > in the child docshell. > > That doesn't make sense to me - why would it depend on the nsIPrincipal of > the global? This is needed to do things like "double-keying" cookies. I.e. for the Tor use case. Tor enables you to browse from website to website within a tab. The set of cookies used in the tabs depends on the origin of the top-level website in the tab. That means that we can simply set the OriginAttributes in the docshell and inherit that into each document. Each document will need to calculate the OriginAttributes that it will use. When such a page the creates an <iframe>, we generally will want to inherit the OriginAttributes from the toplevel document into all documents rendered inside that <iframe>. However I could also see wanting to do more complex things. Like whitelisting some website that *is* allowed to track the user. Or using the whole parent chain as part of the key, rather than simply going to the toplevel document. I don't think that we need to implement all these complex policies. But I want addons to be able to. So, when an <iframe> is created, I want to be able to hand the nsIPrincipal of the owning document to the addon and ask it "what OriginAttributes should we store in the docshell for this iframe". Maybe even better would be to hand the owning document to the addon rather than the nsIPrincipal of the document. So having a function which takes a document and returns the OriginAttributes that the docshell inside <iframe>s should have sounds better. Separately, when we make a network request, I want to be able to hand the channel of the network request (through which you nowadays can get the document which is making the request) to an addon and ask it "what OriginAttributes should we use when looking for cookies for this request". But I think for now it's fine that we don't worry about this.

(In reply to Bobby Holley (:bholley) from comment #29) > Comment on attachment 8643599 [details] [diff] [review] > Part 2: fix LoadContext v2. > > Review of attachment 8643599 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: docshell/base/LoadContext.h > @@ +119,4 @@ > > > > nsWeakPtr mTopFrameElement; > > uint64_t mNestedFrameId; > > uint32_t mAppId; > > We should get rid of mAppId and mIsInBrowserElement in this patch, otherwise > we have redundant information. > So you mean remove these only from the headers, not IDLs, right? Originally I found there are many callers will use nsILoadContext so I didn't remove them from IDL. I'll remove them from Headers here, and file another bug to completely remove appId/browserElement from IDL.

(In reply to Yoshi Huang[:allstars.chh] from comment #31) > So you mean remove these only from the headers, not IDLs, right? I mean that we should not be storing the data in multiple places. It's fine to have legacy accessors to retrieve the data inside the OriginAttributes. > Originally I found there are many callers will use nsILoadContext so I > didn't remove them from IDL. > I'll remove them from Headers here, and file another bug to completely > remove appId/browserElement from IDL. Sure - not a big priority, so probably worth doing other things first.

Hi Bobby My patches fail in http://lxr.mozilla.org/mozilla-central/source/caps/tests/mochitest/test_principal_jarprefix_origin_appid_appstatus.html?force=1#197 The test requires that an app inside a browser is an app. My patch will fail since its parent is a mozbrowser frame, so the child frame will be a mozbrowser frame too. 123 INFO TEST-UNEXPECTED-FAIL | caps/tests/mochitest/test_principal_jarprefix_origin_appid_appstatus.html | check childPrincipal.isInBrowserElement - got true, expected false Is the test case correct? Thanks

Also adding [builtin-class] for nsILoadContext will cause several failures, and the problem is many of them are implementing nsILoadContext in JS. For example, like toolkit/mozapps/downloads/DownloadLastDir.jsm image/test/unit/test_private_channel.js netwerk/test/unit/test_cacheflags.js Will need patches to fix this.

Comment on attachment 8646283 [details] [diff] [review] Part 3: fixing inheriting nsILoadContext in DownloadLastDir I am concerned that this patch regresses the case mentioned by the comment that is being removed: >- // Need this in case the real thing has gone away by the time we need it. >- // We only care about the private browsing state. All the rest of the >- // load context isn't of interest to the content pref service.

(In reply to Josh Matthews [:jdm] from comment #40) > Comment on attachment 8646283 [details] [diff] [review] > Part 3: fixing inheriting nsILoadContext in DownloadLastDir > > I am concerned that this patch regresses the case mentioned by the comment > that is being removed: > > >- // Need this in case the real thing has gone away by the time we need it. > >- // We only care about the private browsing state. All the rest of the > >- // load context isn't of interest to the content pref service. Hi Josh, Thanks for the comment. How do you think if I file another bug to make nsIContentPrefService2 to use nsILoadContextInfo? So I'd completely remove nsILoadContext in DownloadLastDir. Will this address your concern? Or do you have some other suggestion? Thanks

Based on the documentation for nsILoadContextInfo that sounds fine, as long as it's addressed soon.

(In reply to Josh Matthews [:jdm] from comment #42) > Based on the documentation for nsILoadContextInfo that sounds fine, as long > as it's addressed soon. Hi Josh I also met tons of failure in toolkit/components/contentprefs so I guess I have to do it in this bug. Since you're the peer of PrivateBrowsing, can you kindly help how should I continue here? for example, I found dom/html/HTMLInputElement.cpp will call nsIContentPrefService2.getDomainAndName, with nsILoadContext came from nsDocument, and the context is actually a docshell, I am not sure how to fix part correctly. Thanks.

Comment on attachment 8646882 [details] [diff] [review] Part 5: fix inheriting nsILoadContext in netwerk. Review of attachment 8646882 [details] [diff] [review]: ----------------------------------------------------------------- Hi, Jason I am trying to replace nsILoadContext with nsILoadContextInfo in netwerk here, since I mark nsILoadContext as [builtinclass] in Part 2 and make lots of test cases failure. I haven't finished it yet as I think I'll have to modify nsHttpChannel, NeckoParent as well. Could you give me some feedbacks about this? thanks

Hey Yoshi, If making nsILoadContext [builtinclass] requires changing too many things, it's probably not worth it. Instead, you should make the actual C++ class we care about implement a new interface called nsILoadContextInternal (or something), which be [builtinclass], and would inherit nsILoadContext (or be inherited separately, doesn't really matter). Then, we can QI the incoming object to nsILoadContextInternal, and perform the cast if the QI succeeds. If it doesn't, we can probably just assume default OriginAttributes (or even MOZ_CRASH in this case if we think it can never happen). Jonas, does that seem sensible?

Note that it looks like AppUtils.jsm also implements nsILoadContext - sorry I didn't realize that it was so widely implemented in JS. :-(

If nsILoadContext is implemented by script, I would assume that that script might want to provide OriginAttributes as well. Is there a reason we can't make the getter scriptable? Will that result in too much copying in the C++ case?

Comment on attachment 8644856 [details] [diff] [review] Part 1: fix up docshell in ssm v3 + // Call this method to copy over attributes we want to be inherited from + // parent docshell. + // OriginAttributesDictionary (in ChromeUtils.webidl) saying that + // InheritFromDocShellParent() should be updated as well in the case that the + // given attribute should be inherited from a parent docshell. + void InheritFromDocShellParent(const OriginAttributes& aParent); + + void InheritFromDocShell(const nsIDocShell* aDocShell); + I'm having trouble distinguishing these two, so I'm sure others will as well. From my understanding, InheritFromDocShellParent sets inheritable origin attributes on frames. It takes specific origin attributes from the parent and sets them on the child frame. If another grandchild iframe is created, it will also inherit these specific origin attributes. InheritFromDocShell is used when you want to create a principal that uses all the origin attributes that are set on a particular docshell. We should consider renaming these methods, or at least putting in a lot of comments explaining what they are used for. InheritFromDocShellParent inherits inheritable origin attributes so something like InheritableOrignAttributes. InheritFromDocShell inherits all origin attributes so something like InheritAllOriginAttributes.

(In reply to Bobby Holley (Mostly Out This Week) from comment #47) Hi, Bobby Thanks for the feedback, I'll try your comments now. But actually I needinfo? you for the Comment 35. Could you help to check if the test case is correct? > > Instead, you should make the actual C++ class we care about implement a new > interface called nsILoadContextInternal (or something), which be > [builtinclass], and would inherit nsILoadContext (or be inherited > separately, doesn't really matter). Then, we can QI the incoming object to > nsILoadContextInternal, and perform the cast if the QI succeeds. Do you also mean the I move the OriginAttributesRef from nsILoadContext to nsILoadContextInternal? (this may be related to Jonas' question in Comment 49 should we also add a getter for JS?) > If it > doesn't, we can probably just assume default OriginAttributes (or even > MOZ_CRASH in this case if we think it can never happen). I'll try add a MOZ_CRASH there, but GetLoadContextCodeBasePrincipal will be called by GetChannelURIPrincipal, which in turn will be called by Necko, and in many tests, the notificationCallbacks of the channel (nsILoadContext) is set from JS, which may make the QI fail.

(In reply to Jonas Sicking (:sicking) from comment #49) > If nsILoadContext is implemented by script, I would assume that that script > might want to provide OriginAttributes as well. Is there a reason we can't > make the getter scriptable? Will that result in too much copying in the C++ > case? The reason is that XPIDL doesn't give us a good dual representation between C++ and JS. We either need to make the getter return a jsval, in which case it's cumbersome and inefficient to use in C++, or make it return an OriginAttributes&, in which case it can't be scriptable. I don't have a good understanding of the various use-cases from implementing nsILoadContext in script. Is this a problem we need to solve right now, or is making it non-scriptable good enough for now?

(In reply to Tanvi Vyas [:tanvi] from comment #50) > From my understanding, InheritFromDocShellParent sets inheritable origin > attributes on frames. It takes specific origin attributes from the parent > and sets them on the child frame. If another grandchild iframe is created, > it will also inherit these specific origin attributes. Correct. > InheritFromDocShell is used when you want to create a principal that uses > all the origin attributes that are set on a particular docshell. No. InheritFromDocShell copies the properties from the docshell that conceptually "live on the docshell". > We should consider renaming these methods, or at least putting in a lot of > comments explaining what they are used for. > > InheritFromDocShellParent inherits inheritable origin attributes so > something like InheritableOrignAttributes. > > InheritFromDocShell inherits all origin attributes so something like > InheritAllOriginAttributes. This reflects a misunderstanding of what this is about, I think. The point here is that we have some attributes which conceptually live on the docshell. These attributes should be copied over to globals associated with that docshell when they are created. Moreover, the docshells have a relationship between each other in which a subset of the aforementioned attributes get inherited from parent docshells to child docshells.

(In reply to Yoshi Huang[:allstars.chh] from comment #35) > Hi Bobby > My patches fail in > http://lxr.mozilla.org/mozilla-central/source/caps/tests/mochitest/ > test_principal_jarprefix_origin_appid_appstatus.html?force=1#197 Which patches? I applied Part 1 and the test passes. > The test requires that an app inside a browser is an app. > My patch will fail since its parent is a mozbrowser frame, so the child > frame will be a mozbrowser frame too. I don't understand what you mean here - can you be more specific? The case that the test is testing is kind of weird - in general, I don't think we actually have any situations in FFOS where we have an app inside a browser (rather than the reverse). I'm not sure it matters, though Jonas or Paul would know for sure. (In reply to Yoshi Huang[:allstars.chh] from comment #51) > Do you also mean the I move the OriginAttributesRef from nsILoadContext to > nsILoadContextInternal? Correct. > (this may be related to Jonas' question in Comment 49 should we also add a > getter for JS?) Yes. Having a non-scriptable getters makes things easier, so ideally we'd do that - it depends if we ever need one to be scriptable. > I'll try add a MOZ_CRASH there, > but GetLoadContextCodeBasePrincipal will be called by > GetChannelURIPrincipal, which in turn will be called by Necko, and in many > tests, the notificationCallbacks of the channel (nsILoadContext) is set from > JS, which may make the QI fail. Sigh. Yeah, I guess the easiest thing for now is just to do a scriptable getter, which will need to return a jsval (Use xpc::PrivilegedJunkScope to create the jsval). Then you can add a %{C++ getter to nsILoadContext.idl to return an OriginAttributes& directly.

(In reply to Bobby Holley (:bholley) from comment #52) > I don't have a good understanding of the various use-cases from implementing > nsILoadContext in script. Is this a problem we need to solve right now, or > is making it non-scriptable good enough for now? I don't know either. I was surprised that we had more than two implementations of nsILoadContext (docshell and whatever it is that we use for Necko IPC).

Tanvi and I just discussed the naming. She suggested renaming "InheritFromDocShell" to "CopyFromDocShell", which sounds fine to me. Jonas, my understanding of comment 30 is that it would be good to have an API whereby addons could override the logic of selecting OriginAttributes for a given load. However, I don't think we have the bandwidth to design that API right now, and I think we should primarily concern ourselves with the way that Gecko selects the OriginAttributes (we can add such an API later, as appropriate). In terms of the best way for Gecko to select the originAttributes, I think the proposal discussed so far (CopyFromDocShell and InheritFromDocShellParent) is the way to go. Does that seem ok to you?

(In reply to Bobby Holley (:bholley) from comment #54) > (In reply to Yoshi Huang[:allstars.chh] from comment #35) > > Hi Bobby > > My patches fail in > > http://lxr.mozilla.org/mozilla-central/source/caps/tests/mochitest/ > > test_principal_jarprefix_origin_appid_appstatus.html?force=1#197 > > Which patches? I applied Part 1 and the test passes. > Sorry, should be false alarm, now the test passes. > > > (In reply to Yoshi Huang[:allstars.chh] from comment #51) > > Do you also mean the I move the OriginAttributesRef from nsILoadContext to > > nsILoadContextInternal? > > Correct. > > > (this may be related to Jonas' question in Comment 49 should we also add a > > getter for JS?) > > Yes. Having a non-scriptable getters makes things easier, so ideally we'd do > that - it depends if we ever need one to be scriptable. > > > I'll try add a MOZ_CRASH there, > > but GetLoadContextCodeBasePrincipal will be called by > > GetChannelURIPrincipal, which in turn will be called by Necko, and in many > > tests, the notificationCallbacks of the channel (nsILoadContext) is set from > > JS, which may make the QI fail. > > Sigh. Yeah, I guess the easiest thing for now is just to do a scriptable > getter, which will need to return a jsval (Use xpc::PrivilegedJunkScope to > create the jsval). Then you can add a %{C++ getter to nsILoadContext.idl to Do you mean 'nsILoadContextInternal.idl'? Since you mentioned to move OriginAttributesRef into nsILoadContextInternal > return an OriginAttributes& directly.

(In reply to Yoshi Huang[:allstars.chh] from comment #57) > Do you mean 'nsILoadContextInternal.idl'? > Since you mentioned to move OriginAttributesRef into nsILoadContextInternal I'm suggesting that we abandon the idea of nsILoadContextInternal, given that it's seeming complicated. Instead, we have a scriptable jsval getter to nsILoadContext called originAttributes with [implicit_jscontext]. The getter in LoadContext.cpp returns the OriginAttributes via ToJSValue(). For C++ access, we'll need a helper, which can live in a %{C++ block in the .idl file (it doesn't need to be virtual). It can look something like this (very rough, I need to go to bed): %{C++ bool GetAttributes(OriginAttributes& aAttrs) { AutoJSAPI jsapi; bool ok = jsapi.Init(xpc::PrivilegedJunkScope()); NS_ENSURE_TRUE(ok, false); JS::Rooted<JS::Value> v(jsapi.cx()); nsresult rv = GetOriginAttributes(jsapi.cx(), &v); NS_ENSURE_SUCCESS(rv, false); OriginAttributes attrs; ok = attrs.Init(jsapi.cx(), &v); NS_ENSURE_TRUE(ok, false); aAttrs = attrs; return true; } %} Pretty ugly, but expedient for now I guess.

(In reply to Bobby Holley (:bholley) from comment #58) > Instead, we have a scriptable jsval getter to nsILoadContext called > originAttributes with [implicit_jscontext]. The getter in LoadContext.cpp > returns the OriginAttributes via ToJSValue(). > Hi Bobby Adding [implicit_jscontext] seems cause some warning when running test. "JavaScript error: image_load_helpers.js, line 95: Error: IDL methods marked with [implicit_jscontext] or [optional_argc] may not be implemented in JS" Is it okay to ignore this? Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #61) > (In reply to Bobby Holley (:bholley) from comment #58) > > Instead, we have a scriptable jsval getter to nsILoadContext called > > originAttributes with [implicit_jscontext]. The getter in LoadContext.cpp > > returns the OriginAttributes via ToJSValue(). > > > Hi Bobby > > Adding [implicit_jscontext] seems cause some warning when running test. > > "JavaScript error: image_load_helpers.js, line 95: Error: IDL methods marked > with [implicit_jscontext] or [optional_argc] may not be implemented in JS" > Actually the test case fail because of this. bool GetAttributes(OriginAttributes& aAttrs) { AutoJSAPI jsapi; bool ok = jsapi.Init(xpc::PrivilegedJunkScope()); NS_ENSURE_TRUE(ok, false); JS::Rooted<JS::Value> v(jsapi.cx()); nsresult rv = GetOriginAttributes(jsapi.cx(), &v); // the line above will call nsXPCWrappedJSClass::CallMethod magically :P // and return NS_FAILURE_ERROR. Bobby, do you have some suggestion here? Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #61) > (In reply to Bobby Holley (:bholley) from comment #58) > > Instead, we have a scriptable jsval getter to nsILoadContext called > > originAttributes with [implicit_jscontext]. The getter in LoadContext.cpp > > returns the OriginAttributes via ToJSValue(). > > > Hi Bobby > > Adding [implicit_jscontext] seems cause some warning when running test. > > "JavaScript error: image_load_helpers.js, line 95: Error: IDL methods marked > with [implicit_jscontext] or [optional_argc] may not be implemented in JS" Oh yeah sorry, forgot about that issue. Just remove the [implicit_jscontext] and do the following to access the caller's JSContext instead: // Note: we can't use [implicit_jscontext] here because there are JS implementations of this method. JSContext* cx = nsContentUtils::GetCurrentJSContext(); MOZ_ASSERT(cx);

Comment on attachment 8651726 [details] [diff] [review] Part 1: fix up docshell in ssm v6 Review of attachment 8651726 [details] [diff] [review]: ----------------------------------------------------------------- re-r? again for adding the change of CopyFromDocShell and InitFromDocShellParent Also remove implicit_jscontext from nsIDocShell for the Part 2 change.

Comment on attachment 8651728 [details] [diff] [review] Part 2: fix LoadContext v6 Review of attachment 8651728 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/tests/mochitest/test_principal_jarprefix_origin_appid_appstatus.html @@ -204,5 @@ > - src: "http://example.org/chrome/", > - isapp: true, > - }, > - test: [ "child-has-different-eo", "child-has-different-appstatus", "child-has-different-appid" ], > - }, This test case is removed base on Comment 35 and Comment 54. In this test case, nsScriptSecurityManager::GetLoadContextCodebasePrincipal will be called, however the instance of the aLoadContext is actually nsDocShell, so nsDocShell::GetOriginAttributes will be called in turn. From Comment 15 the frame inside a mozBrowser frame should also be a mozBrowser frame, which contradicts the test here, therefore I remove it.

Comment on attachment 8651726 [details] [diff] [review] Part 1: fix up docshell in ssm v6 Review of attachment 8651726 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.cpp @@ +30,5 @@ > > +void OriginAttributes::InheritFromDocShellParent(const OriginAttributes& aParent) > +{ > + mAppId = aParent.mAppId; > + mInBrowser = aParent.mInBrowser; Please add mUserContextId here, which is a new addition. @@ +36,5 @@ > + > +void OriginAttributes::CopyFromDocShell(const nsIDocShell* aDocShell) > +{ > + OriginAttributes attrs = > + nsDocShell::Cast(const_cast<nsIDocShell*>(aDocShell))->OriginAttributesRef(); Why not make a const-valued overload of nsDocShell::Cast and remove the const_cast here? @@ +39,5 @@ > + OriginAttributes attrs = > + nsDocShell::Cast(const_cast<nsIDocShell*>(aDocShell))->OriginAttributesRef(); > + mAppId = attrs.mAppId; > + mInBrowser = attrs.mInBrowser; > + mAddonId = attrs.mAddonId; Please add mUserContextId here as well. ::: caps/BasePrincipal.h @@ +46,5 @@ > > + // Call this method to copy over attributes we want to be inherited from > + // parent docshell. > + // OriginAttributesDictionary (in ChromeUtils.webidl) saying that > + // InheritFromDocShellParent() should be updated as well in the case that the I don't parse this sentence. How about this: // // The docshell often influences the origin attributes of content loaded inside // of it, and in some cases also influences the origin attributes of content loaded // in child docshells. We say that a given attribute "lives on the docshell" to // indicate that this attribute is specified by the docshell (if any) associated // with a given content document. // // In practice, this usually means that we need to store a copy of those attributes // on each docshell, or provide methods on the docshell to compute them on-demand. // We could track each of these attributes individually, but since the majority of // the existing origin attributes currently live on the docshell, it's cleaner to // simply store an entire OriginAttributes struct on each docshell, and // selectively copy them to child docshells and content principals in a manner that // implements our desired semantics. // // This method is used to propagate attributes from parent to child docshells. void InheritFromDocShellParent(const OriginAttributes& aParent); // This method is used to pass the appropriate attributes from the docshell to content // running inside that docshell. This effectively encodes the list of attributes that // "live on the docshell". @@ +50,5 @@ > + // InheritFromDocShellParent() should be updated as well in the case that the > + // given attribute should be inherited from a parent docshell. > + void InheritFromDocShellParent(const OriginAttributes& aParent); > + > + void CopyFromDocShell(const nsIDocShell* aDocShell); Need a comment here too. How about doing the comments as follows: ::: docshell/base/nsDocShell.cpp @@ +13686,5 @@ > + if (mFrameType == eFrameTypeBrowser) { > + mOriginAttributes.mInBrowser = true; > + } > + > + return mOriginAttributes; This doesn't look right - if we're recomputing the OriginAttributes each time, why do we store them as a member at all? Realistically, there are two ways to do this: (A) Compute the value eagerly, cache it as mOriginAttributes, and recompute it when the docshell tree changes (see RecomputeCanExecuteScripts for an example). (B) Compute the value on-demand, and don't cache it at all. I think (B) is probably fine performance wise, and much much simpler - you basically just take the method you have here and create |OriginAttributes attrs;| instead of using mOriginAttributes. So I'd suggest doing that. @@ +13693,5 @@ > +NS_IMETHODIMP > +nsDocShell::GetOriginAttributes(JS::MutableHandle<JS::Value> aVal) > +{ > + JSContext* cx = nsContentUtils::GetCurrentJSContext(); > + MOZ_ASSERT(cx); For docshell, you _can_ use [implicit_jscontext], and don't need this hack right? IIUC we only need that for nsILoadContext, which is not [builtinclass] (whereas nsIDocShell is).

Comment on attachment 8651726 [details] [diff] [review] Part 1: fix up docshell in ssm v6 Review of attachment 8651726 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsIDocShell.idl @@ +831,5 @@ > /** > + * A dictionary of the non-default origin attributes associated with this > + * nsIDocShell. > + */ > + readonly attribute jsval originAttributes; Also, given that the next patch puts originAttributes on nsILoadContext, and nsDocShell implements both nsIDocShell and nsILoadContext, it seems better to have the .idl getter in only one place. So given that, it seems like this should be removed. Does that make sense?

Comment on attachment 8651728 [details] [diff] [review] Part 2: fix LoadContext v6 Review of attachment 8651728 [details] [diff] [review]: ----------------------------------------------------------------- Need to go to bed, but here's some stuff that needs fixing. :-) ::: caps/nsScriptSecurityManager.cpp @@ +1096,3 @@ > OriginAttributes attrs; > + bool result = aLoadContext->GetOriginAttributes(attrs); > + NS_ENSURE_TRUE(result, NS_ERROR_FAILURE); I think we also want to use CopyFromDocShell here, which suggests that it should probably be called CopyFromDocShellOrLoadContext. ::: docshell/base/SerializedLoadContext.cpp @@ +62,5 @@ > aLoadContext->GetIsContent(&mIsContent); > aLoadContext->GetUsePrivateBrowsing(&mUsePrivateBrowsing); > aLoadContext->GetUseRemoteTabs(&mUseRemoteTabs); > + aLoadContext->GetAppId(&mOriginAttributes.mAppId); > + aLoadContext->GetIsInBrowserElement(&mOriginAttributes.mInBrowser); Shouldn't we just do aLoadContext->GetOriginAttributes? ::: docshell/base/SerializedLoadContext.h @@ +65,5 @@ > WriteParam(aMsg, aParam.mIsPrivateBitValid); > WriteParam(aMsg, aParam.mUsePrivateBrowsing); > WriteParam(aMsg, aParam.mUseRemoteTabs); > + WriteParam(aMsg, aParam.mOriginAttributes.mAppId); > + WriteParam(aMsg, aParam.mOriginAttributes.mInBrowser); Instead of doing this, we should fix this up to use CreateSuffix here and PopulateFromSuffix below. ::: dom/ipc/TabParent.cpp @@ +2866,5 @@ > nsCOMPtr<nsILoadContext> loadContext; > if (mLoadContext) { > loadContext = mLoadContext; > } else { > + OriginAttributes attrs = OriginAttributes(OwnOrContainingAppId(), IsBrowserElement()); This will need to be fixed up in bug 1191740. Can you file a blocker against bug 1191740 to make sure we remember to do that? ::: netwerk/protocol/http/HttpChannelParent.cpp @@ +457,2 @@ > if (mLoadContext) { > + mLoadContext->GetOriginAttributes(attrs); we should use CopyFromDocShellOrLoadContext here. ::: uriloader/prefetch/OfflineCacheUpdateParent.cpp @@ +50,5 @@ > //----------------------------------------------------------------------------- > // OfflineCacheUpdateParent <public> > //----------------------------------------------------------------------------- > > +// TODO: Bug 1191740 - Add OriginAttributes in TabContext Can you include this in the bug you file against bug 1191740 to make sure it gets done? We also need to make sure we call CopyFromDocShellOrLoadContext at the appropriate time when that happens.

Since nsDocShell also inherits nsILoadContext, also we try to have a function called CopyFromDocShellOrLoadContext. I merge the two parts into one This is still a WIP patch, as I found some tests are still failing.

Comment on attachment 8653433 [details] [diff] [review] WIP - Patch. v7 Review of attachment 8653433 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/nsScriptSecurityManager.cpp @@ +1110,5 @@ > nsIPrincipal** aPrincipal) > { > + OriginAttributes attrs; > + nsDocShell* docShell= nsDocShell::Cast(aDocShell); > + bool result = attrs.CopyFromLoadContext(docShell); Hi Bobby I'd like to have your comment first about this, as I am not sure if this is what you prefer. I am not sure the type of arguments in CopyFromDocShellOrLoadContext, since nsIDocShell doesn't inherit nsILoadContext. So I change it to CopyFromLoadContext(nsILoadContext*), and do a Cast from nsIDocShell* to nsDocShell*. Sorry for bothering you so many times, Could you help to check this? Thanks

fixed test cases error.

Comment on attachment 8654015 [details] [diff] [review] Patch. v7 Review of attachment 8654015 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/BasePrincipal.cpp @@ +40,5 @@ > + OriginAttributes attrs; > + > + // For some test cases in Necko, the nsILoadContext is implemented in JS, > + // so we need to call JS getter instead of C++ one. > + mozilla::AutoSafeJSContext cx; I am not sure which context is correct to use. nsContentUtils::GetCurrentJSContext(); or AutoSafrJSContext As I found they work in some tests but will assert in other tests. ::: caps/nsScriptSecurityManager.cpp @@ +1110,5 @@ > nsIPrincipal** aPrincipal) > { > + OriginAttributes attrs; > + nsDocShell* docShell= nsDocShell::Cast(aDocShell); > + bool result = attrs.CopyFromLoadContext(docShell); Please see my Comment 75 for this. ::: docshell/base/SerializedLoadContext.cpp @@ +64,5 @@ > aLoadContext->GetUseRemoteTabs(&mUseRemoteTabs); > + > + // For some test cases in Necko, the nsILoadContext is implemented in JS, > + // so we need to call JS getter instead of C++ one. > + mozilla::AutoSafeJSContext cx; Here too, some tests will assert here.

Comment on attachment 8654015 [details] [diff] [review] Patch. v7 Review of attachment 8654015 [details] [diff] [review]: ----------------------------------------------------------------- Hi Boris Since Bobby is taking leave now, could you help to check this and provide some feedback for me? As this patch still has some test cases failure on the JSContext but I am not sure which one is correct to use (See comment 77) Also see Comment 75 for the Cast from the nsIDocShell* to nsDocShell* and convert it to nsILoadContext*. This is requested by Bobby in comment 72 and Comment 73. Thanks

(In reply to Yoshi Huang[:allstars.chh](OOO 9.4 ~ 9.20) from comment #76) > Created attachment 8654015 [details] [diff] [review] > Patch. v7 > > fixed test cases error. Yoshi, this patch no longer applies on top of the current m-c. Can you please rebase?

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #79) > (In reply to Yoshi Huang[:allstars.chh](OOO 9.4 ~ 9.20) from comment #76) > > Created attachment 8654015 [details] [diff] [review] > > Patch. v7 > > > > fixed test cases error. > > Yoshi, this patch no longer applies on top of the current m-c. Can you > please rebase? Sorry, my tree was not up to date :) Patch applies correctly!

I have problems with this patch (v7), it asserts at: void check(JSCompartment* c) { if (c && !compartment->runtimeFromAnyThread()->isAtomsCompartment(c)) { if (!compartment) compartment = c; else if (c != compartment) > fail(compartment, c); } } called from C++: bool GetOriginAttributes(mozilla::OriginAttributes& aAttrs) { mozilla::dom::AutoJSAPI jsapi; bool ok = jsapi.Init(xpc::PrivilegedJunkScope()); NS_ENSURE_TRUE(ok, false); JS::Rooted<JS::Value> v(jsapi.cx()); nsresult rv = GetOriginAttributes(&v); NS_ENSURE_SUCCESS(rv, false); mozilla::OriginAttributes attrs; > ok = attrs.Init(jsapi.cx(), v); NS_ENSURE_TRUE(ok, false); aAttrs = attrs; return true; } xul!js::CompartmentChecker::fail+0x2d xul!js::CompartmentChecker::check+0x54 xul!js::CompartmentChecker::check+0x1e xul!js::CompartmentChecker::check<JSObject *>+0x1a xul!js::assertSameCompartment<JS::Handle<JSObject *> >+0x4c xul!JS_ObjectIsDate+0x10 xul!mozilla::dom::IsNotDateOrRegExp+0x5c xul!mozilla::dom::IsObjectValueConvertibleToDictionary+0x49 xul!mozilla::dom::IsConvertibleToDictionary+0x2f xul!mozilla::dom::OriginAttributesDictionary::Init+0xb7 xul!IPC::SerializedLoadContext::Init+0x1b1 xul!IPC::SerializedLoadContext::SerializedLoadContext+0x52 xul!mozilla::net::HttpChannelChild::ContinueAsyncOpen+0xc82 xul!mozilla::net::HttpChannelChild::AsyncOpen+0x447 xul!NS_InvokeByIndex+0x27 Being invoked from am xpcshell test.

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #81) > I have problems with this patch (v7), it asserts at: Yeah, I've mentioned in my last comment 77 but I don't know how to fix this. :(

Comment on attachment 8654015 [details] [diff] [review] Patch. v7 Sorry for the lag here. Still digging out from under the vacation mail pile. :( > nsContentUtils::GetCurrentJSContext(); or AutoSafrJSContext It depends on what you're trying to do, exactly. If you're trying to "act like whoever called me" then you want GetCurrentJSContext. Presumably this is in cases when someone called you from script, of course. If you're just trying to find a safe non-window-associated place to do stuff, then AutoSafeJSContext is fine. It looks to me like your uses in this patch are reasonable. As for the asserts, you presumably need to enter the right compartment on the JSContext. I assume GetOriginAttributes is supposed to return an object, right? So for example in SerializedLoadContext::Init you can do: AutoSafeJSContext cx; JS::Rooted<JS::Value> v(cx); nsresult rv = aLoadContext->GetOriginAttributes(&v); if (NS_FAILED(rv)) { // whatever } if (!v.isObject()) { // more whatever; callee screwed up } JSAutoCompartment ac(cx, &v.toObject()); and now you can call mOriginAttributes.Init(cx, v) without assertions, since you're operating in the right compartment. Similar in CopyFromLoadContext and in the C++ version of GetOriginAttributes in the IDL file. The C++ impl of LoadContext doesn't really require the compartment stuff there, since it already uses the caller compartment, but the JS impl can totally end up with compartment mismatches without it. This business of having script-implemented LoadContexts here is a mess; it causes this code to be a lot more complicated and fragile (and slow!) than it would need to be otherwise. I wish we could just not support that or something. > So I change it to CopyFromLoadContext(nsILoadContext*), and do a Cast from > nsIDocShell* to nsDocShell*. Is there a reason not to just QI the docshell to nsILoadContext instead of casting? I mean, I guess it works either way, but the QI makes it clearer what's going on, I think.

And, thanks Boris!

(In reply to Bobby Holley (PTO through Sept 13) from comment #56) > Tanvi and I just discussed the naming. She suggested renaming > "InheritFromDocShell" to "CopyFromDocShell", which sounds fine to me. My concern with these names is that there are several types of inheritance involved here, and it's not clear which one(s) the above implement. As far as I can tell, there are at least the following: * Inheriting OriginAttributes from docshell to document when the user navigates. * Inheriting OriginAttributes from document to child docshell when an <iframe> is created. * Inheriting OriginAttributes from document to necko when a network request is made. Which of these do "InheritFromDocShell" and "CopyFromDocShell" implement? See potential other name suggestions below. > Jonas, my understanding of comment 30 is that it would be good to have an > API whereby addons could override the logic of selecting OriginAttributes > for a given load. However, I don't think we have the bandwidth to design > that API right now Agreed. My thinking was that we add three function: InheritFromDocshellToDoc InheritFromDocToIframe InheritFromDocToNetwork Then, at some later point, the implementations of these functions could be changed to allow addons to participate in the process.

Comment on attachment 8657470 [details] [diff] [review] v7 - update idiff, to make it work (for reference) God! This can only be used on the main thread... and we are f***ed again...

Updated according Boris' comments. If there is no originAttributes implemented on the JS object, just use the default OriginAttributes class values (copy defaults). This makes all tests missing proper nsILoadContext implementation work.

allstart, ping. This bug becomes more and more important, the functionality here is needed in other bugs.

Yoshi is still on vacation :( Back on Sep 20 according to bugzilla name.

Comment on attachment 8660333 [details] [diff] [review] 8657470: v7 - update idiff v2, to make it work Review of attachment 8660333 [details] [diff] [review]: ----------------------------------------------------------------- I can review this interdiff. ::: caps/BasePrincipal.cpp @@ +47,5 @@ > nsresult rv = aLoadContext->GetOriginAttributes(&v); > NS_ENSURE_SUCCESS(rv, false); > > + // If it's not an object (probably not correctly implemeted on nsILoadContext) > + // just use the default values of OriginAttributes. Please just use the helper on nsILoadContext.idl here. ::: docshell/base/SerializedLoadContext.cpp @@ +61,5 @@ > mIsPrivateBitValid = true; > aLoadContext->GetIsContent(&mIsContent); > aLoadContext->GetUsePrivateBrowsing(&mUsePrivateBrowsing); > aLoadContext->GetUseRemoteTabs(&mUseRemoteTabs); > + aLoadContext->GetOriginAttributes(mOriginAttributes); Please check the result for failure and warn at the very least (since the method returns void). ::: docshell/base/nsILoadContext.idl @@ +137,5 @@ > * The C++ getter for origin attributes. > */ > bool GetOriginAttributes(mozilla::OriginAttributes& aAttrs) > { > + mozilla::AutoSafeJSContext cx; AutoSafeJSContext is deprecated, please don't use it. Given that we enter the compartment of |v.toObject()|, I agree that the privileged junk scope is unnecessary here. Instead, just do: AutoJSAPI jsapi; jsapi.Init(); Which inits us in a null compartment (which is fine).

(In reply to Jonas Sicking (:sicking) from comment #86) > Agreed. My thinking was that we add three function: > > InheritFromDocshellToDoc > InheritFromDocToIframe > InheritFromDocToNetwork This sounds good to me, except I'd rename the second one to |InheritFromDocToChildDocshell| since it happens for all subframes, not just iframes.

Sounds great to me!

(In reply to Bobby Holley (:bholley) from comment #91) > Comment on attachment 8660333 [details] [diff] [review] > 8657470: v7 - update idiff v2, to make it work > > Review of attachment 8660333 [details] [diff] [review]: > ----------------------------------------------------------------- > > I can review this interdiff. > AutoSafeJSContext is deprecated, please don't use it. > > Given that we enter the compartment of |v.toObject()|, I agree that the > privileged junk scope is unnecessary here. Instead, just do: > > AutoJSAPI jsapi; > jsapi.Init(); > > Which inits us in a null compartment (which is fine). Dear Bobby Holley, what you suggest is, after spending an hour on it, confirmed as NOT WORKING: code: bool GetOriginAttributes(mozilla::OriginAttributes& aAttrs) { nsresult rv; bool st; mozilla::dom::AutoJSAPI jsapi; jsapi.Init(); JS::Rooted<JS::Value> v(jsapi.cx()); rv = GetOriginAttributes(&v); NS_ENSURE_SUCCESS(rv, false); // If it's not an object (probably not correctly implemeted on nsILoadContext) // just use the default values of OriginAttributes. mozilla::OriginAttributes attrs; if (v.isObject()) { JSAutoCompartment ac(jsapi.cx(), &v.toObject()); // ** st = attrs.Init(jsapi.cx(), v); NS_ENSURE_TRUE(st, false); } aAttrs = attrs; return true; } stack: inline js::Handle<js::GlobalObject*> ExclusiveContext::global() const { /* * It's safe to use |unsafeGet()| here because any compartment that is * on-stack will be marked automatically, so there's no need for a read * barrier on it. Once the compartment is popped, the handle is no longer * safe to use. */ > MOZ_ASSERT(compartment_, "Caller needs to enter a compartment first"); return Handle<GlobalObject*>::fromMarkedLocation(compartment_->global_.unsafeGet()); } xul!js::ExclusiveContext::global+0x28 xul!js::NewObjectWithClassProtoCommon+0x7a xul!js::NewObjectWithClassProto+0x1c xul!js::NewBuiltinClassInstance+0x22 xul!js::NewBuiltinClassInstance+0x28 xul!js::NewBuiltinClassInstance<js::PlainObject>+0x18 xul!JS_NewPlainObject+0x86 xul!mozilla::dom::OriginAttributesDictionary::ToObjectInternal+0x4a xul!mozilla::dom::ToJSValue<mozilla::OriginAttributes>+0x13 xul!mozilla::LoadContext::GetOriginAttributes+0x66 xul!nsILoadContext::GetOriginAttributes+0x6e xul!NS_GetOriginAttributes+0x4e xul!mozilla::net::GetLoadContextInfo+0x86 xul!mozilla::net::nsHttpChannel::OpenCacheEntry+0x639 xul!mozilla::net::nsHttpChannel::Connect+0x8a3 xul!mozilla::net::nsHttpChannel::ContinueBeginConnectWithResult+0x17b xul!mozilla::net::nsHttpChannel::ContinueBeginConnect+0x11 xul!mozilla::net::nsHttpChannel::BeginConnect+0x169c xul!mozilla::net::nsHttpChannel::OnProxyAvailable+0xe7 xul!nsAsyncResolveRequest::DoCallback+0x28e Removing the // ** line just makes things even worse. In my update patch I'm just following the suggestion from Boris in comment 83. Please figure out something that works and is not obsolete. In the meantime I stick with my patch. Thanks.

Ah right, I guess in the C++-implemented case the nsILoadContext implementation gets the cx off the JSContext stack, and therefore it depends on it being in the correct compartment. Sorry about that. In that case, please leave the existing code for getting a cx the way it was before your patch (initing the AutoJSAPI with xpc::PrivilegedJunkScope()), which should do the trick. Please let me know if it doesn't.

(In reply to Bobby Holley (:bholley) from comment #95) > In that case, please leave the existing code for getting a cx the way it was > before your patch (initing the AutoJSAPI with xpc::PrivilegedJunkScope()), > which should do the trick. Please let me know if it doesn't. It doesn't, hence my patch :D

Can you be more specific about why it doesn't? Happy to talk about this on IRC if it's easier.

Is the issue that this needs to be used off-main-thread?

Hm, that can't be it, given that the patch uses AutoSafeJSContext, which isn't threadsafe.

(In reply to Bobby Holley (:bholley) from comment #99) > Hm, that can't be it, given that the patch uses AutoSafeJSContext, which > isn't threadsafe. Comment 81. I can reach you tomorrow if that doesn't explain the problem.

Right - you still need the JSAutoCompartment part of the patch - I'm saying that you should revert the AutoSafeJSContext part.

(In reply to Bobby Holley (:bholley) from comment #101) > Right - you still need the JSAutoCompartment part of the patch - I'm saying > that you should revert the AutoSafeJSContext part. Comment 94 :D

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #102) > (In reply to Bobby Holley (:bholley) from comment #101) > > Right - you still need the JSAutoCompartment part of the patch - I'm saying > > that you should revert the AutoSafeJSContext part. > > Comment 94 :D Yes, and I explained how to fix that in comment 95.

(In reply to Bobby Holley (:bholley) from comment #103) > (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #102) > > (In reply to Bobby Holley (:bholley) from comment #101) > > > Right - you still need the JSAutoCompartment part of the patch - I'm saying > > > that you should revert the AutoSafeJSContext part. > > > > Comment 94 :D > > Yes, and I explained how to fix that in comment 95. Aha! I probably don't follow what to do then. Can you give me a code snippet/pseudo code so I can put it into my patch? I don't know the JS C++ code at all!

Give this a try? I haven't compiled it, but this is roughly what I'm suggesting: bool GetOriginAttributes(mozilla::OriginAttributes& aAttrs) { mozilla::dom::AutoJSAPI jsapi; bool ok = jsapi.Init(xpc::PrivilegedJunkScope()); NS_ENSURE_TRUE(ok, false); JS::Rooted<JS::Value> v(jsapi.cx()); nsresult rv = GetOriginAttributes(&v); NS_ENSURE_SUCCESS(rv, false); NS_ENSURE_TRUE(v.isObject(), false); JS::Rooted<JSObject*> obj(jsapi.cx(), &v.toObject()); // If we're JS-implemented, the object will be left in a different (System-Principaled) // scope, so we may need to enter its compartment. MOZ_ASSERT(nsContentUtils::IsSystemPrincipal(nsContentUtils::ObjectPrincipal(obj))); JSAutoCompartment ac(jsapi.cx(), obj); mozilla::OriginAttributes attrs; ok = attrs.Init(jsapi.cx(), obj); NS_ENSURE_TRUE(ok, false); aAttrs = attrs; return true; } Let me know if you run into problems.

(And to be clear, I think we should fail in the case where |v| isn't an object, rather than silently doing the default, because this is security-sensitive code)

I updated Honza's patches with comment 105 and addressed the issues in comment 91. https://treeherder.mozilla.org/#/jobs?repo=try&revision=82326788ae8e

Thanks Honza and Valentin for updating the patch. I saw Valentin is on vacation now, so I'll take this bug back and finish this.

Comment on attachment 8663797 [details] [diff] [review] interdiff Review of attachment 8663797 [details] [diff] [review]: ----------------------------------------------------------------- ::: docshell/base/nsILoadContext.idl @@ +12,5 @@ > %{C++ > #include "mozilla/BasePrincipal.h" // for OriginAttributes > #include "mozilla/dom/ScriptSettings.h" // for AutoJSAPI > #include "xpcpublic.h" // for PrivilegedJunkScope > +#include "nsContentUtils.h" // for IsSystemPrincipal I found this will cause build error on Mac 00:08:59 INFO - In file included from /builds/slave/try-m64-0000000000000000000000/build/src/browser/components/shell/nsMacShellService.cpp:25: 00:08:59 INFO - In file included from ../../../dist/include/nsILoadContext.h:26: 00:08:59 INFO - In file included from ../../../dist/include/nsContentUtils.h:26: 00:08:59 INFO - In file included from ../../../dist/include/nsContentListDeclarations.h:12: 00:08:59 INFO - ../../../dist/include/nsStringFwd.h:15:2: error: Internal string headers are not available from external-linkage code. 00:08:59 INFO - #error Internal string headers are not available from external-linkage code. 00:08:59 INFO - ^ 00:08:59 INFO - 1 error generated. Will try to fix problem as well.

merged my v7 patch and Valentin's patch, also fixed some xpcshell-test error.

(In reply to Bobby Holley (:bholley) from comment #92) > (In reply to Jonas Sicking (:sicking) from comment #86) filed Bug 1209162 to fix the naming.

We need to back this out. The phone is constantly crashing.

I already have some patch in Bug 1210459.