Closed Bug 670900 Opened 13 years ago Closed 13 years ago

font downloader does not handle 404 errors well

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla8

People

(Reporter: jfkthame, Assigned: jfkthame)

References

Details

Attachments

(2 files, 1 obsolete file)

patch, check for HTTP errors
2.07 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
reftests, updated to use a single .sjs file
56.71 KB, patch
roc
: review+
Details | Diff | Splinter Review 
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?
(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+
http://hg.mozilla.org/mozilla-central/rev/944c450fc328
Status: NEW → RESOLVED
Closed: 13 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.
(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.
Attached patch reftests (obsolete) — — Splinter Review 
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.
(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.
(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?
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+
Depends on: 701262
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: