Integrate websocket connection creation into fetch network stack

[jdm]

Right now, the DOM implementation of WebSocket sends a special message to the resource thread asking for a websocket connection to a particular URL. #14895 allows us to model the code more like the specification instead, and make the DOM code look like other code that initiates network connections. To start with, let's remove the special case (WebsocketConnection) from CoreResourceMsg, and make the resource thread check the request's mode and use the websocket loader if applicable (instead of the regular fetch code).

@dowoncha Would you like to do this?

[dowoncha]

Yeah, I will take this. If the websocketconnection is removed from CoreResourceMsg, where would I get the WebSocketCommunicate, and WebSocketConnectData to pass to websocket_connect? Also would I be adding this check inside the thread builder in CoreResourceManager::fetch?

[jdm]

Those are good questions! I propose we add the WebSocketCommunicate and protocols (from WebSocketConnectData) as fields of the WebSocket request mode enum variant; the other data should already be present in the RequestInit structure that we send with the Fetch message. This check can live outside of the thread builder, since websocket_loader already creates its own thread.

[dowoncha]

Attempting to add fields to the WebSocket enum variant throws an error as WebSocket does not implement Copy. Am i allowed to make WebSocketCommunicate copyable? I also noticed there is a WebSocketProtocol type, but in WebSocketConnectData, protocols is set to Vec<String> which is also not copyable.

[jdm]

WebSocketCommunicate cannot be Copy, but it should be ok to remove the Copy derive from RequestMode and use .clone() everywhere that's necessary.

[dowoncha]

Sorry for the constant questions, removing Copy derive still requires WebSocketCommunicate to derive
Clone, HeapSizeOf, and PartialEq.

[jdm]

That's fine!

[jdm]

More specifically, if HeapSizeOf causes problems you can use #[ignore_heap_size_of (look for other uses in the code), and if PartialEq causes problems you can remove PartialEq from RequestMode and modify the code that relies on it to use match or if let instead.

[dowoncha]

I am currently trying the modify the DOM implementation of WebSocket.rs to send a fetch to the core resource thread rather than the removed WebsocketCommunicate enum variant. The resource fetch requires a sender: IpcSender<FetchResponseMsg>, could you explain the which one to send for sender? I am storing the connect, connect_data in the RequestMode in init.

[jdm]

Grumble. Ok, let's modify the Fetch message to contain a FetchListener enum as the second argument, which contains two variants - a NonWebSocket one which contains the IpcSender<FetchResponseMsg>, and a WebSocket one which doesn't. I think that's as close as we can get right now until we look at other ways we can use the Rust websocket library we rely on.

[KiChjang]

Taking a lot at this. Gotta love removing code!

[KiChjang]

Instead of trying to shoehorn WebSocketConnect into the RequestMode enum, I would like to propose adding a new enum called FetchCommunicationChannel which would look like

enum FetchCommunicationChannel {
    Fetch(IpcSender<FetchResponseMsg>),
    WebSocket {
        event_sender: IpcSender<WebSocketNetworkEvent>,
        action_receiver: IpcReceiver<WebSocketDomAction>
    }
}

This enum will be sent as part of the arguments for CoreResourceMsg::Fetch:

enum CoreResourceMsg {
    Fetch(RequestInit, FetchCommunicationChannels),
    // other variants...
}

That way we wouldn't run into issues when trying to derive Clone, Copy, Send or Sync for RequestMode, and we would not need to create an IpcSender<FetchResponseMsg> for WebSocket fetches when we don't need to use one.

[KiChjang]

I believe it's best to wait until #19825 is resolved first before we integrate any further, since hyper seems to have plans for handling WebSocket upgrades as well.

[jdm]

When this work eventually happens, we'll want to be sure that it includes #19266.

[KiChjang]

Given #21386, I'm not sure if this work is necessary anymore.

[KiChjang]

If I'm not mistaken, the ws crate seems to have its own implementation of the fetch stack that is opaque to us.

[jdm]

Yeah, I don't see any way to have actual integration with the fetch code. We'll need to duplicate the relevant changes in the code that prepares the HTTP request, unfortunately.

[Eijebong]

It might be possible:

https://docs.rs/ws/0.7.8/ws/trait.Factory.html
https://docs.rs/ws/0.7.8/src/ws/lib.rs.html#283

[KiChjang]

@Eijebong I still think this is impossible, even when we implement our own Factory type. What was your plan to integrate Factory with the rest of the fetch infrastructure?