Skip to content

Integrate fetch into WebSocket implementation#36616

Merged
yezhizhen merged 2 commits intoservo:mainfrom
pylbrecht:websocket-fetch-wedding
Oct 18, 2025
Merged

Integrate fetch into WebSocket implementation#36616
yezhizhen merged 2 commits intoservo:mainfrom
pylbrecht:websocket-fetch-wedding

Conversation

@pylbrecht
Copy link
Contributor

@pylbrecht pylbrecht commented Apr 19, 2025

Description

For the initial WebSocket handshake, we need to create a proper handshake request and fetch() it. Creating this handshake request begins in WebSocketMethods::Constructor(), which sends the partially prepared RequestBuilder over to components/net/resource_thread.rs. There we handle the incoming CoreResourceMsg::Fetch message and finish creating the handshake request in components/net/websocket_loader.rs::create_handshake_request(). Finally we fetch the request by calling components/net/fetch/methods.rs::fetch().

fetch() eventually calls http_network_fetch(). This is where the "actual fetching" of the request takes place on a network level, which means we need to handle WebSocket handshake requests differently than non-WebSocket requests. I mostly moved the existing code, which uses tungstenite, with some type massaging (thanks again, @jdm, for helping me out here!). This included converting from tungstenite types to Servo's net types (request/response).

The tricky bits

In order to fetch the handshake request via components/net/fetch/methods.rs::fetch(), we need to convert the request URL's scheme from ws(s) to http(s). Then, we need to "undo" this conversion again when doing CSP checks (i.e. http(s) back to ws(s)). To avoid having this "undoing" logic in a bunch of places we introduced Request::original_url(), holding the URL before the scheme conversion took place. Unfortunately this only gets as so far. There are still some places, where we need to explicitly check and/or convert the URL scheme (e.g. retroactively upgrading to a secure scheme).

Related


Fixes: #35028

@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch from a391b0c to 4d28c0a Compare April 19, 2025 18:25
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch from 4d28c0a to 4d9592a Compare May 7, 2025 19:03
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch 3 times, most recently from 9a5a2d6 to ef6ed36 Compare June 18, 2025 19:53
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch from 051c265 to 84bebba Compare July 2, 2025 18:14
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch 6 times, most recently from 7be9a4f to 6ade97e Compare July 16, 2025 05:12
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch 2 times, most recently from 883dd29 to 7e9a409 Compare July 31, 2025 17:45
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch 4 times, most recently from 26c3c14 to f20136b Compare August 13, 2025 19:30
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch from f20136b to 31e19ed Compare August 21, 2025 13:17
@jdm
Copy link
Member

jdm commented Aug 22, 2025

I'm a big fan of the direction that the latest commit is going in. 🚀

@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch 2 times, most recently from a69555b to 57ad9a7 Compare August 24, 2025 05:13
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch from 57ad9a7 to e5f9da2 Compare September 6, 2025 04:18
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch from 91d0b94 to 9ae2752 Compare September 10, 2025 19:15
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Sep 10, 2025
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch from 9ae2752 to 412d26a Compare September 10, 2025 19:28
pylbrecht added a commit to pylbrecht/servo that referenced this pull request Oct 14, 2025
Resolving
servo#36616 (comment).

Signed-off-by: pylbrecht <pylbrecht@mailbox.org>
pylbrecht added a commit to pylbrecht/servo that referenced this pull request Oct 14, 2025
Resolving
servo#36616 (comment).

Signed-off-by: pylbrecht <pylbrecht@mailbox.org>
pylbrecht added a commit to pylbrecht/servo that referenced this pull request Oct 14, 2025
Resolving
servo#36616 (comment).

Signed-off-by: pylbrecht <pylbrecht@mailbox.org>
pylbrecht added a commit to pylbrecht/servo that referenced this pull request Oct 15, 2025
Resolving servo#36616 (comment).

Signed-off-by: pylbrecht <pylbrecht@mailbox.org>
pylbrecht added a commit to pylbrecht/servo that referenced this pull request Oct 15, 2025
Resolving
servo#36616 (comment).

Signed-off-by: pylbrecht <pylbrecht@mailbox.org>
pylbrecht added a commit to pylbrecht/servo that referenced this pull request Oct 15, 2025
Resolving
servo#36616 (comment).

Signed-off-by: pylbrecht <pylbrecht@mailbox.org>
@pylbrecht pylbrecht changed the title Draft: marry websockets to fetch Integrate fetch() into WebSocket implementation Oct 17, 2025
@pylbrecht pylbrecht changed the title Integrate fetch() into WebSocket implementation Integrate fetch into WebSocket implementation Oct 17, 2025
Copy link
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this cleaned up and merged!!

@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch from 979fe24 to 737be28 Compare October 17, 2025 18:28
@pylbrecht pylbrecht marked this pull request as ready for review October 17, 2025 18:33
@pylbrecht pylbrecht requested a review from gterzian as a code owner October 17, 2025 18:33
@servo-highfive servo-highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Oct 17, 2025
For the initial WebSocket handshake, we need to create a proper
handshake request and `fetch()` it. Creating this handshake request
begins in `WebSocketMethods::Constructor()`, which sends the partially
prepared `RequestBuilder` over to `components/net/resource_thread.rs`.
There we handle the incoming `CoreResourceMsg::Fetch` message and finish
creating the handshake request in
`components/net/websocket_loader.rs::create_handshake_request()`.
Finally we fetch the request by calling
`components/net/fetch/methods.rs::fetch()`.

`fetch()` eventually calls `http_network_fetch()`. This is where the
"actual fetching" of the request takes place on a network level, which
means we need to handle WebSocket handshake requests differently than
non-WebSocket requests. I mostly moved the existing code, which uses
`tungstenite`, with some type massaging (thanks again, @jdm, for helping
me out here!). This included converting from tungstenite types to
Servo's net types (request/response).

# The tricky bits

In order to fetch the handshake request via
`components/net/fetch/methods.rs::fetch()`, we need to convert the request
URL's scheme from ws(s) to http(s). Then, we need to "undo" this
conversion again when doing CSP checks (i.e. http(s) back to ws(s)). To
avoid having this "undoing" logic in a bunch of places we introduced
`Request::original_url()`, holding the URL before the scheme conversion
took place. Unfortunately this only gets as so far. There are still some
places, where we need to explicitly check and/or convert the URL scheme
(e.g. retroactively upgrading to a secure scheme).

# Related

* https://websockets.spec.whatwg.org/#concept-websocket-establish
* https://fetch.spec.whatwg.org/#http-network-fetch
* w3c/webappsec-csp#532

---

Fixes: servo#35028

---

Signed-off-by: pylbrecht <pylbrecht@mailbox.org>
Tiny drive-by improvement.

Signed-off-by: pylbrecht <pylbrecht@mailbox.org>
@pylbrecht pylbrecht force-pushed the websocket-fetch-wedding branch from 737be28 to 948a742 Compare October 17, 2025 18:33
@jdm jdm enabled auto-merge October 17, 2025 18:36
@jdm jdm added this pull request to the merge queue Oct 17, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 17, 2025
@yezhizhen yezhizhen removed this pull request from the merge queue due to a manual request Oct 18, 2025
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 18, 2025
@yezhizhen yezhizhen added this pull request to the merge queue Oct 18, 2025
@servo-highfive servo-highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 18, 2025
@yezhizhen
Copy link
Member

yezhizhen commented Oct 18, 2025

Requeue this and #39940 to make sure #39968 merge first to improve entire CI speed.. None of tasks have started anyway, nothing wasted.

image

Merged via the queue into servo:main with commit 72f6e72 Oct 18, 2025
33 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-awaiting-review There is new code that needs to be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Websocket connection is not integrated into main fetch implementation

4 participants