Let hyper automatically set the host header if needed

[Darkspirit]

Google's gws web server did not expect to receive an unneeded Host header via HTTP/2, therefore it responded with 400 Bad Request over a working HTTP/2 connection.

https://tools.ietf.org/html/rfc7540#page-55

Clients that generate HTTP/2 requests directly SHOULD use the ":authority" pseudo-header field instead of the Host header field.

It's hyper's job to take care of this for the HTTP/1 case, therefore we can remove old code.
When calling Client::builder() we do not disable hyper's default set_host config option, therefore hyper automatically adds a Host header for non-HTTP/2 connections based on the URI.

r? @jdm

---

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

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/net/resource_thread.rs, components/net/http_loader.rs

[highfive]

Warning

• These commits modify net code, but no tests are modified. Please consider adding a test!

[Darkspirit]

Two tests failed before. These tests compare request headers before hyper plays its role. When building a request we don't yet know if legacy HTTP/1.1 or modern HTTP/2 will be used when the actual connection is established, therefore it's rather an internal matter of hyper to add a Host header in case HTTP/2 is not used.

---- fetch::test_fetch_with_devtools stdout ----
thread 'fetch::test_fetch_with_devtools' panicked at 'assertion failed: (left == right)
left: HttpRequest { url: "http://localhost:45979/", method: GET, headers: {"accept": "*/*", "accept-language": "en-US, en; q=0.5", "user-agent": "Such Browser. Very Layout. Wow.", "accept-encoding": "gzip, deflate, br"}, body: Some([]), pipeline_id: PipelineId { namespace_id: PipelineNamespaceId(1234), index: PipelineIndex(5678) }, startedDateTime: Tm { tm_sec: 12, tm_min: 3, tm_hour: 16, tm_mday: 14, tm_mon: 11, tm_year: 119, tm_wday: 6, tm_yday: 347, tm_isdst: 0, tm_utcoff: 0, tm_nsec: 893941184 }, timeStamp: 1576339392, connect_time: 0, send_time: 36, is_xhr: true },
right: HttpRequest { url: "http://localhost:45979/", method: GET, headers: {"accept-encoding": "gzip, deflate, br", "host": "localhost:45979", "accept": "*/*", "accept-language": "en-US, en; q=0.5", "user-agent": "Such Browser. Very Layout. Wow."}, body: Some([]), pipeline_id: PipelineId { namespace_id: PipelineNamespaceId(1234), index: PipelineIndex(5678) }, startedDateTime: Tm { tm_sec: 12, tm_min: 3, tm_hour: 16, tm_mday: 14, tm_mon: 11, tm_year: 119, tm_wday: 6, tm_yday: 347, tm_isdst: 0, tm_utcoff: 0, tm_nsec: 893941184 }, timeStamp: 1576339392, connect_time: 0, send_time: 36, is_xhr: true }', components/net/tests/fetch.rs:1114:5

request expectation (right): host header containing localhost:45979
actual request (left): no host header

Explanation:
Now removed code previously derived a Host header from URI containing localhost:45979. Now it is gone. Host header should be removed from expectation. Host is still compared as part of HttpRequest.url.

---- http_loader::test_request_and_response_data_with_network_messages stdout ----
thread 'http_loader::test_request_and_response_data_with_network_messages' panicked at 'assertion failed: (left == right)
left: HttpRequest { url: "http://localhost:37201/", method: GET, headers: {"host": "bar.foo", "accept": "text/html, application/xhtml+xml, application/xml; q=0.9, */*; q=0.8", "accept-language": "en-US, en; q=0.5", "user-agent": "Such Browser. Very Layout. Wow.", "accept-encoding": "gzip, deflate, br"}, body: Some([]), pipeline_id: PipelineId { namespace_id: PipelineNamespaceId(1234), index: PipelineIndex(5678) }, startedDateTime: Tm { tm_sec: 13, tm_min: 3, tm_hour: 16, tm_mday: 14, tm_mon: 11, tm_year: 119, tm_wday: 6, tm_yday: 347, tm_isdst: 0, tm_utcoff: 0, tm_nsec: 174672457 }, timeStamp: 1576339393, connect_time: 1, send_time: 3, is_xhr: false },
right: HttpRequest { url: "http://localhost:37201/", method: GET, headers: {"accept-encoding": "gzip, deflate, br", "host": "localhost:37201", "accept": "text/html, application/xhtml+xml, application/xml; q=0.9, */*; q=0.8", "accept-language": "en-US, en; q=0.5", "user-agent": "Such Browser. Very Layout. Wow."}, body: Some([]), pipeline_id: PipelineId { namespace_id: PipelineNamespaceId(1234), index: PipelineIndex(5678) }, startedDateTime: Tm { tm_sec: 13, tm_min: 3, tm_hour: 16, tm_mday: 14, tm_mon: 11, tm_year: 119, tm_wday: 6, tm_yday: 347, tm_isdst: 0, tm_utcoff: 0, tm_nsec: 174672457 }, timeStamp: 1576339393, connect_time: 1, send_time: 3, is_xhr: false }', components/net/tests/http_loader.rs:314:5

request expectation (right): host header containing localhost:37201
actual request (left): host header containing bar.foo

Explanation:
Now removed code replaced a possibly existing Host header with the one derived from URI. We should make sure to remove a potentially existing Host header before handling over to hyper as hyper would otherwise always respect the existing one and not derive one from URI only when needed. Host header should be removed from expectation.

[Darkspirit]

Current PR modifies code at https://fetch.spec.whatwg.org/#http-network-or-cache-fetch step 5.16, the spec says:

It would be great if we could make this more normative somehow. At this point headers such as Accept-Encoding, Connection, DNT, and Host, are to be appended if necessary.

Now the question is whether it is necessary or not. It could make sense to remove the host header only as late as possible before actually calling hyper. Probably around this line:

servo/components/net/http_loader.rs

Line 442 in 4eb6e71

*request.headers_mut() = headers.clone();

[Darkspirit]

So far I have not found a reason that would speak against the proposed patch:

• In contrast to the two tests changed by this PR, WPT is testing the full stack including hyper. With this patch applied, ./mach test-wpt tests/wpt/web-platform-tests/referrer-policy/generic/subresource-test/xhr-messaging.html only fails because of a missing Connection header (#22517). Hyper automatically adds a Host header because this is a plaintext HTTP/1.1 request.
• Fetch can not set a Host header because it is on the list of forbidden header names. It doesn't seem one could access all headers of the actual request, but only one's own entries of a custom Headers object. HTTP/2 requests would anyway not contain a Host header.
• Firefox DevTools always uses HTTP/1.1 terminology: It incorrectly lists a Host header even none is sent via HTTP/2. Chrome DevTools precisely shows a HTTP/1.1 Host request header or a HTTP/2 :authority pseudo request header.
  Using a warp tls server with let routes = warp::header::headers_cloned().map(|headers: HeaderMap| { format!("{:?}", headers) }); I have confirmed that Firefox and Chrome indeed do not send a Host request header via HTTP/2. (If I enforce HTTP/1.1 by setting network.http.spdy.enabled to false, Firefox sends a Host header as expected.)

  {"user-agent": "Mozilla/5.0 (X11; Linux x86_64; rv:73.0) Gecko/20100101 Firefox/73.0", "accept": "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,/;q=0.8", "accept-language": "de-DE,de;q=0.8,en-US;q=0.5,en;q=0.3", "accept-encoding": "gzip, deflate, br", "dnt": "1", "upgrade-insecure-requests": "1", "te": "trailers"}

[jdm]

I appreciate the detail you've provided here! I expect we're going to merge this as-is, but I'm waiting to merge any changes to fundamental parts of the engine like the network stack until we're finished submitting our Servo-based browser to the MS app store this week.

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit 19226c5 has been approved by jdm

[bors-servo]

⌛ Testing commit 19226c5 with merge c3e6ee4...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

@bors-servo retry

• #24762

[bors-servo]

⌛ Testing commit 19226c5 with merge e8edc6e...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

@bors-servo retry

• #24762

[bors-servo]

⌛ Testing commit 19226c5 with merge 79408fa...

[bors-servo]

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 79408fa to master...