Closed Bug 1195172 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

Attachments

(3 files, 9 obsolete files)

1.39 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
20.61 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
17.34 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → mozilla
Blocks: 1182535
Duplicate of this bug: 1206962
When converting callsites here we should also investigate:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#895
Status: NEW → ASSIGNED
(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 :-(
We should convert all callsites within layout/style to use asyncOpen2() and also open2() within that bug.
Summary: Use channel->ascynOpen2 layout/style/FontFaceSet → Use channel->ascynOpen2 layout/style
Summary: Use channel->ascynOpen2 layout/style → Use channel->ascynOpen2 layout/style/FontFaceSet.cpp
Summary: Use channel->ascynOpen2 layout/style/FontFaceSet.cpp → Use channel->asyncOpen2 layout/style/FontFaceSet.cpp
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!
Attachment #8712858 - Flags: review?(bzbarsky)
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"
Flags: needinfo?(jonas)
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.
Flags: needinfo?(jonas)
> I would be very surprised if font or stylesheet loading should ever use a real 'omit' policy.

Agreed.
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.
Attachment #8712858 - Flags: review?(bzbarsky)
(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"
Flags: needinfo?(ehsan)
(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.
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.
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...
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!
Flags: needinfo?(cam)
(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.
Flags: needinfo?(ehsan)
(By fixing that code, I mean deleting it.)
> "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
(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.
(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.
(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?
> 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.  ;)
(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?
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.
(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.
Flags: needinfo?(cam)
Using SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS for the synchronous case sounds good.
(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?
Attachment #8712858 - Attachment is obsolete: true
Attachment #8719939 - Flags: feedback?(cam)
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.
Flags: needinfo?(cam)
(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.
Flags: needinfo?(cam)
(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 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.
Attachment #8719939 - Flags: feedback?(cam)
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.
Attachment #8719939 - Attachment is obsolete: true
Attachment #8720029 - Attachment is obsolete: true
Attachment #8720137 - Attachment is obsolete: true
Attachment #8722176 - Flags: review?(cam)
Attachment #8722176 - Flags: review?(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.
Attachment #8722176 - Flags: review?(bzbarsky) → review+
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().
Attachment #8722177 - Flags: review?(bzbarsky) → review-
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.
Attachment #8722176 - Flags: review?(cam) → review+
Attachment #8722181 - Flags: review?(ehsan) → review+
(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.
Attachment #8722177 - Attachment is obsolete: true
Attachment #8724971 - Flags: review?(bzbarsky)
Attached patch fontfaceset_updates.patch (obsolete) — Splinter Review
(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.
Attachment #8724972 - Flags: review?(bzbarsky)
Comment on attachment 8724971 [details] [diff] [review]
bug_1195172_asyncopen2_fontfaceset_tests.patch

r=me
Attachment #8724971 - Flags: review?(bzbarsky) → review+
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.
Attachment #8724972 - Flags: review?(bzbarsky) → review+
Qfolded the innerdiff into the main patch. Carrying over r+ from bz and cam.
Attachment #8722176 - Attachment is obsolete: true
Attachment #8724972 - Attachment is obsolete: true
Attachment #8725406 - Flags: review+
(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
Attachment #8725407 - Flags: review?(bzbarsky)
Comment on attachment 8725407 [details] [diff] [review]
bug_1195172_principal_updates.patch

Good, good.  ;)  r=me
Attachment #8725407 - Flags: review?(bzbarsky) → review+
Thanks Boris!

Qfolded the innerdiff into the main patch. Carrying over r+ from bz.
Attachment #8725406 - Attachment is obsolete: true
Attachment #8725407 - Attachment is obsolete: true
Attachment #8725411 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7251e33ee977
https://hg.mozilla.org/mozilla-central/rev/7bfe98a21f87
https://hg.mozilla.org/mozilla-central/rev/e4d6131af171
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.