Harmonize content sniffing in HTML 5 and Firefox

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Adam Barth, Assigned: Adam Barth)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

3.96 KB, patch
Joe Drew (not getting mail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
My colleagues and I have put together a web site that makes it easy to compare the mime signatures used by Internet Explorer 7, Firefox 3, Safari 3.1, Google Chrome, and the HTML 5 specification:

http://webblaze.cs.berkeley.edu/2009/content-sniffing/

The portion of the HTML 5 that discusses content sniffing is available at:

http://www.whatwg.org/specs/web-apps/current-work/#content-type-sniffing

I'm hoping we can use this information to converge the content sniffing algorithms used by different browsers and then update the HTML 5 spec to reflect the consensus.

Although some changes to the content sniffing algorithm are likely to be contentious, I'm hoping some of the changes will be clearer:

== image/png ==

Firefox 3
DATA[0:3] == 0x89504e47

Internet Explorer 7
DATA[0:7] == 0x89504e470d0a1a0a

Safari 3.1
No signature

Google Chrome
DATA[0:7] == 0x89504e470d0a1a0a

HTML 5
DATA[0:7] == 0x89504e470d0a1a0a

Recommendation: Match HTML 5 spec

== application/postscript ==

Firefox 3
(strncmp(DATA,"%!PS-Adobe-",11) == 0) || 
(strncmp(DATA,"%! PS-Adobe-",12) == 0)

Internet Explorer 7
strncmp(DATA,"%!",2) == 0

Safari 3.1
No signature

Google Chrome
strncmp(DATA,"%!PS-Adobe-",11) == 0

HTML 5
strncmp(DATA,"%!PS-Adobe-",11) == 0

Recommendation: Match HTML 5 spec

== image/gif ==

Firefox 3
strncasecmp(DATA,"GIF8",4) == 0

Internet Explorer 7
(strncasecmp(DATA,"GIF87",5) == 0) || 
(strncasecmp(DATA,"GIF89",5) == 0)

Safari 3.1
No signature

Google Chrome
(strncmp(DATA,"GIF87a",6) == 0) || 
(strncmp(DATA,"GIF89a",6) == 0)

HTML 5
(strncmp(DATA,"GIF87a",6) == 0) || 
(strncmp(DATA,"GIF89a",6) == 0)

Recommendation: Match HTML 5 spec

== image/x-icon ==

Firefox 3
(DATA[0:3] == 0x00000100) || 
(DATA[0:3] == 0x00000200)

HTML 5
(DATA[0:3] == 0x00000100)
(although it uses the mime type image/vnd.microsoft.icon)

Recommendation: Eliminate this signature

We collected data from opt-in Google Chrome users about how often various signatures triggered, and the Firefox signature for image/x-icon almost never triggers.  For more details about this data set, see:

http://webblaze.cs.berkeley.edu/2009/content-sniffing/metrics.html
(Assignee)

Comment 1

9 years ago
Created attachment 348297 [details] [diff] [review]
Conservative patch

This patch makes the PostScript, GIF, and PNG changes.  I didn't touch x-icon or x-xbitmap because removing those signatures is not zero-risk.  I also adjusted the size of the content sniffing buffer to match the HTML5 spec.  This is should almost never make a difference in a non-attack scenario.

BTW, looking at the GIF decoder, I don't think it could decode a GIF that doesn't match this signature anyway.  Not sure who the right reviewers are, but these folks have made changes to / reviewed changes to this code before.
Assignee: nobody → abarth-mozilla
Attachment #348297 - Flags: superreview?
Attachment #348297 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #348297 - Flags: superreview?(bzbarsky)
Attachment #348297 - Flags: superreview?
Attachment #348297 - Flags: review?(pavlov)
Attachment #348297 - Flags: review?
(Assignee)

Comment 2

9 years ago
Comment on attachment 348297 [details] [diff] [review]
Conservative patch

Oops.  Actually nominate reviewers this time.
Not sure whether Stuart or Joe is a better reviewer for the imagelib stuff here.
Comment on attachment 348297 [details] [diff] [review]
Conservative patch

The unknown decoder changes look ok, though watch out for local file regressions from the s/1024/512/ thing.
Attachment #348297 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

9 years ago
Attachment #348297 - Flags: review?(pavlov) → review?(joe)
Comment on attachment 348297 [details] [diff] [review]
Conservative patch

This patch is likely bitrotted due to my inaction, but I'd like to get it in to mozilla-central at least.
Attachment #348297 - Flags: review?(joe) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/e6812f79d2ba

We should really have tests for content sniffing, I guess.

Thanks Adam!
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.