We should revisit all of our documentation on how to create a channel and make sure they are consistent throughout: * nsNetutil.h * nsnetutil.jsm * ioservice.idl * ioservice2.idl ...

I think that should cover all the updates - if we need to update more documentation, please let me know!

Comment on attachment 8576217 [details] [diff] [review] bug_1134096_revise_docs_for_newChannel2.patch Review of attachment 8576217 [details] [diff] [review]: ----------------------------------------------------------------- ::: caps/nsScriptSecurityManager.cpp @@ +308,5 @@ > +/* > + * Returns the Principal of the loading Document. E.g.: if the > + * loading document comes from "http://a.com/", and the channel > + * is loading the URI "http://b.com/whatever", then > + * GetChannelResultPrincipal will return a principal from "http://a.com/". No, this is not correct. I'd say something like: "The principal that the resource returned by this channel will use. So if we are loading a rendered document, it's the principal that scripts loaded in that document will use. If we are loading an image, it's the principal that you need to be same-origin with in order to read pixel data from the image. This is generally the principal of the URI of the channel, i.e. same as GetChannelURIPrincipal. But for example for sandboxed loads it will be a null principal. Think of this as the principal of the resource returned from the load." @@ +348,5 @@ > +/* > + * Returns the Principal of the loading Channel. E.g.: if the > + * loading document comes from "http://a.com/", and the channel > + * is loading the URI "http://b.com/whatever", then > + * GetChannelURIPrincipal will return a principal from "http://b.com/". "The principal of the URI that this channel is loading. This is never affected by things like sandboxed loads, or loads where we forcefully inherit the principal. Think of this as the principal of the server which this channel is loading from."

Comment on attachment 8576217 [details] [diff] [review] bug_1134096_revise_docs_for_newChannel2.patch +/* ***** DEPRECATED ***** + * please use NewChannel2 providing the right arguments for: I think you meant NewChannelFromURIWithProxyFlags2 here instead of NewChannel2 + * * aLoadingNode + * * aLoadingPrincipal + * * aTriggeringPrincipal + * * aSecurityFlags + * * aContentPolicyType + * + * See nsIIoService.idl for a detailed description of those arguments + */ NS_IMETHODIMP nsIOService::NewChannelFromURIWithProxyFlags(nsIURI *aURI, nsIURI *aProxyURI, uint32_t aProxyFlags, nsIChannel **result) {

(In reply to Jonas Sicking (:sicking) from comment #2) > ::: caps/nsScriptSecurityManager.cpp > @@ +308,5 @@ > > +/* > > + * Returns the Principal of the loading Document. E.g.: if the > > + * loading document comes from "http://a.com/", and the channel > > + * is loading the URI "http://b.com/whatever", then > > + * GetChannelResultPrincipal will return a principal from "http://a.com/". > > No, this is not correct. > > I'd say something like: > > "The principal that the resource returned by this channel will use. So if we > are loading a rendered document, it's the principal that scripts loaded in > that document will use. If we are loading an image, it's the principal that > you need to be same-origin with in order to read pixel data from the image. > > This is generally the principal of the URI of the channel, i.e. same as > GetChannelURIPrincipal. But for example for sandboxed loads it will be a > null principal. > > Think of this as the principal of the resource returned from the load." > Hey Jonas, this is really confusing. We should discuss in our next meeting to see if we can explain this in a simpler way. If both me and Chris are confused, then most gecko developers will be too :|

Let's take another crack at this. Jonas, Christoph, please review. GetChannelResultPrincipal comments: "GetChannelResultPrincipal will return the principal that the resource returned by this channel will use. For example, if the resource is in a sandbox, it will return the nullprincipal. If the resource is forced to inherit principal, it will return the principal of its parent. If the load doesn't require sandboxing or inheriting, it will return the same thing as GetChannelURIPrincipal (the principal of the URI that is being loaded)." GetChannelURIPrincipal comments: "The principal of the URI that this channel is loading. This is never affected by things like sandboxed loads, or loads where we forcefully inherit the principal. Think of this as the principal of the server which this channel is loading from. Most callers should use GetChannelResultPrincipal instead of GetChannelURIPrincipal. Only call GetChannelURIPrincipal if you are sure that you want the principal that matches the uri, even in cases when the load is sandboxed or when the load could be a blob or data uri (i.e even when you encounter loads that may or may not get sandboxed and loads that may or may not inherit)." Jonas, looking closely at GetCodeBasePrincipalInternal, it looks like we return nullprincipal if the URI inherits. That doesn't make much sense to me. Do you know why we are doing this? https://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#1087

Here is an updated version - looks good to me. You still need to greenlight tanvi. Let's wait for Jonas' response though.

(In reply to Tanvi Vyas [:tanvi] from comment #5) > Jonas, looking closely at GetCodeBasePrincipalInternal, it looks like we > return nullprincipal if the URI inherits. That doesn't make much sense to > me. Do you know why we are doing this? > https://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager. > cpp#1087 It doesn't make much sense to ask for the principal for a URL if the URL is for a scheme where the principal depends on who loads the URL, not the URL itself.

Looks good

Comment on attachment 8590517 [details] [diff] [review] bug_1134096_revise_docs_for_newChannel2.patch r+, but with 2 quick changes noted below. >diff --git a/caps/nsScriptSecurityManager.cpp b/caps/nsScriptSecurityManager.cpp >+ * sandboxed or when the load could be a blob or data uri (i.e even when >+ * you encounter loads that may or may not get sandboxed and loads Please s/get/be >+ * that may or may not inherit)." >+ */ > NS_IMETHODIMP > nsScriptSecurityManager::GetChannelURIPrincipal(nsIChannel* aChannel, > nsIPrincipal** aPrincipal) >diff --git a/netwerk/base/nsIOService.cpp b/netwerk/base/nsIOService.cpp >+/* ***** DEPRECATED ***** >+ * please use NewChannelFromURIWithProxyFlags providing the right arguments for: You are missing a 2 here. NewChannelFromURIWithProxyFlags2 >+ * * aLoadingNode >+ * * aLoadingPrincipal >+ * * aTriggeringPrincipal >+ * * aSecurityFlags >+ * * aContentPolicyType >+ * >+ * See nsIIoService.idl for a detailed description of those arguments >+ */ > NS_IMETHODIMP > nsIOService::NewChannelFromURIWithProxyFlags(nsIURI *aURI, > nsIURI *aProxyURI, > uint32_t aProxyFlags, > nsIChannel **result) > { >+ NS_ASSERTION(false, "Deprecated, use NewChannelFromURIWithProxyFlags2 providing loadInfo arguments!"); > return NewChannelFromURIWithProxyFlags2(aURI, > aProxyURI, > aProxyFlags,

It seems like we can't land assertions because NetUtil.jsm tests are still calling newChannel() instead of newChannel2(). The reason we haven't updated those tests to call newChannel2() is because addons might still rely on NetUtil.newChannel(). We need to have tests to make sure we don't break that API. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8078bd3e621e For now, I turned assertions into warnings - potentially we can have those tests ignore assertions. Going to land using 'WARNING' for now and we can revisit.