Last Comment Bug 670900 - font downloader does not handle 404 errors well
: font downloader does not handle 404 errors well
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jonathan Kew (:jfkthame)
:
Mentors:
Depends on: 701262
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-12 04:46 PDT by Jonathan Kew (:jfkthame)
Modified: 2011-11-09 18:19 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch, check for HTTP errors (2.07 KB, patch)
2011-07-13 01:38 PDT, Jonathan Kew (:jfkthame)
bzbarsky: review+
Details | Diff | Splinter Review
reftests (107.79 KB, patch)
2011-07-14 04:49 PDT, Jonathan Kew (:jfkthame)
no flags Details | Diff | Splinter Review
reftests, updated to use a single .sjs file (56.71 KB, patch)
2011-07-17 11:05 PDT, Jonathan Kew (:jfkthame)
roc: review+
Details | Diff | Splinter Review

Description Jonathan Kew (:jfkthame) 2011-07-12 04:46:11 PDT
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).
Comment 1 Boris Zbarsky [:bz] 2011-07-12 06:08:11 PDT
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?
Comment 2 Jonathan Kew (:jfkthame) 2011-07-13 01:38:00 PDT
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.
Comment 3 Boris Zbarsky [:bz] 2011-07-13 06:23:27 PDT
Comment on attachment 545622 [details] [diff] [review]
patch, check for HTTP errors

r=me
Comment 4 Jonathan Kew (:jfkthame) 2011-07-13 07:07:39 PDT
http://hg.mozilla.org/mozilla-central/rev/944c450fc328
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-13 19:26:19 PDT
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 John Daggett (:jtd) 2011-07-13 19:53:36 PDT
(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.
Comment 7 Jonathan Kew (:jfkthame) 2011-07-14 04:49:24 PDT
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.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-14 17:19:16 PDT
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.
Comment 9 Jonathan Kew (:jfkthame) 2011-07-16 11:14:21 PDT
(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?
Comment 10 Boris Zbarsky [:bz] 2011-07-16 14:01:46 PDT
Jonathan, https://bug671906.bugzilla.mozilla.org/attachment.cgi?id=546264 has an example of an sjs that reads a file from the file system.
Comment 11 Jonathan Kew (:jfkthame) 2011-07-16 14:35:40 PDT
(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....
Comment 12 Boris Zbarsky [:bz] 2011-07-16 17:53:31 PDT
You may not be missing anything.

Make this a mochitest using WindowSnapshot?
Comment 13 Jonathan Kew (:jfkthame) 2011-07-17 11:05:22 PDT
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? :)
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-17 16:42:50 PDT
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 :-)
Comment 15 Jonathan Kew (:jfkthame) 2011-07-18 06:22:36 PDT
Pushed tests to mozilla-inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/84024a32e8d1

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