Set response mime_type based on Content-Type #24669

glowe · 2019-11-06T01:24:57Z

Set the response's mime_type based on its Content-Type header whenever Response::set_headers is called. This ensures that mime_type is always consistent with Content-Type. The other reason for going with this approach is it relies on extract_mime_type:

servo/components/script/dom/headers.rs

Lines 260 to 261 in afbcbf7

    
           // https://fetch.spec.whatwg.org/#concept-header-extract-mime-type 
        
           pub fn extract_mime_type(&self) -> Vec<u8> {

Besides the consistency argument, I also expect that we'll update the extract_mime_type method to conform with its specification.

Another option I explored was adding a set_mime_type member to Response and calling that with the mime type from Metadata::content_type in FetchResponseListener::process_response. I decided against this since it expanded the interface of Response and didn't have the same benefits of the first option.

Fixes #24628.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #24628.

There are tests for these changes

highfive · 2019-11-06T01:25:05Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.

highfive · 2019-11-06T01:25:06Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/response.rs
@KiChjang: components/script/dom/response.rs

jdm · 2019-11-12T19:18:09Z

Sorry for the review delay, and thanks for the detailed explanation! I agree that this seems like a sensible way to fix the issue.
@bors-servo r+

bors-servo · 2019-11-12T19:18:10Z

📌 Commit 4145d96 has been approved by jdm

bors-servo · 2019-11-12T22:27:52Z

⌛ Testing commit 4145d96 with merge da43524...


        Auto merge of #24669 - glowe:issue-24628/set-mime-type, r=jdm

Set response mime_type based on Content-Type  Set the response's `mime_type` based on its `Content-Type` header whenever `Response::set_headers` is called. This ensures that `mime_type` is always consistent with `Content-Type`. The other reason for going with this approach is it relies on `extract_mime_type`: https://github.com/servo/servo/blob/afbcbf75eaa63ff0eec8fd3858e9155eb8dbadaa/components/script/dom/headers.rs#L260-L261 Besides the consistency argument, I also expect that we'll update the `extract_mime_type` method to conform with its specification. Another option I explored was adding a `set_mime_type` member to `Response` and calling that with the mime type from `Metadata::content_type` in `FetchResponseListener::process_response`. I decided against this since it expanded the interface of `Response` and didn't have the same benefits of the first option. Fixes #24628. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24628.  - [x] There are tests for these changes

bors-servo · 2019-11-12T22:55:09Z

💔 Test failed - linux-rel-css

jdm · 2019-11-12T23:03:54Z

  ▶ ERROR [expected OK] /fetch/corb/script-html-via-cross-origin-blob-url.sub.html
  │   → expected expression, got '<'
  │ 
  └ blob:http://www1.web-platform.test:8000/814fc97067474afd80a7dda71854d8e8:1:0

That's interesting...

jdm · 2019-11-12T23:05:41Z

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/fetch/corb/script-html-via-cross-origin-blob-url.sub.html loads https://searchfox.org/mozilla-central/source/testing/web-platform/tests/fetch/corb/resources/subframe-that-posts-html-containing-blob-url-to-parent.html which does use fetch and Response, so we should dig into what's changed there.

glowe · 2019-11-14T05:43:54Z

It looks like setting the mime type is allowing this test (script-html-via-cross-origin-blob-url.sub.html) to progress to a point where it's revealing a bug in the CORB handling in servo. Here's the relevant excerpt:

servo/tests/wpt/web-platform-tests/fetch/corb/script-html-via-cross-origin-blob-url.sub.html

Lines 21 to 33 in f65cb94

    
           function step2_processSubframeMsg(msg) { 
        
             assert_false(msg.hasOwnProperty('error'), 'unexpected property found: "error"'); 
        
             assert_equals(msg.blob_type, 'text/html'); 
        
             assert_equals(msg.blob_size, 147); 
        
             // With and without CORB loading of a cross-origin blob should be blocked 
        
             // (this is verified by expecting |script.onerror|, but not |script.onload| 
        
             // below). 
        
             var script = document.createElement("script") 
        
             script.src = msg.blob_url; 
        
             script.onerror = t.step_func_done(function(){}) 
        
             script.onload = t.unreached_func("Unexpected load event") 
        
             document.body.appendChild(script)

step2_processSubframeMsg is the function that is called when subframe-that-posts-html-containing-blob-url-to-parent.html posts a message back to its parent. Previously, this function was failing at line 23:

assert_equals(msg.blob_type, 'text/html');

Now that the proper mime type is set, the test progresses but errors out later. It looks like Servo is allowing html-correctly-labeled.html to be added to the document as a script element.

servo/tests/wpt/web-platform-tests/fetch/corb/script-html-via-cross-origin-blob-url.sub.html

Line 33 in f65cb94

document.body.appendChild(script)

The trouble is that this is content actually an HTML page and not javascript, so Servo chokes on the first character of <!DOCTYPE html>. I actually changed the content of html-correctly-labeled.html to javascript and the test continued but failed with:

Servo should actually block this content from being loaded, but I suspect that we haven't implemented the appropriate CORB checks yet.

One option to move things forward would be to update script-html-via-cross-origin-blob-url.sub.html.ini to expect an ERROR rather than a FAIL. Another option would be to leave this PR open until we've implemented the proper CORB checks.

jdm · 2019-11-14T05:59:29Z

Aha, thanks! Your analysis makes complete sense; let's update the expected result since we haven't implemented the CORB specification yet.


        Set response mime_type based on Content-Type

Set the response's mime_type based on its Content-Type header whenever Response::set_headers is called. Fixes #24628.

jdm · 2019-11-18T15:34:39Z

@bors-servo r+

bors-servo · 2019-11-18T15:34:41Z

📌 Commit e73528e has been approved by jdm

bors-servo · 2019-11-18T18:14:14Z

⌛ Testing commit e73528e with merge 2f44c53...


        Auto merge of #24669 - glowe:issue-24628/set-mime-type, r=jdm

Set response mime_type based on Content-Type  Set the response's `mime_type` based on its `Content-Type` header whenever `Response::set_headers` is called. This ensures that `mime_type` is always consistent with `Content-Type`. The other reason for going with this approach is it relies on `extract_mime_type`: https://github.com/servo/servo/blob/afbcbf75eaa63ff0eec8fd3858e9155eb8dbadaa/components/script/dom/headers.rs#L260-L261 Besides the consistency argument, I also expect that we'll update the `extract_mime_type` method to conform with its specification. Another option I explored was adding a `set_mime_type` member to `Response` and calling that with the mime type from `Metadata::content_type` in `FetchResponseListener::process_response`. I decided against this since it expanded the interface of `Response` and didn't have the same benefits of the first option. Fixes #24628. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24628.  - [x] There are tests for these changes

bors-servo · 2019-11-18T20:21:51Z

💔 Test failed - status-taskcluster

jdm · 2019-11-18T20:27:14Z

@bors-servo retry

#23290

bors-servo · 2019-11-18T22:31:49Z

⌛ Testing commit e73528e with merge 18ab860...


        Auto merge of #24669 - glowe:issue-24628/set-mime-type, r=jdm

Set response mime_type based on Content-Type  Set the response's `mime_type` based on its `Content-Type` header whenever `Response::set_headers` is called. This ensures that `mime_type` is always consistent with `Content-Type`. The other reason for going with this approach is it relies on `extract_mime_type`: https://github.com/servo/servo/blob/afbcbf75eaa63ff0eec8fd3858e9155eb8dbadaa/components/script/dom/headers.rs#L260-L261 Besides the consistency argument, I also expect that we'll update the `extract_mime_type` method to conform with its specification. Another option I explored was adding a `set_mime_type` member to `Response` and calling that with the mime type from `Metadata::content_type` in `FetchResponseListener::process_response`. I decided against this since it expanded the interface of `Response` and didn't have the same benefits of the first option. Fixes #24628. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #24628.  - [x] There are tests for these changes

bors-servo · 2019-11-19T00:31:57Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 18ab860 to master...

highfive assigned jdm Nov 6, 2019

highfive added the S-awaiting-review label Nov 6, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 12, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 12, 2019

Set response mime_type based on Content-Type

Loading status checks…

e73528e

Set the response's mime_type based on its Content-Type header whenever Response::set_headers is called. Fixes #24628.

glowe force-pushed the glowe:issue-24628/set-mime-type branch from 4145d96 to e73528e Nov 14, 2019

highfive added S-awaiting-review and removed S-tests-failed labels Nov 14, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 18, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 18, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 18, 2019

bors-servo merged commit e73528e into servo:master Nov 19, 2019
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 19, 2019

servo / servo

Set response mime_type based on Content-Type #24669

Set response mime_type based on Content-Type #24669

glowe commented Nov 6, 2019

highfive commented Nov 6, 2019

highfive commented Nov 6, 2019

jdm commented Nov 12, 2019

bors-servo commented Nov 12, 2019

bors-servo commented Nov 12, 2019

bors-servo commented Nov 12, 2019

jdm commented Nov 12, 2019

jdm commented Nov 12, 2019

glowe commented Nov 14, 2019

jdm commented Nov 14, 2019

jdm commented Nov 18, 2019

bors-servo commented Nov 18, 2019

bors-servo commented Nov 18, 2019

bors-servo commented Nov 18, 2019

jdm commented Nov 18, 2019

bors-servo commented Nov 18, 2019

bors-servo commented Nov 19, 2019

servo / servo

Join GitHub today

Set response mime_type based on Content-Type #24669

Set response mime_type based on Content-Type #24669

Conversation

glowe commented Nov 6, 2019

highfive commented Nov 6, 2019

highfive commented Nov 6, 2019

jdm commented Nov 12, 2019

bors-servo commented Nov 12, 2019

bors-servo commented Nov 12, 2019

bors-servo commented Nov 12, 2019

jdm commented Nov 12, 2019

jdm commented Nov 12, 2019

glowe commented Nov 14, 2019

jdm commented Nov 14, 2019

jdm commented Nov 18, 2019

bors-servo commented Nov 18, 2019

bors-servo commented Nov 18, 2019

bors-servo commented Nov 18, 2019

jdm commented Nov 18, 2019

bors-servo commented Nov 18, 2019

bors-servo commented Nov 19, 2019