1195172 - Use channel->asyncOpen2 layout/style/FontFaceSet.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

When converting callsites here we should also investigate:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#895

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> When converting callsites here we should also investigate:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/
> tabbrowser.xml#895

That was meant to be posted in a different bug :-(

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

We should convert all callsites within layout/style to use asyncOpen2() and also open2() within that bug.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset.patch

Boris, all relevant tests are passing locally, I am not sure about removing CheckLoadAllowed completely though. I left a question within the code to make sure we are not running into any problems with cached resources. What do you think?

Other than that, the changes should be pretty straight forward!

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Jonas, a similar error than within the styleloader happens for the fontloader, which makes me thing that the current securityFlags within the attached patch are incorrect. Or more precise, missing the credential bits which we use within the scriptLoader and the styleLoader, right? Any suggestion on how to move forward here?

+++ b/testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-resources.https.html
@@ -129,14 +129,14 @@ async_test(function(t) {
         script_test(f, REMOTE_URL, 'use-credentials', 'cors', 'include');

         font_face_test(f, LOCAL_URL, 'cors', 'same-origin');
-        font_face_test(f, REMOTE_URL, 'cors', 'omit');
+        // font_face_test(f, REMOTE_URL, 'cors', 'omit'); // expected "omit" but got "same-origin"

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

To my knowledge the only API in the web platform which can create requests with an 'omit' cookie policy, i.e. requests which don't even send cookies for same-origin requests, is fetch().

I would be very surprised if font or stylesheet loading should ever use a real 'omit' policy.

That said, I don't know what the test does, so I can't speak to if changing the test is the right thing to do or not.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> I would be very surprised if font or stylesheet loading should ever use a real 'omit' policy.

Agreed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8712858 [details] [diff] [review]
bug_1195172_asyncopen2_fontfaceset.patch

>+    // no need to set up CORS for javascript:, data:, etc.

Hmm.  Having to do that on the consumer end seems annoying.  Shouldn't it just work even in the SEC_REQUIRE_CORS_DATA_INHERITS case?  I do see that the code used to do this, but this needs some documentation about the _why_ of doing it.  I suggest asking the author of this code for advice.

>+  // ckerschb: might there be a security risk with the cache?

Again, a question for the author of this code.

>@@ -1374,24 +1362,24 @@ FontFaceSet::SyncLoadFontData(gfxUserFon
>+                                            nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,

This needs documentation, since this is not how fonts should normally load.  I expect this is only reached for certain kinds of URIs, which is why it's OK, but that needs to be documented.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #7)
> To my knowledge the only API in the web platform which can create requests
> with an 'omit' cookie policy, i.e. requests which don't even send cookies
> for same-origin requests, is fetch().
> 
> I would be very surprised if font or stylesheet loading should ever use a
> real 'omit' policy.
> 
> That said, I don't know what the test does, so I can't speak to if changing
> the test is the right thing to do or not.

Ehsan, you touched tests/service-workers/service-worker/fetch-request-resources.https.html recently, can you tell us what this test is doing exactly, and most importantly if the test is potentially is wrong and we need to update the following subtest:

> font_face_test(f, REMOTE_URL, 'cors', 'omit'); // expected "omit" but got "same-origin"

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8712858 [details] [diff] [review]
> bug_1195172_asyncopen2_fontfaceset.patch
> 
> >+    // no need to set up CORS for javascript:, data:, etc.
> 
> Hmm.  Having to do that on the consumer end seems annoying.  Shouldn't it
> just work even in the SEC_REQUIRE_CORS_DATA_INHERITS case?

It definitely works for data: I am not sure about javascript: and such. Either way, I'll clarify with the author and add documentation.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why can't font loading simply use SEC_REQUIRE_CORS_DATA_INHERITS?

The whole point of "DATA_INHERITS" is that it automatically inherits for protocols which has the URI_INHERITS_SECURITY_CONTEXT flag set.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I seem to recall this has to do with the sync-loading path fonts do for data:.  But again, that's something to check with someone who remembers the code better than I do...

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Cameron, as the author of Bug 1028497 I was wondering if you can provide some answers to the questions we have within Comment 9 - 13. Thanks!

Comment 15

[(no longer active)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)
> (In reply to Jonas Sicking (:sicking) from comment #7)
> > To my knowledge the only API in the web platform which can create requests
> > with an 'omit' cookie policy, i.e. requests which don't even send cookies
> > for same-origin requests, is fetch().
> > 
> > I would be very surprised if font or stylesheet loading should ever use a
> > real 'omit' policy.
> > 
> > That said, I don't know what the test does, so I can't speak to if changing
> > the test is the right thing to do or not.
> 
> Ehsan, you touched
> tests/service-workers/service-worker/fetch-request-resources.https.html
> recently, can you tell us what this test is doing exactly, and most
> importantly if the test is potentially is wrong and we need to update the
> following subtest:
> 
> > font_face_test(f, REMOTE_URL, 'cors', 'omit'); // expected "omit" but got "same-origin"

I looked over this with Josh, and we both agree that all of these "omit" values are wrong and should be "same-origin".  Sorry about that!

But the more interesting question is why this test is currently passing!  As far as I can tell, it all boils down to this code: <https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#916>.  LOAD_ANONYMOUS gets turned into "omit" here: <https://hg.mozilla.org/mozilla-central/file/tip/dom/fetch/InternalRequest.cpp#l337>.

But note that per the CORS spec, mWithCredentials is supposed to mean "should I block the request after having received a response without Access-Control-Allow-Credentials", not "making the request cookie-less" as the code and the comment in nsCORSListenerProxy::UpdateChannel() suggest.  So it looks like we have mistakenly prevented the credentials from being sent in the first place ever since bug 389508 added support for this.

Fixing that code makes these tests fail, as they should.

Comment 16

[(no longer active)]

(By fixing that code, I mean deleting it.)

Comment 17

[Boris Zbarsky [:bzbarsky]]

> "should I block the request after having received a response without Access-Control-Allow-Credentials",

If mWithCredentials is true we're supposed to use the "include" credentials mode when invoking fetch, per https://xhr.spec.whatwg.org/#dom-xmlhttprequest-send which I agree matches what you describe: only send the main request if the preflight says Access-Control-Allow-Credentials and only make the content available if the main request says Access-Control-Allow-Credentials, when doing a cross-origin request.

But if mWithCredentials is false, we're supposed to use the "same-origin" credentials mode. In this case credentials only get sent if the requests "response tainting" is "basic".  For an actual CORS-enabled cross-origin request that will never be the case.  So if mWithCredentials is false, then we should not send credentials at all for cross-origin requests.

Note that the actual thing on the request should still be "same-origin", but the _behavior_ it has is to not send credentials.

Or at least so I think.  These specs are pretty hard to follow unless you trace from the entry point very very carefully and meticulously.  :(

Then

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to :Ehsan Akhgari from comment #15)
> But note that per the CORS spec, mWithCredentials is supposed to mean
> "should I block the request after having received a response without
> Access-Control-Allow-Credentials", not "making the request cookie-less" as
> the code and the comment in nsCORSListenerProxy::UpdateChannel() suggest. 

Setting withCredentials to false should definitely prevent cookies from being sent if the request is going to a different origin. It also makes us ignore the value of the Access-Control-Allow-Credentials header.

While setting withCredentials to true, means that cookies should be sent, but that we also should check that Access-Control-Allow-Credentials is set to "true" in the response.

Comment 19

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #18)
> (In reply to :Ehsan Akhgari from comment #15)
> > But note that per the CORS spec, mWithCredentials is supposed to mean
> > "should I block the request after having received a response without
> > Access-Control-Allow-Credentials", not "making the request cookie-less" as
> > the code and the comment in nsCORSListenerProxy::UpdateChannel() suggest. 
> 
> Setting withCredentials to false should definitely prevent cookies from
> being sent if the request is going to a different origin. It also makes us
> ignore the value of the Access-Control-Allow-Credentials header.

That seems to not match what the code is actually doing.  When we set withCredentials to false, we prevent from sending all cookies to all origins (including the *same* origin) if the original channel has been AsyncOpen'ed (as opposed to AsyncOpen2'ed.)

As far as I can understand that behavior doesn't match what the spec says we should do.

> While setting withCredentials to true, means that cookies should be sent,
> but that we also should check that Access-Control-Allow-Credentials is set
> to "true" in the response.

Yes, setting the flag to true does the right thing.  It's the false value that is implemented incorrectly.

Comment 20

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #17)
> > "should I block the request after having received a response without Access-Control-Allow-Credentials",
> 
> If mWithCredentials is true we're supposed to use the "include" credentials
> mode when invoking fetch, per
> https://xhr.spec.whatwg.org/#dom-xmlhttprequest-send which I agree matches
> what you describe: only send the main request if the preflight says
> Access-Control-Allow-Credentials and only make the content available if the
> main request says Access-Control-Allow-Credentials, when doing a
> cross-origin request.

Yeah.  In spec terms, here is what we do for AsyncOpen'ed requests: when we observe a @cors=use-credentials attribute on something like an image, we set the credentials mode to "omit" which means don't send the credentials anywhere (including same origin) and do not process Access-Control-Allow-Credentials.

> But if mWithCredentials is false, we're supposed to use the "same-origin"
> credentials mode. In this case credentials only get sent if the requests
> "response tainting" is "basic".  For an actual CORS-enabled cross-origin
> request that will never be the case.  So if mWithCredentials is false, then
> we should not send credentials at all for cross-origin requests.

Yep.  (Basically behave as we do for AsyncOpen2'ed channels.)

> Note that the actual thing on the request should still be "same-origin", but
> the _behavior_ it has is to not send credentials.

Hmm, why do you think that?  AFAICT we should send credentials for same-origin in this case.

> Or at least so I think.  These specs are pretty hard to follow unless you
> trace from the entry point very very carefully and meticulously.  :(

Indeed.

> Then

Partial sentence?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> we set the credentials mode to "omit"

In both same-origin and cross-origin cases?  Do we have any sort of test setup, local or on the web for this sort of thing?

> Hmm, why do you think that?  AFAICT we should send credentials for same-origin in this case.

I was talking about the cross-origin case when the credentials mode is "same-origin".  For same-origin we should be sending credentials no matter what mWithCredentials value is, I agree.

> Partial sentence?

Yes, please ignore.  ;)

Comment 22

[(no longer active)]

(In reply to Boris Zbarsky [:bz] from comment #21)
> > we set the credentials mode to "omit"
> 
> In both same-origin and cross-origin cases?

Yes.

> Do we have any sort of test
> setup, local or on the web for this sort of thing?

Other than the tests that Christoph found?

Comment 23

[(no longer active)]

Based on a conversation with sicking on IRC it looks like I'm wrong about almost everything I said, and I don't know what causes this discrepancy any more.  :(  It may very well be us mapping LOAD_ANONYMOUS to "omit" since at least now that is clear that the two are not the same.

Comment 24

[Cameron McCormack (:heycam)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> Cameron, as the author of Bug 1028497 I was wondering if you can provide
> some answers to the questions we have within Comment 9 - 13. Thanks!

I'm not the original author of StartLoad (I only moved it from nsFontFaceLoader in bug 1028497), but I'll try to answer.

Just so I'm clear on what this bug is doing: we're remove explicit handling of CORS requests and using the functionality that's in asyncOpen2 to do it for us, with no expected change in behaviour, yes?

(In reply to Boris Zbarsky [:bz] from comment #9)
> Comment on attachment 8712858 [details] [diff] [review]
> bug_1195172_asyncopen2_fontfaceset.patch
> 
> >+    // no need to set up CORS for javascript:, data:, etc.
> 
> Hmm.  Having to do that on the consumer end seems annoying.  Shouldn't it
> just work even in the SEC_REQUIRE_CORS_DATA_INHERITS case?  I do see that
> the code used to do this, but this needs some documentation about the _why_
> of doing it.  I suggest asking the author of this code for advice.

Given that StartLoad's switch based on URI_INHERITS_SECURITY_CONTEXT predates SEC_REQUIRE_CORS_DATA_INHERITS's existence, maybe there's no good reason to continue doing this.  From the description of SEC_REQUIRE_CORS_DATA_INHERITS it sounds like what we want here.

> >+  // ckerschb: might there be a security risk with the cache?
> 
> Again, a question for the author of this code.

With the UserFontCache?  As long as FontFaceSet::CheckFontLoad continues to return the document's principal so that it can be stored in the UserFontCache::Entry, the cache should keep working like it does currently.

> >@@ -1374,24 +1362,24 @@ FontFaceSet::SyncLoadFontData(gfxUserFon
> >+                                            nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,
> 
> This needs documentation, since this is not how fonts should normally load. 
> I expect this is only reached for certain kinds of URIs, which is why it's
> OK, but that needs to be documented.

We use SyncLoadFontData for any URL that is nsIProtocolHandler::URI_SYNC_LOAD_IS_OK:

  https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUserFontSet.cpp#491

and that looks to be data: (expected) and rtsp: (surprising!).  So it seems like we should be using either SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS or SEC_REQUIRE_CORS_DATA_INHERITS here.  Probably the former; I have no idea what CORS would mean in the context of RTSP.  And we should probably just make gfxUserFontSet use this sync loading path for data: URLs only, explicitly.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Using SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS for the synchronous case sounds good.

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset.patch

(In reply to Cameron McCormack (:heycam) from comment #24)
> Just so I'm clear on what this bug is doing: we're remove explicit handling
> of CORS requests and using the functionality that's in asyncOpen2 to do it
> for us, with no expected change in behaviour, yes?

Yes, that is correct.

> Given that StartLoad's switch based on URI_INHERITS_SECURITY_CONTEXT
> predates SEC_REQUIRE_CORS_DATA_INHERITS's existence, maybe there's no good
> reason to continue doing this.  From the description of
> SEC_REQUIRE_CORS_DATA_INHERITS it sounds like what we want here.

I am pretty sure myself that SEC_REQUIRE_CORS_DATA_INHERITS is what we want for the asynchronous load and SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS what we want for the sync load (see also Jonas' note in Comment 26).
 
> > >+  // ckerschb: might there be a security risk with the cache?
> > 
> > Again, a question for the author of this code.
> 
> With the UserFontCache?  As long as FontFaceSet::CheckFontLoad continues to
> return the document's principal so that it can be stored in the
> UserFontCache::Entry, the cache should keep working like it does currently.

No changes there, but what I am worried is for example some kind of cache poisoning. What if a Font ends up in the cache and then a different site (which uses CSP |font-src 'none'|) loads that font, we see it in the cache and apply it to the page, no?


> > >@@ -1374,24 +1362,24 @@ FontFaceSet::SyncLoadFontData(gfxUserFon
> > >+                                            nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS,
> > 
> > This needs documentation, since this is not how fonts should normally load. 
> > I expect this is only reached for certain kinds of URIs, which is why it's
> > OK, but that needs to be documented.

Do we still need any documentation here since we now use SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS?

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset_test_updates.patch

TRY looks pretty happy:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dda0553301c

with that web-platform-test update!

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset_tests.patch

Hey Cameron, in addition to comment 26 I wrote that testcase that basically loads a font used by an iframe and then loads another iframe where the CSP of the second iframe does *not* allow using any fonts.

On current mozilla-central the test works correctly. When I apply the patch from this bug and run the test we are experiencing the 'cache-problem' I explained in Comment 26.

It works on current mozilla-central because we consult contentPolicies within CheckLoadAllowed before accessing the cache. I think what we need is another security check that *only* is performed when we are about to load a font from the cache, because otherwise security checks are performed within asyncOpen2() and we are fine.

Comment 29

[Cameron McCormack (:heycam)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #28)
> It works on current mozilla-central because we consult contentPolicies
> within CheckLoadAllowed before accessing the cache. I think what we need is
> another security check that *only* is performed when we are about to load a
> font from the cache, because otherwise security checks are performed within
> asyncOpen2() and we are fine.

OK, that makes sense to me.

Comment 30

[Cameron McCormack (:heycam)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #26)
> > > This needs documentation, since this is not how fonts should normally load. 
> > > I expect this is only reached for certain kinds of URIs, which is why it's
> > > OK, but that needs to be documented.
> 
> Do we still need any documentation here since we now use
> SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS?

I would add a comment to point out that we should only ever get in here for data: URLs anyway, so whether we this is REQUIRE_SAME_ORIGIN or not doesn't matter.

Comment 31

[Cameron McCormack (:heycam)]

Comment on attachment 8719939 [details] [diff] [review]
bug_1195172_asyncopen2_fontfaceset.patch

Looks good so far but I'll await changes to handle the cache issues.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset.patch

We are now adding a specific security check only for the cache. Theoretically we could only add a check for CSP and MixedContentBlocker since ContentPolicies where checked initially, but I think we should do the same that we did within style/Loader.cpp where we also added NS_CheckContentLoadPolicy. The only question is whether we need to pass 'aContext' or not, and if so, how we should query it.

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset_tests.patch

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset_test_updates.patch

Same update as we did for the style/loader, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=1195173#c56

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8722176 [details] [diff] [review]
bug_1195172_asyncopen2_fontfaceset.patch

>@@ -1198,16 +1199,30 @@ gfxUserFontSet::UserFontCache::GetFont(n

This needs comments explaining why we're doing a manual content policy check.

Should aContext really be null here?  Why?  Needs to be documented if it really should.

r=me with those explained/fixed.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8722177 [details] [diff] [review]
bug_1195172_asyncopen2_fontfaceset_tests.patch

This test is racy.  Nothing requires baselineframe to be loaded by the time the first load event fires on testframe.

What I suggest you do instead is wait for the load event on the main page, start the load for the baselineframe, wait for the load event on _that_ to fire, and then attach the load listener on testframe and call loadNextTest().

Comment 37

[Cameron McCormack (:heycam)]

Comment on attachment 8722176 [details] [diff] [review]
bug_1195172_asyncopen2_fontfaceset.patch

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

r=me on the gfx/ and layout/ changes with bz's comments addressed.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset_tests.patch

(In reply to Boris Zbarsky [:bz] from comment #36)
> Comment on attachment 8722177 [details] [diff] [review]
> bug_1195172_asyncopen2_fontfaceset_tests.patch
> 
> This test is racy.  Nothing requires baselineframe to be loaded by the time
> the first load event fires on testframe.
> 
> What I suggest you do instead is wait for the load event on the main page,
> start the load for the baselineframe, wait for the load event on _that_ to
> fire, and then attach the load listener on testframe and call loadNextTest().

Yes, that makes sense - here we go.

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: fontfaceset_updates.patch

(In reply to Boris Zbarsky [:bz] from comment #35)
> Comment on attachment 8722176 [details] [diff] [review]
> bug_1195172_asyncopen2_fontfaceset.patch
> 
> >@@ -1198,16 +1199,30 @@ gfxUserFontSet::UserFontCache::GetFont(n
> 
> This needs comments explaining why we're doing a manual content policy check.
> 
> Should aContext really be null here?  Why?  Needs to be documented if it
> really should.
> 
> r=me with those explained/fixed.

Thinking that through we might run into a problem with the mixedContentBlocker if we don't provide 'aContext', to fix the problem, we actually have to call back into FontFaceSet, otherwise we don't have any document available.

The other minor thing I updated is that we shouldn't use SEC_NORMAL as an argument to NS_NewChannel anymore, instead we should use the most restricitive security flag, which is SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8724971 [details] [diff] [review]
bug_1195172_asyncopen2_fontfaceset_tests.patch

r=me

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8724972 [details] [diff] [review]
fontfaceset_updates.patch

>@@ -1231,17 +1224,17 @@ gfxUserFontSet::UserFontCache::GetFont(n
>+                                nsILoadInfo::SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS,

Please document why; it's not at all obvious to me.

>+FontFaceSet::CheckAllowFontLoad(nsIURI* aFontLocation)
>+                                          mDocument->NodePrincipal(),

Are we sure this is the same as aPrincipal in the caller?  That's not at all obvious to me...

This function should probably be called IsFontLoadAllowed() or something... CheckAllowFontLoad tells you nothing about what the return value means.

>+  if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {
>+      return false;
>+  }
>+  return true;

  return NS_SUCCEEDED(rv) && NS_CP_ACCEPTED(shouldLoad);

r=me with the above addressed, I think, but please do double-check those principals, esp. for cases when user stylesheets apply a localfont to a document.

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset.patch

Qfolded the innerdiff into the main patch. Carrying over r+ from bz and cam.

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_principal_updates.patch

(In reply to Boris Zbarsky [:bz] from comment #41)
> r=me with the above addressed, I think, but please do double-check those
> principals, esp. for cases when user stylesheets apply a localfont to a
> document.

Boris, that is a good catch. In fact the principal passed to NS_CheckContentLoadPolicy was  not always mDocument->NodePrincipal(). So, in fact we should keep that behavior and *explicitly* pass the right principal to IsFontLoadAllowed().

[1] http://mxr.mozilla.org/mozilla-central/source/layout/style/FontFaceSet.cpp#1338

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8725407 [details] [diff] [review]
bug_1195172_principal_updates.patch

Good, good.  ;)  r=me

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1195172_asyncopen2_fontfaceset.patch

Thanks Boris!

Qfolded the innerdiff into the main patch. Carrying over r+ from bz.

Comment 46

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7251e33ee977
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfe98a21f87
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d6131af171

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7251e33ee977
https://hg.mozilla.org/mozilla-central/rev/7bfe98a21f87
https://hg.mozilla.org/mozilla-central/rev/e4d6131af171