Last Comment Bug 391667 - imglib does not gracefully handle unknown images sent with incorrect MIME types
: imglib does not gracefully handle unknown images sent with incorrect MIME types
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9alpha8
Assigned To: Ben Karel [eschew]
:
:
Mentors:
: 374088 (view as bug list)
Depends on:
Blocks: mng 104906
  Show dependency treegraph
 
Reported: 2007-08-10 06:14 PDT by Ben Karel [eschew]
Modified: 2007-11-27 18:06 PST (History)
9 users (show)
asqueella: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
First approach (3.19 KB, patch)
2007-08-10 06:14 PDT, Ben Karel [eschew]
no flags Details | Diff | Splinter Review
Same logic, clearer comment (2.17 KB, patch)
2007-08-21 16:04 PDT, Ben Karel [eschew]
no flags Details | Diff | Splinter Review
Same logic, clearer comment, with .h changes too this time (3.24 KB, patch)
2007-08-21 16:12 PDT, Ben Karel [eschew]
pavlov: review+
pavlov: approval1.9+
Details | Diff | Splinter Review
as checked in (3.27 KB, patch)
2007-09-02 09:13 PDT, Nickolay_Ponomarev
no flags Details | Diff | Splinter Review

Description Ben Karel [eschew] 2007-08-10 06:14:30 PDT
Created attachment 276113 [details] [diff] [review]
First approach

Third-party image decoders only work correctly when images are sent with the correct MIME type. If the server sends an incorrect MIME type, then imglib eats the request and never passes it on to other decoders.

When loading an image from an <img> element, imgRequest currently does two things when trying to find a decoder. First, it uses content sniffing to try to detect common image types -- gif, jpg, png, etc. If that fails, it takes the mime type and tries to find a decoder registered with the contract id "@mozilla.org/image/decoder;2?type=channel_mime_type". If the server sends the correct mime type, then all is well. If, however, the server sends an incorrect mime type (such as "text/plain"), then imglib, finding that there is no decoder registered for type=text/plain, cancels the image request.

Attached is a patch that rectifies the problem. It creates a new category, "image-sniffing-services", under which third-party image decoders may, at their discretion, register nsIContentSniffer implementations. After imglib sniffs for the common types it knows about, and before it relies on the none-too-trustworthy server MIME, imglib queries the nsIContentSniffer implementations registered in the category to try to determine a MIME type.

One disadvantage here is that this approach may blur a line between imglib and necko. That sort of architecture decision is beyond my ken. On the other hand, it incurs zero performance penalty in the common case of gif/jpg/png images.
Comment 1 Ben Karel [eschew] 2007-08-21 16:04:30 PDT
Created attachment 277624 [details] [diff] [review]
Same logic, clearer comment

This patch has the same logic as the previous, but the comment makes it clearer what's happening, and why. I think the structure of the code, with an explicit early return instead of a big if wrapper, better reflects the common and uncommon code paths.
Comment 2 Ben Karel [eschew] 2007-08-21 16:12:32 PDT
Created attachment 277630 [details] [diff] [review]
Same logic, clearer comment, with .h changes too this time

Sorry for the bugspam, I forgot to include imgRequest.h's diff in the previous patch.

On a side note, where's the proper place to document the addition of a category like image-sniffing-services?
Comment 3 Stuart Parmenter 2007-08-21 21:10:57 PDT
not sure where the best place to document it would be.  possibly on the wiki (wiki.mozilla.org) somewhere.
Comment 4 Nickolay_Ponomarev 2007-09-02 09:12:32 PDT
mozilla/modules/libpr0n/src/imgRequest.cpp  1.96
mozilla/modules/libpr0n/src/imgRequest.h    1.37


documentation should be on developer.mozilla.org... not sure where to put the docs for the new category off-hand. Ask dev-mdc perhaps?
Comment 5 Nickolay_Ponomarev 2007-09-02 09:13:23 PDT
Created attachment 279331 [details] [diff] [review]
as checked in
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2007-09-02 11:04:44 PDT
*** Bug 374088 has been marked as a duplicate of this bug. ***
Comment 7 Ben Karel [eschew] 2007-11-27 18:06:50 PST
Added some initial documentation on devmo, including pages for nsIContentSniffer and the image-sniffing-services category, with a link on the Firefox 3 For Developers page.

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