redirect requests for fonts cause font load to fail

VERIFIED FIXED in Firefox 9

Status

()

Core
CSS Parsing and Computation
P1
blocker
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jtd, Assigned: jtd)

Tracking

({regression, verified-aurora, verified-beta})

Trunk
mozilla11
regression, verified-aurora, verified-beta
Points:
---

Firefox Tracking Flags

(firefox8- wontfix, firefox9+ fixed, firefox10+ fixed)

Details

(Whiteboard: [qa!], URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 670900
(Assignee)

Comment 1

6 years ago
Created attachment 573404 [details] [diff] [review]
patch, test response codes directly

Updated

6 years ago
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?
Keywords: regression
(Assignee)

Comment 2

6 years ago
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.
Attachment #573404 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
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 4

6 years ago
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+
(Assignee)

Comment 5

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

Updated

6 years ago
tracking-firefox8: ? → +

Comment 6

6 years ago
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

6 years ago
I want to thank you guys for jumping on this and helping to get it out as soon as possible.
Blocks: 701537
(Assignee)

Comment 8

6 years ago
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 9

6 years ago
Can we add a test to catch this in future?
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 13

6 years ago
Created attachment 574184 [details] [diff] [review]
patch, reftest to test redirection on font loads
Attachment #574184 - Flags: review?(joe)
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+
(Assignee)

Comment 15

6 years ago
Pushed reftest to mozilla-central
https://hg.mozilla.org/mozilla-central/rev/b51eb4007926
No longer blocks: 701537

Updated

6 years ago
tracking-firefox8: + → -
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-

Updated

6 years ago
status-firefox10: --- → fixed
status-firefox9: --- → fixed
tracking-firefox10: --- → +
tracking-firefox9: ? → +
Target Milestone: --- → mozilla11

Comment 17

6 years ago
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.

Comment 19

6 years ago
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.

Comment 21

6 years ago
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
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

Comment 24

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

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