Implement websocket subprotocol negotiation

[jdm]

Spec: https://html.spec.whatwg.org/multipage/comms.html#the-websocket-interface:dom-websocket-protocol and https://tools.ietf.org/html/rfc6455#section-1.9
Tests: ./mach test-wpt tests/wpt/web-platform-tests/websockets/

There are a bunch of tests that fail because the websocket.protocol attribute is missing from WebSocket.webidl. There are also a number of tests that fail because they rely on subprotocols, which we don't support. We should implement the missing piece from the "When the WebSocket connection is established" (see ConnectionEstablishedTask::handler). Additionally, we should set the appropriate header on the request object (see establish_a_websocket_connection and the appropriate header).

Code: components/script/dom/websocket.rs

Note: this issue is already assigned to someone, so please talk with me before starting work on it :)

[anoukruhaak]

Thanks for creating this issue! I hope to make a PR tomorrow :)

[anoukruhaak]

@jdm unfortunately I got stuck after only writing two lines of code. I added

 protocols: Vec<DOMString>,

here:

struct ConnectionEstablishedTask {
    addr: Trusted<WebSocket>,
    sender: Arc<Mutex<Sender<WebSocketStream>>>,
    protocols: Vec<DOMString>,
}

And then add protocols as an argument:

 let open_task = box ConnectionEstablishedTask {
                addr: address.clone(),
                sender: ws_sender.clone(),
                protocols: protocols.iter().cloned().collect(),
            };

When I do this I get an error:

211:48 error: `protocols` does not live long enough

I tried a bunch of things to convert the Slice of DOMStrings to a Vec (the version above just being the most recent) and it appears the compiler thinks the protocols provided in the construtor (below) are not living long enough to be passed as an argument.

 pub fn Constructor(global: GlobalRef,
                       url: DOMString,
                       protocols: Option<DOMString>)
                       -> Fallible<Root<WebSocket>> {

Any suggestions?

[anoukruhaak]

ps. All of this is in the components/script/dom/websocket.rs file!

[jdm]

Ah, I see the issue. We need to create the Vec<DOMString> value outside of the spawn_named call, which creates a new thread. This will allow us to move the Vec value into the new thread, so there won't be lifetime problems. Does that make sense?

[anoukruhaak]

I think so, yes! Thank you!

[jdm]

@jmr0 is going to work on this.

[jmr0]

Hey @jdm, I'm going over this now and wanted to make sure I captured the flow right:

• establish_a_websocket_connection should set the protocol header on the request object
• ConnectionEstablishedTask::handler will check the "protocol in use" response from the server's handshake (that we would've stored from establish_a_websocket_connection?) and verify against the client-provided list, then assign that to the protocol field of the WebSocket object if it's a match

Thanks!

[jdm]

Yes, that looks correct to me!

[jmr0]

I ended up breaking the Create-valid-url-array-protocols and Create-Secure-valid-url-array-protocols tests by adding protocol support but don't see an easy way of fixing them, since we don't seem to support IDL array arguments.

Protocol arrays are currently passed as a comma-separated string, so it wasn't immediately obvious how one would differentiate between a proper comma-separated list of protocols or some bogus protocol that happened to have a comma as part of its name. Let me know if you have any thoughts on this (hopefully that all makes sense).

And feel free to take a look: jmr0@271df3f

Thanks again :)

[jdm]

Oh yeah, #544 :( I'm ok with breaking those tests, and I don't think it's worth implementing pseudo-support for the protocols by splitting on "," or anything like that right now. When we fix #544 then the tests should start passing again. That commit looks like it's on the right track; nice work!

[frewsxcv]

#544 has been solved

[jdm]

#8825 merged, so we can open more specific issues for remaining pieces of this.