Closed Bug 1038756 Opened 10 years ago Closed 10 years ago

Extending LoadInfo and storing LoadInfo on all channels created through NS_NewChannel

Categories

(Core :: DOM: Security, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 4 open bugs)

Details

Attachments

(34 files, 66 obsolete files)

9.78 KB, patch
ckerschb
: review+
sicking
: review+
Details | Diff | Splinter Review
27.49 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
33.84 KB, patch
jst
: review+
Details | Diff | Splinter Review
1.64 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
3.65 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
3.22 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.17 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
19.51 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
25.43 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
10.49 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.50 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
19.74 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
1.61 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
12.72 KB, patch
jimm
: review+
Details | Diff | Splinter Review
1.66 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.17 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
3.32 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.15 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
4.72 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
3.01 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
1.19 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
4.87 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.70 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.01 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
3.01 KB, patch
mwu
: review+
Details | Diff | Splinter Review
4.17 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
4.72 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.28 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
2.21 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
1.63 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
1.80 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
18.37 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
16.93 KB, patch
bzbarsky
: review+
peterv
: review+
Details | Diff | Splinter Review
1.14 KB, patch
sicking
: review+
Details | Diff | Splinter Review
Currently, the LoadInfo is only set on a sparse number of channels. We could extend the LoadInfo [1] with a contentPolicyType and also a loadingContext which would allow us to classify and handle redirects for Mixed Content Blocking [2] correctly if set on all channels.

+ interface nsILoadInfo : nsISupports
+ {
+ ...
+   readonly attribute nsContentPolicyType contentPolicyType;
+   readonly attribute nsISupports loadingContext;

Further down the road, this would also allow us to replace the |channelPolicy| currently set on channels to handle redirects for Content Security Policy (CSP).

Instead of setting the LoadInfo after creation of a channel, we could hand the needed information (principal, context, contentPolicyType), to NS_NewChannel, which would then create the LoadInfo and set it on every channel. Agreed, not on every channel, because not all the creations of channels go through NS_NewChannel in nsNetutil.h, but at least all the channels we do care about for CSP and MCB are created through NS_NewChannel.

For this approach to work we would also have to set the LoadInfo on channels where the principal currently is null, e.g. in nsDocshell, which calls SetupChannelOwner [3] which currently does *not* create a LoadInfo in case the principal is null. Bz, would it cause any problems to have a Loadinfo on a channel where the principal is null?

I think it's best if we extend NS_NewChannel with 5 arguments

inline nsresult
NS_NewChannel(nsIChannel           **result,
              nsIURI                *uri,
              nsIIOService          *ioService = nullptr,    // pass in nsIIOService to optimize callers
              nsILoadGroup          *loadGroup = nullptr,
              nsIInterfaceRequestor *callbacks = nullptr,
              uint32_t               loadFlags = nsIRequest::LOAD_NORMAL,
              nsIChannelPolicy      *channelPolicy = nullptr,
+             nsContentPolicyType    aType,
+             nsIPrincipal*          aRequestingPrincipal,
+             nsINode*               aRequestingNode,
+             InheritType            aInheritPrincipal,
+             SandboxType            aSandboxed);

Please note, that we have to set both, aRequestingNode as well as aRequestingPrincipal, because we do not always have a RequestingNode available from which we could query the principal through node->RequestingPrincipal() when creating a channel through NS_NewChannel. In general, we should never end up with a channel without a principal, so we can always reason about security.


Fixing this bug would also be a perfect start on the way to implement Jonas proposal [4,5] where we call contentPolicies after a channel is created.

Bz, what's your opinion on this?


[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadInfo.idl#14
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=418354
[3] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#6501
[4] https://etherpad.mozilla.org/BetterNeckoSecurityHooks
[5] https://bugzilla.mozilla.org/show_bug.cgi?id=1006881
Flags: needinfo?(bzbarsky)
Blocks: 418354
Depends on: 965413
Blocks: 1037669
WIP patch extending nsILoadInfo
WIP patch for extending NS_NewChannel API
Now we have to update all the callsites to pass in the required info to NS_NewChannel. The big question is: Can we use the systemPrincipal for cases like nsStringBundle [1]. In my opinion we can use the systemPrincipal, because right now that case does not even undergo a shouldLoad call and hence is not evaluated by contentPolcies anyway. Anyone knows?

[1] http://mxr.mozilla.org/mozilla-central/source/intl/strres/src/nsStringBundle.cpp#73
Using a system principal for stringbundle sounds fine to me.
Flags: needinfo?(bzbarsky)
Blocks: 994872
> Bz, would it cause any problems to have a Loadinfo on a channel where the principal is
> null?

Yes: current consumers expect it to have a non-null principal and will crash if it's null.

That said, we could just create a NullPrincipal in this situation.  Note bug 1037271.

> In general, we should never end up with a channel without a principal, so we can always
> reason about security.

Right.  Which means we should figure out what cases currently pass null to SetupChannelOwner and see whether they should do something else.
Attachment #8456333 - Attachment is obsolete: true
Attachment #8456334 - Attachment is obsolete: true
Adding the first set of WIP patches; we still need to update callers of NS_NewInputStreamChannel.
Boris, can you take a first look and provide some feedback? In general, I would really like to pass info about principal/node/contentPolicyType to NS_NewChannel and all other API functions in nsNetutil.h that end up calling NS_NewChannel.
For this to work, we have to modify SetupChannelOwner (InheritChannelOwner) because we don't want to set the loadInfo after cration of the channel, but rather at creation time to make sure all channels end up having the loadInfo.

The ultimate goal of Jonas proposal is to have the loadInfo on *all* channels, not only those created in nsNetutil.h, but that's further down the road - in a different patch.
Flags: needinfo?(bzbarsky)
CC'ing smaug on this bug as well, because he was one of the reviewers for the initial LoadInfo patch - so he might have some input as well.
Mike, there's docshell stuff in this bug.  ;)
Those WIP patches is all the work I would like to happen in this bug so that all channels created through NS_NewChannel have the loadingInfo attached. We have to be really careful and evaluate carefully which node/principal/type goes into what channel. Probably we should replace all the systemPrincpals with NullPrincipals. I am not sure, probably it would be safer, but then, once we move all security checks (ContentPolicy) checks to AsyncOpen all of these channels will fail, so probably keeping the systemPrincipal is the better strategy.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #17)
> Those WIP patches is all the work I would like to happen in this bug so that
> all channels created through NS_NewChannel have the loadingInfo attached. We
> have to be really careful and evaluate carefully which node/principal/type
> goes into what channel. Probably we should replace all the systemPrincpals
> with NullPrincipals. I am not sure, probably it would be safer, but then,
> once we move all security checks (ContentPolicy) checks to AsyncOpen all of
> these channels will fail, so probably keeping the systemPrincipal is the
> better strategy.

We should try to find the actual loading principal and loading node for as many places as possible.  The loadinfo will likely be used by consumers besides the security checking code in the future.
Christoph, it seems like the obvious thing is to change SetupChannelOwner to return an nsILoadInfo (and probably a boolean for whether to reset the channel owner to null, or move that SetOwner(nullptr) logic into callers based on the sandbox flag in the loadinfo) which will then be used when the channel is actually created.

Or even just the boolean for whether to inherit, I guess: that's the one part SetupChannelOwner seriously computes, other than the actual channel-munging it does.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #19)
> Christoph, it seems like the obvious thing is to change SetupChannelOwner to
> return an nsILoadInfo (and probably a boolean for whether to reset the
> channel owner to null, or move that SetOwner(nullptr) logic into callers
> based on the sandbox flag in the loadinfo) which will then be used when the
> channel is actually created.
> 
> Or even just the boolean for whether to inherit, I guess: that's the one
> part SetupChannelOwner seriously computes, other than the actual
> channel-munging it does.

If you look at patch 5 [1], we change SetupChannelOwner to InheritChannelOwner which returns a boolean whether to inherit the channel owner or not and resets the owner (SetOwner(null)) on the channel if needed right after channel creation. Please note that I moved SetupChannelOwner (InheritChannelOwner) to be called right before creation of the channel.

If you look at patch 2 [2], we change the function signature for NS_NewChannel, which then internally creates the loadInfo, so that all channels created in nsNetUtil.h have the loadInfo attached at creation time. Hence I would rather have the former SetupChannelOwner only return a boolean whether to inherit or not and hand all the information needed to create a loadInfo to NS_NewChannel (patch 3) [3].

I applied the patches (slightly modified) and ran mochitests (locally), it seems to perform the same task and behaves the same way. I think the next step is to make sure that we use the correct node/principal in all cases.

Any objections to this design?

The ultimate goal is to have the loadInfo on all channels, so after landing this patch we would probably change *.idl files of ioService so that we can hand the loadInfo to 'newChannel', but that will break addons in turn, so I would like to do as much work (add loadInfo to as many channel as possible) before we have to break addons. Probably we can implement a shim, some kind of wrapper as an interim solution; but lets focus on what's important for this bug.

[1] https://bug1038756.bugzilla.mozilla.org/attachment.cgi?id=8458134
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8458131
[3] https://bugzilla.mozilla.org/attachment.cgi?id=8458132
Blocks: 1006868
> we change SetupChannelOwner to InheritChannelOwner which returns a boolean

Right, that was my suggestion at the end of comment 19 as well.

Looking at the patch, more carefully, that means we end up duplicating a bunch of logic (and in particular the conversion from booleans to loadinfo enums) and the SetOwner(nullptr) at the callsites, which is a bit unfortunate.

We could fix the booleans bit via having InheritChannelOwner() (maybe call it ChannelShouldInheritPrincipal?) return the enum and by having an accessor on the document that returns the sandboxed enum instead of duplicating the GetSandboxFlags() & SANDBOXED_ORIGIN thing and its conversion to enums.

We could also push the SetOwner(nullptr) bit down into NS_NewChannel, right?  With that done, I think this would look reasonable.

The SetOwner(nullptr) bit is definitely NOT a no-op, unless I'm seriously missing something, at least as long as we have protocol handler implementations that create channels with the owner already set.
(In reply to Boris Zbarsky [:bz] from comment #21)
> > we change SetupChannelOwner to InheritChannelOwner which returns a boolean
> 
> Right, that was my suggestion at the end of comment 19 as well.
> 
> Looking at the patch, more carefully, that means we end up duplicating a
> bunch of logic (and in particular the conversion from booleans to loadinfo
> enums) and the SetOwner(nullptr) at the callsites, which is a bit
> unfortunate.

I don't like that either, so my plan was to file a follow up and collapse all securityFlags into one unsigned long, where we 'or' all the different flags. There are more flags btw, see Jonas proposal at line 86:
https://etherpad.mozilla.org/BetterNeckoSecurityHooks

Anyway, as a first step I moved the enums into the idl, mostly because I couldn't access them in compiled code tests, but also as a first step towards having only *one* unsigned long that holds all the different flags. Similar to the loadFLags.

> We could fix the booleans bit via having InheritChannelOwner() (maybe call
> it ChannelShouldInheritPrincipal?) return the enum and by having an accessor
> on the document that returns the sandboxed enum instead of duplicating the
> GetSandboxFlags() & SANDBOXED_ORIGIN thing and its conversion to enums.
> 
> We could also push the SetOwner(nullptr) bit down into NS_NewChannel, right?
> With that done, I think this would look reasonable.

That sounds like the best solution to me, we should have that part in NS_NewChannel where we can call SetOwner(nullptr) immediately after the channel was created by the ioservice.

> The SetOwner(nullptr) bit is definitely NOT a no-op, unless I'm seriously
> missing something, at least as long as we have protocol handler
> implementations that create channels with the owner already set.

Oh oh - my bad - facepalm. For some reason I had the impression the channel owner is a nullptr initially.
Blocks: 1041176
Blocks: 965637
Blocks: 1041180
Blocks: 1006881
No longer blocks: 1006868
Since the patches here probably are going to change a lot I created a repo for our patches:
http://hg.mozilla.org/users/mozilla_christophkerschbaumer.com/revamp-patches/

where we can track progress.
Boris: when storing the loadInfo on all channels that are created through NS_NewChannel we are going to run into a problem, which is best visualized when running test |test_CrossSiteXHR.html|. Especially relevant are the CORS tests (introduced/modified in this bug [1]) that cause problems - fail.

Taking a closer look reveals that GetChannelPrincipal in the scriptSecurityManager returns the principal stored in the loadInfo [2] (because we have it on so many channels now). Since XMLHttpRequests use InheritPrincipal it returns too early; in fact it should return the CodeBasePrincipal [3].

In the end that causes the CorsListenerProxy to compare the wrong principals [4] and the tests to fail. What do you think is the best way to fix this problem?

Thanks!

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=814141
[2] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#277
[3] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#294
[4] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsCrossSiteListenerProxy.cpp#727
Flags: needinfo?(bzbarsky)
Hmm.

So maybe the right answer is to not tell XHR (and probably various other callsites) to inherit the principal.  I'm not sure why we're doing that anyway, since XHR then later stamps the principal on the document itself.  Check the blame?

The other option is to have two different GetChannelPrincipal functions, one of which considers inheritance and one of which does not, for answering the "what principal should I use for this data" and "where is this data coming from" questions.  Sort of like for scripts we have distinct principals and originPrincipals...
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #25)
> Hmm.
> 
> So maybe the right answer is to not tell XHR (and probably various other
> callsites) to inherit the principal.

I think you mean *not* inherit the principal, because at the moment XML reqeuests inherit.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1978

I'm not sure why we're doing that
> anyway, since XHR then later stamps the principal on the document itself. 

Anyway, as a first attempt I changed XHR to not inherit the principal, but that causes the test_XHR* test family to break :-(

> The other option is to have two different GetChannelPrincipal functions, one
> of which considers inheritance and one of which does not, for answering the
> "what principal should I use for this data" and "where is this data coming
> from" questions.  Sort of like for scripts we have distinct principals and
> originPrincipals...

I would really like to have that separated as well. The current situation seems like a mix of channel-owner and principal in the loadInfo, where I am not sure when do use which one and sometimes they should be in sync and sometimes not. I have a meeting with Jonas in 30 minutes, will ask for more input from his side too.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26)
> (In reply to Boris Zbarsky [:bz] from comment #25)
> > Hmm.
> > 
> > So maybe the right answer is to not tell XHR (and probably various other
> > callsites) to inherit the principal.
> 
> I think you mean *not* inherit the principal, because at the moment XML
> reqeuests inherit.
> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsXMLHttpRequest.cpp#1978

Oh boy, my english :-( I think you meant the right thing. To be explicit, if I change XMLHttpRequests to *not* inherit the principal, then test_CrossSiteXHR.html passes, but test_XHR*.html tests are failing and vice versa.
> but that causes the test_XHR* test family to break

Can you tell why?

> The current situation seems like a mix of channel-owner and principal in the loadInfo

Yes, but we should be phasing out usage of "owner" until we kill it off completely.  The split I'm talking about is unrelated to whether we have "owner" or not.
(In reply to Boris Zbarsky [:bz] from comment #28)
> Yes, but we should be phasing out usage of "owner" until we kill it off
> completely.  The split I'm talking about is unrelated to whether we have
> "owner" or not.

Borisl: Jonas is on the same page with you. So we agreed to add a new function to the scriptSecurityManager. I am going to inline the hot part here:

>  NS_IMETHODIMP
> +nsScriptSecurityManager::GetChannelURIPrincipal(nsIChannel* aChannel,
> +                                                nsIPrincipal** aPrincipal)
> +{
> +    NS_PRECONDITION(aChannel, "Must have channel!");
> +
> +    // Get the principal from the URI.  Make sure this does the same thing
> +    // as nsDocument::Reset and XULDocument::StartDocumentLoad.
> +    nsCOMPtr<nsIURI> uri;
> +    nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    nsCOMPtr<nsILoadContext> loadContext;
> +    NS_QueryNotificationCallbacks(aChannel, loadContext);
> +
> +    if (loadContext) {
> +        return GetLoadContextCodebasePrincipal(uri, loadContext, aPrincipal);
> +    }
> +
> +    return GetCodebasePrincipalInternal(uri, UNKNOWN_APP_ID,
> +        /* isInBrowserElement */ false, aPrincipal);
> +}
> +
> +
> +NS_IMETHODIMP
>  nsScriptSecurityManager::GetChannelPrincipal(nsIChannel* aChannel,
>                                               nsIPrincipal** aPrincipal)
>  {
>      NS_PRECONDITION(aChannel, "Must have channel!");
>      nsCOMPtr<nsISupports> owner;
>      aChannel->GetOwner(getter_AddRefs(owner));
>      if (owner) {
>          CallQueryInterface(owner, aPrincipal);
> @@ -281,32 +305,17 @@ nsScriptSecurityManager::GetChannelPrinc
>          rv = loadInfo->GetForceInheritPrincipal(&inheritPrincipal);
>          NS_ENSURE_SUCCESS(rv, rv);
>          if (inheritPrincipal == nsILoadInfo::eInheritPrincipal) {
>            NS_ADDREF(*aPrincipal = loadInfo->LoadingPrincipal());
>            return NS_OK;
>          }
>      }
>  
> -    // OK, get the principal from the URI.  Make sure this does the same thing
> -    // as nsDocument::Reset and XULDocument::StartDocumentLoad.
> -    nsCOMPtr<nsIURI> uri;
> -    nsresult rv = NS_GetFinalChannelURI(aChannel, getter_AddRefs(uri));
> -    NS_ENSURE_SUCCESS(rv, rv);
> -
> -
> -    nsCOMPtr<nsILoadContext> loadContext;
> -    NS_QueryNotificationCallbacks(aChannel, loadContext);
> -
> -    if (loadContext) {
> -        return GetLoadContextCodebasePrincipal(uri, loadContext, aPrincipal);
> -    }
> -
> -    return GetCodebasePrincipalInternal(uri, UNKNOWN_APP_ID,
> -        /* isInBrowserElement */ false, aPrincipal);
> +    return GetChannelURIPrincipal(aChannel, aPrincipal);
>  }

Inside of nsCrossSiteListenerProxy::AsyncOnChannelRedirect we are now calling GetChannelURIPrincipal instead of GetChannelPrincipal which resolves the problem.
Boris, Jonas: Since we still got some failures when running mochis but also in respect to future patches it would be great to inspect all the callsites of 'GetChannelPrincipal' and update callsites to call GetChannel*URI*Principal. At the moment we might get lucky to fall through to GetChannelURIPrincipal when calling GetChannelPrincipal if there is no loadInfo set on the channel (see current codebase [1], but also Comment 29). In the future, once we have loadInfos and all channels, we are going to run into that problem again. It would be great to fix up all callsites with this patch!

I don't feel confident updating callsites to call GetChannel*URI*Principal where I am not 100% sure this is the right thing to do. Could any of you browse over the callsites [2] and let me know where we need to call GetChannel*URI*Principal instead of GetChannelPrincipal?

If it is of any help, here are testcases that are failing, that seem to closely related due to querying the wrong principal from the channel:
> parser/htmlparser/tests/mochitest/test_bug715739.html
> browser/base/content/test/general/browser_bug484315.js
> browser/modules/test/browser_UITour_annotation_size_attributes.js
> browser/modules/test/browser_UITour_detach_tab.js

[1] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#269
[2] http://mxr.mozilla.org/mozilla-central/search?string=GetChannelPrincipal
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
I think the right thing to do here is for you to go through this list and for each callsite say which function it should use and why.  And if you're not sure, why you're not sure.

Then Jonas or I can look at that list and help you resolve the ones you're not sure about.
Flags: needinfo?(bzbarsky)
Driveby glance, but I'm wondering if you can shove all of the new fields (or even all of the old ones) into a single idl so that one someone wants to extend loadinfo even more, they don't have to change all of the callsites of NS_NewChannel.

The reason I ask is that I need to shove something in, too! (the URI of the toplevel window would be particularly helpful)
There's only 29 callsites of GetChannelPrincipal. http://mxr.mozilla.org/mozilla-central/search?string=GetChannelPrincipal

Maybe create a patch that:

1. Adds GetChannelURIPrincipal
2. Renames GetChannelPrincipal to GetChannelResultPrincipal
3. Changes GetChannelPrincipal to GetChannelURIPrincipal or GetChannelResultPrincipal as you think is
   appropriate.

This way bz and I can review and see if things look good.
Flags: needinfo?(jonas)
Monica, you should be able to calculate the top-window URI using the node that's already available from the LoadInfo. So you could either modify the LoadInfo constructor to calculate it, add a new function on LoadInfo to calculate it, or simply calculate it in your code externally from the channel/loadinfo.
Attachment #8458130 - Attachment is obsolete: true
Attachment #8458131 - Attachment is obsolete: true
Attachment #8458132 - Attachment is obsolete: true
Attachment #8458133 - Attachment is obsolete: true
Attachment #8458134 - Attachment is obsolete: true
Attachment #8458135 - Attachment is obsolete: true
Attachment #8458229 - Attachment is obsolete: true
Attachment #8458230 - Attachment is obsolete: true
Attached patch f2_nsDocShell.patch (obsolete) — Splinter Review
Attached patch f3_imageLoader.patch (obsolete) — Splinter Review
Attached patch f4_getChannelPrincipal.patch (obsolete) — Splinter Review
Boris, Jonas: I put four patches up, which show the transition from SetupChannelOwner to ShouldInheritPrincipal for docShell and imgLoader. The ImgLoader needs special attention, because all of the tests that are still failing on TBPL [1] seem to be closely related to the imgLoader. I can't tell for sure because tests are timing out, and as of now, I don't really know why.

The last patch (f4_getChannelPrincipal.patch) is a diff of the changes of the separation of calling GetChannelResultPrincipal and GetChannelURIPrincipal.

Any feedback, in particular on the imgLoader part is highly appreciated. Please note that I filed a follow up bug for collapsing inheritPrincipal and isSandboxed, so we do not need two separate bools, enums whatever, but only one unsigned long where we can 'or' the flags.

[1] https://tbpl.mozilla.org/?tree=Try&rev=3b2d8fb5e213
Flags: needinfo?(jonas)
Flags: needinfo?(bzbarsky)
Maybe one thing of interest worth sharing is the part in NS_NewChannel where we set the LoadInfo.

> +NS_NewChannel(nsIURI*                         aUri,
> +              nsIIOService*                   aIoService,      // pass in nsIIOService to optimize callers
> +              nsILoadGroup*                   aLoadGroup,
> +              nsIInterfaceRequestor*          aCallbacks,
> +              uint32_t                        aLoadFlags,     // nsIRequest::LOAD_NORMAL,
> +              nsIChannelPolicy*               aChannelPolicy,
> +              nsIPrincipal*                   aRequestingPrincipal,
> +              nsINode*                        aRequestingNode,
> +              InheritType                     aInheritPrincipal,
> +              SandboxType                     aSandboxed,
> +              nsContentPolicyType             aContentPolicyType,
> +              nsIChannel**                    outChannel)
>  {
>  ...
> +
> +  nsCOMPtr<nsILoadInfo> loadInfo =
> +    new mozilla::LoadInfo(aRequestingPrincipal,
> +                          aRequestingNode,
> +                          aInheritPrincipal,
> +                          aSandboxed,
> +                          aContentPolicyType);
> +  channel->SetLoadInfo(loadInfo);
> +
> +  // If we're sandboxed, make sure to clear any owner the channel
> +  // might already have.
> +  if (aSandboxed == nsILoadInfo::eSandboxed) {
> +    channel->SetOwner(nullptr);
> +    // Make sure we have set a NullPrincipal
> +    MOZ_ASSERT(aRequestingPrincipal->GetIsNullPrincipal(), "sandboxed channel needs a nullPrincipal");
> +  }
> +
> +  channel.forget(outChannel);
> +  return NS_OK;
> + }

Formely the LoadInfo was set individually on channels after creation, now we attach a loadInfo to every channel *if* created through NS_NewChannel in nsNetutil.h
> MOZ_ASSERT(aRequestingPrincipal->GetIsNullPrincipal(), "sandboxed channel needs a nullPrincipal");

That looks wrong to me.  The whole point of having the sandboxed state in the LoadInfo is that we can have a useful aRequestingPrincipal even for sandboxed stuff, no?

>@@ -595,27 +597,46 @@ static nsresult NewImageChannel(nsIChann
>+    requestingPrincipal = do_GetService(NS_NULLPRINCIPAL_CONTRACTID);

This needs to be do_CreateInstance.

Nothing else is obviously jumping out at me in the imgLoader changes.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #41)
> > MOZ_ASSERT(aRequestingPrincipal->GetIsNullPrincipal(), "sandboxed channel needs a nullPrincipal");
> 
> That looks wrong to me.  The whole point of having the sandboxed state in
> the LoadInfo is that we can have a useful aRequestingPrincipal even for
> sandboxed stuff, no?

That is a good point - I will investigate that part again. But if we do not have a principal in nsDocShell or also imgLoader we should rather fall back to the NullPrincipal instead of the SystemPrincipal. Do you agree?

> 
> >@@ -595,27 +597,46 @@ static nsresult NewImageChannel(nsIChann
> >+    requestingPrincipal = do_GetService(NS_NULLPRINCIPAL_CONTRACTID);
> 
> This needs to be do_CreateInstance.

Obviously you are right - that problem was fixed in the wrong patch, hence my patch queue was messed up before exporting.
 
> Nothing else is obviously jumping out at me in the imgLoader changes.

Did you (can you) also browse over the changes where we query the ChannelPrincipal and see if we need to call GetChannel*URI*Principal instead of GetChannel*Result*Principal in one case or the other? I inspected all of the callsites, and it seems right at the moment, but I am not 100% sure.
In the current version Boris manually sets the LoadInfo once the channel is created in nsDocShell::DoURILoad. For what we are trying to accomplish in this bug we moved that logic a little further up so that the loadInfo-information gets passed to NS_NewChannel, which then creates and attaches the loadInfo to the channel.

Problem is/was, that there is an if-else, where in the else branch the channel doesn't get created through NS_NewChannel, but rather by calling NS_NewInputStreamChannel [1]. Hence we have to manually set the loadInfo on such channels. That was causing a problem in particular for 'about:srcdoc'. Once we land patches following that initial groundwork here, we can delete all of the code that manually attaches the loadInfo to channels after creation because that information will be passed along to channel constructors. Anyway, this fix should resolve all of our problems at the moment.

[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#9900

Also, clearing the needinfo flag for Jonas.
Flags: needinfo?(jonas)
> Did you (can you) also browse over the changes where we query the ChannelPrincipal

Will try to look tomorrow.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #44)
> > Did you (can you) also browse over the changes where we query the ChannelPrincipal
> 
> Will try to look tomorrow.

Thanks Boris. Given the fix from Comment 43 it seems we are getting pretty close:

https://tbpl.mozilla.org/?tree=Try&rev=b22afaa4a2dc

We still have some assertion failures, will debug those today, but overall looks pretty good.
When debugging test crashtests/914521.html [1], I encountered the following problem:

In nsGlobalWindow an assertion is triggered [2] where the uri in question is
"moz-nullprincipal:{60b2b627-30d8-425e-b53e-06f8e4c363f1}" instead of "about:blank".

Here is the information we used to create that channel:

> NS_NewChannel {
>   uri(host): about:blank
>   aRequestingPrincipal: moz-nullprincipal:{60b2b627-30d8-425e-b53e-06f8e4c363f1}
>   aRequestingNode: yes
>   aInheritPrincipal: yes
>   aSandboxed: no
>   aContentPolicyType: 6 (TYPE_DOCUMENT)
> }

Interesting is, that when creating the channel, that in nsAboutProtocolHandler [3] we set the owner to null:

> if (IsSafeForUntrustedContent(aboutMod, uri)) {
>   (*result)->SetOwner(nullptr);
> }

Later, when calling GetChannelPrincipal [4] we return the wrong principal because the inheritFlag is set and hence we return the nullPrincipal from the loadinfo instead of falling through to getting the uri (GetChannelURIPrincipal in the new code) of that channel.

The reason why that is currently working (in mozilla-central) is because SetupChanelOwner does not set a loadInfo on the channel if the principal in nsDocShell is null. In our code, where we *always* want a principal in the loadInfo on the channel we fall back to the NullPrincipal if the loadingPrincipal is a nullptr. Currenlty that is the problem, the loadPrincipal in nsDocShell is null, we fall back to the nullPrincipal which we then set in the loadInfo. When quering the principal from the channel we get the nullPrincipal back because the owner was set to null in nsAboutProtocolHandler.

The problem, in fact, happens a little earlier than where the assertion is triggered. Namely in nsDocument::Reset() [5] where we call GetChannelPrincipal (GetChannelResultPrincipal in the new code). Simply changing GetChannelResultPrincipal to GetChannelURIPrincipal (see attached patch 'f4_getChannelPrincipal.patch') in nsDocument::Reset() is obviously not correct, because we always want the 'ResultPrincipal' except for that special case in 'about:blank' described above.

At the moment I am not sure what the best way forward is here, Boris, Jonas, any suggestions?


[1] http://mxr.mozilla.org/mozilla-central/source/docshell/base/crashtests/914521.html?force=1
[2] http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#2073
[3] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#143
[4] http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#340
[5] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2198
Probably the right thing to do is to fall back to the CodebasePrincipal instead of using the NullPrincipal if no requestingPrincipal is provided in nsDocShell.cpp.

>     nsCOMPtr<nsIPrincipal> requestingPrincipal = do_QueryInterface(aOwner);
>     if (!requestingPrincipal) {
> -      requestingPrincipal = do_CreateInstance(NS_NULLPRINCIPAL_CONTRACTID, &rv);
> -      NS_ENSURE_SUCCESS(rv, rv);
> +      nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager();
> +      NS_ENSURE_TRUE(secMan, NS_ERROR_NOT_AVAILABLE);
> +      rv = secMan->GetCodebasePrincipal(aURI,
> +                                        getter_AddRefs(requestingPrincipal));
>     }

What other implications would that have?
> Boris, Jonas, any suggestions?

We shouldn't set the inherit boolean to true if the owner in docshell was null.

> Probably the right thing to do is to fall back to the CodebasePrincipal

No, that would be pretending like the target of the load is responsible for the load, whereas in reality we just have no idea who's responsible.

The _really_ ideal thing, of course, would be to make sure that the owner is never null... so we always know who's responsible for the load.  That's a pretty delicate operation with security implications, though, so best done as a separate patch from everything else.
Flags: needinfo?(bzbarsky)
Comment on attachment 8465029 [details] [diff] [review]
f4_getChannelPrincipal.patch

>+    nsIPrincipal GetChannelResultPrincipal(in nsIChannel aChannel);

getChannelResultPrincipal

The rest of this looks good to me.
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #48)
> > Boris, Jonas, any suggestions?
> 
> We shouldn't set the inherit boolean to true if the owner in docshell was
> null.

That seems to work just fine. To be consistent I also did this in imgLoader.cpp and also in nsObjectLoadingContent.cpp to be consistent. Basically everywhere we call SetupChannelOwner.


> The _really_ ideal thing, of course, would be to make sure that the owner is
> never null... so we always know who's responsible for the load.  That's a
> pretty delicate operation with security implications, though, so best done
> as a separate patch from everything else.

Yes, I agree, that would be ideal. We have to investigate closely. In general, we have to revisit all the callsites and make sure we hand the right/correct principal to NS_NewChannel.
> To be consistent I also did this in imgLoader.cpp and also in nsObjectLoadingContent.cpp 

I would think the principal is never null in nsObjectLoadingContent.
(In reply to Boris Zbarsky [:bz] (On vacation Aug 5-18) from comment #51)
> > To be consistent I also did this in imgLoader.cpp and also in nsObjectLoadingContent.cpp 
> 
> I would think the principal is never null in nsObjectLoadingContent.

So far I haven't encountered a nullptr as principal in nsObjectLoadingContent when running mochis. Rather be on the safe side though.
> So far I haven't encountered a nullptr as principal in nsObjectLoadingContent 

Right, because it uses thisContent->NodePrincipal(), which is guaranteed to never ever return null.
Boris, any particular reason you are creating the LoadInfo using 'new' instead of calling 'do_CreateInstance'? It seems that is causing linker problems for compiled code tests. Would you mind if I change to use 'do_CreateInstance' and call a init() right after?
> any particular reason you are creating the LoadInfo using 'new' instead of calling
> 'do_CreateInstance'

Yes, because it's way saner and simpler to read and doesn't obfuscate what's going on.  It's also much faster.  And doesn't require LoadInfo to be an XPCOM object inheriting from nsISupports and all that gunk.

> It seems that is causing linker problems for compiled code tests

These are test not linked into libxul?  Why are they trying to create LoadInfo, exactly?  Are they just calling NS_NewChannel, basically?

> Would you mind if I change to use 'do_CreateInstance'

Yes.  If we just need to export a factory function from libxul for use by these tests, we can do that, but that doesn't involve setting this thing up to be an XPCOM object.
Patrick, for this project we are going to change NS_NewChannel so that callsites either have to provide a RequestingNode or a RequestingPrincipal which then gets set in the LoadInfo and stored on every channel. In my opinion it would be the cleanest solution to have two NS_NewChannel functions which internally call a 'common' function so that we do not have to duplicate code for the actual creation of the channel through the ioService. I don't want that common function to be visible/accessible from anything outside nsNetUtil. As discussed with Jonas, we would preferably only have two functions accessible for counsumers to create a channel, because we are worried that consumers accidentally call the wrong function.

So here is the actual question: Can we move the various functions to create a channel in nsNetUtil.h (NewChannel, NS_OpenURI, NS_NewStreamLoader) to the *.cpp file? I see that the are all declared inline, so I am assuming that is the reason why they are in the *.h file, right? Is there any other reason or would it be feasible to move them to the *.cpp file?
Flags: needinfo?(mcmanus)
Attached patch 1_loadinfo_changes.patch (obsolete) — Splinter Review
Attachment #8465026 - Attachment is obsolete: true
Attachment #8465027 - Attachment is obsolete: true
Attachment #8465028 - Attachment is obsolete: true
Attachment #8465029 - Attachment is obsolete: true
Attached patch 2_nsNetUtil_changes.patch (obsolete) — Splinter Review
Attached patch 3_b_callsites_in_dom.patch (obsolete) — Splinter Review
Attached patch 3_h_callsites_in_intl.patch (obsolete) — Splinter Review
Jonas, 

these patches extend nsNetutil.h to have 3 functions available for creating new channels:
a) takes a reqeusting nsINode (where we then query the principal from that node)
b) takes a principal (in case no node is available; node ends up being a nullptr in the loadinfo)
3) NS_NewChannelInternal in general should only be called from within a) or b), with the following exceptions:
   * nsDocShell.cpp
   * txMozillaStylesheetCompiler.cpp
   * nsXBLService.cpp
   * Loader.cpp
   * nsCrossSiteListenerProxy.cpp
   * nsXMLHttpRequest.cpp
   * EventSource.cpp
   * imgLoader.cpp
where the Node's principal and the requesting principal are not identical. This happens for example in an XML-request where the requesting principal would be the systemPrincipal, but the channel shouldn't get the systemPrincipal. See current codebase:
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1969

Further, we separated callsite updates in different patches (3a-3i). Please keep in mind that 3 files:
  * nsObjectLoadingContent.cpp
  * nsDocShell.cpp
  * imgLoader.cpp
currently use "SetupChannelOwner" which is modified and renamed to "ShouldInheritPrincipal" in patch 4.

Finally, we have to split GetChannelPrincipal into GetChannelResultPrincipal and GetChannelURIPrincipal (patch 5).
http://mxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#322


Jonas, asking your for feedback on the patches, but don't want to flag all of the patches. Just setting the NeedInfo flag.
Flags: needinfo?(jonas)
Regarding commented compiled code tests:

Since we added the following code NS_newChannel in nsNetutil.h:

+ nsCOMPtr<nsILoadInfo> loadInfo =
+   new mozilla::LoadInfo(aRequestingPrincipal,
+                         aRequestingNode,
+                         aInheritPrincipal,
+                         aSandboxed,
+                         aContentPolicyType);
+ channel->SetLoadInfo(loadInfo);

we are facing the following linker error for TestCSPParser.cpp (and also for other compiled code tests):

> 0:04.06 Executing: /usr/bin/ccache clang++ -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -I/home/ckerschb/moz/mc/build/unix/headers -pthread -pipe -DDEBUG -DTRACING -g -fno-omit-frame-pointer -o TestCSPParser /home/ckerschb/moz/mc-obj-dbg/content/base/test/tmpHl1n0G.list -lpthread -Wl,-z,noexecstack -Wl,-z,text -Wl,--build-id -B ../../../build/unix/gold -Wl,-rpath-link,/home/ckerschb/moz/mc-obj-dbg/dist/bin -Wl,-rpath-link,/usr/local/lib -Wl,--whole-archive ../../../dist/lib/libmozglue.a ../../../dist/lib/libmemory.a -Wl,--no-whole-archive -rdynamic ../../../xpcom/glue/libxpcomglue_s.a ../../../memory/mozalloc/libmozalloc.so ../../../nsprpub/lib/ds/libplds4.so ../../../nsprpub/lib/libc/src/libplc4.so ../../../nsprpub/pr/src/libnspr4.so ../../../toolkit/library/libxul.so -ldl
> 0:04.06 /home/ckerschb/moz/mc-obj-dbg/content/base/test/tmpHl1n0G.list:
> 0:04.06     INPUT("TestCSPParser.o")
> 0:04.06 
> 0:04.06 ../../../dist/include/nsNetUtil.h:256: error: undefined reference to 'mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int)'
> 0:04.06 clang: error: linker command failed with exit code 1 (use -v to see invocation)
> 0:04.07 make[2]: *** [TestCSPParser] Error 1
> 0:04.07 make[1]: *** [content/base/test/target] Error 2
> 0:04.07 make[1]: *** Waiting for unfinished jobs....

Surprising, because libxul holds a function with exactly that signature:

> ckerschb@ckerschb-optiplex:~/moz/mc-obj-dbg/dist/bin$ nm -C libxul.so | grep "mozilla::LoadInfo::LoadInfo("
> 00000000043defd0 t mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int)
> 00000000043defd0 t mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int)

Probably the symbols are not exported correctly. I tried to statically link the docshell_base by adding it to USE_LIBS in the moz.build, but that's just the tip of the iceberg, which results in many other linker errors:

> USE_LIBS += [
>+    'docshell_base',
>     'mozalloc',
>     'nspr',
>     'xpcomglue_s',
>     'xul',
> ]

Since other compiled code tests are also commented out, and also that comment here:
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/moz.build#37
make me believe there is not an easy fix for this problem.

Any suggestions are greatly appreciated!
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #56)

> So here is the actual question: Can we move the various functions to create
> a channel in nsNetUtil.h (NewChannel, NS_OpenURI, NS_NewStreamLoader) to the
> *.cpp file? I see that the are all declared inline, so I am assuming that is
> the reason why they are in the *.h file, right? Is there any other reason or
> would it be feasible to move them to the *.cpp file?

I don't know the historical answers to your question.

Looking forward, I'm fine with your suggestion - though the linking in there can be a mess at times, not unlike the string classes. I see you've found that with comment 71.
Flags: needinfo?(mcmanus)
Comment on attachment 8470155 [details] [diff] [review]
1_loadinfo_changes.patch

Review of attachment 8470155 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/LoadInfo.cpp
@@ +70,5 @@
> +LoadInfo::GetLoadingContext(nsISupports** outLoadingContext)
> +{
> +  nsCOMPtr<nsISupports> context = do_QueryReferent(mLoadingContext);
> +  if (context) {
> +    NS_ADDREF(*outLoadingContext = context);

This has the problem that you'll do two addrefs and one release whenever this function is called. That's excessive. And it has the problem that outLoadingContext isn't set to null if the QI fails. A better solution is:

context.forget(outLoadingContext)

Or even better, skip the nsCOMPtr entirely and simply do

CallQueryInterface(mLoadingContext, outLoadingContext)

Or, why have the QI at all? You can always cast to nsISupports:

NS_IF_ADDREF(*outLoadingContext = mLoadingContext);

::: docshell/base/nsILoadInfo.idl
@@ +57,5 @@
>     * use loadingPrincipal for its principal, even when the data is loaded over
>     * http:// or another protocol that would normally use a URI-based principal.
>     * This attribute will never be true when loadingSandboxed is true.
>     */
> +  readonly attribute unsigned long forceInheritPrincipal;

Why this change? Having boolean accessor functions that can get at the individual flags still seems like a good thing, even if we make the constructor take a bitmask in the future.

Generally speaking, I think the old setup resulted in better code. At some point we should switch the constructor from taking multiple arguments to taking a bitmask, but I don't think this moves us closer to that approach.

Even once we take a bitmask it still makes sense to have easy-to-use C++ functions for getting at the individual bits.

The only thing that will change if/when we move to a bitmask is the constructor signature, as well as the internal storage.

@@ +70,5 @@
> +  /**
> +   * The contentPolicyType of the channel, used for security checks
> +   * like MCB, and CSP.
> +   */
> +   readonly attribute nsContentPolicyType contentPolicyType;

Can you mark this as [infallible] to create a cleaner signature for C++ callers?

::: dom/src/jsurl/nsJSProtocolHandler.cpp
@@ +159,5 @@
> +        rv = aChannel->GetLoadInfo(getter_AddRefs(loadInfo));
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        if (loadInfo) {
> +            InheritType inheritPrincipal = nsILoadInfo::eDontInheritPrincipal;
> +            rv = loadInfo->GetForceInheritPrincipal(&inheritPrincipal);

This is a good example of why it's useful to still have a GetForceIneritPrincipal with the old signature.
Comment on attachment 8470159 [details] [diff] [review]
3_c_callsites_in_netwerk.patch

Review of attachment 8470159 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/test/TestPageLoad.cpp
@@ +304,5 @@
> +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +    RETURN_IF_FAILED(rv, rv, "Couldn't get script security manager!");
> +       nsCOMPtr<nsIPrincipal> systemPrincipal;
> +    rv = secman->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
> +    RETURN_IF_FAILED(rv, rv, "Couldn't get system principal!");

Why can't you use nsContentUtils::GetSystemPrincipal here? Doesn't matter much though given that this is just a text file. But there's always a risk that people will copy this pattern into real code.

::: netwerk/test/TestProtocols.cpp
@@ +635,5 @@
> +          do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +           nsCOMPtr<nsIPrincipal> systemPrincipal;
> +        rv = secman->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
> +        NS_ENSURE_SUCCESS(rv, rv);

Same here. Generally speaking I think we can use nsContentUtils::GetSystemPrincipal pretty much everywhere.

::: netwerk/test/moz.build
@@ +41,4 @@
>  #    TestSocketTransport',
>  #    TestStreamChannel',
>  #    TestStreamPump',
> +#    TestStreamLoader',

Why these changes?
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #74)
> ::: netwerk/test/TestPageLoad.cpp
> @@ +304,5 @@
> > +      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> > +    RETURN_IF_FAILED(rv, rv, "Couldn't get script security manager!");
> > +       nsCOMPtr<nsIPrincipal> systemPrincipal;
> > +    rv = secman->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
> > +    RETURN_IF_FAILED(rv, rv, "Couldn't get system principal!");
> 
> Why can't you use nsContentUtils::GetSystemPrincipal here? Doesn't matter
> much though given that this is just a text file. But there's always a risk
> that people will copy this pattern into real code.
> ::: netwerk/test/TestProtocols.cpp
> @@ +635,5 @@
> > +          do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
> > +        NS_ENSURE_SUCCESS(rv, rv);
> > +           nsCOMPtr<nsIPrincipal> systemPrincipal;
> > +        rv = secman->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
> > +        NS_ENSURE_SUCCESS(rv, rv);
> 
> Same here. Generally speaking I think we can use
> nsContentUtils::GetSystemPrincipal pretty much everywhere.

Generally, I use nsContentUtils::GetSystemPrincipal everywhere in the code, besides in compiled code tests, because all compiled code tests get the systemPrincipal from the securityManager. Presumably it has to do with linking problems.

> ::: netwerk/test/moz.build
> @@ +41,4 @@
> >  #    TestSocketTransport',
> >  #    TestStreamChannel',
> >  #    TestStreamPump',
> > +#    TestStreamLoader',
> 
> Why these changes?

Linker problems; see detailed explanation of the problem here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c71
Comment on attachment 8470167 [details] [diff] [review]
4_setupChannelOwner_changes.patch

Review of attachment 8470167 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsContentUtils.cpp
@@ +6499,5 @@
>  }
>  
>  // static
>  bool
> +nsContentUtils::ChannelShouldInheritPrincipal(nsIPrincipal* aLoadingPrincipal,

It sounds weird that this function takes a aForceInherit boolean. I would expect a function called ChannelShouldInheritPrincipal to always set forceInherit.

Also, Boris definitely needs to review the changes here.
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #73)
> ::: docshell/base/LoadInfo.cpp
> NS_IF_ADDREF(*outLoadingContext = mLoadingContext);

Indeed. Initially I tried to make everything work passing an nsINode around instead of using nsISupports. In which case we would have needed to query the referent. Anyway, I updated the part of the code.

> @@ +70,5 @@
> > +  /**
> > +   * The contentPolicyType of the channel, used for security checks
> > +   * like MCB, and CSP.
> > +   */
> > +   readonly attribute nsContentPolicyType contentPolicyType;
> 
> Can you mark this as [infallible] to create a cleaner signature for C++
> callers?

Unfortunately not:

error: [infallible] only works on builtin types (numbers, booleans, and raw char types), ../../../dist/idl/nsILoadInfo.idl line 55:25
 0:07.71    [infallible] readonly attribute nsContentPolicyType contentPolicyType;
(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #76)
> Comment on attachment 8470167 [details] [diff] [review]
> 4_setupChannelOwner_changes.patch
> 
> Review of attachment 8470167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsContentUtils.cpp
> @@ +6499,5 @@
> >  }
> >  
> >  // static
> >  bool
> > +nsContentUtils::ChannelShouldInheritPrincipal(nsIPrincipal* aLoadingPrincipal,
> 
> It sounds weird that this function takes a aForceInherit boolean. I would
> expect a function called ChannelShouldInheritPrincipal to always set
> forceInherit.
> 
> Also, Boris definitely needs to review the changes here.

Indeed, Boris needs to review that part - I didn't want to change too much of his code re-used from 'SetupChannelOwner.
Comment on attachment 8470157 [details] [diff] [review]
3_a_callsites_in_content.patch

* content/base/src/ImportManager.cpp
Question for reviewer - Are mImportParent->MasterDocument()'s ScriptObjectPrincipal and mImportParent->NodePrincipal the same?  If they are, we can call NS_NewChannel with mImportParent as the requestingNode. If they aren't, we need to instead of calling NS_NewChannelInternal and pass both of them. 

* I think we need to use NS_NewChannelInternal() in nsHTMLMediaElement.cpp and nsHTMLTrackElement.cpp

* In nsHTMLDocument.cpp :: Open(), we should use callerDoc and call NS_NewChannel() with a node instead of a principal.

* In nsHTMLDocument.cpp :: CreateAndAddWyciwygChannel(), can we also get a callerDoc with 
nsCOMPtr<nsIDocument> callerDoc = nsContentUtils::GetDocumentFromContext(); 
Is NodePrincipal() really the loadingPrincipal, or is it just the Principal?

* In content/media/MediaResource.cpp, element is the media element itself, not the requestingContext.  We need to ask reviewers what the loadInfo should be here.

* content/xul/document/src/XULDocument.cpp - the XULDocument has an mChannel.  In LoadOverlayInternal we create a new channel.  Can we use the loadInfo from mChannel to populate the new channel?

Also, in XULDocument::LoadScript(), we call nsScriptLoader::ShouldLoadScript() and pass in "this" for the requestingNode.  This calls content policies.  Later we call, NS_NewStreamLoader().  We should set the requestingNode to this instead of using system principal.  Note that we currently can't do this, because NS_NewStreamLoader() takes a principal but not a node.
> Can we move the various functions to create
> a channel in nsNetUtil.h (NewChannel, NS_OpenURI, NS_NewStreamLoader) to the
> *.cpp file? I see that the are all declared inline, so I am assuming that is
> the reason why they are in the *.h file, right? Is there any other reason or
> would it be feasible to move them to the *.cpp file?

Historically everything was in the .h file to avoid external programs (tests, etc) not being able to see the symbols in a .o file (this was back when we enforced IDL-only linkage between modules).  We've got (bitrotted) patches for splitting up nsNetUtil.h into .h/.cpp files in bug 905127 but it got stalled on some linker issues (which may be resolved: we need to unbitrot and see).  For now I suggest you just dump stuff into the .h file and we'll worry about moving large functions into a .cpp file as part of that bug, not this one (you have plenty to do :)
As requested by smaug for review; combining all the patches into one massiv patch. Once we got smaugs review, we can split them again for the individual reviewers.
Attachment #8470155 - Attachment is obsolete: true
Attachment #8470156 - Attachment is obsolete: true
Attachment #8470157 - Attachment is obsolete: true
Attachment #8470158 - Attachment is obsolete: true
Attachment #8470159 - Attachment is obsolete: true
Attachment #8470160 - Attachment is obsolete: true
Attachment #8470161 - Attachment is obsolete: true
Attachment #8470162 - Attachment is obsolete: true
Attachment #8470163 - Attachment is obsolete: true
Attachment #8470165 - Attachment is obsolete: true
Attachment #8470166 - Attachment is obsolete: true
Attachment #8470167 - Attachment is obsolete: true
Attachment #8470169 - Attachment is obsolete: true
Attachment #8472737 - Flags: review?(bugs)
Comment on attachment 8470157 [details] [diff] [review]
3_a_callsites_in_content.patch

Commenting on patch 3_b for callsites in /dom (even though the patch has been consolidated with the other code for now).

* dom/base/Navigator.cpp :: MozIsLocallyAvailable()
Looks like we have a doc here. Why do we use system principal? (Note that I can't find any callers to this function anyway.  It's in an idl, so maybe addons call it.)


* dom/plugins/base/nsPluginHost.cpp
Looks like we use element in the call to content policies and we use the documents principal.  Perhaps we should use NS_NewChannelInternal() with the element and doc->NodePrincipal() instead of NS_NewChannel with a potentially null doc.
(Note in overall revamp code, we have the following comment:
+                      doc); // TODO, doc might be null here!!! 
)

* dom/plugins/base/nsPluginStreamListenerPeer.cpp
We use systemPrincipal here, but perhaps we can get the doc instead by doing something like:
nsRefPtr<nsPluginInstanceOwner> owner = mPluginInstance->GetOwner();
if (owner) {
  nsCOMPtr<nsIDocument> doc;
  nsresult rv = owner->GetDocument(getter_AddRefs(doc));
}

* dom/workers/ScriptLoader.cpp
Are parentDoc's principal and "principal" the same here?  If they are potentially different, we should call NS_NewChannelInternal and pass both.

* dom/xbl/nsXBLService.cpp
Expand the comment a bit
// FetchBindingDocument().  LoadInfo will end up with no principal or node in those cases, so we use systemPrincipal.
// This achieves the same result of bypassing security checks, but it gives the wrong information to potential future consumers of loadInfo.

* dom/xml/XMLDocument.cpp

See this comment: https://mxr.mozilla.org/mozilla-central/source/dom/xml/XMLDocument.cpp#323
principal appears to be the principal of the XMLDocument being loaded.  callingDoc is the loadingContext.  In the call to content policies, we use callingDoc as the context and principal as the requestingPrincipal.  Why?  These changes were made by Jonas in bug 490790 so maybe he knows why.  This may be a mistake where we send the principal instead of the requestingPrincipal.  Moreover, why do we fallback to using this (the currently loading doc) if callingDoc doesn't exist?  Needinfo'ing Jonas just for this question.

* dom/xslt/base/txURIUtils.cpp
Should we use TYPE_XSLT here instead of TYPE_OTHER?
Flags: needinfo?(jonas)
Comment on attachment 8470159 [details] [diff] [review]
3_c_callsites_in_netwerk.patch

Commenting on patch 3_b for callsites in /netwerk

In /netwerk/protocol/http/HttpChannelParent.cpp, we have DoAsyncOpen() that takes a bunch of parameters (https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#157).  The parameters come from HttpChannelOpenArgs, which are set by the HttpChannelChild::AsyncOpen() (https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1235).  Can we use "this" in HttpChannelChild::AsyncOpen to get the loadInfo of the current channel, set that in HttpChannelOpenArgs, and then use it in HttpChannelParent::DoAsyncOpen()'s call to NS_NewChannel instead of passing systemPrincipal?

Similarly, for netwerk/protocol/file/FTPChannelParent.cpp
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Comment on attachment 8470161 [details] [diff] [review]
3_e_callsites_in_layout.patch

diff --git a/layout/mathml/nsMathMLChar.cpp b/layout/mathml/nsMathMLChar.cpp
--- a/layout/mathml/nsMathMLChar.cpp
+++ b/layout/mathml/nsMathMLChar.cpp
@@ -145,18 +146,23 @@ static nsresult
 LoadProperties(const nsString& aName,
                nsCOMPtr<nsIPersistentProperties>& aProperties)
 {
   nsAutoString uriStr;
   uriStr.AssignLiteral("resource://gre/res/fonts/mathfont");
   uriStr.Append(aName);
   uriStr.StripWhitespace(); // that may come from aName
   uriStr.AppendLiteral(".properties");
-  return NS_LoadPersistentPropertiesFromURISpec(getter_AddRefs(aProperties), 
-                                                NS_ConvertUTF16toUTF8(uriStr));
+  return NS_LoadPersistentPropertiesFromURISpec(NS_ConvertUTF16toUTF8(uriStr),
+                                                nullptr,   // aCharset
+                                                nullptr,   // aBaseURI
+                                                nullptr,   // aIoService
+                                                nsContentUtils::GetSystemPrincipal(),
+                                                nsIContentPolicy::TYPE_OTHER,
+                                                getter_AddRefs(aProperties))

Would this be considered TYPE_FONT?



In layout/style/Loader.cpp, content policies are called with CheckLoadAllowed(), which takes a requestingPrincipal and requestingContext.  NS_NewChannel() is called by LoadSheet().  Callers of LoadSheet() (like LoadStyleLink(), LoadChildSheet(), and InternalLoadNonDocumentSheet()) call CheckLoadAllowed() and then call LoadSheet().

When we call NS_NewChannel(), we set mDocument to be the requestingNode.  But calls to CheckLoadAllowed() try to set the requestingContext to something else, and then fallback to the document (examples in LoadStyleLink(), LoadChildSheet()).  Hence, we should add a third parameter to LoadSheet() that includes the requestingContext, and then use that in the call to NS_Newchannel.  Alternatively, we could add the requestingContext to the first parameter of LoadSheet() - SheetLoadData (https://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#106).
Comment on attachment 8470161 [details] [diff] [review]
3_e_callsites_in_layout.patch

diff --git a/layout/style/Loader.cpp b/layout/style/Loader.cpp
--- a/layout/style/Loader.cpp
+++ b/layout/style/Loader.cpp
@@ -1420,19 +1420,28 @@ Loader::LoadSheet(SheetLoadData* aLoadDa
       mozilla::net::PredictorLearn(aLoadData->mURI, mDocument->GetDocumentURI(),
                                    nsINetworkPredictor::LEARN_LOAD_SUBRESOURCE,
                                    mDocument);
     }
 
     // Just load it
     nsCOMPtr<nsIInputStream> stream;
     nsCOMPtr<nsIChannel> channel;
-    rv = NS_OpenURI(getter_AddRefs(stream), aLoadData->mURI, nullptr,
-                    nullptr, nullptr, nsIRequest::LOAD_NORMAL,
-                    getter_AddRefs(channel));
+    rv = NS_OpenURI(aLoadData->mURI,
+                    nullptr,   // aIoService
+                    nullptr,   // aLoadGroup
+                    nullptr,   // aCallbacks
+                    nsIRequest::LOAD_NORMAL,
+                    nsContentUtils::GetSystemPrincipal(),
+                    nsILoadInfo::eDontInheritPrincipal,
+                    nsILoadInfo::eNotSandboxed,
+                    nsIContentPolicy::TYPE_OTHER,
+                    getter_AddRefs(channel),        // outChannel
+                    getter_AddRefs(stream));
+

For NS_OpenURI, we should use aLoadData->mLoaderPrincipal instead of SystemPrincipal and we should also pass in the requestingContext from the LoadSheet() parameters (see the previous comment).  This means we need an "NS_OpenURIInternal()" that takes both a node and a principal.  We couldn't use an "NS_OpenURINode()" because the NodePrincipal and aLoadData->mLoaderPrincipal may be different (ex: one may be the principal of the stylesheet while the other is the principal of the document, but I'm a little fuzzy on how this works).
Comment on attachment 8470161 [details] [diff] [review]
3_e_callsites_in_layout.patch

For layout/style/nsFontFaceLoader.cpp, it looks like the csp for channelPolicy is taken off aProxy->mPrincipal and aFontToLoad->mPrincipal, respectively.  Should we pass these in as requestingPrincipals?
Comment on attachment 8470162 [details] [diff] [review]
3_f_callsites_in_widget.patch


widget/windows/WinUtils.cpp :: AsyncFaviconDataReady()::OnFaviconDataNotAvailable(void)
We are calling NS_NewChannel() with TYPE_OTHER and systemPrincipal.  Assuming this is using a generic favicon and no network request is being made, systemPrincipal is okay, but we should change to TYPE_IMAGE.

widget/windows/nsDataObj.cpp
Not sure what this code does.  The reviewer for the /widget callsites will have to look carefully at this one to check its use of systemPrincipal.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #71)
> > 0:04.06 ../../../dist/include/nsNetUtil.h:256: error: undefined reference to 'mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int)'
> > 0:04.06 clang: error: linker command failed with exit code 1 (use -v to see invocation)
> > 0:04.07 make[2]: *** [TestCSPParser] Error 1
> > 0:04.07 make[1]: *** [content/base/test/target] Error 2
> > 0:04.07 make[1]: *** Waiting for unfinished jobs....

Getting help from people in #build, it seems we have three options to solve the linker problem here:
a) We convert the 5 failing tests (TestOpen, TestPageLoad, TestProtocols, TestStreamLoader, TestCSPParser) to gtests, which seems like a lot of unecessary work at the moment, see: https://developer.mozilla.org/en-US/docs/GTest
b) We export the symbols for LoadInfo, which no one is really in favor of.
c) We call do_CreateInstance in NSNetutil.h if MOZILLA_INTERNAL_API is not defined, so that production code creates a LoadInfo calling 'new', and compiled code tests create the Loadinfo through do_createInstance.

I incorporated changes for (c), which seems the best option for me at the moment. If we can come up with a better solution, I am happy to incorporate those changes.
b) sounds better than c) to me. But I'm fine with leaving this up to bz.
Comment on attachment 8472737 [details] [diff] [review]
bug_1038756_combined_patch_for_smaug.patch

>@@ -194,17 +194,23 @@ interface nsIScriptSecurityManager : nsI
>     void checkSameOriginURI(in nsIURI aSourceURI,
>                             in nsIURI aTargetURI,
>                             in boolean reportError);
>     /**
>      * Get the principal for the given channel.  This will typically be the
>      * channel owner if there is one, and the codebase principal for the.
>      * channel's URI otherwise.  aChannel must not be null.
>      */
>-    nsIPrincipal getChannelPrincipal(in nsIChannel aChannel);
>+    nsIPrincipal getChannelResultPrincipal(in nsIChannel aChannel);
>+
>+    /**
>+     * Get the codebase principal for the channel's URI.
>+     * aChannel must not be null.
>+     */
>+    nsIPrincipal getChannelURIPrincipal(in nsIChannel aChannel);

This naming could be better. Otherwise getChannelURIPrincipal will be used when getChannelResultPrincipal should be.
Because of loadcontext usage, it isn't really principal from the URI, but principal from the
channel + uri.

I think I would keep getChannelPrincipal as such, and then getCodebasePrincipalFromChannelURI() (that is long and terrible enough name for people to avoid it ;) )
Or something like that. I know jonas suggested the naming in the patch, but IMO it is just a bit too error prone.
Could be because of my non-native-English.


still looking at the rest...
Or perhaps getChannelResultPrincipal is ok too.
Doesn't "getChannelURIPrincipal" indicate that the principal is the result of both URI and channel? Both parts are right there in the name.

The goal is to eventually make getChannelURIPrincipal return a system principal whenever a chrome URI is loaded, or whenever about:config is loaded. So I'd like to avoid getCodebasePrincipalFromChannelURI.
Flags: needinfo?(jonas)
Comment on attachment 8472737 [details] [diff] [review]
bug_1038756_combined_patch_for_smaug.patch

>@@ -2005,22 +2005,20 @@ public:
>    * the principal no matter what, as long as the principal is not null.
>    *
>    * If aIsSandboxed is true, then aLoadingPrincipal must not be null.  In this
>    * case, the owner on the channel, if any, will be reset to null and the
>    * nsILoadInfo will say the channel should be sandboxed.
>    *
>    * The return value is whether the channel was told to inherit the principal.
>    */
>-  static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal,
>-                                nsIChannel* aChannel,
>-                                nsIURI* aURI,
>-                                bool aInheritForAboutBlank,
>-                                bool aIsSandboxed,
>-                                bool aForceInherit);
>+  static bool ChannelShouldInheritPrincipal(nsIPrincipal* aLoadingPrincipal,
>+                                            nsIURI* aURI,
>+                                            bool aInheritForAboutBlank,
>+                                            bool aForceInherit);
I'm missing something here. The new name talks about Channel, yet there is no Channel around.
And the comment for the method still talks about aChannel.



>+typedef unsigned long InheritType;
>+typedef unsigned long SandboxType;
Could you please not do this.
The exiting enums are there to force right usage which improves
code readability . At least in C++ when LoadInfo is create we should pass enum values.


>   /**
>+   * The loadingContext of that channel, used for security checks
>+   * like MCB, and CSP.
>+   * Warning: The loadingContext can be null!
>+   */
>+  readonly attribute nsISupports loadingContext;
This needs better documentation (to prevent the mess we have with nsIContentPolicy)
But actually, is this ever something else than a node? If not, why not use nsIDOMNode here?



>+++ b/dom/base/Navigator.cpp
>@@ -957,18 +957,28 @@ Navigator::MozIsLocallyAvailable(const n
> 
>   nsCOMPtr<nsILoadGroup> loadGroup;
>   nsCOMPtr<nsIDocument> doc = mWindow->GetDoc();
>   if (doc) {
>     loadGroup = doc->GetDocumentLoadGroup();
>   }
> 
>   nsCOMPtr<nsIChannel> channel;
>-  rv = NS_NewChannel(getter_AddRefs(channel), uri,
>-                     nullptr, loadGroup, nullptr, loadFlags);
>+  rv = NS_NewChannel(uri,
>+                     nullptr,   // aIoService
>+                     loadGroup,
>+                     nullptr,   // aCallbacks
>+                     loadFlags,
>+                     nullptr,   // aChannelPolicy
>+                     nsContentUtils::GetSystemPrincipal(),
Why SystemPrincipal?

>+++ b/dom/plugins/base/nsPluginStreamListenerPeer.cpp
>@@ -637,17 +637,28 @@ nsPluginStreamListenerPeer::RequestRead(
>   if (numRequests == 0)
>     return NS_ERROR_FAILURE;
> 
>   nsresult rv = NS_OK;
> 
>   nsCOMPtr<nsIInterfaceRequestor> callbacks = do_QueryReferent(mWeakPtrChannelCallbacks);
>   nsCOMPtr<nsILoadGroup> loadGroup = do_QueryReferent(mWeakPtrChannelLoadGroup);
>   nsCOMPtr<nsIChannel> channel;
>-  rv = NS_NewChannel(getter_AddRefs(channel), mURL, nullptr, loadGroup, callbacks);
>+  rv = NS_NewChannel(mURL,
>+                     nullptr,   // aIoService
>+                     loadGroup,
>+                     callbacks,
>+                     nsIRequest::LOAD_NORMAL,
>+                     nullptr,   // aChannelPolicy
>+                     nsContentUtils::GetSystemPrincipal(),
Really?



>+/*
>+ * How to create a new Channel using NS_NewChannel:
>+ *  1) Please try to call NS_NewChannel providing a requesting *nsINode*
>+ *  2) If no requesting nsINode is available,
>+ *     call NS_NewChannel providing a requesting *nsIPrincipal*.
>+ *  3) Call NS_NewChannelInternal *only* if requesting Principal and
>+ *     the Node's Principal have to be different.
>+ *     >> Most likely this is not the case! <<
>+ *     Needs special approval! NS_NewChannelInternal should
>+ *     only be called from within nsNetutil.h.
Yet you call NS_NewChannelInternal in many other place. Rather odd.
Attachment #8472737 - Flags: review?(bugs) → feedback-
(In reply to Jonas Sicking (:sicking) from comment #92)
> Doesn't "getChannelURIPrincipal" indicate that the principal is the result
> of both URI and channel? Both parts are right there in the name.
The main issue is that at least to me getChannelURIPrincipal vs getChannelResultPrincipal
doesn't tell clearly enough what the difference is.
What is ChannelURI and what is ChannelResult. Or one can read that 
Channel's URI's Principal vs Channel's Result's Principal etc.
So I'm worried that wrong principal will be used accidentally, if the method names aren't clear enough.
I'm definitely open for other suggestions. The two functions that we have are
* One function returns the principal that the "server" returning a response has. This is only affected by
  the URI that we're loading from as well as the "data jar" (i.e. app-id etc).
* One function returns the principal that we should use for the resource returned by the channel. So it's
  affected by things like sandbox flags, "force inherit" flags (set when doing XHR CORS loads) and probably
  other types of context in the future.

I definitely agree that we should have clear names. The current ones were the best I could think of.
(In reply to Tanvi Vyas [:tanvi] from comment #79)
> Comment on attachment 8470157 [details] [diff] [review]
> * I think we need to use NS_NewChannelInternal() in nsHTMLMediaElement.cpp
> and nsHTMLTrackElement.cpp

Yes, that makes sense, incorporated that change.
 
> * In nsHTMLDocument.cpp :: Open(), we should use callerDoc and call
> NS_NewChannel() with a node instead of a principal.

I agree here as well - done.
 
I leave the other questions from Comment 79 for reviewers.
(In reply to Tanvi Vyas [:tanvi] from comment #82)
> Commenting on patch 3_b for callsites in /dom (even though the patch has
> been consolidated with the other code for now).
> 
> * dom/base/Navigator.cpp :: MozIsLocallyAvailable()
> Looks like we have a doc here. Why do we use system principal? (Note that I
> can't find any callers to this function anyway.  It's in an idl, so maybe
> addons call it.)
 
Agreed, should be the doc.
 
> * dom/plugins/base/nsPluginHost.cpp
> Looks like we use element in the call to content policies and we use the
> documents principal.  Perhaps we should use NS_NewChannelInternal() with the
> element and doc->NodePrincipal() instead of NS_NewChannel with a potentially
> null doc.
> (Note in overall revamp code, we have the following comment:
> +                      doc); // TODO, doc might be null here!!! 
> )

Yes, we can use doc->NodePrincipal, but if there is no doc, we should use a nullPrincipal as the principal, otherwise we hit assertions because every loadInfo needs a principal. I incorporated that change, makes sense to me.

 
> * dom/plugins/base/nsPluginStreamListenerPeer.cpp
> We use systemPrincipal here, but perhaps we can get the doc instead by doing
> something like:
> nsRefPtr<nsPluginInstanceOwner> owner = mPluginInstance->GetOwner();
> if (owner) {
>   nsCOMPtr<nsIDocument> doc;
>   nsresult rv = owner->GetDocument(getter_AddRefs(doc));
> }

Leave that for reviewers - probably that is a good idea.
 
> * dom/workers/ScriptLoader.cpp
> Are parentDoc's principal and "principal" the same here?  If they are
> potentially different, we should call NS_NewChannelInternal and pass both.

I think it makes sense to call NS_NewChannelInternal here. I did that.

Leaving the rest of your comments/questions for individual reviewers.
(In reply to Tanvi Vyas [:tanvi] from comment #84)
> Comment on attachment 8470161 [details] [diff] [review]
> 3_e_callsites_in_layout.patch
> NS_LoadPersistentPropertiesFromURISpec(NS_ConvertUTF16toUTF8(uriStr),
> +                                                nullptr,   // aCharset
> +                                                nullptr,   // aBaseURI
> +                                                nullptr,   // aIoService
> +                                               
> nsContentUtils::GetSystemPrincipal(),
> +                                               
> nsIContentPolicy::TYPE_OTHER,
>
> Would this be considered TYPE_FONT?

Sounds right to me, incorporated that.
 
> In layout/style/Loader.cpp, content policies are called with
> CheckLoadAllowed(), which takes a requestingPrincipal and requestingContext.
> NS_NewChannel() is called by LoadSheet().  Callers of LoadSheet() (like
> LoadStyleLink(), LoadChildSheet(), and InternalLoadNonDocumentSheet()) call
> CheckLoadAllowed() and then call LoadSheet().
> 
> When we call NS_NewChannel(), we set mDocument to be the requestingNode. 
> But calls to CheckLoadAllowed() try to set the requestingContext to
> something else, and then fallback to the document (examples in
> LoadStyleLink(), LoadChildSheet()).  Hence, we should add a third parameter
> to LoadSheet() that includes the requestingContext, and then use that in the
> call to NS_Newchannel.  Alternatively, we could add the requestingContext to
> the first parameter of LoadSheet() - SheetLoadData
> (https://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#106).

I will leave this one up for reviewers, but when browsing over that code in layout/style/Loader.cpp, I think that the call to NS_OpenURI should use the same principal as NS_newChannel. incorporated that change there.
(In reply to Tanvi Vyas [:tanvi] from comment #85)
> Comment on attachment 8470161 [details] [diff] [review]
> 3_e_callsites_in_layout.patch
> 
> diff --git a/layout/style/Loader.cpp b/layout/style/Loader.cpp
> --- a/layout/style/Loader.cpp
> +++ b/layout/style/Loader.cpp
> @@ -1420,19 +1420,28 @@ Loader::LoadSheet(SheetLoadData* aLoadDa
>        mozilla::net::PredictorLearn(aLoadData->mURI,
> mDocument->GetDocumentURI(),
>                                    
> nsINetworkPredictor::LEARN_LOAD_SUBRESOURCE,
>                                     mDocument);
>      }
>  
>      // Just load it
>      nsCOMPtr<nsIInputStream> stream;
>      nsCOMPtr<nsIChannel> channel;
> -    rv = NS_OpenURI(getter_AddRefs(stream), aLoadData->mURI, nullptr,
> -                    nullptr, nullptr, nsIRequest::LOAD_NORMAL,
> -                    getter_AddRefs(channel));
> +    rv = NS_OpenURI(aLoadData->mURI,
> +                    nullptr,   // aIoService
> +                    nullptr,   // aLoadGroup
> +                    nullptr,   // aCallbacks
> +                    nsIRequest::LOAD_NORMAL,
> +                    nsContentUtils::GetSystemPrincipal(),
> +                    nsILoadInfo::eDontInheritPrincipal,
> +                    nsILoadInfo::eNotSandboxed,
> +                    nsIContentPolicy::TYPE_OTHER,
> +                    getter_AddRefs(channel),        // outChannel
> +                    getter_AddRefs(stream));
> +
> 
> For NS_OpenURI, we should use aLoadData->mLoaderPrincipal instead of
> SystemPrincipal and we should also pass in the requestingContext from the
> LoadSheet() parameters (see the previous comment).  This means we need an
> "NS_OpenURIInternal()" that takes both a node and a principal.  We couldn't
> use an "NS_OpenURINode()" because the NodePrincipal and
> aLoadData->mLoaderPrincipal may be different (ex: one may be the principal
> of the stylesheet while the other is the principal of the document, but I'm
> a little fuzzy on how this works).

Oh, here we go, haven't read that comment yet. But just incorporated that change. At least we found the same problem with that part of the code.
(In reply to Tanvi Vyas [:tanvi] from comment #86)
> Comment on attachment 8470161 [details] [diff] [review]
> 3_e_callsites_in_layout.patch
> 
> For layout/style/nsFontFaceLoader.cpp, it looks like the csp for
> channelPolicy is taken off aProxy->mPrincipal and aFontToLoad->mPrincipal,
> respectively.  Should we pass these in as requestingPrincipals?

I think we should keep using ps->GetDocument(), which is what NS_CheckContentLoadPolicy uses as the context.
(In reply to Tanvi Vyas [:tanvi] from comment #87)
> widget/windows/WinUtils.cpp ::
> AsyncFaviconDataReady()::OnFaviconDataNotAvailable(void)
> We are calling NS_NewChannel() with TYPE_OTHER and systemPrincipal. 
> Assuming this is using a generic favicon and no network request is being
> made, systemPrincipal is okay, but we should change to TYPE_IMAGE.

I leave the decision about the systemPrincipal do the reviewers, but TYPE_IMAGE sounds right to me.
 
> widget/windows/nsDataObj.cpp
> Not sure what this code does.  The reviewer for the /widget callsites will
> have to look carefully at this one to check its use of systemPrincipal.

Indeed.
(In reply to Olli Pettay [:smaug] from comment #93)
> Comment on attachment 8472737 [details] [diff] [review]
> bug_1038756_combined_patch_for_smaug.patch
> 
> >@@ -2005,22 +2005,20 @@ public:
> >    * the principal no matter what, as long as the principal is not null.
> >    *
> >    * If aIsSandboxed is true, then aLoadingPrincipal must not be null.  In this
> >    * case, the owner on the channel, if any, will be reset to null and the
> >    * nsILoadInfo will say the channel should be sandboxed.
> >    *
> >    * The return value is whether the channel was told to inherit the principal.
> >    */
> >-  static bool SetUpChannelOwner(nsIPrincipal* aLoadingPrincipal,
> >-                                nsIChannel* aChannel,
> >-                                nsIURI* aURI,
> >-                                bool aInheritForAboutBlank,
> >-                                bool aIsSandboxed,
> >-                                bool aForceInherit);
> >+  static bool ChannelShouldInheritPrincipal(nsIPrincipal* aLoadingPrincipal,
> >+                                            nsIURI* aURI,
> >+                                            bool aInheritForAboutBlank,
> >+                                            bool aForceInherit);
> I'm missing something here. The new name talks about Channel, yet there is
> no Channel around.
> And the comment for the method still talks about aChannel.

We updated that comment to be more explicit about what this function is for.

> >+typedef unsigned long InheritType;
> >+typedef unsigned long SandboxType;
> Could you please not do this.
> The exiting enums are there to force right usage which improves
> code readability . At least in C++ when LoadInfo is create we should pass
> enum values.

There is a comment right above which mentions that in bug 1041176 we collapse the flags into one unsigned long and use individual bits (similar to loadFlags). Bug 1041176 will land at the same time.
 
> 
> >   /**
> >+   * The loadingContext of that channel, used for security checks
> >+   * like MCB, and CSP.
> >+   * Warning: The loadingContext can be null!
> >+   */
> >+  readonly attribute nsISupports loadingContext;
> This needs better documentation (to prevent the mess we have with
> nsIContentPolicy)
> But actually, is this ever something else than a node? If not, why not use
> nsIDOMNode here?

Initially I wanted to use nsINode, but that's doesn't work because there is no *.idl for nsINode, hence we fall back to nsISupports which I didn't really like because it's too generic. Using nsIDOMNode is the better solution. I incorporated that change. Exactly what I wanted it to be. We also extended the comment on top.

> >+/*
> >+ * How to create a new Channel using NS_NewChannel:
> >+ *  1) Please try to call NS_NewChannel providing a requesting *nsINode*
> >+ *  2) If no requesting nsINode is available,
> >+ *     call NS_NewChannel providing a requesting *nsIPrincipal*.
> >+ *  3) Call NS_NewChannelInternal *only* if requesting Principal and
> >+ *     the Node's Principal have to be different.
> >+ *     >> Most likely this is not the case! <<
> >+ *     Needs special approval! NS_NewChannelInternal should
> >+ *     only be called from within nsNetutil.h.
> Yet you call NS_NewChannelInternal in many other place. Rather odd.

We expanded that comment, but in general it means: Everyone who is going to create a channel after that initial patch landed shouldn't use NSNewChannelInternal but rather NS_NewChannel providing either a node or a principal.
> error: [infallible] only works on builtin types (numbers, booleans, and raw char types), ../../../dist/idl/nsILoadInfo.idl line 55:25

Seems like we should consider fixing that.

Or failing that manually adding a %C++ block that does the right thing.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #88)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #71)
> > > 0:04.06 ../../../dist/include/nsNetUtil.h:256: error: undefined reference to 'mozilla::LoadInfo::LoadInfo(nsIPrincipal*, nsISupports*, unsigned int, unsigned int, unsigned int)'
> > > 0:04.06 clang: error: linker command failed with exit code 1 (use -v to see invocation)
> > > 0:04.07 make[2]: *** [TestCSPParser] Error 1
> > > 0:04.07 make[1]: *** [content/base/test/target] Error 2
> > > 0:04.07 make[1]: *** Waiting for unfinished jobs....
> 
> Getting help from people in #build, it seems we have three options to solve
> the linker problem here:
> a) We convert the 5 failing tests (TestOpen, TestPageLoad, TestProtocols,
> TestStreamLoader, TestCSPParser) to gtests, which seems like a lot of
> unecessary work at the moment, see:
> https://developer.mozilla.org/en-US/docs/GTest
> b) We export the symbols for LoadInfo, which no one is really in favor of.
> c) We call do_CreateInstance in NSNetutil.h if MOZILLA_INTERNAL_API is not
> defined, so that production code creates a LoadInfo calling 'new', and
> compiled code tests create the Loadinfo through do_createInstance.
> 
> I incorporated changes for (c), which seems the best option for me at the
> moment. If we can come up with a better solution, I am happy to incorporate
> those changes.

Apparently Jonas (Comment 89) is more in favor of b). Boris, do you feel the same way? If so, can you provide some guidance how I can export those symbols so that compiled code tests link correctly? (I described the exact problem in detail Comment 71).
Flags: needinfo?(bzbarsky)
I somewhat prefer (b) as well.  

You can use MOZ_EXPORT to export things.
Flags: needinfo?(bzbarsky)
Summary for all reviewers:
Channels know very little about who initiated the request for the data they are fetching.  But there are use cases where we need some loading information during the lifetime of the channel.  Hence, in bug 965413, bz added a LoadInfo object that hangs off channels.  This bug is to populate that LoadInfo object at channel creation time (via NS_NewChannel and friends).  In this code, we pass a loadingPrincipal and/or a loadingNode to NS_NewChannel and NS_NewChannel then creates the loadInfo object with the passed in values.

Please take a careful look at the code that we have flagged you for.  In particular, look to see if we have identified the correct loading node and/or loading principal.  We are not looking for the node and principal corresponding to the data that the channel is fetching, but the node and principal of the entity that is loading that data.  These values are crucial to security checking code that is currently being reimplemented.  If we pass the wrong values, we may end up allowing resource loads that should be blocked by the browser.  If we use systemPrincipal, that will mean that all loads will be permitted. So please check that the load is coming from the system and that any needed security checks have already been done.
Attached patch 1_loadinfo_changes.patch (obsolete) — Splinter Review
Attachment #8472737 - Attachment is obsolete: true
Attachment #8478590 - Flags: review?(bzbarsky)
Attached patch 2_nsNetutil_changes.patch (obsolete) — Splinter Review
Since we are going to change the argument list for NS_NewChannel, NS_OpenURI and NS_NewStreamLoader I think it's also good timing to polish the code within those function bodies a little bit.
Attachment #8478591 - Flags: review?(mcmanus)
In nsHTMLDocument.cpp :: CreateAndAddWyciwygChannel(), can we get a callerDoc with nsCOMPtr<nsIDocument> callerDoc = nsContentUtils::GetDocumentFromContext(); 
Also, is NodePrincipal() really the loadingPrincipal, or is it just the Principal?
Attachment #8478592 - Flags: review?(jst)
Are mImportParent->MasterDocument()'s ScriptObjectPrincipal and mImportParent->NodePrincipal the same?  If they are, we can call NS_NewChannel with mImportParent as the requestingNode. If they aren't, we need to instead of calling NS_NewChannelInternal and pass both of them.
Attachment #8478594 - Flags: review?(gkrizsanits)
content/xul/document/src/XULDocument.cpp - the XULDocument has an mChannel.  In LoadOverlayInternal we create a new channel.  Can we use the loadInfo from mChannel to populate the new channel?
Attachment #8478595 - Flags: review?(bzbarsky)
In content/media/MediaResource.cpp, element is the media element itself, not the requestingContext.  What should the loadInfo should be here - do we have the document that initiated the media load and the loading principal?
Attachment #8478596 - Flags: review?(roc)
dom/plugins/base/nsPluginStreamListenerPeer.cpp - We try and get the loading document here by doign the following.  Is that correct?  Does this give us the loading doc?
nsRefPtr<nsPluginInstanceOwner> owner = mPluginInstance->GetOwner();
if (owner) {
  nsCOMPtr<nsIDocument> doc;
  nsresult rv = owner->GetDocument(getter_AddRefs(doc));
}

** dom/xslt/base/txURIUtils.cpp
Should we use TYPE_XSLT here instead of TYPE_OTHER?
Attachment #8478597 - Flags: review?(jst)
Attachment #8478599 - Flags: review?(bent.mozilla)
Attached patch 3_07_callsites_in_netwerk.patch (obsolete) — Splinter Review
Wait to request review on this one until we have the parent/child httpchannel args code done.

We use #undef METER because otherwise we get compile errors in nsIncrementalDownload, having an 'already defined error' of METER, because it's defined in nsBufferedStreams.cpp as well as HashTable.h, where Hashtable undefs it, at the end, see:
http://mxr.mozilla.org/mozilla-central/source/js/public/HashTable.h#1681
We should also undef it in nsBufferedStreams.cpp.
Attachment #8478600 - Flags: review?(mcmanus)
Attachment #8478601 - Flags: review?(mcmanus)
Attached patch 3_09_callsites_in_docshell.patch (obsolete) — Splinter Review
Attachment #8478602 - Flags: review?(bzbarsky)
Attachment #8478603 - Flags: review?(roc)
**In layout/style/Loader.cpp, content policies are called with CheckLoadAllowed(), which takes a requestingPrincipal and requestingContext.  NS_NewChannel() is called by LoadSheet().  Callers of LoadSheet() (like LoadStyleLink(), LoadChildSheet(), and InternalLoadNonDocumentSheet()) call CheckLoadAllowed() and then call LoadSheet().

When we call NS_NewChannel(), we set mDocument to be the requestingNode.  But calls to CheckLoadAllowed() try to set the requestingContext to something else, and then fallback to the document (examples in LoadStyleLink(), LoadChildSheet()).  Hence, can we add a third parameter to LoadSheet() that includes the requestingContext, and then use that in the call to NS_NewChannel?  Alternatively, could we add the requestingContext to the first parameter of LoadSheet() - SheetLoadData (https://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#106) ?

**For layout/style/nsFontFaceLoader.cpp, it looks like the csp for channelPolicy is taken off aProxy->mPrincipal and aFontToLoad->mPrincipal, respectively.  Should we pass these in as requestingPrincipals? Or should we use ps->GetDocument(), which is passed into content policies, but in a completely different part of the code.  Not sure how we go from checking the policies in CheckLoadAllowed() to StartLoad() where NS_NewChannel() is called.  Is ps->GetDocument()'s principal the same as mPrincipal?
Attachment #8478605 - Flags: review?(dbaron)
Attachment #8478607 - Flags: review?(smichaud)
** widget/windows/WinUtils.cpp :: AsyncFaviconDataReady()::OnFaviconDataNotAvailable(void) - We are calling NS_NewChannel() with systemPrincipal.  Assuming this is using a generic favicon and no network request is being made, systemPrincipal is okay.  Can you confirm this?  If a network request is being made, we need to find the real loading principal.

** widget/windows/nsDataObj.cpp
Not sure what this code does.  Please look carefully at this one to check its use of systemPrincipal.  Can we identify the loader principal, or is this loaded by system?
Attachment #8478608 - Flags: review?(jmathies)
Attachment #8478613 - Flags: review?(roc)
Attachment #8478620 - Flags: review?(paolo.mozmail)
Attached patch 3_18_callsites_in_intl.patch (obsolete) — Splinter Review
*  Does passing systemPrincipal as the loadingPrincipal make sense for these files?  Are the channels created by system operations?  For intl/strres/nsStringBundleTextOverride.cpp and intl/hyphenation/hnjstdio.cpp its probably okay.  But for /intl/strres/nsStringBundle.cpp it's less clear.
Attachment #8478621 - Flags: review?(jshin1987)
Attached patch 3_19_callsites_in_xpfe.patch (obsolete) — Splinter Review
Attachment #8478622 - Flags: review?(timeless)
Attached patch 3_20_callsites_in_xpcom.patch (obsolete) — Splinter Review
Attachment #8478623 - Flags: review?(benjamin)
Attachment #8478624 - Flags: review?(honzab.moz)
Attached patch 3_22_callsites_in_rdf.patch (obsolete) — Splinter Review
Attachment #8478625 - Flags: review?(l10n)
Attached patch 3_23_callsites_in_parser.patch (obsolete) — Splinter Review
Attachment #8478626 - Flags: review?(mrbkap)
Attachment #8478605 - Flags: review?(dbaron) → review?(bzbarsky)
Attached patch 3_24_callsites_in_modules.patch (obsolete) — Splinter Review
Attachment #8478627 - Flags: review?(mwu)
Attached patch 3_25_callsites_in_js.patch (obsolete) — Splinter Review
Attached patch 3_27_callsites_in_gfx.patch (obsolete) — Splinter Review
Attachment #8478633 - Flags: review?(roc)
Attachment #8478634 - Flags: review?(ehsan)
* What does nsWebBrowserPersist::SaveURIInternal() do?  Is there a way to get the loading Document here so we don't use systemPrincipal?  Or is this really a system operation?
Attachment #8478635 - Flags: review?(bzbarsky)
Attached patch 3_30_callsites_in_editor.patch (obsolete) — Splinter Review
Attachment #8478636 - Flags: review?(ehsan)
Attachment #8478637 - Flags: review?(bzbarsky)
Attachment #8478638 - Flags: review?(bzbarsky)
Attachment #8478628 - Flags: review?(mrbkap)
Attachment #8478632 - Flags: review?(seth)
Attachment #8478634 - Flags: review?(ehsan) → review+
Comment on attachment 8478636 [details] [diff] [review]
3_30_callsites_in_editor.patch

Review of attachment 8478636 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/nsHTMLDataTransfer.cpp
@@ +1091,5 @@
> +                      nsILoadInfo::eDontInheritPrincipal,
> +                      nsILoadInfo::eNotSandboxed,
> +                      nsIContentPolicy::TYPE_OTHER,
> +                      nullptr,   // outChannel
> +                      getter_AddRefs(imageStream));  

Nit: please remove the trailing whitespace.
Attachment #8478636 - Flags: review?(ehsan) → review+
Reposting this for reviewers who may not have seen it - 

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #106)
> Summary for all reviewers:
> Channels know very little about who initiated the request for the data they
> are fetching.  But there are use cases where we need some loading
> information during the lifetime of the channel.  Hence, in bug 965413, bz
> added a LoadInfo object that hangs off channels.  This bug is to populate
> that LoadInfo object at channel creation time (via NS_NewChannel and
> friends).  In this code, we pass a loadingPrincipal and/or a loadingNode to
> NS_NewChannel and NS_NewChannel then creates the loadInfo object with the
> passed in values.
> 
> Please take a careful look at the code that we have flagged you for.  In
> particular, look to see if we have identified the correct loading node
> and/or loading principal.  We are not looking for the node and principal
> corresponding to the data that the channel is fetching, but the node and
> principal of the entity that is loading that data.  These values are crucial
> to security checking code that is currently being reimplemented.  If we pass
> the wrong values, we may end up allowing resource loads that should be
> blocked by the browser.  If we use systemPrincipal, that will mean that all
> loads will be permitted. So please check that the load is coming from the
> system and that any needed security checks have already been done.
Comment on attachment 8478607 [details] [diff] [review]
3_12_callsites_in_widget_cocoa.patch

I've read comment #106, and I *think* I understand it.  Which leads me to ask the following question:

nsISound.play(in nsIURL aURL) is scriptable, so it can be called from user-level code, on an arbitrary URL.  In that case, is it correct to always pass the system principal to NS_NewStreamLoader(), as this patch does?
Comment on attachment 8478596 [details] [diff] [review]
3_04_callsites_in_content_media.patch

Review of attachment 8478596 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaResource.cpp
@@ +939,5 @@
> +                              nullptr,   // aChannelPolicy
> +                              element,
> +                              nsILoadInfo::eDontInheritPrincipal,
> +                              nsILoadInfo::eNotSandboxed,
> +                              nsIContentPolicy::TYPE_OTHER,

Shouldn't this be TYPE_MEDIA?

@@ +1448,5 @@
> +                  nullptr,   // aChannelPolicy
> +                  element,
> +                  nsILoadInfo::eDontInheritPrincipal,
> +                  nsILoadInfo::eNotSandboxed,
> +                  nsIContentPolicy::TYPE_OTHER,

TYPE_MEDIA
Attachment #8478596 - Flags: review?(roc) → review-
Comment on attachment 8478603 [details] [diff] [review]
3_10_callsites_in_layout_mathml.patch

Review of attachment 8478603 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/mathml/nsMathMLChar.cpp
@@ +155,5 @@
> +                                                nullptr,   // aCharset
> +                                                nullptr,   // aBaseURI
> +                                                nullptr,   // aIoService
> +                                                nsContentUtils::GetSystemPrincipal(),
> +                                                nsIContentPolicy::TYPE_FONT,

This isn't an actual font. I think it should be TYPE_OTHER.
Attachment #8478603 - Flags: review?(roc) → review-
Comment on attachment 8478633 [details] [diff] [review]
3_27_callsites_in_gfx.patch

Review of attachment 8478633 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxUserFontSet.cpp
@@ +1088,5 @@
> +                                nullptr,   // aChannelPolicy
> +                                aPrincipal,
> +                                nsILoadInfo::eDontInheritPrincipal,
> +                                nsILoadInfo::eNotSandboxed,
> +                                nsIContentPolicy::TYPE_OTHER,

This is a font, but we're not really loading it here, so I guess this is OK.
Attachment #8478633 - Flags: review?(roc) → review+
(In reply to Steven Michaud from comment #143)
> nsISound.play(in nsIURL aURL) is scriptable, so it can be called from
> user-level code, on an arbitrary URL.  In that case, is it correct to always
> pass the system principal to NS_NewStreamLoader(), as this patch does?

What does "user-level code" mean? Is that as opposed to kernel code, i.e. that JS code that's part of the Firefox UI, a.k.a. chrome JS? Or does that mean that a webpage can cause that to be called using a webpage provided URI?

Chrome JS, addons, and C++ code is generally considered to be part "the system", and so passing a system principal is generally ok.

However if webpage code is the one that triggers a request with a webpage controller URI, then passing a system principal is not ok. So if the code path is 

webpage --calls--> someDOMfunction(uri) --calls--> someChromeFunction(uri) --calls--> nsISound.play(uri)

then passing a system principal is not ok. There the webpage principal needs to be used.

However if the webpage does something, for example close a tab, which triggers some chrome UI, and the chrome UI decides that as part of a transition or whatever we want to play a "woooosh" sound effect, then passing a system principal is ok.

I.e. if a network load happens on the request of a webpage, including using a webpage provided URI, then the principal needs to be the principal of the webpage.

If a network load happens on request of chrome code, then the principal should be the system principal.


Another way to think about it is that loading some URIs can transfer the user's money from the user's bank account. If you want a security check to happen before the network load starts as to ensure that such a URI isn't being loaded, then don't pass a system principal. If you are sure that the URI is fine, then go ahead and use a system principal.


A good example is, the favicon displayed in each tab. That should not be loaded using the system principal. However the lock icon displayed in the URL bar on https websites should be loaded using the system principal.


I hope that clears it up.
Comment on attachment 8478607 [details] [diff] [review]
3_12_callsites_in_widget_cocoa.patch

(In reply to comment #147)

Actually you can't create an nsISound object in non-privileged (non-Chrome) JavaScript code.  I just realized this when I tried it and couldn't get it to work.

So there shouldn't be any problem passing the system principal to NS_NewStreamLoader() in this patch.
Attachment #8478607 - Flags: review?(smichaud) → review+
> Actually you can't create an nsISound object in non-privileged (non-Chrome) JavaScript code.

Or presumably any XPCOM object.
> * What does nsWebBrowserPersist::SaveURIInternal() do?

Implement things like "save link as" and the saving of subresources for "save document as".

> Or is this really a system operation?

It sort of kind of is, maybe.  At least for the "save link as" case.  For the case when we're saving a document and all it subresources, we should probably use the document's principal here.
(Following up comment #148)

Actually, rereading comment #147, things aren't quite so simple as I say in comment #148.

But there's no way to tell from widget code what ultimately triggered a call to nsISound.play() -- a webpage, or just Chrome code.  And widget code knows nothing about web pages.  So we're (more or less) forced to default to passing the system principal to NS_NewStreamLoader().
Comment on attachment 8478625 [details] [diff] [review]
3_22_callsites_in_rdf.patch

Redirecting to RDF-pike instead of L10N-pike
Attachment #8478625 - Flags: review?(l10n) → review?(axel)
Comment on attachment 8478625 [details] [diff] [review]
3_22_callsites_in_rdf.patch

Review of attachment 8478625 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, Benjamin is a better reviewer for this.
Attachment #8478625 - Flags: review?(axel) → review?(benjamin)
Comment on attachment 8478615 [details] [diff] [review]
3_15_callsites_in_toolkit_components_url-classifier.patch

Review of attachment 8478615 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp
@@ +103,5 @@
> +                     nsContentUtils::GetSystemPrincipal(),
> +                     nsILoadInfo::eDontInheritPrincipal,
> +                     nsILoadInfo::eNotSandboxed,
> +                     nsIContentPolicy::TYPE_OTHER,
> +                     getter_AddRefs(mChannel));

We'll end up modifying the LoadContext on this channel, as per https://hg.mozilla.org/mozilla-central/rev/61817a92297c but I guess that's irrelevant to this patch?
Attachment #8478615 - Flags: review?(gpascutto) → review+
Attachment #8478623 - Flags: review?(benjamin) → review+
Comment on attachment 8478625 [details] [diff] [review]
3_22_callsites_in_rdf.patch

We should just remove the RDF code entirely, but while it's here I'm pretty certain that this is the correct setup.
Attachment #8478625 - Flags: review?(benjamin) → review+
(In reply to Steven Michaud from comment #151)
> (Following up comment #148)
> 
> Actually, rereading comment #147, things aren't quite so simple as I say in
> comment #148.
> 
> But there's no way to tell from widget code what ultimately triggered a call
> to nsISound.play() -- a webpage, or just Chrome code.  And widget code knows
> nothing about web pages.  So we're (more or less) forced to default to
> passing the system principal to NS_NewStreamLoader().

Can we add a parameter to play() telling us whether chrome code called it?  In those cases, we can use systemPrincipal, and if its a webpage we can use nullprincipal?
> Can we add a parameter to play() telling us whether chrome code called it?

I frankly don't know if this is reasonable or feasible.  What do you think, Boris?
Flags: needinfo?(bzbarsky)
(In reply to Tanvi Vyas [:tanvi] from comment #156)
> Can we add a parameter to play() telling us whether chrome code called it? 
> In those cases, we can use systemPrincipal, and if its a webpage we can use
> nullprincipal?

Don't use the nullprincipal for webpage use (docshell is an exception).

The right solution would be to add a node/principal argument to play().

But it would be good if someone that knows how/when we use nsISound could actually verify if any of this is needed.
Comment on attachment 8478592 [details] [diff] [review]
3_01_callsites_in_content_base-html.patch

Review of attachment 8478592 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1757,5 @@
> +                             nullptr,   // aIoService
> +                             loadGroup,
> +                             nullptr,   // aCallbacks
> +                             nsIRequest::LOAD_BACKGROUND,
> +                             nullptr,   // aChannelPolicy

Note to myself: Obviously this should not a nullptr, but rather the channelPolicy from above.
Comment on attachment 8478608 [details] [diff] [review]
3_13_callsites_in_widget_windows.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #121)
> Created attachment 8478608 [details] [diff] [review]
> 3_13_callsites_in_widget_windows.patch
> 
> ** widget/windows/WinUtils.cpp ::
> AsyncFaviconDataReady()::OnFaviconDataNotAvailable(void) - We are calling
> NS_NewChannel() with systemPrincipal.  Assuming this is using a generic
> favicon and no network request is being made, systemPrincipal is okay.  Can
> you confirm this?  If a network request is being made, we need to find the
> real loading principal.

This is loading a moz-icon which I'm pretty sure is restricted to local access. They aren't designed to go out onto the network since they load icons out of local files for proper desktop integration in areas like helper application settings.

> ** widget/windows/nsDataObj.cpp
> Not sure what this code does.  Please look carefully at this one to check
> its use of systemPrincipal.  Can we identify the loader principal, or is
> this loaded by system?

This is used in drag and drop on Windows. nsDataObj is a COM object that windows speaks two when the user is dragging things like images out of content. Since nsIRequest::LOAD_FROM_CACHE doesn't guarantee cache only hits, it sounds like the right principal needs to be set. The object gets created when the drag session is invoked in nsDragService here - 

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsDragService.cpp#174
http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsClipboard.cpp#115

You can pass whatever information that's needed/available when the object is created.
Attachment #8478608 - Flags: review?(jmathies)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #144)
> 3_04_callsites_in_content_media.patch
> ::: content/media/MediaResource.cpp
> > +                              nsILoadInfo::eNotSandboxed,
> > +                              nsIContentPolicy::TYPE_OTHER,
> 
> Shouldn't this be TYPE_MEDIA?

Indeed - good catch!
Attachment #8478596 - Attachment is obsolete: true
Attachment #8479349 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #145)
> 3_10_callsites_in_layout_mathml.patch
> ::: layout/mathml/nsMathMLChar.cpp
> > +                                                nsContentUtils::GetSystemPrincipal(),
> > +                                                nsIContentPolicy::TYPE_FONT,
> 
> This isn't an actual font. I think it should be TYPE_OTHER.

Updated!
Attachment #8478603 - Attachment is obsolete: true
Attachment #8479350 - Flags: review?(roc)
Attachment #8478628 - Flags: review?(mrbkap) → review+
Attachment #8478626 - Flags: review?(mrbkap) → review+
Comment on attachment 8478594 [details] [diff] [review]
3_02_callsites_in_content_base_src_ImportManager.patch

Review of attachment 8478594 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #110)
>  Are mImportParent->MasterDocument()'s ScriptObjectPrincipal and
> mImportParent->NodePrincipal the same? 

Yes they are.
Attachment #8478594 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8478620 [details] [diff] [review]
3_17_callsites_in_toolkit_components_downloads.patch

Note that nsDownloadManager.cpp is only used on Firefox for Android now. I believe it is still covered by automated tests on that platform, though I'm not sure whether resuming is one of them.

This change however looks straightforward, and is likely to work if it builds.
Attachment #8478620 - Flags: review?(paolo.mozmail) → review+
Attachment #8478599 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8478622 [details] [diff] [review]
3_19_callsites_in_xpfe.patch

TYPE_OTHER should probably
Attachment #8478622 - Flags: review?(timeless) → review?(neil)
Comment on attachment 8478622 [details] [diff] [review]
3_19_callsites_in_xpfe.patch

TYPE_OTHER should probably be TYPE_DOCUMENT
Attachment #8478622 - Flags: review?(neil) → review+
Comment on attachment 8478590 [details] [diff] [review]
1_loadinfo_changes.patch

>+++ b/docshell/base/LoadInfo.cpp
>+                   nsIDOMNode* aLoadingContext,

Why nsIDOMNode*?  Why not just nsINode*?  Seems like the main consumer has an nsINode* anyway...

The IDL-visible thing would obviously keep being nsIDOMNode.

>+++ b/docshell/base/LoadInfo.h
>+#include "nsIDOMNode.h"

This should probably be a forward-declare of nsINode, with the actual #include in the .cpp.

>+++ b/docshell/base/nsILoadInfo.idl
>+/* In bug 1041176, the flags will be collapsed into on

s/on/an/, but if this is just getting removed maybe not worth bothering.

>+  const unsigned long eInheritPrincipal = 0;
>+  const unsigned long eDontInheritPrincipal = 1;

I think these should probably be the other way around, since it's the "inherit the principal" case that should have the extra flag.  I guess bug 1041176 fixes this up too?

>+   * like MCB, and CSP. The loadingContext of a channel is the

It took me a bit to figure out what "MCB" is.  Since that's not a standard abbreviation, worth spelling it out.

>+   *  if http://a.com
>+   * includes an image from http://b.com, the loadingContext
>+   * attached to http://b.com's channel is the node for http://a.com

Should probably be:

  if a document loaded from http://a.com includes an image loaded from
  http://b.com, the loadingContext attached to the image load's channel is the
  <img> node in the http://a.com document.

or so.

>+  readonly attribute nsIDOMNode loadingContext;

It would be nice to have a C++-friendly getter here too.  Something [notxpcom,nostdcall,noscript] returning an already_AddRefed<nsINode>, say.  Some of the other patches in this bug could benefit from such a thing.  You can see nsIFrameLoader.idl's GetFrameLoader for an example of how to declare such a thing.

>+   readonly attribute nsContentPolicyType contentPolicyType;

[infallible], right?

r=me with those nits fixed.
Attachment #8478590 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #167)
> Comment on attachment 8478590 [details] [diff] [review]
> 1_loadinfo_changes.patch
> >+++ b/docshell/base/LoadInfo.cpp
> >+   *  if http://a.com
> >+   * includes an image from http://b.com, the loadingContext
> >+   * attached to http://b.com's channel is the node for http://a.com
> 
> Should probably be:
> 
>   if a document loaded from http://a.com includes an image loaded from
>   http://b.com, the loadingContext attached to the image load's channel is
> the
>   <img> node in the http://a.com document.
> 
> or so.
> 

That's not quite right. The loadingContext attached to the image load's channels is the node for the http://a.com document.  It's not the <img> node itself.
> It's not the <img> node itself.

Ah, I see.  In that case <img> may not be the best example to use here.
Comment on attachment 8478590 [details] [diff] [review]
1_loadinfo_changes.patch

>diff --git a/docshell/base/nsILoadInfo.idl b/docshell/base/nsILoadInfo.idl
> 
>   /**
>+   * The loadingContext of the channel, used for security checks
Replace with:
The loadingContext of the channel is used for security checks
> Can we add a parameter to play() telling us whether chrome code called it?

As far as I can tell from MXR, we never use nsISound::Play in Firefox code.

We can add a parameter, but we obviously can't enforce addons using it so far, right?  So I think it's fine to treat nsISound::Play as a chrome load for now and if we think that addons can screw things up by passing untrusted URIs to it we should file a followup bug to see what we can do about that.  Most simply, by checking whether addons really use it and if not removing the API.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #167)
> >+  const unsigned long eInheritPrincipal = 0;
> >+  const unsigned long eDontInheritPrincipal = 1;
> 
> I think these should probably be the other way around, since it's the
> "inherit the principal" case that should have the extra flag.  I guess bug
> 1041176 fixes this up too?

Yes, bug 1041176 cleans that up too.
 
> >+   readonly attribute nsContentPolicyType contentPolicyType;
> 
> [infallible], right?

error: [infallible] only works on builtin types (numbers, booleans, and raw char types), ../../../dist/idl/nsILoadInfo.idl line 55:25
 0:07.71    [infallible] readonly attribute nsContentPolicyType contentPolicyType;

We could leave as is and follow up on it once we allow other types than builtin types to be infallible, or we could make it an unsigned long and not use type nsContentPolicyType. I would rather follow up on it later. But in general, i totally agree, it should be infallible.
Comment on attachment 8478595 [details] [diff] [review]
3_03_callsites_in_content_xul.patch

I think this patch shows that the new NS_NewChannel signature needs work.

Specifically, it has the following _required_ arguments, for any sane Gecko caller:  URI, loadgroup, principal, two loadinfo bits, content policy type, outparam.

It also has the following arguments that are fundamentally optional, in order from most likely to be used to least likely to be used: channelPolicy, callbacks, ioservice.

Those three arguments should probably stay optional.  The callbacks and ioservice should _definitely_ stay optional.  Yes, this means having the outparam come before them; that's why it used to be in the first spot.

Putting the loadgroup arg before the ioservice arg will incidentally fix a problem the current API has, as long as we're changing this stuff...

Similar for NS_NewStreamLoader.

r=me with that fixed so people don't have to pass quite as many bajillions of arguments.
Attachment #8478595 - Flags: review?(bzbarsky) → review+
> error: [infallible] only works on builtin types

Oh, right, that was discussed earlier and I forgot about it.  :(  Sorry.

I think the simplest expedient thing to do here is to manually do the work that [infallible] does for you.  That is, throw in a %C++ block that has:

  inline nsContentPolicyType GetContentPolicyType()
  {
    nsContentPolicyType result;
    mozilla::DebugOnly<nsresult> rv = GetContentPolicyType(&result);
    MOZ_ASSERT(NS_SUCCEEDED(rv));
    return result;
  }
(In reply to Boris Zbarsky [:bz] from comment #173)
> Comment on attachment 8478595 [details] [diff] [review]
> 3_03_callsites_in_content_xul.patch
> 
> I think this patch shows that the new NS_NewChannel signature needs work.
> 
> Specifically, it has the following _required_ arguments, for any sane Gecko
> caller:  URI, loadgroup, principal, two loadinfo bits, content policy type,
> outparam.
> 
> It also has the following arguments that are fundamentally optional, in
> order from most likely to be used to least likely to be used: channelPolicy,
> callbacks, ioservice.

Bug 1041180 removes the channelpolicy completely, since we now store the requestingPrincipal and the contentPolicyType in the loadgroup. I already have a patch for that and verified that that is working as expected. Will put it on bugzilla soon. So that argument goes away.

> Those three arguments should probably stay optional.  The callbacks and
> ioservice should _definitely_ stay optional.  Yes, this means having the
> outparam come before them; that's why it used to be in the first spot.

Any chance I could convince you that we remain with the current API? Otherwise I will go ahead and make them optinal again.
Comment on attachment 8478602 [details] [diff] [review]
3_09_callsites_in_docshell.patch

>+    nsCOMPtr<nsINode> requestingNode = nullptr;

Can this just be nsINode*?

You definitely don't need the "= nullptr" bit; you're assigning the variable on the very next line.

I think using GetFrameElementInternal in all cases here is wrong.  That will be the <xul:browser> for a toplevel tab, no?  Do we not have tests for that?

The right thing to do is to use GetFrameElementInternal() only when aContentPolicyType is subdocument, I would think.

>+      nsCOMPtr<nsPIDOMWindow> win = do_QueryObject(mScriptGlobal);

This is completely pointless, no?  mScriptGlobal is already a subclass of nsPIDOMWindow.

>+        nsPIDOMWindow* innerWin = mScriptGlobal->GetCurrentInnerWindow();
>+        if (innerWin) {
>+          requestingNode = innerWin->GetExtantDoc();
>+        }

This is equivalent to:

  requestingNode = mScriptGlobal->GetExtantDoc();

But why are we using the currently-loaded document as the "source node" for the load anyway?  This is a toplevel load, so null might seem like a saner value here?

>+    // Please note that in comparsion to NS_CheckContentLoadPolicy we do not
>+    // use the referrer to create a principal which would be wrong, because
>+    // "about:blank" would have the principal "chrome://browser/content/browser.xul"

So wait.  How is that going to work once you push the NS_CheckContentLoadPolicy check down into the channel implementation like you plan to?  We should get this sorted out.

Also, the comment is talking about some special cases of about:blank, not about:blank in general, even though it sure tries to sound like it's the latter....

>+        // we have to manually set the loadInfo here, since the channel within
>+        // that else branch is not created through NS_NewChannel and hence

You mean "the channel we just created"?  I'm not sure what "else branch" this is referring to, but this is clearly talking about the channel that was just created.

I don't quite understand this patch.  It does all this stuff, but then we still end up calling SetupChannelOwner later, right?  Which is good, because the loadinfo it creates in the srcdoc case is totally bogus (claims to be not sandboxed when it might need to be, claims to not inherit the principal when it should).

I'm going to assume that this is fixed up in the SetupChannelOwner patch in the queue...

r=me with the above fixed and the interaction with referrer sorted out.  It does make sense to me to use a nullprincipal here, but then we need to make sure it's not used for any security checks on the load itself that we really care about.  :(  Which cases have a null owner here anyway?
Attachment #8478602 - Flags: review?(bzbarsky) → review+
Comment on attachment 8478635 [details] [diff] [review]
3_29_callsites_in_embedding.patch

r=me
Attachment #8478635 - Flags: review?(bzbarsky) → review+
Comment on attachment 8478638 [details] [diff] [review]
5_GetChannelPrincipal_changes.patch

Fwiw, I think the diff would be smaller and the blame better preserved if the .cpp file had nsScriptSecurityManager::GetChannelURIPrincipal after GetChannelResultPrincipal.  Please make that change.

Please document in nsCrossSiteListenerProxy.cpp why it's not using GetChannelResultPrincipal.

r=me with that
Attachment #8478638 - Flags: review?(bzbarsky) → review+
Comment on attachment 8478605 [details] [diff] [review]
3_11_callsites_in_layout_style.patch

>+++ b/layout/style/Loader.cpp
>+  nsCOMPtr<nsIPrincipal> requestingPrincipal = aLoadData->mLoaderPrincipal;

Seems like this could just be an nsIPrincipal*, right?

>+    requestingPrincipal = do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);

This doesn't look right to me.  At least if we plan to use this principal for security checks.   The only time aLoadData->mLoaderPrincipal is null here is if InternalLoadNonDocumentSheet got called with a null principal.  That can happen via:

  LoadSheetSync
  LoadSheet with a null aOriginPrincipal
  
All the LoadSheetSync callsites are system.  The only caller of LoadSheet that passes a null aOriginPrincipal is nsHTMLEditor::ReplaceStyleSheet, which is system code.

So !requestingPrincipal should be treated as the system-principal case.  You can also tell this because in this case we currently skip sheet loading security checks.

>+++ b/layout/style/nsFontFaceLoader.cpp
>@@ -338,24 +338,27 @@ nsUserFontSet::StartLoad(gfxMixedFontFam

Hmm.  So this is getting the CSP from aProxy->mPrincipal.  But it's using ps->GetDocument() as the node and not passing in a principal, right?

That looks wrong.  The reason there's a separate principal on aProxy is so that user stylesheets applied to the document can load fonts that the document itself can't load.  See nsUserFontSet::CheckFontLoad.

So this should be passing in aProxy->mPrincipal for the principal, in addition to passing a node.

In general, it's worth making sure that the principals you pass in for all these patches match the principals security checks are actually done on.

Similar in SyncLoadFontData but with aFontToLoad.

r=me with the above fixed.
Attachment #8478605 - Flags: review?(bzbarsky) → review+
Comment on attachment 8478637 [details] [diff] [review]
4_setupChannelOwner_changes.patch

r=me, but the nullprincipal bits in the image loader patch context look wrong to me for the same reasons that they look wrong to me in the docshell code...  no principal provided is more likely to be a system load than a "don't do this load" load, and using a nullprincipal for a load almost certainly means the load will be blocked once we push the security checks down, no?
Attachment #8478637 - Flags: review?(bzbarsky) → review+
In general, I think any place where we cons up a NullPrincipal and then use it in loadinfo in these patches is almost certainly a bug...  If we have to land some of them for now, we should have clear bugs filed on getting them fixed before we push security checks down into the channel, so we don't have to go reaudit all this code again.
Comment on attachment 8478591 [details] [diff] [review]
2_nsNetutil_changes.patch

Skipping the review for now - since Boris requested some changes in the API. No need to review it before the change.
Attachment #8478591 - Flags: review?(mcmanus)
(In reply to Boris Zbarsky [:bz] from comment #179)
> Comment on attachment 8478605 [details] [diff] [review]
> 3_11_callsites_in_layout_style.patch
> 
> >+++ b/layout/style/Loader.cpp
> >+  nsCOMPtr<nsIPrincipal> requestingPrincipal = aLoadData->mLoaderPrincipal;
> 
> Seems like this could just be an nsIPrincipal*, right?
> 
> >+    requestingPrincipal = do_CreateInstance("@mozilla.org/nullprincipal;1", &rv);
> 
> This doesn't look right to me.  At least if we plan to use this principal
> for security checks.   The only time aLoadData->mLoaderPrincipal is null
> here is if InternalLoadNonDocumentSheet got called with a null principal. 
> That can happen via:
> 
>   LoadSheetSync
>   LoadSheet with a null aOriginPrincipal
>   
> All the LoadSheetSync callsites are system.  The only caller of LoadSheet
> that passes a null aOriginPrincipal is nsHTMLEditor::ReplaceStyleSheet,
> which is system code.
> 
> So !requestingPrincipal should be treated as the system-principal case.  You
> can also tell this because in this case we currently skip sheet loading
> security checks.
> 
> >+++ b/layout/style/nsFontFaceLoader.cpp
> >@@ -338,24 +338,27 @@ nsUserFontSet::StartLoad(gfxMixedFontFam
> 
> Hmm.  So this is getting the CSP from aProxy->mPrincipal.  But it's using
> ps->GetDocument() as the node and not passing in a principal, right?
> 
> That looks wrong.  The reason there's a separate principal on aProxy is so
> that user stylesheets applied to the document can load fonts that the
> document itself can't load.  See nsUserFontSet::CheckFontLoad.
> 
> So this should be passing in aProxy->mPrincipal for the principal, in
> addition to passing a node.
> 
> In general, it's worth making sure that the principals you pass in for all
> these patches match the principals security checks are actually done on.
> 
> Similar in SyncLoadFontData but with aFontToLoad.
> 
> r=me with the above fixed.

Hi Boris,

Did you happen to see comment 119?  It discusses similar points and asks some questions.  I went through almost all the callsites and tried to manually inspect that we were passing the same values to NS_NewChannel as we pass to content policies.  Where there were discrepancies, we asked the reviewers.  Since dbaron passed this review to you, you probably didn't see the comments included when we flagged him for review.

For the layout/style/Loader.cpp, I propose passing in the requestingContext instead of using mDocument (mDocument doesn't look right to me for the reasons mentioned in the comment).  However, this doesn't address the case where we have a null requestingPrincipal.  We can either fall back to systemPrincipal or fall back to nullPrincipal for now and create another bug to remove nullPrincipal before we move security checks to asyncOpen.

For nsFontFaceLoader, it sounds like we can pass in aProxy->mPrincipal to the first call to NS_NewChannelInternal and aFontToLoad->mPrincipal to the second call to NS_NewChannelInternal.
> Did you happen to see comment 119? 

No, I hadn't, sorry.  This bug is too long.  :(

> I propose passing in the requestingContext instead of using mDocument

That seems fine, yes.

> We can either fall back to systemPrincipal or fall back to nullPrincipal

You want the system principal.

> we can pass in aProxy->mPrincipal to the first call to NS_NewChannelInternal and
> aFontToLoad->mPrincipal to the second call

Yes, exactly.
Comment on attachment 8478624 [details] [diff] [review]
3_21_callsites_in_uriloader.patch

Review of attachment 8478624 [details] [diff] [review]:
-----------------------------------------------------------------

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +382,5 @@
>  
> +    rv = NS_NewChannel(mURI,
> +                       nullptr,   // aIoService
> +                       nullptr,   // loadGroup
> +                       this,

nit, maybe leave the comment that these are the callbacks
Attachment #8478624 - Flags: review?(honzab.moz) → review+
Comment on attachment 8478627 [details] [diff] [review]
3_24_callsites_in_modules.patch

Review of attachment 8478627 [details] [diff] [review]:
-----------------------------------------------------------------

This code is in a channel, so could we reuse some of the load info that's already set on the channel?
(In reply to Michael Wu [:mwu] from comment #186)
> Comment on attachment 8478627 [details] [diff] [review]
> 3_24_callsites_in_modules.patch
> 
> Review of attachment 8478627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This code is in a channel, so could we reuse some of the load info that's
> already set on the channel?

Unfortunately all the information we are about to set in the loadInfo for this patch is currently not available on a channel. Please see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c106
Flags: needinfo?(mwu)
Comment on attachment 8478627 [details] [diff] [review]
3_24_callsites_in_modules.patch

Review of attachment 8478627 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libjar/nsJARChannel.cpp
@@ +862,5 @@
> +                            mLoadFlags & ~(LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS),
> +                            nsContentUtils::GetSystemPrincipal(),
> +                            nsILoadInfo::eDontInheritPrincipal,
> +                            nsILoadInfo::eNotSandboxed,
> +                            nsIContentPolicy::TYPE_OTHER);

This does not seem right. Why not use the loadinfo that is on the jar-channel? nsJarChannel *is* a channel so you should have all this information available from whoever created the nsJarChannel. (I think that's what mwu was saying too)
(In reply to Jonas Sicking (:sicking) from comment #188)
> Comment on attachment 8478627 [details] [diff] [review]
> 3_24_callsites_in_modules.patch
> 
> Review of attachment 8478627 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libjar/nsJARChannel.cpp
> @@ +862,5 @@
> > +                            mLoadFlags & ~(LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS),
> > +                            nsContentUtils::GetSystemPrincipal(),
> > +                            nsILoadInfo::eDontInheritPrincipal,
> > +                            nsILoadInfo::eNotSandboxed,
> > +                            nsIContentPolicy::TYPE_OTHER);
> 
> This does not seem right. Why not use the loadinfo that is on the
> jar-channel? nsJarChannel *is* a channel so you should have all this
> information available from whoever created the nsJarChannel. (I think that's
> what mwu was saying too)

Ah, that makes sense now as I re-read Michael's comment. Clearing needinfo-flag and will incorporate that change.
Flags: needinfo?(mwu)
Comment on attachment 8478600 [details] [diff] [review]
3_07_callsites_in_netwerk.patch

Review of attachment 8478600 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/nsBufferedStreams.cpp
@@ +814,5 @@
>      NS_IF_ADDREF(*aStream);
>      return NS_OK;
>  }
>  
> +#undef METER

is this a ridealong?
Attachment #8478600 - Flags: review?(mcmanus) → review+
Attachment #8478601 - Flags: review?(mcmanus) → review+
(In reply to Boris Zbarsky [:bz] from comment #173)
> Comment on attachment 8478595 [details] [diff] [review]
> 3_03_callsites_in_content_xul.patch
> 
> I think this patch shows that the new NS_NewChannel signature needs work.
> 
> Specifically, it has the following _required_ arguments, for any sane Gecko
> caller:  URI, loadgroup, principal, two loadinfo bits, content policy type,
> outparam.
> 
> It also has the following arguments that are fundamentally optional, in
> order from most likely to be used to least likely to be used: channelPolicy,
> callbacks, ioservice.

Changed the API, so the signature looks like this:

> inline nsresult /* NS_NewChannelNode */
> NS_NewChannel(nsIChannel**           outChannel,
>               nsIURI*                aUri,
>               nsINode*               aRequestingNode,
>               nsSecurityFlags        aSecurityFlags,
>               nsContentPolicyType    aContentPolicyType,
>               nsIChannelPolicy*      aChannelPolicy = nullptr,
>               nsILoadGroup*          aLoadGroup = nullptr,
>               nsIInterfaceRequestor* aCallbacks = nullptr,
>                nsLoadFlags            aLoadFlags = nsIRequest::LOAD_NORMAL,
>               nsIIOService*          aIoService = nullptr)


Agreed, that change cleans up callsites; I incorporated similar changes for NS_OpenURI, and NS_NewStreamLoader.
Attachment #8478627 - Flags: review?(mwu)
(In reply to Patrick McManus [:mcmanus] from comment #190)
> Comment on attachment 8478600 [details] [diff] [review]
> 3_07_callsites_in_netwerk.patch
> 
> Review of attachment 8478600 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/src/nsBufferedStreams.cpp
> @@ +814,5 @@
> >      NS_IF_ADDREF(*aStream);
> >      return NS_OK;
> >  }
> >  
> > +#undef METER
> 
> is this a ridealong?

This bug is getting way to long :-(. I wrote a quick note when I flagged you for review:
Comment 115
Comment on attachment 8478592 [details] [diff] [review]
3_01_callsites_in_content_base-html.patch

As discussed in person, I will put up a new patch since this one got bitroted by changes requested by other reviewers.
Attachment #8478592 - Flags: review?(jst)
Comment on attachment 8478597 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

As discussed in person, I will put up a new patch since this one got bitroted by changes requested by other reviewers.
Attachment #8478597 - Flags: review?(jst)
Comment on attachment 8478632 [details] [diff] [review]
3_26_callsites_in_image.patch

Review of attachment 8478632 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

You need to update the commit message to say "r=seth" instead of "r=joe".
Attachment #8478632 - Flags: review?(seth) → review+
Comment on attachment 8478632 [details] [diff] [review]
3_26_callsites_in_image.patch

Review of attachment 8478632 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry if I'm misunderstanding the code, but this doesn't look right to me.

::: image/src/imgLoader.cpp
@@ +1402,5 @@
>                           mAcceptHeader,
>                           aLoadFlags,
>                           aPolicy,
> +                         aLoadingPrincipal,
> +                         aCX);

Isn't aCX a JS-context? Those are not nodes so the QI in NewImageChannel will fail and return null.

@@ +1902,5 @@
>                           mAcceptHeader,
>                           requestFlags,
>                           aPolicy,
> +                         aLoadingPrincipal,
> +                         aCX);

Same here.
Attachment #8478632 - Flags: feedback-
> Isn't aCX a JS-context?

No.  It's the random nsISupports argument to imgILoader.loadImage that's supposed to represent some sort of "load context" for the image.  In practice, it's a document, and we should stop the silly pretense that imagelib is somehow standalone and change the signature to be nsIDocument and rename the argument.

Trying to QI an actual JSContext* wouldn't compile, since it's not nsISupports.  ;)
(In reply to timeless from comment #166)
> Comment on attachment 8478622 [details] [diff] [review]
> 3_19_callsites_in_xpfe.patch
> 
> TYPE_OTHER should probably be TYPE_DOCUMENT

Looked at this a little more carefully.

* https://mxr.mozilla.org/mozilla-central/source/xpfe/components/directory/nsDirectoryViewer.cpp#1257
The NS_NewChannel call in nsDirectoryViewerFactory::CreateInstance() only gets called if the user sets an about:config pref.  Moreover, that functionality is broken (setting the pref and opening an ftp page causes tabs to continuosly open).  We can use TYPE_OTHER and systemPrincipal for this.  And we should probably file a bug to to remove this code.

* https://mxr.mozilla.org/mozilla-central/source/xpfe/components/directory/nsDirectoryViewer.cpp#908
The NS_NewChannel call in nsHTTPIndex::FireTimer is a bit more tricky.  I believe the channel is used to get data from the ftp server about the directories and files they serve.  The data is read in as text/html and formatted by the browser in a xul page.  Who requests this content?  Is it always a top level load - if so, using systemPrincipal would be fine.  Also, what do we call this ftp data?  It's not quite the traditional TYPE_DOCUMENT, but it is at the top-level (http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIContentPolicy.idl).  This feels more like TYPE_OTHER.
Attachment #8478617 - Flags: review?(mak77) → review+
Depends on: 1062529
Comment on attachment 8479350 [details] [diff] [review]
3_10_callsites_in_layout_mathml_v2.patch

Since this patch is highly independent from the rest of the code updated in this bug, we land this part ahead of time in Bug 1062529. Marking it as obsolete becasue of that change.
Attachment #8479350 - Attachment is obsolete: true
Comment on attachment 8479350 [details] [diff] [review]
3_10_callsites_in_layout_mathml_v2.patch

Oh oh, that was the wrong one.
Attachment #8479350 - Attachment is obsolete: false
Comment on attachment 8478638 [details] [diff] [review]
5_GetChannelPrincipal_changes.patch

This is the right one to mark obsolete.
Attachment #8478638 - Attachment is obsolete: true
Comment on attachment 8478621 [details] [diff] [review]
3_18_callsites_in_intl.patch

Jungshik is not available to do the review for that patch, but he recommended smontagu. Any chance you can take a look at this?

To get an understanding what we are trying to accomplish, please also see Comment 106.
Attachment #8478621 - Flags: review?(jshin1987) → review?(smontagu)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #126)
> Created attachment 8478621 [details] [diff] [review]
> 3_18_callsites_in_intl.patch
> 
> *  Does passing systemPrincipal as the loadingPrincipal make sense for these
> files?  Are the channels created by system operations?  For
> intl/strres/nsStringBundleTextOverride.cpp and intl/hyphenation/hnjstdio.cpp
> its probably okay.  But for /intl/strres/nsStringBundle.cpp it's less clear.

I'm not sure I understand the question "Are the channels created by system operations", can you clarify that for me? nsStringBundle::LoadProperties could be called by an extension doing formatStringFromName, if that's relevant.
Addons count as "system" code still.

Webpages are however not system.

See comment 147 for a more detailed explanation.

I'm fairly sure that all stringbudle loading counts as being triggered by the system.
Comment on attachment 8478621 [details] [diff] [review]
3_18_callsites_in_intl.patch

Review of attachment 8478621 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Jonas, and apologies for not having read the whole bug report ;-)
Attachment #8478621 - Flags: review?(smontagu) → review+
Depends on: 1063837
Attachment #8478590 - Attachment is obsolete: true
Attachment #8478591 - Attachment is obsolete: true
Attachment #8478592 - Attachment is obsolete: true
Attachment #8478594 - Attachment is obsolete: true
Attachment #8478595 - Attachment is obsolete: true
Attachment #8478597 - Attachment is obsolete: true
Attachment #8478599 - Attachment is obsolete: true
Attachment #8478600 - Attachment is obsolete: true
Attachment #8478601 - Attachment is obsolete: true
Attachment #8478602 - Attachment is obsolete: true
Attachment #8478605 - Attachment is obsolete: true
Attachment #8478607 - Attachment is obsolete: true
Attachment #8478608 - Attachment is obsolete: true
Attachment #8478613 - Attachment is obsolete: true
Attachment #8478615 - Attachment is obsolete: true
Attachment #8478617 - Attachment is obsolete: true
Attachment #8478620 - Attachment is obsolete: true
Attachment #8478621 - Attachment is obsolete: true
Attachment #8478622 - Attachment is obsolete: true
Attachment #8478623 - Attachment is obsolete: true
Attachment #8478624 - Attachment is obsolete: true
Attachment #8478625 - Attachment is obsolete: true
Attachment #8478626 - Attachment is obsolete: true
Attachment #8478627 - Attachment is obsolete: true
Attachment #8478628 - Attachment is obsolete: true
Attachment #8478632 - Attachment is obsolete: true
Attachment #8478633 - Attachment is obsolete: true
Attachment #8478634 - Attachment is obsolete: true
Attachment #8478635 - Attachment is obsolete: true
Attachment #8478636 - Attachment is obsolete: true
Attachment #8478637 - Attachment is obsolete: true
Attachment #8479349 - Attachment is obsolete: true
Attachment #8479350 - Attachment is obsolete: true
Attachment #8486684 - Flags: review+
Attachment #8486685 - Flags: review?(mcmanus)
Attachment #8486704 - Flags: review+
Comment on attachment 8486712 [details] [diff] [review]
3_24_callsites_in_modules.patch

Review of attachment 8486712 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libjar/nsJARChannel.cpp
@@ +853,5 @@
>          // kick off an async download of the base URI...
>          rv = NS_NewDownloader(getter_AddRefs(mDownloader), this);
> +        if (NS_SUCCEEDED(rv)) {
> +            // Since we might not have a loadinfo on all channels yet
> +            // we have to provide default arguments in case mLoadInfo is null;

I thought this bug ensures loadinfo is set on all channels.. is there going to be a followup to ensure loadinfo is always set?

@@ +863,5 @@
> +                                      mLoadGroup,
> +                                      mCallbacks,
> +                                      mLoadFlags & ~(LOAD_DOCUMENT_URI | LOAD_CALL_CONTENT_SNIFFERS));
> +            }
> +            else {

Put else on the same line as }.
Attachment #8486712 - Flags: review?(mwu) → review+
> I thought this bug ensures loadinfo is set on all channels..

... that Gecko creates.

Extensions can nsIIOService.newChannel and then not set it (and likely do just that).
s/extensions/extensions or other JS code/
Comment on attachment 8486699 [details] [diff] [review]
3_13_callsites_in_widget_windows.patch

Review of attachment 8486699 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/nsITransferable.idl
@@ +196,5 @@
>    [noscript] attribute boolean isPrivateData;
>  
> +  /**
> +   * Only used on windows to transfer the requestingNode to the
> +   * loadInfo of the new channel.

You added support for saving this node, its just that some platforms don't have a use for it. Lets describe what this does, with a note about its only use being on windows. Something like -

The source dom node this transferable was created from.

Note, currently only in use on Windows for network principal information in drag operations.

::: widget/windows/WinUtils.cpp
@@ +61,5 @@
>  
>  namespace mozilla {
>  namespace widget {
> +
> +#define ENTRY(_msg) { #_msg, _msg }

nit - some sort of white space touchup here? Please check the patch over for unnecessary changes.

::: widget/windows/nsDataObj.cpp
@@ +66,5 @@
>  {
>    nsresult rv;
> +  rv = NS_NewChannel(getter_AddRefs(mChannel),
> +                     pSourceURI,
> +                     aRequestingNode,

Does NS_NewChannel handle null aRequestingNode nodes gracefully? Looks like that could happen here occasionally.
Attachment #8486699 - Flags: review?(jmathies) → review+
Comment on attachment 8486686 [details] [diff] [review]
3_01_callsites_in_content_base-html.patch

- In nsXMLHttpRequest::Open():

+  if (IsSystemXHR()) {
+    // Don't give this document the system principal.  We need to keep track of
+    // mPrincipal being system because we use it for various security checks
+    // that should be passing, but the document data shouldn't get a system
+    // principal.
+    nsresult rv;

rv is already declared outside of this scope, so no need to re-declare it here.

r=jst
Attachment #8486686 - Flags: review?(jst) → review+
Comment on attachment 8486690 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

r=jst, but I'd love to get bz to look over specifically the plugin and xbl bits here.
Attachment #8486690 - Flags: review?(jst)
Attachment #8486690 - Flags: review?(bzbarsky)
Attachment #8486690 - Flags: review+
Comment on attachment 8486690 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

Review of attachment 8486690 [details] [diff] [review]:
-----------------------------------------------------------------

I'd also like to have peterv look at the XSLT bits given that they are fairly complex.
Attachment #8486690 - Flags: review?(peterv)
Blocks: 1064439
Attachment #8486695 - Flags: review+ → review?(bzbarsky)
Attachment #8486685 - Flags: review?(mcmanus) → review+
Comment on attachment 8486695 [details] [diff] [review]
3_09_callsites_in_docshell.patch

Review of attachment 8486695 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +9978,5 @@
>      nsCOMPtr<nsIChannel> channel;
>  
>      bool isSrcdoc = !aSrcdoc.IsVoid();
> +
> +    nsCOMPtr<nsINode> requestingNode = mScriptGlobal->GetExtantDoc();

note to myself: mScriptGlobal might be null here.
Blocks: 1063197
(In reply to Jim Mathies [:jimm] from comment #242)
> Comment on attachment 8486699 [details] [diff] [review]
> 3_13_callsites_in_widget_windows.patch
> ::: widget/windows/nsDataObj.cpp
> @@ +66,5 @@
> >  {
> >    nsresult rv;
> > +  rv = NS_NewChannel(getter_AddRefs(mChannel),
> > +                     pSourceURI,
> > +                     aRequestingNode,
> 
> Does NS_NewChannel handle null aRequestingNode nodes gracefully? Looks like
> that could happen here occasionally.

In what situations would we get a null aRequestingNode?
Flags: needinfo?(jmathies)
Comment on attachment 8486695 [details] [diff] [review]
3_09_callsites_in_docshell.patch

>+nsDocShell::createPrincipalFromReferrer(nsIURI*        aReferrer,

"CreatePrincipalFromReferrer".

>+    nsCOMPtr<nsINode> requestingNode = mScriptGlobal->GetExtantDoc();

Er... why?  Shouldn't this be the frameelement in some cases?

r=me with those fixed.
Attachment #8486695 - Flags: review?(bzbarsky) → review+
(In reply to Tanvi Vyas [:tanvi] from comment #247)
> (In reply to Jim Mathies [:jimm] from comment #242)
> > Comment on attachment 8486699 [details] [diff] [review]
> > 3_13_callsites_in_widget_windows.patch
> > ::: widget/windows/nsDataObj.cpp
> > @@ +66,5 @@
> > >  {
> > >    nsresult rv;
> > > +  rv = NS_NewChannel(getter_AddRefs(mChannel),
> > > +                     pSourceURI,
> > > +                     aRequestingNode,
> > 
> > Does NS_NewChannel handle null aRequestingNode nodes gracefully? Looks like
> > that could happen here occasionally.
> 
> In what situations would we get a null aRequestingNode?

Someone fails to set the requesting node? Maybe the safe thing to do here is return failure if aRequestingNode is null on entry to nsDataObj::CStream::Init(). We handle that gracefully.
Flags: needinfo?(jmathies)
Comment on attachment 8486690 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

In Navigator::MozIsLocallyAvailable, what should happen when doc is null?  Right now we'll crash with this patch as-is, right?  That seems dubious.

I'm not sure in which situation we would not have a "doc" in the pluginhost code.  Using a nullprincipal there is probably fine, given that.  But you're not passing in the nullprincipal you create; you're passing in "doc", no?  And if that's null, we'll crash.

Same for nsPluginStreamListenerPeer.

Why is nsPluginStreamListenerPeer getting SEC_INHERIT_PRINCIPAL?  That doesn't match the old behavior, afaict.

>+  // This is because the principal and node could be different. (BZ, can you confirm?)

I believe so, yes.  The origin principal will be that of the stylesheet that had the -moz-binding declaration, while the node is the bound document.

That said, I'm not sure about passing in the bound document at all here: we don't want to consider it in security checks, because we want to allow chrome to bind stuff in content documents, right?

Please ask Jonas or Peter to look at the xslt changes; I didn't really look at those.
Attachment #8486690 - Flags: review?(bzbarsky) → review-
Blocks: 1063837
No longer depends on: 1063837
Comment on attachment 8486684 [details] [diff] [review]
1_loadinfo_changes.patch

Review of attachment 8486684 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good otherwise.

::: docshell/base/nsILoadInfo.idl
@@ +25,5 @@
> +  const unsigned long SEC_NORMAL = 0;
> +
> +  /**
> +   * Inherit the Principal. If this flag is set, then
> +   * SEC_SANDBOXED can *not* be set.

We should say what setting this flag actually does. I would say something like:

Force inheriting of the Principal. The resulting resource will use the principal of the document which is doing the load. Setting this flag will cause GetChannelResultPrincipal to return the same principal as the loading principal that's passed in when creating the channel.

This will happen independently of the scheme of the URI that the channel is loading.

So if the loading document comes from "http://a.com/", and the channel is loading the URI "http://b.com/whatever", GetChannelResultPrincipal will return a principal from "http://a.com/".

This flag can not be used together with SEC_SANDBOXED.

@@ +27,5 @@
> +  /**
> +   * Inherit the Principal. If this flag is set, then
> +   * SEC_SANDBOXED can *not* be set.
> +   */
> +  const unsigned long SEC_INHERIT_PRINCIPAL = 0x01;

I'd like to name this SEC_FORCE_INHERIT_PRINCIPAL. To set it apart from "normal" inheritance, like how data: can inherit the principal of the loader.

@@ +31,5 @@
> +  const unsigned long SEC_INHERIT_PRINCIPAL = 0x01;
> +
> +  /**
> +   * Sandbox the load. If this flag is set, then
> +   * SEC_INHERIT_PRINCIPAL can *not* be set.

Maybe something like:

Sandbox the load. The resulting resource will use a freshly created null principal. So GetChannelResultPrincipal will always return a null principal whenever this flag is set.

This will happen independently of the scheme of the URI that the channel is loading.

This flag can not be used together with SEC_INHERIT_PRINCIPAL.

@@ +57,5 @@
>  
> +
> +  /**
> +   * The loadingDocument of the channel, used for security checks
> +   * like Mixed Content Blocker, and CSP. The loadingDocument of a channel is

I'd leave out the "used for security checks like MCB and CSP" part. Lots of properties on nsILoadInfo will be used for security checks.

@@ +59,5 @@
> +  /**
> +   * The loadingDocument of the channel, used for security checks
> +   * like Mixed Content Blocker, and CSP. The loadingDocument of a channel is
> +   * the document that requested the load of the resource that is tied
> +   * to this channel. It is *NOT* the resource itself, it's the

Leave out "that is tied to this channel". I think that part is clear but is making the sentence hard to read.

@@ +65,5 @@
> +   * Script Example: Assume a document who's origin is http://a.com
> +   * embeds a script from http://b.com. The loadingDocument for the channel
> +   * associated with the http://b.com script load is the document with origin
> +   * http://a.com
> +   * Frame Example: Assume a document with origin http://a.com embeds a frame

Say "iframe" rather than "frame". The word "frame" can mean very many different things.

You might even want to say "<script> example" and "<iframe> example" in the titles above (though no need to say it in the text).

@@ +66,5 @@
> +   * embeds a script from http://b.com. The loadingDocument for the channel
> +   * associated with the http://b.com script load is the document with origin
> +   * http://a.com
> +   * Frame Example: Assume a document with origin http://a.com embeds a frame
> +   * from http://b.com. The loadingDocument for the channel associated with

Rather than saying "a frame from http://b.com", say "a <iframe src="http://b.com>"

@@ +67,5 @@
> +   * associated with the http://b.com script load is the document with origin
> +   * http://a.com
> +   * Frame Example: Assume a document with origin http://a.com embeds a frame
> +   * from http://b.com. The loadingDocument for the channel associated with
> +   * the http://b.com frame load is the document who's origin is http://a.com.

Rather than "frame load" say "network request".

@@ +72,5 @@
> +   * Now assume the frame to http://b.com then further embeds a script from 
> +   * http://c.com. The loadingDocument for the channel associated with the 
> +   * http://c.com script load is the frame with origin http://b.com.
> +   *
> +   * Warning: The loadingDocument can be null!

Are we enforcing that loadingDocument is only null when loadingPrincipal is the system principal? If so we should say that here.

If not we should try to enforce that in later patches in followup bugs.
Attachment #8486684 - Flags: review+
Attachment #8486690 - Flags: review?(jonas)
Comment on attachment 8486690 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

As discussed on IRC, Peter will have a look tomorrow morning. No need for Jonas to review it today then. Cancelling review request.
Attachment #8486690 - Flags: review?(jonas)
Peterv: The goal is that for loading stylesheets, the document that we pass in should be the source document that's being transformed. The principal that we pass in should be the principal of whoever linked to the stylesheet. So either the principal of the stylesheet that contains the <xsl:import>/<xsl:include>, or the principal of the source document for <?xml-stylesheet href=...?>.

For document() loads, I guess we should pass in the source document as the document, and the principal of the stylesheet that contained the xpath expression as the principal.
Comment on attachment 8486690 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

Review of attachment 8486690 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xml/nsXMLPrettyPrinter.cpp
@@ +100,5 @@
>                     NS_LITERAL_CSTRING("chrome://global/content/xml/XMLPrettyPrint.xsl"));
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      nsCOMPtr<nsIDOMDocument> xslDocument;
> +    rv = nsSyncLoadService::LoadDocument(xslUri, nsContentUtils::GetSystemPrincipal(), nullptr, true,

This line is too long. Limit to 80 columns.

::: dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
@@ -369,3 @@
>  public:
>      txCompileObserver(txMozillaXSLTProcessor* aProcessor,
>                        nsILoadGroup* aLoadGroup);

I suspect that we need to change the flow here.

We should probably change nsIDocumentTransformer.init to take an nsIDocument rather than an nsIPrincipal.

Then change txMozillaXSLTProcessor::LoadStyleSheet, TX_LoadSheet and txCompileObserver::txCompileObserver to not take a loadgroup.

Then change txCompileObserver::startLoad to get the document from mProcessor and get both principal and loadgroup from that document.

Then pass principal, loadgroup and document to NS_NewChannel

I haven't looked at all the other code paths yet.
Comment on attachment 8486690 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

Review of attachment 8486690 [details] [diff] [review]:
-----------------------------------------------------------------

-'ing because the change in TX_LoadSheet doesn't actually do anything and it seems you expect it to.

::: dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
@@ +433,5 @@
>      if (NS_CP_REJECTED(shouldLoad)) {
>          return NS_ERROR_DOM_BAD_URI;
>      }
>  
> +    // ckerschb: we shouldn't use nullptr for the node, can we get it somehow?

There often is no node, because we don't create nodes for the stylesheet. I don't know what these nodes are used for, but if we really need one and we don't actually care that it's not the stylesheet-loading element, then we could use the source document. That would mean storing it in the txCompileObserver, passed in from TX_LoadSheet through the txCompileObserver constructor.

@@ +534,5 @@
>      nsRefPtr<txStylesheetCompiler> compiler =
>          new txStylesheetCompiler(NS_ConvertUTF8toUTF16(spec), observer);
>      NS_ENSURE_TRUE(compiler, NS_ERROR_OUT_OF_MEMORY);
>  
> +    nsCOMPtr<nsINode> requestingNode(do_QueryInterface(aProcessor->GetSourceContentModel()));

AFAICT this will be null, since we call LoadStyleSheet (and TX_LoadSheet) before we call SetSourceContentModel. You'll probably have to pass mDocument from nsXMLContentSink::LoadXSLStyleSheet to LoadStyleSheet and on to TX_LoadSheet.
Attachment #8486690 - Flags: review?(peterv) → review-
(In reply to Jonas Sicking (:sicking) from comment #253)
> So either the principal of the stylesheet that contains the
> <xsl:import>/<xsl:include>, or the principal of the source document for
> <?xml-stylesheet href=...?>.

We use the source document's principal for <?xml-stylesheet href=...?> and GetSimpleCodebasePrincipal on the referrer uri to calculate the principal for <xsl:import>/<xsl:include>, so that part is ok already?

> For document() loads, I guess we should pass in the source document as the
> document, and the principal of the stylesheet that contained the xpath
> expression as the principal.

Passing in the document would mean changing nsSyncLoadService::LoadDocument so that we can pass it in. And we'd need a way to communicate the principal to txParseDocumentFromURI (storing it in the DocumentFunctionCall?).
(In reply to Peter Van der Beken [:peterv] from comment #255)
> 3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch
> 
> Review of attachment 8486690 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> -'ing because the change in TX_LoadSheet doesn't actually do anything and it
> seems you expect it to.

The primary goal for all the patches is to basically duplicate current logic and put the same information (node,principal) that we use to call contenPolicies into the LoadInfo.

> > +    // ckerschb: we shouldn't use nullptr for the node, can we get it somehow?
> There often is no node, because we don't create nodes for the stylesheet. I
> don't know what these nodes are used for, but if we really need one and we
> don't actually care that it's not the stylesheet-loading element, then we
> could use the source document. That would mean storing it in the
> txCompileObserver, passed in from TX_LoadSheet through the txCompileObserver
> constructor.

Right, there is no node in that case.
http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp#428
I guess my comment was misleading. For now I suggest we use 'nullptr' and file a followup to actually should pass the source-document around and also use it for contentPolicies. In general, your suggestion seems right to me, so let's incorporate that change in the follow up.

> @@ +534,5 @@
> >      nsRefPtr<txStylesheetCompiler> compiler =
> >          new txStylesheetCompiler(NS_ConvertUTF8toUTF16(spec), observer);
> >      NS_ENSURE_TRUE(compiler, NS_ERROR_OUT_OF_MEMORY);
> >  
> > +    nsCOMPtr<nsINode> requestingNode(do_QueryInterface(aProcessor->GetSourceContentModel()));
> 
> AFAICT this will be null, since we call LoadStyleSheet (and TX_LoadSheet)
> before we call SetSourceContentModel. You'll probably have to pass mDocument
> from nsXMLContentSink::LoadXSLStyleSheet to LoadStyleSheet and on to
> TX_LoadSheet.

If that is indeed null, than it's also null in the contentPolicy check right above:
http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp#514
which means we are still duplicating current logic, which seems fine at the moment, but of course we will follow up on it. Again, your suggestion seems right to me in that case as well.

To sum it up, we will file a follow up where in the end we will use the same document/node that we use for contentPolicies in our loadInfo.
Blocks: 1066833
Blocks: 1066843
Blocks: 1066868
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #257)
> If that is indeed null, than it's also null in the contentPolicy check right
> above:
> http://mxr.mozilla.org/mozilla-central/source/dom/xslt/xslt/
> txMozillaStylesheetCompiler.cpp#514
> which means we are still duplicating current logic, which seems fine at the
> moment, but of course we will follow up on it.

It's possible that the existing logic is buggy, my suggestion is to actually fix it. But if you don't want to do that then you should explicitly pass in null instead of pretending that we're passing in something by calling GetSourceContentModel.
Blocks: 1067468
Blocks: 1067471
Blocks: 1067517
As discussed on IRC and over email, let's just call NS_NewChannel[Principal]() for now in xslt/txMozillaStylesheetCompiler.cpp leaving the nullptr Node for a future patch. I filed Bug 1067517 where we can follow up on the problem.
Attachment #8486690 - Attachment is obsolete: true
Attachment #8489542 - Flags: review?(peterv)
Attachment #8489542 - Flags: review?(bzbarsky)
The following followup bugs were filed:
* https://bugzilla.mozilla.org/show_bug.cgi?id=1066833 - followup to determine if TYPE_IMAGE, TYPE_STYLESHEET, and TYPE_SCRIPT callsites should pass the loading element instead or in additon to the loading document (or at least to try and figure out what they should take)

* https://bugzilla.mozilla.org/show_bug.cgi?id=1066843 - nsISound::Play shouldn't pass systemprincipal

* https://bugzilla.mozilla.org/show_bug.cgi?id=1066868 - fix the way we get principal/context in nsDocShell file followup bug for docshell

* https://bugzilla.mozilla.org/show_bug.cgi?id=1066826 - Remove mozislocallyavailable api from navigator.cpp

* https://bugzilla.mozilla.org/show_bug.cgi?id=1067517 - followup from comment 254-259 to fix code in txMozillaStylesheetCompiler.cpp
Comment on attachment 8489542 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

Why do we need the copy/paste bits in the plugin code?  Why not just use NS_NewChannelInternal and pass both doc and principal?  Or does that expect a non-null doc?
(In reply to Boris Zbarsky [:bz] from comment #261)
> Why do we need the copy/paste bits in the plugin code?  Why not just use
> NS_NewChannelInternal and pass both doc and principal?  Or does that expect
> a non-null doc?

The doc is allowed to be null if we call NS_NewChannelInternal(). The ultimate goal here is to call NS_NewChannel[Node] wherever we can and completely eliminate NS_NewChannelInternal to be called from anything outside of nsNetutil.h at some point.

If you want we can call NS_NewChannlInternal() getting the principal of the doc, if there is one, and otherwise use the nullPrincipal.

To answer your question, we agreed that we should try to avoid calling NS_NewChannelInternal() wherever we can. That's the only reason.
> If you want we can call NS_NewChannlInternal() getting the principal of the doc, if there > is one, and otherwise use the nullPrincipal.

I'd prefer that to having two separate NS_NewChannel calls, I think.

And please file a followup to simplify this if/when we can prove that the doc is in fact never null here.
(In reply to Boris Zbarsky [:bz] from comment #263)
> I'd prefer that to having two separate NS_NewChannel calls, I think.

Sure thing - here it is.

> And please file a followup to simplify this if/when we can prove that the
> doc is in fact never null here.

Sure will do.
Attachment #8489542 - Attachment is obsolete: true
Attachment #8489542 - Flags: review?(peterv)
Attachment #8489542 - Flags: review?(bzbarsky)
Attachment #8489810 - Flags: review?(bzbarsky)
Attachment #8489810 - Flags: review?(peterv)
Comment on attachment 8489810 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

r+ on the XSLT/pretty printer bits.
Attachment #8489810 - Flags: review?(peterv) → review+
Comment on attachment 8489810 [details] [diff] [review]
3_05_callsites_in_dom_base-xbl-xml-xslt-plugins.patch

r=me on the plugin/dom/xbl bits.
Attachment #8489810 - Flags: review?(bzbarsky) → review+
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=4120e1cbb5b4
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 1071497
Depends on: 1071805
(In reply to Boris Zbarsky [:bz] from comment #197)
> > Isn't aCX a JS-context?
> 
> No.  It's the random nsISupports argument to imgILoader.loadImage that's
> supposed to represent some sort of "load context" for the image.  In
> practice, it's a document, and we should stop the silly pretense that
> imagelib is somehow standalone and change the signature to be nsIDocument
> and rename the argument.
> 
> Trying to QI an actual JSContext* wouldn't compile, since it's not
> nsISupports.  ;)

Filed followup bug https://bugzilla.mozilla.org/show_bug.cgi?id=1073179 for imgLoader to switch from nsISupports to nsIDocument
Depends on: 1073282
Can someone here look at
Bug 1073838 - TB build bustage (comm-central): NS_NewStreamLoader and NS_NewChannel 
and see what changes are necessary there to compile TB successfully?

TIA
See Also: → 1073838
Blocks: 647010
https://hg.mozilla.org/mozilla-central/rev/d0f7f15e15fb
https://hg.mozilla.org/mozilla-central/rev/d20696b5c309
https://hg.mozilla.org/mozilla-central/rev/666d31a3753a
https://hg.mozilla.org/mozilla-central/rev/2e52e8387850
https://hg.mozilla.org/mozilla-central/rev/8511e9341536
https://hg.mozilla.org/mozilla-central/rev/8175d676cc6f
https://hg.mozilla.org/mozilla-central/rev/a12b926cff35
https://hg.mozilla.org/mozilla-central/rev/b968440b6204
https://hg.mozilla.org/mozilla-central/rev/5609cb03aacc
https://hg.mozilla.org/mozilla-central/rev/cc656384d43e
https://hg.mozilla.org/mozilla-central/rev/0dabb0a747fc
https://hg.mozilla.org/mozilla-central/rev/2d6e88e962a7
https://hg.mozilla.org/mozilla-central/rev/7fc6512d34b1
https://hg.mozilla.org/mozilla-central/rev/de85d97eac27
https://hg.mozilla.org/mozilla-central/rev/55806e3e450f
https://hg.mozilla.org/mozilla-central/rev/20bb35fc802a
https://hg.mozilla.org/mozilla-central/rev/4afe0fb6dd86
https://hg.mozilla.org/mozilla-central/rev/1a1712c6b142
https://hg.mozilla.org/mozilla-central/rev/2382ff174f22
https://hg.mozilla.org/mozilla-central/rev/218ea09a7286
https://hg.mozilla.org/mozilla-central/rev/3910656e0a2c
https://hg.mozilla.org/mozilla-central/rev/da34da9570d2
https://hg.mozilla.org/mozilla-central/rev/5ea42524c416
https://hg.mozilla.org/mozilla-central/rev/ecc3d0491803
https://hg.mozilla.org/mozilla-central/rev/d4ced32fc77b
https://hg.mozilla.org/mozilla-central/rev/02bc405eeb7a
https://hg.mozilla.org/mozilla-central/rev/49775ca3647f
https://hg.mozilla.org/mozilla-central/rev/5b066ef38ccf
https://hg.mozilla.org/mozilla-central/rev/d6bd6192791b
https://hg.mozilla.org/mozilla-central/rev/8dbb44fb03c1
https://hg.mozilla.org/mozilla-central/rev/6812665c79b3
https://hg.mozilla.org/mozilla-central/rev/9ddccdec7224
https://hg.mozilla.org/mozilla-central/rev/68e95822939b
https://hg.mozilla.org/mozilla-central/rev/c09922195848
https://hg.mozilla.org/mozilla-central/rev/4120e1cbb5b4
Blocks: 1076978
Blocks: 947079
Blocks: 1083422
Blocks: 1092055
Depends on: 1104623
Not sure if it's worth a new bug, but there's a typo in one of the patches here: https://hg.mozilla.org/mozilla-central/rev/666d31a3753a#l17.40

This check duplicates the one right above it and was probably supposed to be removed along with the SetLoadInfo call:
   17.52 -  rv = channel->SetLoadInfo(loadInfo);
   17.53    if (rv.Failed()) {
   17.54      return nullptr;
   17.55    }
(In reply to Nickolay_Ponomarev from comment #272)
> Not sure if it's worth a new bug, but there's a typo in one of the patches
> here: https://hg.mozilla.org/mozilla-central/rev/666d31a3753a#l17.40
> 
> This check duplicates the one right above it and was probably supposed to be
> removed along with the SetLoadInfo call:
>    17.52 -  rv = channel->SetLoadInfo(loadInfo);
>    17.53    if (rv.Failed()) {
>    17.54      return nullptr;
>    17.55    }

Yes, that is a typo and yes, I don't think we need to file a new bug for the removal of those 3 lines. Thanks for pointing it out.
Attachment #8601219 - Flags: review?(jonas)
Depends on: 1198559
You need to log in before you can comment on or make changes to this bug.