font downloader does not handle 404 errors well

RESOLVED FIXED in mozilla8

Status

()

Core
Layout: Text
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

Trunk
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
See bug 494130 comment 14. If a resource pointed to by @font-face returns a 404 (not found) error, the downloader does not set a suitable error code but just returns the 404 error page as "font data" (which then gets rejected by the sanitizer as invalid).
I assume we don't do a MIME type check here, which is why we treat it as font data?

Then again, if the 404 error page were font data with a font MIME type, would we want to use it?
(Assignee)

Comment 2

6 years ago
Created attachment 545622 [details] [diff] [review]
patch, check for HTTP errors

(In reply to comment #1)
> I assume we don't do a MIME type check here, which is why we treat it as
> font data?

Yes, there's no MIME type checking for downloaded font data. (IIRC, there isn't actually a standardized MIME type for OpenType/TrueType fonts yet.)

> Then again, if the 404 error page were font data with a font MIME type,
> would we want to use it?

I can't think of any sensible use-case for this. The error status indicates an error, and we should simply treat it as a failure to get the requested resource.
Assignee: nobody → jfkthame
Attachment #545622 - Flags: review?(bzbarsky)
Comment on attachment 545622 [details] [diff] [review]
patch, check for HTTP errors

r=me
Attachment #545622 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 4

6 years ago
http://hg.mozilla.org/mozilla-central/rev/944c450fc328
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
How about a test? Should be pretty easy, just try to use a font with a URI that maps to an .sjs that returns a real font but with an HTTP error code.

Comment 6

6 years ago
(In reply to comment #5)
> How about a test? Should be pretty easy, just try to use a font with a URI
> that maps to an .sjs that returns a real font but with an HTTP error code.

Note that we already have reftests that test the other way, namely when the data is a 404 error page.
(Assignee)

Comment 7

6 years ago
Created attachment 545888 [details] [diff] [review]
reftests

Something like this? The idea is to test that loading a font via an .sjs script works as expected (mainly to make sure I got the test structure to work), and that if the same font data is returned with a 404 status, we don't load it in this case.
Attachment #545888 - Flags: review?(roc)
Comment on attachment 545888 [details] [diff] [review]
reftests

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

Get into the habit of putting <!DOCTYPE HTML> in all your reftests, just to make sure that standards mode is triggered. It matters more for IE.

Instead of including the file as an array, I think you can just read it from the file system. See update.sjs for some code that does that.
(Assignee)

Comment 9

6 years ago
(In reply to comment #8)
> Instead of including the file as an array, I think you can just read it from
> the file system. See update.sjs for some code that does that.

It's not obvious to me how the .sjs can know the path to the font file in order to read it.... any suggestions?
Jonathan, https://bug671906.bugzilla.mozilla.org/attachment.cgi?id=546264 has an example of an sjs that reads a file from the file system.
(Assignee)

Comment 11

6 years ago
(In reply to comment #10)
> Jonathan, https://bug671906.bugzilla.mozilla.org/attachment.cgi?id=546264
> has an example of an sjs that reads a file from the file system.

Yes, but that's in a mochitest, reading a file that's also within the mochitest subtree... it can locate that by starting from CurWorkD (which is apparently the $objdir/_tests/testing/mochitest/ directory, under which all the test files have been linked or copied during the build) and appending path elements.

For reftest, however, it appears that CurWorkD simply points to my objdir, and I don't know how to reliably get from there to a file in the reftest subtree. The reftests don't get copied under the objdir, like mochitests do, so I can't get there from CurWorkD. AFAICS I'd need the absolute path to the original source tree (or the reftests, anyway). Or I could locate the font file relative to the .sjs file, if I knew its path at runtime, but I haven't succeeded in getting that either.

Am I missing something embarrassingly simple? It doesn't seem like this should be so hard....
You may not be missing anything.

Make this a mochitest using WindowSnapshot?
(Assignee)

Comment 13

6 years ago
Created attachment 546431 [details] [diff] [review]
reftests, updated to use a single .sjs file

I don't see how to directly read the font file when this is implemented as a reftest (see discussion above). Converting this to a mochitest seems like overkill, though. Here's a version that consolidates the two .sjs files into one, so that we just have a single copy of the font dumped into byte-array form; how's that for a compromise? :)
Attachment #545888 - Attachment is obsolete: true
Attachment #546431 - Flags: review?(roc)
Attachment #545888 - Flags: review?(roc)
Comment on attachment 546431 [details] [diff] [review]
reftests, updated to use a single .sjs file

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

Sure, you've spent enough time on this :-)
Attachment #546431 - Flags: review?(roc) → review+
(Assignee)

Comment 15

6 years ago
Pushed tests to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/84024a32e8d1
http://hg.mozilla.org/mozilla-central/rev/84024a32e8d1
Target Milestone: --- → mozilla8

Updated

6 years ago
Depends on: 701262
You need to log in before you can comment on or make changes to this bug.