Investigate Content-Type failures from tests/wpt/xhr/setrequestheader-content-type.htm

[jdm]

We certainly try to implement step 4 of https://xhr.spec.whatwg.org/#the-send()-method in XMLHttpRequest::Send, but there are a bunch of test failures shown in https://github.com/servo/servo/blob/master/tests/wpt/metadata/xhr/setrequestheader-content-type.htm.ini that demonstrate that our implementation is not correct. We should investigate precisely what is causing the tests not to pass.

[jdm]

@nupurbaghel This might be a good next issue if you're looking for one.

[jdm]

A good place to start is by deleting the ini file so the full error messages are shown (including expected vs. actual values).

[jdm]

Note: any errors about ReadableStreams can be ignored, since we don't implement that yet.

[nupurbaghel]

Thank you so much for suggesting 😄 !
I would really like to work on this issue next :)

[nupurbaghel]

A good place to start is by deleting the ini file

I tried deleting the ini file, and running mach build. But it was build successfully without any errors. Do we need to alter anything else as well?

[jdm]

./mach build is unrelated to these tests. You'll want to run ./mach test-wpt --no-pause-after-test tests/wpt/web-platform-tests/xhr/setrequestheader-content-type.htm to see the errors that occur when running the test.

[nupurbaghel]

On comparing the test results with the actual values, I noticed that the majority of tests were failing because of addition of an extra space.
Also, there was mismatch of lower/upper case. The test files had 'Content-Type' and the results obtained from request had 'content-type'. For the time being I have converted everything to small-case.

[nupurbaghel]

Also, I was trying to understand the flow of data during a request from xhr/xmlhttprequest.rs .Until now I have figured out that the final text_response generated, comes after decoding from self.response which itself is assigned value during process_partial_response. I wanted to confirm I am going in the right direction :)

[jdm]

The actual network data is transferred to XMLHttpRequest via the callbacks in the impl FetchResponseListener for XHRContext, which calls methods like XMLHttpRequest::process_headers_available and XMLHttpRequest::process_data_avaialable, which ends up in XMLHttpRequest::process_partial_response like you said.

[fadedreamz]

@jdm I would like to try it out to get familiar with both rust and servo. Is it something that requires extensive knowledge of the servo? If not please assign it to me.

[jdm]

@fadedreamz This issue should be pretty self-contained, so it does sound like a reasonable introduction to Servo to me.

[fadedreamz]

@jdm Thanks, I would like to take a shot at it, can you please mark it assigned?

[fadedreamz]

@jdm when I ran the ./mach test-wpt --no-pause-after-test tests/wpt/xhr/setrequestheader-content-type.htm command I get the following error:
0:01.12 INFO Using 12 client processes
0:01.12 ERROR Unable to find any tests at the path(s):
0:01.12 ERROR tests/wpt/xhr/setrequestheader-content-type.htm
0:01.12 ERROR Please check spelling and make sure there are tests in the specified path(s).
0:01.12 INFO Closing logging queue
0:01.12 INFO queue closed

I checked and as reported there is no setrequestheader-content-type.htm. Am I missing some steps?

[jdm]

It's tests/wpt/web-platform-tests/xhr/setrequestheader-content-type.htm.

[jdm]

Sorry, I have corrected the incorrect path in the earlier comment.

[fadedreamz]

@jdm can you provide some suggestion on where the headers are being adjusted, I looked into process_headers_available, SetRequestHeader and process_partial_response. I am having hard time to pinpoint where the headers are being adjusted. For example, a failing test case

function _String() { return "test()"; },
        {"Content-Type": "text/javascript;charset=ASCII"},
        "text/javascript;charset=UTF-8",
        "String request keeps setRequestHeader() Content-Type, with charset adjusted to UTF-8"

I am trying to pinpoint where the adjustment of ASCII to UTF-8 is happening. TIA

[jdm]

My first suspicion is uses of final_charset and final_mime_type in that file. Alternatively, I can't easily check the test on my phone right now, but is the test currently failing because we are not adjusting the charset when we should be?

[fadedreamz]

My suspicion is that when adjusting the charset it is introducing a space, which is leading to three failed test case. I will check the mentioned function and update the ticket.

[fadedreamz]

The test case is failing because adjusting it leaves the value to

text/javascript; charset=utf-8

So, an extra space and lower case utf-8 than the expected value. (seems like same root cause for 3 failing test cases).

[fadedreamz]

@jdm sorry for some time break, I investigated a little bit more, but could not find where actual adjusting is happening. Maybe my lack of knowledge with rust and servo is not helping me. Any suggestions?

[jdm]

I'm pretty sure that this code is responsible. For a string request body (as in the failing test case), we default to UTF-8 encoding. Then if the request includes a Content-Type header, we extract its MIME type but ignore its charset value and replace it with the encoding we decided on earlier. That gives us text/javascript; charset=UTF-8, like you said.

[jdm]

This is steps 4.1-4.4 of the XHR send algorithm from the specification.

[jdm]

According to https://mimesniff.spec.whatwg.org/#serialize-a-mime-type we should not be including the space in the resulting serialized mime type.

[fadedreamz]

@jdm thank you. So, I removed the extra space but could not solve the lower case upper case issue. The root cause is when we are unwrapping the serialized mime string to Mime type via parse(), the UTF8 is being converted to lower case.
So later on when we are rebuilding the header we are getting lower cap "utf-8". Possible fix -
a) change the test expected case from upper case to lower case
b) file a bug report/feature request (?) for mime crate?
c) any other suggestions?

[fadedreamz]

@jdm as I couldn't find an easy way to change the mime to uppercase, I changed the value in the test case to lower case. I believe the problem is mime type uses http lowercase notation where xhr expects uppercase notation (Please do correct me if I am wrong) and in rust mime crate all the constant values are in lowercases. I also fixed the mime suffix (e,g - +xml) bug which was leading to other test case failures. At this point now I am left with 4 failing test cases in URLSearchParam (1) and in ReadableStream(3). Now according to xhr (4.1) -
If body is a Document or a USVString, then
so for URLSearchParam it should not change the case, if so, then why in test case we are expecting it to be adjusted?