Last Comment Bug 701262 - redirect requests for fonts cause font load to fail
: redirect requests for fonts cause font load to fail
Status: VERIFIED FIXED
[qa!]
: regression, verified-aurora, verified-beta
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 blocker (vote)
: mozilla11
Assigned To: John Daggett (:jtd)
:
: Jet Villegas (:jet)
Mentors:
http://chinesefaith.org/webink/stage3...
Depends on:
Blocks: 670900
  Show dependency treegraph
 
Reported: 2011-11-09 18:19 PST by John Daggett (:jtd)
Modified: 2011-12-06 01:55 PST (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
fixed
+
fixed


Attachments
patch, test response codes directly (990 bytes, patch)
2011-11-09 18:36 PST, John Daggett (:jtd)
no flags Details | Diff | Splinter Review
patch v2, use the correct channel from the stream loader (914 bytes, patch)
2011-11-09 20:39 PST, John Daggett (:jtd)
jonas: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑release-
Details | Diff | Splinter Review
patch, reftest to test redirection on font loads (2.03 KB, patch)
2011-11-13 14:10 PST, John Daggett (:jtd)
joe: review+
Details | Diff | Splinter Review

Description John Daggett (:jtd) 2011-11-09 18:19:05 PST
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) 2011-11-09 18:36:28 PST
Created attachment 573404 [details] [diff] [review]
patch, test response codes directly
Comment 2 John Daggett (:jtd) 2011-11-09 20:39:15 PST
Created attachment 573421 [details] [diff] [review]
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 2011-11-09 20:57:58 PST
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 christian 2011-11-09 21:10:39 PST
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) 2011-11-09 22:00:27 PST
Logged bug 701288 as a follow-on bug to fix the other uses of mChannel within nsFontDownloader.
Comment 6 Alex Keybl [:akeybl] 2011-11-10 14:32:08 PST
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 2011-11-10 14:40:12 PST
I want to thank you guys for jumping on this and helping to get it out as soon as possible.
Comment 9 JP Rosevear [:jpr] 2011-11-10 16:32:41 PST
Can we add a test to catch this in future?
Comment 10 John Daggett (:jtd) 2011-11-10 16:49:17 PST
(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 [:bz] (still a bit busy) 2011-11-11 12:08:41 PST
You can do a mochitest that compares window snapshots (using WindowSnapshot.js).
Comment 12 Joe Drew (not getting mail) 2011-11-12 19:18:41 PST
Alternately you could do an HTTP reftest and just write sjs/^headers^ files.
Comment 13 John Daggett (:jtd) 2011-11-13 14:10:18 PST
Created attachment 574184 [details] [diff] [review]
patch, reftest to test redirection on font loads
Comment 14 Joe Drew (not getting mail) 2011-11-13 14:15:00 PST
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) 2011-11-13 15:58:23 PST
Pushed reftest to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/b51eb4007926
Comment 16 Alex Keybl [:akeybl] 2011-11-14 15:57:18 PST
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 2011-11-22 10:40:16 PST
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] 2011-11-22 11:35:21 PST
(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 2011-11-30 16:25:24 PST
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 [:bz] (still a bit busy) 2011-11-30 16:28:03 PST
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 2011-11-30 16:34:09 PST
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 [:bz] (still a bit busy) 2011-11-30 16:41:28 PST
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] 2011-12-05 00:06:58 PST
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 2011-12-05 10:13:51 PST
Thanks for checking Tony. We have verified with the latest 9 beta's as well.

Thanks everyone
Comment 25 Virgil Dicu [:virgil] [QA] 2011-12-06 01:55:50 PST
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.

Note You need to log in before you can comment on or make changes to this bug.