1434137 - Implement websockets over http/2

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[u408661]

Spec at https://tools.ietf.org/id/draft-mcmanus-httpbis-h2-websockets-02.html

The vast majority of this work will be in the http/2 code. There may be a touch of work needed in the websocket code to make it talk to an http/2 session instead of a socket.

Michal - I may need to rope you in for at least a walkthrough of the websockets code, but I'll cross that bridge if/when I come to it. For now, I'm just cc'ing you so you're aware that I'm taking on the bulk of this work.

Comment 1

[u408661]

Attachment: Bug 1434137 - Implement websockets over http/2 - RFC 8441 r?michal!,dragana!

https://tools.ietf.org/html/rfc8441

This uses our existing http/2 CONNECT infrastructure (modified) to
enable the new extended CONNECT form defined by 8441, and pretend for
the websocket's sake that an http/2 stream is actually a socket. From
the websocket's point of view, this is relatively non-invasive - a few
things have changed (http response code, absence of some headers) versus
http/1.1 websockets, but for the most part, the websocket code doesn't
care.

Comment 2

[u408661]

Dragana & Michal, just so it's explicit for you both, I'm looking to Dragana to review the bits under netwerk/protocol/http and Michal to review the bits under netwerk/protocol/websocket, given your relative experience levels with those two areas. You're both more than welcome to comment on things you see outside those areas, though (and please add someone else to review, if you'd like someone else to look over something).

Thanks!

Comment 3

[u408661]

Michal - I've done a try run with the added assertion mentioned in phabricator, and it seems to be hitting (this is http/1.1 websockets, so not an h2-specific problem): https://treeherder.mozilla.org/logviewer.html#?job_id=205114850&repo=try&lineNumber=2097

Is this a bug in websockets? Was there just something missed in our discussion over in phabricator?

Comment 4

[Michal Novotny [:michal]]

WebSocketChannel::Close calls WebSocketChannel::DoStopSession on the main thread because data hasn't started yet. mSocketIn should be null here, but in theory it could happen that it is called after WebSocketChannel::OnTransportAvailable and before WebSocketChannel::OnStartRequest, so data hasn't started yet but mSocketIn isn't null. Anyway, I think we shouldn't drain the socket in this case, or should we?

Comment 5

[u408661]

Attachment: moz.log.gz

Dragana, here's the log you requested. All the websocket requests are to libwebsockets.org over http/2. (Requesting feedback just to ensure it gets your attention.)

Comment 6

[u408661]

(In reply to Michal Novotny [:michal] from comment #4)
> WebSocketChannel::Close calls WebSocketChannel::DoStopSession on the main
> thread because data hasn't started yet. mSocketIn should be null here, but
> in theory it could happen that it is called after
> WebSocketChannel::OnTransportAvailable and before
> WebSocketChannel::OnStartRequest, so data hasn't started yet but mSocketIn
> isn't null. Anyway, I think we shouldn't drain the socket in this case, or
> should we?

OK, I guess I misunderstood your comment, which explains why my assertion was wrong :)

As for your theoretical case... I'm not certain that can ever happen (unless I'm mixing something up). WebSocketChannel::OnTransportAvailable is caused (through a bit of indirection) by nsHttpChannel::OnStopRequest, while WebSocketChannel::OnStartRequest should be caused by nsHttpChannel::OnStartRequest (or the same thing that causes the http channel's onstart). Since, IIRC, OnStart must happen before OnStop, I don't think things could ever happen in the order you propose above.

Given that, I think we should be able to MOZ_ASSERT(OnSocketThread() || !mSocketIn). What do you think?

Comment 7

[u408661]

See comment 6

Comment 8

[Michal Novotny [:michal]]

(In reply to Nicholas Hurley [:nwgh][:hurley] (he/him) from comment #6)
> As for your theoretical case... I'm not certain that can ever happen (unless
> I'm mixing something up). WebSocketChannel::OnTransportAvailable is caused
> (through a bit of indirection) by nsHttpChannel::OnStopRequest, while
> WebSocketChannel::OnStartRequest should be caused by
> nsHttpChannel::OnStartRequest (or the same thing that causes the http
> channel's onstart). Since, IIRC, OnStart must happen before OnStop, I don't
> think things could ever happen in the order you propose above.

AFAICS, WebSocketChannel is written to handle situation when OnTransportAvailable is called before OnStartRequest. See members mGotUpgradeOK and mRecvdHttpUpgradeTransport and their usage in those methods. If this can really never happen, we could get rid of them...

> Given that, I think we should be able to MOZ_ASSERT(OnSocketThread() ||
> !mSocketIn). What do you think?

That sounds good.

Comment 9

[u408661]

(In reply to Michal Novotny [:michal] from comment #8)
> (In reply to Nicholas Hurley [:nwgh][:hurley] (he/him) from comment #6)
> > As for your theoretical case... I'm not certain that can ever happen (unless
> > I'm mixing something up). WebSocketChannel::OnTransportAvailable is caused
> > (through a bit of indirection) by nsHttpChannel::OnStopRequest, while
> > WebSocketChannel::OnStartRequest should be caused by
> > nsHttpChannel::OnStartRequest (or the same thing that causes the http
> > channel's onstart). Since, IIRC, OnStart must happen before OnStop, I don't
> > think things could ever happen in the order you propose above.
> 
> AFAICS, WebSocketChannel is written to handle situation when
> OnTransportAvailable is called before OnStartRequest. See members
> mGotUpgradeOK and mRecvdHttpUpgradeTransport and their usage in those
> methods. If this can really never happen, we could get rid of them...

Sounds good. I've filed bug 1501055 to track that work as a followup.

> > Given that, I think we should be able to MOZ_ASSERT(OnSocketThread() ||
> > !mSocketIn). What do you think?
> 
> That sounds good.

Alright. So far, so good with that assertion on my try run. Should have everything ready for round 2 of reviews later today.

Comment 10

[Michal Novotny [:michal]]

Attachment: don't drain input socket if data hasn't started

In case target thread isn't the same as main thread, it could still happen that mSocketIn is set while mDataStarted is false (between calls to WebSocketChannel::StartWebsocketData on main thread and on target thread). This is one possibility how to fix it.

Comment 11

[Michal Novotny [:michal]]

Attachment: set mDataStarted on main thread

Alternative fix to attachment 9019148 [details] [diff] [review].

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/31c304e56e00
Implement websockets over http/2 - RFC 8441 r=michal,dragana

Comment 13

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/31c304e56e00

Comment 14

[Chris Mills [:cmills]]

Note to MDN writers:

I've added a note ot the Fx65 rel notes to cover this:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#Networking

In terms of documentation, we'll need to investigate exactly what is needed here. We also need to document HTTP/2 at some point.

Comment 15

[Chris Mills [:cmills]]

I've removed the rel note about this from https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65 — looks like it was disabled by default for now; see https://bugzilla.mozilla.org/show_bug.cgi?id=1523978.

Comment 16

[virattara]

Hi, is this feature indefinitely stalled? I followed the thread here: https://bugzilla.mozilla.org/show_bug.cgi?id=1523427&GoAheadAndLogIn=1
where the problem seems to be connections over a proxy.
I was trying to get an update on the implementation of RFC 8441 on major browsers. Can someone please share the reason if any for not proceeding with this feature, if so.

Comment 17

[Dragana Damjanovic [:dragana]]

(In reply to virattara from comment #16)

Hi, is this feature indefinitely stalled? I followed the thread here: https://bugzilla.mozilla.org/show_bug.cgi?id=1523427&GoAheadAndLogIn=1
where the problem seems to be connections over a proxy.
I was trying to get an update on the implementation of RFC 8441 on major browsers. Can someone please share the reason if any for not proceeding with this feature, if so.

No it is not stalled. (Bug 1523427 has been fixed).
The feature is working, there are some problems with proxies(there is a work around for that problem). The proper fix for proxies needs a big refactor that is plan for the near future.

Comment 18

[virattara]

Oh good to know! Servers behind a proxy seem to be a problem here. If this is true then I guess it could be resolved by enabling sticky sessions on the server side, still not sure how reverse proxies like Nginx will work that do not support h2 in their proxy module (https://trac.nginx.org/nginx/ticket/923). Since server side setting is out of your scope, how do you plan to overcome this hurdle?