Closed Bug 701262 Opened 8 years ago Closed 8 years ago

redirect requests for fonts cause font load to fail


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




Tracking Status
firefox8 - wontfix
firefox9 + fixed
firefox10 + fixed


(Reporter: jtd, Assigned: jtd)




(Keywords: regression, verified-aurora, verified-beta, Whiteboard: [qa!])


(2 files, 1 obsolete file)

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.


1. Load the URL in Firefox 8 or trunk

Result: fonts don't load

The changeset for bug 670900:

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 +    }

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

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

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.
Blocks: 670900
Keywords: regression
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.
Attachment #573404 - Attachment is obsolete: true
Attachment #573421 - Flags: review?(jonas)
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.
Attachment #573421 - Flags: review?(jonas) → review+
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.
Attachment #573421 - Flags: approval-mozilla-release?
Attachment #573421 - Flags: approval-mozilla-beta+
Attachment #573421 - Flags: approval-mozilla-aurora+
Logged bug 701288 as a follow-on bug to fix the other uses of mChannel within nsFontDownloader.
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).
I want to thank you guys for jumping on this and helping to get it out as soon as possible.
Can we add a test to catch this in future?
(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.
You can do a mochitest that compares window snapshots (using WindowSnapshot.js).
Alternately you could do an HTTP reftest and just write sjs/^headers^ files.
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.
Attachment #574184 - Flags: review?(joe) → review+
Pushed reftest to mozilla-central
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.
Attachment #573421 - Flags: approval-mozilla-release? → approval-mozilla-release-
Target Milestone: --- → mozilla11
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.
(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.
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
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.
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.

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.
Whiteboard: [qa+]
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.
Keywords: verified-beta
Thanks for checking Tony. We have verified with the latest 9 beta's as well.

Thanks everyone
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.
Keywords: verified-aurora
Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.