Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add context-based MIME sniffing #4183

Closed
IdeaHat opened this issue Dec 2, 2014 · 13 comments
Closed

Add context-based MIME sniffing #4183

IdeaHat opened this issue Dec 2, 2014 · 13 comments
Labels
A-network C-assigned There is someone working on resolving the issue E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss

Comments

@IdeaHat
Copy link
Contributor

IdeaHat commented Dec 2, 2014

Follow on issue to #3144

Context based mime sniffing is described in https://mimesniff.spec.whatwg.org/#context-specific-sniffing. Certain types (such as fonts) can only be sniffed from a context. However, this will require more metadata to be handed to the sniffer task.

@jdm jdm added the A-network label Dec 3, 2014
@jdm jdm added the E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss label Aug 26, 2015
@jdm
Copy link
Member

jdm commented Aug 26, 2015

We should add a Context enum and require it to be passed to the LoadData constructor. We can then use this in start_sending_sniffed_opt (pass it as an argument) and have special rules for each of the contexts described in the spec.

@jdm
Copy link
Member

jdm commented Aug 26, 2015

Code: components/net_traits/lib.rs

@Yoric
Copy link
Contributor

Yoric commented Sep 29, 2015

I can try to work on that. @jdm, do I understand correctly that the Context enum would be one of Browsing/Image/AudioVideo/Plugin/Style/Script/Font/Text Track/Cache Manifest?

@jdm
Copy link
Member

jdm commented Sep 29, 2015

That looks fine for a first pass. We'll probably end up mapping those to the values for initiator, type, and destination in the fetch specification at some point, but let's focus on the MIME sniffing spec for now.

@Yoric
Copy link
Contributor

Yoric commented Sep 29, 2015

I'll do that. Any suggestion how I can test it?

@jdm
Copy link
Member

jdm commented Sep 29, 2015

Using a local web server and running Servo with --sniff-mime-types will allow the snifing code to actually execute.

@jdm
Copy link
Member

jdm commented Sep 29, 2015

A straightforward testcase to use will be loading a URL which has a non-text/html Content-Type header but returns HTML content.

@Yoric
Copy link
Contributor

Yoric commented Sep 30, 2015

Testing in progress. Do you think it would be possible to put something in the testsuite?

@Yoric
Copy link
Contributor

Yoric commented Sep 30, 2015

So, I configured Apache to associate mp4 to application/video and I ran the following:

./mach run --sniff-mime-types http://localhost/~david/test/text_html_content_with_non_text_html_mime_type.mp4

where my file is actually html content.

Servo renders it as html, regardless of --sniff-mime-types, I'm not sure how I should interpret the result. In contrast, Firefox attempts to render as video.

@jdm
Copy link
Member

jdm commented Sep 30, 2015

Oh right, #7706 is about changing our HTML parser to actually care about the MIME type. If you use a type like image/jpeg instead, that should show a difference with --sniff-mime-types.

@Yoric
Copy link
Contributor

Yoric commented Sep 30, 2015

How so? Won't Servo attempt to sniff it in a Browser context regardless of my patch?

@jdm
Copy link
Member

jdm commented Sep 30, 2015

http://mxr.mozilla.org/servo/source/components/script/dom/servohtmlparser.rs#115 looks at the computed mime type and creates an image document if we think we have an image. If we sniff the response first and discover HTML content, the result should be a non-image document with sniffing enabled.

@Yoric
Copy link
Contributor

Yoric commented Oct 2, 2015

I still don't follow. Unless I misunderstand the specs, by default, Servo will be in Browser context, so it will sniff exactly the same thing regardless of my patch, right?

Yoric added a commit to Yoric/servo that referenced this issue Oct 20, 2015
The version of the standard is not finalized at the time of this writing.
Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .
jdm pushed a commit to jdm/servo that referenced this issue Oct 25, 2015
The version of the standard is not finalized at the time of this writing.
Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .
jdm pushed a commit to jdm/servo that referenced this issue Nov 12, 2015
The version of the standard is not finalized at the time of this writing.
Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .
jdm added a commit to jdm/servo that referenced this issue Nov 12, 2015
@jdm jdm added the C-assigned There is someone working on resolving the issue label Nov 17, 2015
jdm pushed a commit to jdm/servo that referenced this issue Nov 25, 2015
The version of the standard is not finalized at the time of this writing.
Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .
jdm pushed a commit to jdm/servo that referenced this issue Nov 25, 2015
The version of the standard is not finalized at the time of this writing.
Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .
jdm pushed a commit to jdm/servo that referenced this issue Nov 26, 2015
The version of the standard is not finalized at the time of this writing.
Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .
jdm pushed a commit to jdm/servo that referenced this issue Dec 30, 2015
The version of the standard is not finalized at the time of this writing.
Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .
bors-servo pushed a commit that referenced this issue Dec 31, 2015
Implemementing context-based MIME type sniffing

This is a rebase of #7842 that also adds a test.
Fixes #4183.

@Yoric, how's this look to you?

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8190)
<!-- Reviewable:end -->
jdm pushed a commit to jdm/servo that referenced this issue Dec 31, 2015
The version of the standard is not finalized at the time of this writing.
Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .
jdm pushed a commit to jdm/servo that referenced this issue Dec 31, 2015
The version of the standard is not finalized at the time of this writing.
Specifications may be found here: https://mimesniff.spec.whatwg.org/#context-specific-sniffing .
bors-servo pushed a commit that referenced this issue Dec 31, 2015
Implemementing context-based MIME type sniffing

This is a rebase of #7842 that also adds a test.
Fixes #4183.

@Yoric, how's this look to you?

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8190)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Dec 31, 2015
Implemementing context-based MIME type sniffing

This is a rebase of #7842 that also adds a test.
Fixes #4183.

@Yoric, how's this look to you?

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8190)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-network C-assigned There is someone working on resolving the issue E-more-complex Variable effort required; may require a mentor. Recommended solution is clearly described in the iss
Projects
None yet
Development

No branches or pull requests

3 participants