Skip to content

/ servo

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

Jump to bottom

Can't load google.com #25286

Closed
jdm opened this issue Dec 13, 2019 · 1 comment
Closed

Can't load google.com #25286

jdm opened this issue Dec 13, 2019 · 1 comment
Labels
A-network

Comments

@jdm
Copy link
Member

@jdm jdm commented Dec 13, 2019

We regressed the ability to load https://google.com recently. 6dad51f shows this output:

godot:master-servo jdm$ RUST_LOG=net ./mach run https://google.com
[2019-12-13T19:00:49Z DEBUG net::image_cache] New image cache
[2019-12-13T19:00:51Z INFO  net::cookie_storage]  === COOKIES SENT:
[2019-12-13T19:00:51Z DEBUG net::http_cache] trying to construct cache response for "https://google.com/"
[2019-12-13T19:00:51Z INFO  net::http_loader] request for https://google.com/ (GET)
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("accept", "text/html, application/xhtml+xml, application/xml; q=0.9, */*; q=0.8")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("accept-language", "en-US, en; q=0.5")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("user-agent", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:63.0) Servo/1.0 Firefox/63.0")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("host", "google.com")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("accept-encoding", "gzip, deflate, br")
[2019-12-13T19:00:51Z DEBUG net::http_loader] Not notifying devtools (no request_id)
[2019-12-13T19:00:51Z INFO  net::http_loader] response for https://google.com/
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("location", "https://www.google.com/")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("content-type", "text/html; charset=UTF-8")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("date", "Fri, 13 Dec 2019 19:00:51 GMT")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("expires", "Sun, 12 Jan 2020 19:00:51 GMT")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("cache-control", "public, max-age=2592000")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("server", "gws")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("content-length", "220")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("x-xss-protection", "0")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("x-frame-options", "SAMEORIGIN")
[2019-12-13T19:00:51Z INFO  net::http_loader]  - ("alt-svc", "quic=\":443\"; ma=2592000; v=\"46,43\",h3-Q050=\":443\"; ma=2592000,h3-Q049=\":443\"; ma=2592000,h3-Q048=\":443\"; ma=2592000,h3-Q046=\":443\"; ma=2592000,h3-Q043=\":443\"; ma=2592000")
[2019-12-13T19:00:51Z DEBUG net::http_loader] got 301 response for "https://google.com/"
[2019-12-13T19:00:51Z DEBUG net::http_loader] successfully finished response for "https://google.com/"

Recent revisions show this output:

godot:servo jdm$ RUST_LOG=net ./mach run -d https://google.com
[2019-12-13T20:38:49Z DEBUG net::image_cache] New image cache
[2019-12-13T20:38:51Z INFO  net::cookie_storage]  === COOKIES SENT:
[2019-12-13T20:38:51Z DEBUG net::http_cache] trying to construct cache response for "https://google.com/"
[2019-12-13T20:38:51Z INFO  net::http_loader] request for https://google.com/ (GET)
[2019-12-13T20:38:51Z INFO  net::http_loader]  - ("accept", "text/html, application/xhtml+xml, application/xml; q=0.9, */*; q=0.8")
[2019-12-13T20:38:51Z INFO  net::http_loader]  - ("accept-language", "en-US, en; q=0.5")
[2019-12-13T20:38:51Z INFO  net::http_loader]  - ("user-agent", "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:63.0) Servo/1.0 Firefox/63.0")
[2019-12-13T20:38:51Z INFO  net::http_loader]  - ("host", "google.com")
[2019-12-13T20:38:51Z INFO  net::http_loader]  - ("accept-encoding", "gzip, deflate, br")
[2019-12-13T20:38:51Z DEBUG net::http_loader] Not notifying devtools (no request_id)
[2019-12-13T20:38:51Z INFO  net::http_loader] response for https://google.com/
[2019-12-13T20:38:51Z INFO  net::http_loader]  - ("content-type", "text/html; charset=UTF-8")
[2019-12-13T20:38:51Z INFO  net::http_loader]  - ("referrer-policy", "no-referrer")
[2019-12-13T20:38:51Z INFO  net::http_loader]  - ("content-length", "1555")
[2019-12-13T20:38:51Z INFO  net::http_loader]  - ("date", "Fri, 13 Dec 2019 20:38:51 GMT")
[2019-12-13T20:38:51Z DEBUG net::http_loader] got 400 response for "https://google.com/"
[2019-12-13T20:38:51Z DEBUG net::http_loader] finished response for "https://google.com/" with error

I'm betting it's #24976.
@jdm jdm added the A-network label Dec 13, 2019
@jdm
Copy link
Member Author

@jdm jdm commented Dec 13, 2019

Yep, confirmed that #24976 broke this. cc @Darkspirit. Specifically, the addition of cfg.set_alpn_protos(alpn) triggered this; when I comment it out then loading works fine.
@jdm jdm mentioned this issue Dec 13, 2019
Merged
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Dec 13, 2019
@bors-servo
Auto merge of #25287 - jdm:google-load, r=pcwalton
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
8e2e735 
Disable H2 ALPN.

This was introduced by #24976, and it breaks loading https://google.com. Without any network specialists currently contributing to Servo regularly, I would rather revert the particular change that broke it rather than attempt to continue investigating.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #25286
- [x] These changes do not require tests because no H2 testing, no HTTPS configuration testing.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@Darkspirit Darkspirit mentioned this issue Dec 14, 2019
Merged
3 of 3 tasks complete
bors-servo added a commit that referenced this issue Dec 17, 2019
@bors-servo
Auto merge of #25294 - Darkspirit:h2, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
c3e6ee4 
Let hyper automatically set the host header if needed

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()](https://github.com/servo/servo/blob/1974c875a1fb12103a6916c58ed2cbef8c3e32a2/components/net/connector.rs#L116-L119) we do not disable hyper's default [set_host](https://github.com/hyperium/hyper/blob/4b6099c7aa558e6b1fda146ce6179cb0c67858d7/src/client/mod.rs#L1019-L1024) config option, therefore hyper [automatically adds a Host header for non-HTTP/2 connections](https://github.com/hyperium/hyper/blob/4b6099c7aa558e6b1fda146ce6179cb0c67858d7/src/client/mod.rs#L289-L292)  based on the [URI](https://github.com/servo/servo/blob/3f663d7ab216a841e6250b5b10ce64d34caff97c/components/net/http_loader.rs#L418-L421).

r? @jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #25286.
bors-servo added a commit that referenced this issue Dec 17, 2019
@bors-servo
Auto merge of #25294 - Darkspirit:h2, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e8edc6e 
Let hyper automatically set the host header if needed

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()](https://github.com/servo/servo/blob/1974c875a1fb12103a6916c58ed2cbef8c3e32a2/components/net/connector.rs#L116-L119) we do not disable hyper's default [set_host](https://github.com/hyperium/hyper/blob/4b6099c7aa558e6b1fda146ce6179cb0c67858d7/src/client/mod.rs#L1019-L1024) config option, therefore hyper [automatically adds a Host header for non-HTTP/2 connections](https://github.com/hyperium/hyper/blob/4b6099c7aa558e6b1fda146ce6179cb0c67858d7/src/client/mod.rs#L289-L292)  based on the [URI](https://github.com/servo/servo/blob/3f663d7ab216a841e6250b5b10ce64d34caff97c/components/net/http_loader.rs#L418-L421).

r? @jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #25286.
bors-servo added a commit that referenced this issue Dec 17, 2019
@bors-servo
Auto merge of #25294 - Darkspirit:h2, r=jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
79408fa 
Let hyper automatically set the host header if needed

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()](https://github.com/servo/servo/blob/1974c875a1fb12103a6916c58ed2cbef8c3e32a2/components/net/connector.rs#L116-L119) we do not disable hyper's default [set_host](https://github.com/hyperium/hyper/blob/4b6099c7aa558e6b1fda146ce6179cb0c67858d7/src/client/mod.rs#L1019-L1024) config option, therefore hyper [automatically adds a Host header for non-HTTP/2 connections](https://github.com/hyperium/hyper/blob/4b6099c7aa558e6b1fda146ce6179cb0c67858d7/src/client/mod.rs#L289-L292)  based on the [URI](https://github.com/servo/servo/blob/3f663d7ab216a841e6250b5b10ce64d34caff97c/components/net/http_loader.rs#L418-L421).

r? @jdm

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #25286.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-network
Projects
None yet
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Disable H2 ALPN.
Let hyper automatically set the host header if needed
1 participant
@jdm
You can’t perform that action at this time.