1439713 - Change nsIContentPolicy shouldLoad to take an nsILoadInfo instead of the various arguments

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Christoph Kerschbaumer [:ckerschb]]

Now that legacy extensions are gone, we can finally change nsIContentPolicy shouldLoad to take a loadInfo instead of the various arguments it does now [1].

In particular, it should look like:
short shouldLoad(in nsIURI aContentLocation,
                 in nsILoadInfo aLoadInfo);

Doing so allows us for example to flag the loadinfo within contentPolicy implementations for later processing instead of having to do the various workarounds we do at the moment. E.g. for upgrade-insecure-requests. Also contentpolices have the entire information available stored in the loadInfo instead of having to rely only on the 5 args passed in.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsIContentPolicy.idl#457

Comment 1

[Boris Zbarsky [:bzbarsky]]

Can we just pass the channel instead of the (URI, loadinfo) pair?  Or are content policies being called before we set up the channel?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> Can we just pass the channel instead of the (URI, loadinfo) pair?  Or are
> content policies being called before we set up the channel?

I think that should be doable. There is one case within docshell that I know of, but in that case we don't have a loadinfo yet either. Probably we need to special case that anyway. Other than that I think it's better to pass a channel than a <uri, loadinfo> pair.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

Hey Boris,

I wanted to run this by you to make sure we are on the right track here. As already discussed, ideally we pass a channel to NS_CheckContentLoadPolicy() and ultimately shouldLoad(). I guess we also should also keep the mimetype. While this approach seems to work pretty well it has the downside that for the following callsites we have to create a dummychannel:

* nsDocShell.cpp
* nsContentUtils.cpp
* nsObjectLoadingContent.cpp
* ScriptLoader.cpp
* ServiceWorkerManager.cpp
* WebSocket.cpp
* nsXMLContentSink.cpp
* imgLoader.cpp
* FontFaceSet.cpp
* Loader.cpp

What do you think? Acceptable?

Comment 4

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> I wanted to run this by you to make sure we are on the right track here. As
> already discussed, ideally we pass a channel to NS_CheckContentLoadPolicy()
> and ultimately shouldLoad(). I guess we also should also keep the mimetype.
> While this approach seems to work pretty well it has the downside that for
> the following callsites we have to create a dummychannel:

I suspect that most of these can eventually be refactored to have a channel available at the time we do the checks. But in the mean time, I think having a dummy channel is a good compromise for the sake of having a consistent and reasonable interface.

Can we add a NS_CheckContentLoadPolicy() overload that creates the dummy channel based on the context node and (optional) triggering principal, though, rather than duplicating the dummy channel logic everywhere we need to do this?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actually, if we have this many places that don't have a channel, it's worth revisiting the assumption that we will have a channel.  Do all of them have a (URI, loadinfo) pair?

Comment 6

[Boris Zbarsky [:bzbarsky]]

And in particular, there are all sorts of problems with dummy channels, so I would rather avoid them if we can.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> Actually, if we have this many places that don't have a channel, it's worth
> revisiting the assumption that we will have a channel.  Do all of them have
> a (URI, loadinfo) pair?

Most of the NS_CheckContentLoadPolicy() calls got removed from the various places within the codebase when we started to move content security checks into AsyncOpen2(). The remaining places where we still call NS_CheckContentLoadPolicy() have to perform additional security checks. In those places we don't have a channel available and unfortunately also not a loadInfo. So even if we would just pass a <uri, loadInfo> pair we would still have to create a tmp loadInfo. I don't have a strong opinion whether to pass a channel or a <uri, loadinfo> pair.

I know it's suboptimal to create a tmp channel or even a tmp loadInfo but the benefit would be that content policies have all the information from the loadInfo available and not just the specific args passed in at the moment. In particular, I when moving the CSP from the principal to the loadInfo (Bug 965637) that would be very beneficial.

To get a better feeling of how the new world would look like I attached a patch that passes all CSP and Mixed Content Blocker tests locally. Any chance I could convince you that it makes sense to change the interface even though it has the downside of creating a tmp channel or tmp LoadInfo?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I have no problem with creating a tmp loadinfo.  It's a much saner proposition than creating a dummy channel:

1)  The loadinfo API is much smaller than the channel API, so there's 
    less that the callees can do with it to mess up.
2)  We can actually pass in a loadinfo, albeit not the "real" one for the load.
    We don't have to implement some sort of dummy class.  And implementing a 
    dummy channel class that is not all sorts of broken is a nontrivial task.  At
    the same time, passing a non-dummy channel but no the one the load will
    actually happen on has its own confusion problems.
3)  A loadinfo has a lot less mutable state than a channel does, so it's less likely
    that people will write content policies that mutate its state.
4)  If we decide that we want to ignore mutations made by content policies,
    we can always pass a clone of the "real" channel loadinfo.

The biggest danger is not setting up the tmp loadinfo correctly.  But since these are _additional_ checks, and we still do checks on the real loadinfo later, even that is somewhat mitigated, right?

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)

A agree, with all of what you write above. In particular going back to using <uri, loadInfo> instead of setting up a channel simplifies the interface quite a bit. E.g. each callsite does not have to pull the right URI out of the channel and does not have to perform fixups like for websockets or also wyciwyg channels.

> The biggest danger is not setting up the tmp loadinfo correctly.  But since
> these are _additional_ checks, and we still do checks on the real loadinfo
> later, even that is somewhat mitigated, right?

Yes, those are additional checks we are performing. Mostly for loading cached resources. Before any data is loaded over the wire we *always* perform the security checks we already have in place within AsyncOpen2().

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

Boris, I split the changeset up into 4 pieces:

1) bug_1439713_change_shouldload.patch
All the required changes to nsIContentPolicy. Please note that I defined variables at top of each function using a leading 'a' on purpose to keep the patch as small as possible. E.g. I am still using aContentPolicyType. If you disagree I can change that for the final patch.

2) bug_1439713_extra_arg_to_loadinfo.patch
Within Bug 1329288 we used 'aExtra' to transfer info from docShell.cpp into WebRequestContent.js. We can simply set a flag on the loadinfo to transfer that information now.

3) bug_1439713_test_updates.patch
Updates tests relying on nsIContentPolicy.

4) bug_1439713_change_shouldload_addoncompat.patch
I separated changes from toolkit/components/addoncompat because I don't fully understand the setup there, e.g. is | cpows.node = node; | still needed after the change of contentPolices, or do we just need to serialize the loadInfo. I will flag kmag and mconley for ni? on that part.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_extra_arg_to_loadinfo.patch

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_test_updates.patch

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload_addoncompat.patch

:kmag, :mconley can you help me understand of RemoteAddonsChild.jsm and RemoteAddonsParent.jsm work? In particular, when changing the API of nsIContentPolicy do we still need all the code that relies on |node|. E.g. |cpows.node = node;| and such? Or can we just serialize and deserialize the loadinfo an that's it? There are also timers, are those still needed after the change?

Comment 14

[Kris Maglione [:kmag]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Created attachment 8956889 [details] [diff] [review]
> bug_1439713_change_shouldload_addoncompat.patch
> 
> :kmag, :mconley can you help me understand of RemoteAddonsChild.jsm and
> RemoteAddonsParent.jsm work? In particular, when changing the API of
> nsIContentPolicy do we still need all the code that relies on |node|. E.g.
> |cpows.node = node;| and such? Or can we just serialize and deserialize the
> loadinfo an that's it? There are also timers, are those still needed after
> the change?

Those are going away. If you're blocked on them, I can remove them in the next day or two.

Comment 15

[Mike Conley (:mconley) (:⚙️)]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #14)
> Those are going away. If you're blocked on them, I can remove them in the
> next day or two.

This is the best option, yes.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #14)
> Those are going away. If you're blocked on them, I can remove them in the
> next day or two.

Code removal hurray. Yeah, would be great if you can remove those, because other than that the bug is ready to go.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8956889 [details] [diff] [review]
bug_1439713_change_shouldload_addoncompat.patch

Marking as obsolete because the code in that patch will be removed.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

Boris, I just found a flaw in the old patch which I updated within this one. In the old version I used |loadinfo.loadingDocument| as the context/node within:
* mobile/android/components/ImageBlockingPolicy.js
* toolkit/modules/addons/WebRequestContent.js

which is wrong; in particular for iframe loads. When we still used to pass the context on the callsite of shouldLoad (e.g. within the ContentSecurityManager) we used to use the loadingNode which was not available within JS land. I updated the loadinfo to expose the loadingNode to JS and now I query that loadingNode within the 2 js implementations of shouldLoad.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8957522 [details] [diff] [review]
bug_1439713_change_shouldload.patch

>+++ b/docshell/base/nsDocShell.cpp
>+    // Special LoadInfo Constructor only available for loads of TYPE_DOCUMENT,

That's a preexisting constructor, looks like?

Please file a followup to:

1)  Make this constructor protected.
2)  Create a DocumentLoadInfo subclass of LoadInfo which just has
    a constructor that calls the protected constructor.  

That will make it harder for someone to do the wrong thing and use the special constructor for a non-document load.

In any case, the comment here should just say that we want to use basically the same loadinfo as in DoURILoad.  And comment there saying that it should match this one when both are applicable.

Comparing the two callsites, why is this using SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED?  Seems like it should be at least SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL or something.  Otherwise this check will _always_ fail for something like <a href="data:..." target="newwindowname"> or <a href="cross-origin-stuff">, no?  Not sure whether it needs to faithfully duplicate the other security flags.  Certainly we should add tests for these cases.

>-                                   requestingContext,

So this is a behavior change for the e10s case.  Right now we pass our window; we will start to pass null.  There should at least be something in the commit message explaining why the behavior change.

>                                    EmptyCString(),  // mime guess

As a followup, we should think about what to do with this mime guess thing.  It might make sense to pull it into loadinfo too, or kill it off entirely.

>+++ b/dom/base/nsContentPolicy.cpp
> nsContentPolicy::CheckPolicy(CPMethod          policyMethod,
>+    nsCOMPtr<nsIURI> requestingLocation;

You need to actually set that, since this function uses it.

>-        if (mMixedContentBlocker == entries[i] || mCSPService == entries[i]) {

You can remove the mMixedContentBlocker and mCSPService members.  This was the only use.

>+++ b/dom/base/nsContentUtils.cpp
+  nsCOMPtr<nsINode> requestingNode = do_QueryInterface(aContext);

The only caller of CanLoadImage is passing an nsIContent* anyway.  Just change the type of the arg to nsINode*, here or as a followup.

Also please file a followup to remove the last two args of CanLoadImage, since they're unused.

>+                 nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

Why is this the right security flag?  Image loads normally do _not_ require same-origin.

>+++ b/dom/base/nsDataDocumentContentPolicy.cpp
>+  uint32_t aContentType = aLoadInfo->GetExternalContentPolicyType();

The naming makes sense to make it clearer what is or is not changing in this changeset.  But please file a followup to rename without the leading 'a' once this lands.  That applies to a number of places in this patch.

>+++ b/dom/base/nsObjectLoadingContent.cpp
>@@ -1469,24 +1470,27 @@ nsObjectLoadingContent::CheckLoadPolicy(
>+                 doc,

The context used to be thisContent.  Why is it being changed to doc?

>+                 nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

I have a very hard time believing this is the right security flag.

>@@ -1522,25 +1526,28 @@ nsObjectLoadingContent::CheckProcessPoli

Same issues here: wrong loading context, wrong security flag.  Needs tests.

>+++ b/dom/html/ImageDocument.cpp
>-  nsresult rv = NS_CheckContentProcessPolicy(nsIContentPolicy::TYPE_INTERNAL_IMAGE,

So... the type of the loadinfo here (presumably DOCUMENT or SUBDOCUMENT) is not the type we need here.  We need to clone the loadinfo with the right type or something.

We should probably have a test for this.

>+++ b/dom/script/ScriptLoader.cpp
>+                               nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

Again, this doesn't look like the right sec flag.

>+++ b/dom/security/nsContentSecurityManager.cpp
>-      requestingContext = aLoadInfo->ContextForTopLevelLoad();

So all the places where we are now getting the context from the loadinfo's LoadingNode() are wrong for TYPE_DOCUMENT loads, right?  That needs to be fixed.

>+++ b/dom/xml/nsXMLContentSink.cpp
>@@ -714,25 +715,28 @@ nsXMLContentSink::MaybeProcessXSLTLink(
>+                               mDocument,

The context used to be ToSupports(aProcessingInstruction).  Why the change?

>+                               nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

Is this the right thing?

>+++ b/image/imgLoader.cpp
>+                 nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

Pretty sure this is not how image loads work.

>+++ b/layout/style/FontFaceSet.cpp
>+    new mozilla::net::LoadInfo(mDocument->NodePrincipal(), // loading principal

This can just be "net::LoadInfo".  

That said, why did |new LoadInfo| with bareword LoadInfo work elsewhere?  Maybe because they were including ipc/glue/BackgroundUtils.h somehow and that does "using namespace mozilla::net" in the header?  Would be better to qualify as net::LoadInfo everwhere that "uses namespace mozilla".

Also, please file a followup bug to remove that "using" bit from BackgroundUtils.h.  That sort of thing in headers is not ok.

>+                               nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

Not how font loads work either.

>+++ b/layout/style/Loader.cpp
>+  nsCOMPtr<nsINode> requestingNode = do_QueryInterface(aContext);

Please file a followup; all callers of this method have an nsINode context, so the arg type should just be nsINode*.

>+                               nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED,

Not how stylesheet loads work.

>+++ b/netwerk/base/LoadInfo.cpp
>+  nsCOMPtr<nsINode> node = do_QueryReferent(mLoadingContext);
>+  if (node) {
>+    nsCOMPtr<nsIDOMNode> domNode = node->AsDOMNode();
>+    domNode.forget(aResult);
>+  }

This seems like a complicated way of writing:

  nsCOMPtr<nsIDOMNode> node = do_QueryReferent(mLoadingContext);
  node.forget(aResult);

But also, we are trying to get rid of nsIDOM* bits; we shouldn't be adding new uses.  Since this function is just for JS consumers, it can just as easily return nsISupports (with a comment about how it should be Node once bug 1444991 is fixed); xpconnect + bindings will do the right thing.  And if aResult is nsISupports**, then I expect you can just do:

  nsCOMPtr<nsINode> node = LoadingNode();
  node.forget(aResult);

>+  nsCOMPtr<nsISupports> context = do_QueryReferent(mContextForTopLevelLoad);

This should do:

  nsCOMPTr<nsISupports> context = ContextForTopLevelLoad();

and not duplicate the logic (including asserts) from that function.

Also, please file a followup to make ContextForTopLevelLoad() return already_AddRefed<nsISupports>; what it's doing right now is incredibly unsafe!

>+++ b/netwerk/base/nsILoadInfo.idl
>+  readonly attribute nsISupports topLevelLoadContext;

This naming doesn't match the documentation.  It should probably be called contextForTopLevelLoad.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8956886 [details] [diff] [review]
bug_1439713_extra_arg_to_loadinfo.patch

>+LoadInfo::SetSkipContentPolicyCheckForWebRequest(bool aSkipContentPolicyCheckForWebRequest)

Just name the arg aSkip?

>+LoadInfo::GetSkipContentPolicyCheckForWebRequest(bool* aSkipContentPolicyCheckForWebRequest)

And here.

r=me

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8956887 [details] [diff] [review]
bug_1439713_test_updates.patch

r=me

I assume you're going to fold these patches together before landing, right?

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)

Boris, it seems I forgot to clearly communicate the choice of the security flags to the tmp loadInfo. Let me clarify, for all of those 'explicit' nsIContentPolicy checks we only create a tmp Loadinfo so the security flags do not get evaluated and are in fact unused, hence I initially applied the most restrictive flag for all of the secCheckLoadInfos. But since you brought it up, I had a better idea: I created a new security flag 'SEC_ONLY_FOR_EXPLICIT_CONTENTSEC_CHECK' which can only be used for those tmp loadInfos. Within the contentSecurityManager we have a function called ValidateSecurityFlags() to make sure that that security flag can never be used on a loadinfo for an actual channel. Having said that, I guess there is no need for new tests, right?

> >+++ b/docshell/base/nsDocShell.cpp
> >+    // Special LoadInfo Constructor only available for loads of TYPE_DOCUMENT,
> That's a preexisting constructor, looks like?
> 
> Please file a followup to:
> 1)  Make this constructor protected.

Completely agree, I filed: Bug 1446916 - Create subclass of LoadInfo explicit for load of TYPE_DOCUMENT

> >-                                   requestingContext,
> 
> So this is a behavior change for the e10s case.  Right now we pass our
> window; we will start to pass null.  There should at least be something in
> the commit message explaining why the behavior change.

Agreed, updated the commit message to reflect that change.

> >                                    EmptyCString(),  // mime guess
> 
> As a followup, we should think about what to do with this mime guess thing. 
> It might make sense to pull it into loadinfo too, or kill it off entirely.

Filed Bug 1446918 - Consider putting the mime type guess into the LoadInfo.

> >+++ b/dom/base/nsContentPolicy.cpp
> > nsContentPolicy::CheckPolicy(CPMethod          policyMethod,
> >+    nsCOMPtr<nsIURI> requestingLocation;
> You need to actually set that, since this function uses it.

I looked at that part actually and my thinking was the the code there is more accurate. It tries to query the URI from the document first if there is one VS using the URI of the loadingPrincipal. In case of a data: URI load the requestingLocation might be a moz-nullprincipal vs the data: URI, right? Not sure what we prefer - for now I followed your suggestion.

> >+++ b/dom/base/nsContentUtils.cpp
> +  nsCOMPtr<nsINode> requestingNode = do_QueryInterface(aContext);
> 
> The only caller of CanLoadImage is passing an nsIContent* anyway.  Just
> change the type of the arg to nsINode*, here or as a followup.

Hah, makes sense. Also filed:
Bug 1446921 - Remove last two arguments of nsContentUtils::CanLoadImage because they are unused

> >+++ b/dom/base/nsDataDocumentContentPolicy.cpp
> >+  uint32_t aContentType = aLoadInfo->GetExternalContentPolicyType();
> 
> The naming makes sense to make it clearer what is or is not changing in this
> changeset.  But please file a followup to rename without the leading 'a'
> once this lands.  That applies to a number of places in this patch.

I filed Bug 1446922 - Remove the leading 'a' from variables within the various ::ShouldLoad() implementations
:vino can help us address these follow ups. I will make sure that happens soonish.

> >+++ b/dom/base/nsObjectLoadingContent.cpp
> >@@ -1469,24 +1470,27 @@ nsObjectLoadingContent::CheckLoadPolicy(
> >+                 doc,
> 
> The context used to be thisContent.  Why is it being changed to doc?

So, this is an artifact of the Loadinfo constructor. In case consumers pass a principal as well as a context, then we assert the the principal matches the NodePrincipal of the context:

MOZ_ASSERT(!aLoadingContext || !aLoadingPrincipal ||
           aLoadingContext->NodePrincipal() == aLoadingPrincipal); 
See:
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#116-119

Since the initial version was passing doc->NodePrincipal() I was assuming we can use the doc as the loadingContext, but not entirely sure which way is better, using the same loadingPrincipal as in the old version or using the old context. Or are both wrong and we need a new loadinfo constructor?

> >+++ b/dom/html/ImageDocument.cpp
> >-  nsresult rv = NS_CheckContentProcessPolicy(nsIContentPolicy::TYPE_INTERNAL_IMAGE,
> 
> So... the type of the loadinfo here (presumably DOCUMENT or SUBDOCUMENT) is
> not the type we need here.  We need to clone the loadinfo with the right
> type or something.

Good catch, I updated the patch to query the corresponding arguments from the loadinfo like in the version that's currently in the tree. But again, we are facing the same problem as within nsObjectLoadingContent where the NodePrincipal and the channelPrincipal do not match.

> We should probably have a test for this.

How relevant is ShouldProcess these days? Would it make sense to re-evaluate it's existence?
Anyway, we have a test that exercises this codepath, it's dom/security/test/csp/test_ping.html which causes an assertion failure at the end of the test that the loadingPrincipal does not match the NodePrincipal() if we pass the channelPrincipal instead of the actual NodePrincipal() which the I ende up using.

> >+++ b/dom/security/nsContentSecurityManager.cpp
> >-      requestingContext = aLoadInfo->ContextForTopLevelLoad();
> 
> So all the places where we are now getting the context from the loadinfo's
> LoadingNode() are wrong for TYPE_DOCUMENT loads, right?  That needs to be
> fixed.

That is correct. I updated that throught the patch.


> >+++ b/dom/xml/nsXMLContentSink.cpp
> >@@ -714,25 +715,28 @@ nsXMLContentSink::MaybeProcessXSLTLink(
> >+                               mDocument,
> 
> The context used to be ToSupports(aProcessingInstruction).  Why the change?

Same problem as described within nsObjectLoadingContent.cpp above.

> >+++ b/layout/style/FontFaceSet.cpp
> >+    new mozilla::net::LoadInfo(mDocument->NodePrincipal(), // loading principal
> 
> This can just be "net::LoadInfo".  
> 
> That said, why did |new LoadInfo| with bareword LoadInfo work elsewhere? 
> Maybe because they were including ipc/glue/BackgroundUtils.h somehow and
> that does "using namespace mozilla::net" in the header?  Would be better to
> qualify as net::LoadInfo everwhere that "uses namespace mozilla".

That is possible and I agree with you. Updated!

> Also, please file a followup bug to remove that "using" bit from
> BackgroundUtils.h.  That sort of thing in headers is not ok.

I filed Bug 1446933 - Remove 'using namespace mozilla::net' from BackgroundUtils.h

> >+++ b/layout/style/Loader.cpp
> >+  nsCOMPtr<nsINode> requestingNode = do_QueryInterface(aContext);
> 
> Please file a followup; all callers of this method have an nsINode context,
> so the arg type should just be nsINode*.

Filed Bug 1446935 - Callsites ending up calling NS_CheckContentLoadPolicy can use nsINode isntead of nsISupports

> >+++ b/netwerk/base/LoadInfo.cpp
> But also, we are trying to get rid of nsIDOM* bits; we shouldn't be adding
> new uses.  Since this function is just for JS consumers, it can just as
> easily return nsISupports

Ok, didn't know that - updated!

> Also, please file a followup to make ContextForTopLevelLoad() return
> already_AddRefed<nsISupports>; what it's doing right now is incredibly
> unsafe!

Filed Bug 1446937 - Have ContextForTopLevelLoad return already_AddRefed<nsISupports> within LoadInfo

> >+++ b/netwerk/base/nsILoadInfo.idl
> >+  readonly attribute nsISupports topLevelLoadContext;
> 
> This naming doesn't match the documentation.  It should probably be called
> contextForTopLevelLoad.

Updated!

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

Forgot to qfold one of my patches. Anyway, please see my answers in Comment 22 - thanks!

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8960149 [details] [diff] [review]
bug_1439713_change_shouldload.patch

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

::: dom/html/ImageDocument.cpp
@@ +108,4 @@
>  
>    int16_t decision = nsIContentPolicy::ACCEPT;
> +  nsresult rv = NS_CheckContentProcessPolicy(channelURI,
> +                                             secCheckLoadInfo,

Locally that worked for all of the contentpolicy tests I ran, but when pushing to TRY I realized that the requestingNode might be null, hence I guess we have to do the following and fall back to the channelPrincipal as the old version did:

   nsCOMPtr<nsINode> requestingNode = domWindow->GetFrameElementInternal();
+  nsCOMPtr<nsIPrincipal> loadingPrincipal;
+  if (requestingNode) {
+    loadingPrincipal = requestingNode->NodePrincipal();
+  }
+  else {
+    nsContentUtils::GetSecurityManager()->
+      GetChannelResultPrincipal(channel, getter_AddRefs(loadingPrincipal));
+  }
 
   nsCOMPtr<nsILoadInfo> secCheckLoadInfo =
-    new net::LoadInfo(requestingNode->NodePrincipal(),
+    new net::LoadInfo(loadingPrincipal,
                       loadInfo ? loadInfo->TriggeringPrincipal() : nullptr,
                       requestingNode,
                       nsILoadInfo::SEC_ONLY_FOR_EXPLICIT_CONTENTSEC_CHECK,
                       nsIContentPolicy::TYPE_INTERNAL_IMAGE);

Comment 25

[Boris Zbarsky [:bzbarsky]]

> we only create a tmp Loadinfo so the security flags do not get evaluated and are in fact unused

I see.  Are they going to stay unused?

I guess it's ok.  I like your proposed SEC_ONLY_FOR_EXPLICIT_CONTENTSEC_CHECK.

> I guess there is no need for new tests, right?

If none of the content policies use the flags, right.

> Agreed, updated the commit message to reflect that change.

Hmm.  So my question is: was someone depending on this?

Why is continuing to pass the thing we used to pass impossible in the loadinfo setup?  Loadinfo has a concept of "context for toplevel load", right?

> Not sure what we prefer - for now I followed your suggestion.

I'd prefer to keep existing behavior in the mass-change in this bug and change it separately if we want to.

> In case consumers pass a principal as well as a context, then we assert the the principal matches the
> NodePrincipal of the context

OK.  But in this case the context used to be "thisContent" and "doc" is "thisContent->OwnerDoc()".  So thisContent->NodePrincipal() == doc->NodePrincipal().  It's just faster to get it from "doc".

> but not entirely sure which way is better

Passing "thisContent" is clearly better, because it indicates where the load is actually happening.  This is a must for adblockers and the like.

The whole point of the context arg is to provide _more_ information than the principals provide...

> But again, we are facing the same problem as within nsObjectLoadingContent where
> the NodePrincipal and the channelPrincipal do not match.

Right.  This case is harder, because the channel principal is the thing we loaded while the node is the iframe we're loading into.  

Using LoadInfo for the ShouldProcess case is a bit complicated, as this example shows, because its state is not a good fit for the concept of "processing" (unlike "loading", where it's a good fit by definition).

> How relevant is ShouldProcess these days?

That's a good question.  It looks like webextensions don't really expose it.  We should check what our internal policies look like and see.

Specifically for this case, we should at least check what happens and what should happen if images are disabled by pref (which we implement via policy) and then you navigate to an image url.  And in general think about any policies which do something other than "accept" in ShouldProcess.

For now, passing the NodePrincipal() of the frame element here is probably ok.  Except when the frame element is null, in which case I expect your patch to crash (image load in toplevel window).

> Same problem as described within nsObjectLoadingContent.cpp above.

Same answer: the PI and the document have the same principal.

> Filed Bug 1446935 - Callsites ending up calling NS_CheckContentLoadPolicy 
> can use nsINode isntead of nsISupports

I meant that callers of the check in the CSS loader can do that.  
NS_CheckContentLoadPolicy doesn't seem like it can if we keep doing windows as contexts.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #25)
> > I guess there is no need for new tests, right?
> If none of the content policies use the flags, right.

None of the security flags are evaluated and used within content policies - correct.

> Hmm.  So my question is: was someone depending on this?

Well, there was Bug 1329288, where we also added test_contentpolicy_block_window.html. Within Bug 1329288 we incorrectly removed the entire content policy check, which was brought back. But to make sure we are on the same page: we are still passing requestingContext which was the same requestingContext as before. Content policies now query .topLevelContext which returns the same loading context as before, right? Nothing changed here compared to what's currently in mozilla-central.

> OK.  But in this case the context used to be "thisContent" and "doc" is
> "thisContent->OwnerDoc()".  So thisContent->NodePrincipal() ==
> doc->NodePrincipal().  It's just faster to get it from "doc".

Correct - it's the loadinfo constructor that requires an nsINode as LoadingContext, see
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#49
Hence I am passing the document instead of thisContent - acceptable?

To answer the question why it was not possible in the current loadInfo setup: That was only the case in the ImageDocument.cpp case where I initially passed domWindow->GetFrameElementInternal() as the context and channelPrincipal as the loadingPrincipal, but in that case the NodePrincipal() and the loadingPrincipal do not match - sorry for the confusion.

> > How relevant is ShouldProcess these days?
> That's a good question.  It looks like webextensions don't really expose it.
> We should check what our internal policies look like and see.

To investigate whether ShouldProcess is still relevant these days I filed:
Bug 1447256 - Evaluate if ShouProcess on nsIContentPolicy can be dreprecated
 
> Specifically for this case, we should at least check what happens and what
> should happen if images are disabled by pref (which we implement via policy)
> and then you navigate to an image url.  And in general think about any
> policies which do something other than "accept" in ShouldProcess.

To make sure we are speaking about the same test, here is what I tested:
1) I flipped the pref |permissions.default.image| to 2 (which is never load images)
2) When loading a page I get the 'alt' text displayed for images on a webpage
==> I think this is what should be happening
3) I loaded an image in the top-level context (right-click->view-image), nothing is displayed and I see the following error in the console:
   |TypeError: req is null  browser-child.js:447:9|
==> Not displaying the image seems correct. Probably we should file a follow up to fix the problem within browser-child.js

> For now, passing the NodePrincipal() of the frame element here is probably
> ok.  Except when the frame element is null, in which case I expect your
> patch to crash (image load in toplevel window).

Within ImageDocument.cpp I now query the info from the previous loadInfo, but fall back to using the channelPrincipal in case GetFrameElementInternal() returns null.

> Same answer: the PI and the document have the same principal.

Also in the case of nsXMLContentSink.cpp I am using the document because the LoadInfo constructor requires an nsINode as the context - acceptable?
 
> > Filed Bug 1446935 - Callsites ending up calling NS_CheckContentLoadPolicy 
> > can use nsINode isntead of nsISupports
> I meant that callers of the check in the CSS loader can do that.  
> NS_CheckContentLoadPolicy doesn't seem like it can if we keep doing windows
> as contexts.

I saw your comment on the bug. I filed a more generic bug because I thought it can't hurt evaluating all the callsits to functions that end up NS_CheckContentLoadPolicy. Either way, we definitely can and should simplify in the CSS loader.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here; I spent a while combing through things to make sure I was understanding correctly... and I think I am, but just failing to explain my concerns.

> Content policies now query .topLevelContext which returns the same loading context as before, right?

That fixes the issue I was having, right.  Good.

> Correct - it's the loadinfo constructor that requires an nsINode as LoadingContext, see
> https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#49
> Hence I am passing the document instead of thisContent - acceptable?

I feel like we're talking past each other somehow here...  We used to pass thisContent as the context.  Now we're passing "doc".  thisContent is an nsINode (it's an nsIContent, which is a subclass of nsINode), so there should be no problem passing thisContent as the context.  Why are we not doing it?

This is in the nsObjectLoadingContent code.

> To make sure we are speaking about the same test, here is what I tested:

Just make sure we're on the same page, was that with the ShouldProcess call left in, or taken out?  I was talking about how we need to check what happens here if it's taken out.  With it left in, and a content policy like the one permissions.default.image implements (which doesn't care about the context), of course the load would get blocked by the ShouldProcess call.

> Also in the case of nsXMLContentSink.cpp I am using the document because the LoadInfo
> constructor requires an nsINode as the context - acceptable?

Again, we seem to be talking past each other somehow, so let me try to spell this out.  The current code (before your changes) uses ToSupports(aProcessingInstruction) as the context.  aProcessingInstruction is of type mozilla::dom::ProcessingInstruction, which is a subclass of nsINode.  Why can't you just pass aProcessingInstruction as the context to the loadinfo, preserving current behavior?

I'm going to wait for answers to the above before I finish going through the patch, but here are some comments on the other changes so far:

* I'm not sure what "once both become available" means in the nsDocShell.cpp comments.
  The two are never in existence at the same time....
* The "check if the type is document, get the context different ways" code snippet
  is copy/pasted a bunch of times.  It would be better to just have a loadinfo method
  for this, either in the IDL directly or inline in a C++ block in the .idl.
* There's a random whitespace line added in nsContentSecurityManager::doContentSecurityChecks
  that should go away again.

Again, sorry for the lag.  Shouldn't happen any more on this bug now that I think I have a firm grasp of everything.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

Boris, thanks for your comments and help moving this bug forward.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #27)
> I feel like we're talking past each other somehow here...  We used to pass
> thisContent as the context.  Now we're passing "doc".  thisContent is an
> nsINode (it's an nsIContent, which is a subclass of nsINode), so there
> should be no problem passing thisContent as the context.  Why are we not
> doing it?
> This is in the nsObjectLoadingContent code.

Yes, sorry, I fixed that within nsObjectLoadingContent.cpp; using 'thisContent' now.
 
> Just make sure we're on the same page, was that with the ShouldProcess call
> left in, or taken out?  I was talking about how we need to check what
> happens here if it's taken out.  With it left in, and a content policy like
> the one permissions.default.image implements (which doesn't care about the
> context), of course the load would get blocked by the ShouldProcess call.

Yes, that's with the ShouldProcess call left in. I guess we leave that for another day and re-evaluate the usefulness of ShouldProcess within Bug 1447256 - Evaluate if ShouProcess on nsIContentPolicy can be dreprecated.
 
> Again, we seem to be talking past each other somehow, so let me try to spell
> this out.  The current code (before your changes) uses
> ToSupports(aProcessingInstruction) as the context.  aProcessingInstruction
> is of type mozilla::dom::ProcessingInstruction, which is a subclass of
> nsINode.  Why can't you just pass aProcessingInstruction as the context to
> the loadinfo, preserving current behavior?

Yes, sorry, I fixed that within nsXMLContentSink.cpp; using aProcessingInstruction now.

> I'm going to wait for answers to the above before I finish going through the
> patch, but here are some comments on the other changes so far:
> 
> * I'm not sure what "once both become available" means in the nsDocShell.cpp
> comments.

Sorry for the confusion, I added that comment because I thought this is what you requested within comment 19.
Apparently not exactly, I updated that comment again; let me know if you would prefer a different comment.

> * The "check if the type is document, get the context different ways" code
> snippet
>   is copy/pasted a bunch of times.  It would be better to just have a
> loadinfo method
>   for this, either in the IDL directly or inline in a C++ block in the .idl.

I filed Bug 1448316 - Encapsulate the functionality for querying the toplevel context within LoadInfo.

> * There's a random whitespace line added in
> nsContentSecurityManager::doContentSecurityChecks
>   that should go away again.

Removed!

Comment 29

[Boris Zbarsky [:bzbarsky]]

> I filed Bug 1448316 - Encapsulate the functionality for querying the toplevel context within LoadInfo.

That fix should just happen here (or as a prereq to this patch), instead of being after-the-fact cleanup.

> Yes, that's with the ShouldProcess call left in.

OK.  Clearly with it left in things will get blocked as needed.  ;)

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #29)
> > I filed Bug 1448316 - Encapsulate the functionality for querying the toplevel context within LoadInfo.
> 
> That fix should just happen here (or as a prereq to this patch), instead of
> being after-the-fact cleanup.

Sure, I changed the dependency and have marked Bug 1448316 blocking this bug.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> > That fix should just happen here (or as a prereq to this patch), instead of
> > being after-the-fact cleanup.
> 
> Sure, I changed the dependency and have marked Bug 1448316 blocking this bug.

Well, I think it actually makes sense to incorporate that change within this bug as you suggested. In fact, I extended nsILoadinfo.idl by a loadingContext. In turn, this change simplified nsILoadInfo.idl again, because there is no need for expsoing the loadingNode as well as the contextForTopLevelLoad separately. All we need is the loadingContext.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

Incorporated a few more nits.

Boris, please see previous comment - thanks!

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8962278 [details] [diff] [review]
bug_1439713_change_shouldload.patch

There are unused requestingLocation temporaries in nsContentPolicy::ShouldLoad and nsContentPolicy::ShouldProcess that should go away.

>+++ b/dom/base/nsObjectLoadingContent.cpp

The context in nsObjectLoadingContent::CheckProcessPolicy is still wrong (document vs the content node).

>+++ b/dom/security/nsContentSecurityManager.cpp
>+        nsCOMPtr<nsISupports> requestingContext = aLoadInfo->LoadingNode();
>         nsCOMPtr<nsINode> node = do_QueryInterface(requestingContext);
>         MOZ_ASSERT(!node || node->NodeType() == nsINode::DOCUMENT_NODE,

That's a bizarre piece of code.  Why not just do:

  nsCOMPtr<nsINode> node = aLoadInfo->LoadingNode();

?  There are a bunch of copies of this sort of thing in this file.

Also, after that can "node" be null?  I'd guess not, since LoadingNode() is not named GetLoadingNode()....  but if it can, please file a followup bug to rename LoadingNode properly.

>+++ b/netwerk/base/LoadInfo.cpp
>+nsISupports*
>+LoadInfo::LoadingContext()

This should be returning already_AddRefed.  Returning a raw pointer like this is very unsafe, especially given where it's coming from in the non-node case.

Also, this can return null, right?  If so, it should be named GetLoadingContext.

>+LoadInfo::GetLoadingContext(nsISupports** aResult)

And this should probably be named GetLoadingContextXPCOM on the C++ side, if the other thing is called GetLoadingContext.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #33)
> There are unused requestingLocation temporaries in
> nsContentPolicy::ShouldLoad and nsContentPolicy::ShouldProcess that should
> go away.

Thanks for pointing that out. In fact the requestingLocation is used within LOG_CHECK, hence I moved the initialization of requestingLocation into that macro.
 
> >+++ b/dom/base/nsObjectLoadingContent.cpp
> The context in nsObjectLoadingContent::CheckProcessPolicy is still wrong
> (document vs the content node).

rrrh - sorry I missed that - updated!
 
> >+++ b/dom/security/nsContentSecurityManager.cpp
> >+        nsCOMPtr<nsISupports> requestingContext = aLoadInfo->LoadingNode();
> >         nsCOMPtr<nsINode> node = do_QueryInterface(requestingContext);
> That's a bizarre piece of code.  Why not just do:

I tried to minimize the changes for this patch, but you are absolutely right, updated!

> Also, after that can "node" be null?  I'd guess not, since LoadingNode() is
> not named GetLoadingNode()....  but if it can, please file a followup bug to
> rename LoadingNode properly.

The LoadingNode can be null, I filed the following bug to address that naming convention:
Bug 1449475 - Update naming of LoadInfo::LoadingNode() since it might return null

> >+++ b/netwerk/base/LoadInfo.cpp
> >+nsISupports*
> >+LoadInfo::LoadingContext()
> This should be returning already_AddRefed.

Absolustely - updated!

> Also, this can return null, right?  If so, it should be named
> GetLoadingContext.

I suppose you mean that the C++ version should be called GetLoadingContextXPCOM() - hopefully I was not misinterpreting your suggestion!

Comment 35

[Boris Zbarsky [:bzbarsky]]

> I suppose you mean that the C++ version should be called GetLoadingContextXPCOM()

No, I meant what I said.  You want something like this:

  [binaryname(GetLoadingContextXPCOM)]
  readonly attribute nsISupports loadingContext;
  
  [noscript, notxpcom, nostdcall, binaryname(GetLoadingContext)]
  LoadContextRef binaryGetLoadingContext();

so that the C++ callers look reasonable ("foo->GetLoadingContext()") and so do the JS ones (foo.loadingContext).

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8963047 [details] [diff] [review]
bug_1439713_change_shouldload.patch

Looks good now, except for comment 35.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1439713_change_shouldload.patch

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #35)
> No, I meant what I said.  You want something like this:
> 
>   [binaryname(GetLoadingContextXPCOM)]
>   readonly attribute nsISupports loadingContext;

Thanks Boris for the clarification and also your help getting this bug ready to land. The auto-generated code already prepends 'Get', hence I ended up with:

+  [binaryname(LoadingContextXPCOM)]
+  readonly attribute nsISupports loadingContext;

Comment 38

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8963479 [details] [diff] [review]
bug_1439713_change_shouldload.patch

r=me.  Thank you!

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/22d7899c79f8
Change nsIContentPolicy shouldLoad to take an <uri, loadInfo> pair instead of the various args. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/65df341041e1
Add flag to loadinfo for skipping certain security policy checks. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f3fcb76b657
Update tests relying on nsIContentPolicy. r=bz

Comment 40

[Boris Zbarsky [:bzbarsky]]

Christoph, I had explicitly asked you to fold the patches together before landing so we wouldn't have known-failing changesets in the repository... :(

Comment 41

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/22d7899c79f8
https://hg.mozilla.org/mozilla-central/rev/65df341041e1
https://hg.mozilla.org/mozilla-central/rev/3f3fcb76b657

Comment 42

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8963479 [details] [diff] [review]
bug_1439713_change_shouldload.patch

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

::: layout/style/FontFaceSet.cpp
@@ +1377,5 @@
>      mDocument->StartBufferingCSPViolations();
>    }
>  
> +  nsCOMPtr<nsILoadInfo> secCheckLoadInfo =
> +    new net::LoadInfo(mDocument->NodePrincipal(), // loading principal

Why was the loading principal changed here? Was this intended / fine? aPrincipal used to be null for data: URIs.

Just curious, since I was rebasing the patches from bug 1420680 and got conflicts here.

Comment 43

[Emilio Cobos Álvarez (:emilio)]

See comment 42.

Comment 44

[Boris Zbarsky [:bzbarsky]]

> Why was the loading principal changed here?

Because loadinfo enforces that the loading principal matches the principal of the loading node.

> aPrincipal used to be null for data: URIs.

Are you sure?  It looks to me like UserFontCache::UpdateAllowedFontSets passes aUserFontSet->GetStandardFontLoadPrincipal() in the data: case, which is not null...  But maybe I'm not following the code correctly?

Does "null" here mean nullptr or nullprincipal?

Comment 45

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #44)
> > Why was the loading principal changed here?
> 
> Because loadinfo enforces that the loading principal matches the principal
> of the loading node.
> 
> > aPrincipal used to be null for data: URIs.
> 
> Are you sure?  It looks to me like UserFontCache::UpdateAllowedFontSets
> passes aUserFontSet->GetStandardFontLoadPrincipal() in the data: case, which
> is not null...  But maybe I'm not following the code correctly?
> 
> Does "null" here mean nullptr or nullprincipal?

nullptr, see[1]. What UsertFontCache::UpdateAllowedFontSets does is not relevant, because we don't use that map for data: uris anyway, see[2].

[1]: https://searchfox.org/mozilla-central/rev/42b34ba1961e37e0d2236deafdd126a0ba21b9ec/gfx/thebes/gfxUserFontSet.cpp#1333
[2]: https://searchfox.org/mozilla-central/rev/42b34ba1961e37e0d2236deafdd126a0ba21b9ec/gfx/thebes/gfxUserFontSet.cpp#557

Comment 46

[Boris Zbarsky [:bzbarsky]]

Hmm.  How does it make any sense to use a nullptr loading and triggering principal?

Also, I still feel like I'm missing something.  UserFontCache::GetFont is called from only one place: gfxUserFontEntry::DoLoadNextSrc.  In this method, the principal passed in comes from an mFontSet->CheckFontLoad() call.  And UserFontSet::CheckFontLoad calls FontFaceSet::CheckFontLoad which does the GetStandardFontLoadPrincipal thing too.

Comment 47

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #46)
> Hmm.  How does it make any sense to use a nullptr loading and triggering
> principal?

That I don't know :). I'm happy to leave the code as is, just wanted to double-check that this was intended.

> Also, I still feel like I'm missing something.  UserFontCache::GetFont is
> called from only one place: gfxUserFontEntry::DoLoadNextSrc.  In this
> method, the principal passed in comes from an mFontSet->CheckFontLoad()
> call.  And UserFontSet::CheckFontLoad calls FontFaceSet::CheckFontLoad which
> does the GetStandardFontLoadPrincipal thing too.

Note that it can also use the "origin principal", for @font-face in user sheets[1]. Haven't tested if this change breaks it, I'll test and fix if so I guess.

[1]: https://searchfox.org/mozilla-central/rev/42b34ba1961e37e0d2236deafdd126a0ba21b9ec/layout/style/FontFaceSet.cpp#1360

Comment 48

[Boris Zbarsky [:bzbarsky]]

I _think_ as long as we use that principal as the triggering principal things should be fine...  But worth checking, yes.

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #47)
> (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from
> comment #46)
> > Hmm.  How does it make any sense to use a nullptr loading and triggering
> > principal?

Most likely content policy's queried the principal from the loadingContext first anyway and only used to fall back to using the 'explicit' loadingPrincipal in case the loadingContext (in that case mDocument) was null. Either way, the new setup is actually more correct in my opinion because the loadingPrincipal should match the loadingContext's principal if both are available.

Comment 50

[Mike Kaply [:mkaply]]

Is this documented anywhere besides the IDL?

Should

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIContentPolicy

be updated? (or marked as deprecated)

Comment 51

[Kris Maglione [:kmag]]

(In reply to Mike Kaply [:mkaply] from comment #50)
> Is this documented anywhere besides the IDL?
> 
> Should
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIContentPolicy
> 
> be updated? (or marked as deprecated)

Should be archived.

Comment 52

[Mike Kaply [:mkaply]]

Marking dev-doc-needed if only to remove/add warning to the docs.

Comment 53

[Chris Mills [:cmills]]

Archived: https://developer.mozilla.org/en-US/docs/Archive/Mozilla/nsIContentPolicy