701262 - redirect requests for fonts cause font load to fail

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[John Daggett (:jtd)]

Extensis is reporting that font loading for their service is broken in Firefox 8.  It appears the code we added in bug 670900 to handle request errors better is not handling redirect requests for fonts correctly.

Steps:

1. Load the URL in Firefox 8 or trunk

Result: fonts don't load

The changeset for bug 670900:
http://hg.mozilla.org/mozilla-central/rev/944c450fc328

Part of the fix from the changeset above:

1.18 +    nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
1.19 +    if (httpChannel) {
1.20 +      PRBool succeeded;
1.21 +      nsresult rv = httpChannel->GetRequestSucceeded(&succeeded);
1.22 +      if (NS_SUCCEEDED(rv) && !succeeded) {
1.23 +        aStatus = NS_ERROR_NOT_AVAILABLE;
1.24 +      }
1.25 +    }

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.cpp#224

The code in nsFontFaceLoader::OnStreamComplete calls
GetRequestSucceeded on the channel to assure that the request succeeded,
so that the code doesn't attempt to load the error pages from 404
responses as fonts.

However, the logic in GetRequestSucceeded treats 3xx response codes as
"failures":

> NS_IMETHODIMP
> HttpBaseChannel::GetRequestSucceeded(bool *aValue)
> {
>   if (!mResponseHead)
>     return NS_ERROR_NOT_AVAILABLE;
>   PRUint32 status = mResponseHead->Status();
>   *aValue = (status / 100 == 2);
>   return NS_OK;
> }

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1120

The simple fix I think would be to not use GetRequestSucceeded but only
treat 4xx and 5xx responses as explicit failures.  For other responses,
let the font loading code work out whether the font payload is correct
or not.

Comment 1

[John Daggett (:jtd)]

Attachment: patch, test response codes directly

Comment 2

[John Daggett (:jtd)]

Attachment: patch v2, use the correct channel from the stream loader

Speaking with Jonas, it appears that the underlying problem is in the
way the existing code uses mChannel.  On redirects, the stream loader
will close the channel and open a new one transparently.  So mChannel is
actually the *old* request, not the current request being handled by the
stream loader.  So the correct fix here is to fetch the channel from the
stream loader.

Note that this has implications for how the load timer functions, it
appears this means that it won't show the fallback font when loaded via
a redirect request until the follow-on request completes or times out. 
Unfortunately, the stream loader code only updates the channel at the
end of a request.

This patch fixes the immediate problem but doesn't fix the underlying
problem of getting the current channel from the stream loader.  Will
file as a separate bug.

Comment 3

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

Comment on attachment 573421 [details] [diff] [review]
patch v2, use the correct channel from the stream loader

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

r=me with the below fix. But do file a followup on fixing the streamloader as well as on removing mChannel

::: layout/style/nsFontFaceLoader.cpp
@@ +224,5 @@
> +    nsCOMPtr<nsIRequest> request;
> +    nsCOMPtr<nsIHttpChannel> httpChannel;
> +    aLoader->GetRequest(getter_AddRefs(request));
> +    if (request) {
> +      httpChannel = do_QueryInterface(request);

do_QI is null-safe so no need for the if-statement here.

Comment 4

[LegNeato]

Comment on attachment 573421 [details] [diff] [review]
patch v2, use the correct channel from the stream loader

Please land this on mozilla-aurora and Mozilla-beta.

Comment 5

[John Daggett (:jtd)]

Logged bug 701288 as a follow-on bug to fix the other uses of mChannel within nsFontDownloader.

Comment 6

[Alex Keybl [:akeybl]]

Please land this on aurora and beta ASAP, we are considering for release and would like to know about any build issues as soon as possible (so there are no surprises if/when we transplant).

Comment 7

[Brad Dunzer]

I want to thank you guys for jumping on this and helping to get it out as soon as possible.

Comment 8

[John Daggett (:jtd)]

https://hg.mozilla.org/mozilla-central/rev/cd90cf2bad07
https://hg.mozilla.org/releases/mozilla-aurora/rev/13095f4e038d
https://hg.mozilla.org/releases/mozilla-beta/rev/c8aaf836cfe8

Comment 9

[JP Rosevear [:jpr]]

Can we add a test to catch this in future?

Comment 10

[John Daggett (:jtd)]

(In reply to JP Rosevear [:jpr] from comment #9)
> Can we add a test to catch this in future?

Yes, but tests for this are a bit tricky.  I think it will need to be a mochitest rather than a reftest (due to redirection being involved) that uses glyph metrics to test whether the font loaded or not.

Comment 11

[Boris Zbarsky [:bzbarsky]]

You can do a mochitest that compares window snapshots (using WindowSnapshot.js).

Comment 12

[Joe Drew (not getting mail)]

Alternately you could do an HTTP reftest and just write sjs/^headers^ files.

Comment 13

[John Daggett (:jtd)]

Attachment: patch, reftest to test redirection on font loads

Comment 14

[Joe Drew (not getting mail)]

Comment on attachment 574184 [details] [diff] [review]
patch, reftest to test redirection on font loads

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

I presume onload is blocked until the font resource is loaded? If so, r=joe

::: layout/reftests/font-face/reftest.list
@@ +137,5 @@
>  # (using Chunkfive font data returned from a .sjs file)
>  HTTP(..) == font-error-404-1.html font-error-404-1-ref.html # HTTP status 404, don't load
>  HTTP(..) == font-error-404-2.html font-error-404-2-ref.html # HTTP status 200, load
>  HTTP(..) != font-error-404-1.html font-error-404-2.html # sanity-check that the results differ
> +HTTP(..) == font-redirect.html order-1-ref.html

Should probably add a separate comment here, or at least separate with a blank line.

Comment 15

[John Daggett (:jtd)]

Pushed reftest to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/b51eb4007926

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 573421 [details] [diff] [review]
patch v2, use the correct channel from the stream loader

[Triage Comment]
Denying for release as we are no longer considering shipping 8.0.1 to all Firefox users - it will be a targeted release for those who are, or could potentially be, affected by bug 700835 or bug 691271.

Comment 17

[Brad Dunzer]

I would like to make a plea for you guys to push out a 8.0.2 patch with this fix in it. This is posing us a bit of a larger issue now.

Comment 18

[Alex Keybl [:akeybl]]

(In reply to Brad Dunzer from comment #17)
> I would like to make a plea for you guys to push out a 8.0.2 patch with this
> fix in it. This is posing us a bit of a larger issue now.

Our decision not to update our current Windows 8.0 Firefox users to 8.0.1 (due to startup crash 691847, which can occur upon system restore) limited us to taking no additional rendering changes that could make our users out-of-sync.

Unfortunately, we believe that the affected audience there is larger (and the harm worse) than here. The same logic applies to not going out with an 8.0.2 unless deemed absolutely necessary. We'll keep this bug in mind if we do decide to put out an 8.0.2 for all FF users.

Comment 19

[Brad Dunzer]

Alex thanks for the reply.

I am sure I know the answer but would an 8.0.2 be considered even after 9.0 is shipped? We have just found another reason that the 8.0.2 fixed would be huge for us.

For some reason we are able to workaround the 8.0 bug by changing our font calls from http to https. However, we are making some changes to our delivery network and found in testing that our network change broke this https fix in 8.0 and if we move to the new network setup any 8.0 clients in perpetuity will not be able to see web fonts. So I guess I am trying to stress the importance of that fix in 8.0 making it out into the wild.

Again thanks for the consideration

Comment 20

[Boris Zbarsky [:bzbarsky]]

Brad, once 9.0 is shipped 8.0.x is EOL.  9.0 is the planned security update for 8.0.1 at this point.

As in, once 9.0 has shipped there should be no more people using 8.0.x.

Comment 21

[Brad Dunzer]

Hi Boris,

Thanks for the rapid reply. Version 8.0 will auto-update to 9.0 in all instances? Or is this something that is user action required. Just trying to gauge the percentage of audience that will be effected.

Thanks

Comment 22

[Boris Zbarsky [:bzbarsky]]

Version 8.0.1 will update to 9.0 in exactly the same way as it would update to 8.0.2, using the same exact mechanism.  It obviously won't be all instances, since users can disable updates altogether, but it should be the same set of users.

Comment 23

[Tony Chung [:tchung]]

Verified fix on 9.0beta 4. :  Mozilla/5.0 (Windows NT 6.1; rv:9.0)
Gecko/20100101 Firefox/9.0, branch GECKO90_2011113005_RELBRANCH. 

ran tests in URL.

Comment 24

[Brad Dunzer]

Thanks for checking Tony. We have verified with the latest 9 beta's as well.

Thanks everyone

Comment 25

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0

Verified Firefox 9 Beta 4, Aurora and Nightly on Mac OS 10.6, Ubuntu 11.10, Windows 7 and XP using the URL. Fonts loaded correctly for every build.