Closed Bug 1434137 Opened 7 years ago Closed 6 years ago

Implement websockets over http/2

Categories

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

Product:
Core
Component:
Networking: HTTP
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: u408661, Assigned: u408661)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed, Whiteboard: [necko-triaged])

Attachments

(4 files)

Bug 1434137 - Implement websockets over http/2 - RFC 8441 r?michal!,dragana!
(deleted), text/x-phabricator-request
Details
moz.log.gz
(deleted), application/x-gzip
dragana
: feedback+
Details
don't drain input socket if data hasn't started
(deleted), patch
Details | Diff | Splinter Review
set mDataStarted on main thread
(deleted), patch
Details | Diff | Splinter Review
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.
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.
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!
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?
Flags: needinfo?(michal.novotny)
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?
Flags: needinfo?(michal.novotny)
Attached file moz.log.gz (deleted) —
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.)
Attachment #9019099 - Flags: feedback?(dd.mozilla)
(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?
See comment 6
Flags: needinfo?(michal.novotny)
(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.
Flags: needinfo?(michal.novotny)
(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.
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.
Attachment #9019099 - Flags: feedback?(dd.mozilla) → feedback+
Pushed by hurley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/31c304e56e00 Implement websockets over http/2 - RFC 8441 r=michal,dragana
https://hg.mozilla.org/mozilla-central/rev/31c304e56e00
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1505388
Depends on: 1514688
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.
Depends on: 1515459
No longer depends on: 1515459
Depends on: 1515459
Depends on: 1523427

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.

Depends on: 1536787

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.

(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.

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?

Flags: needinfo?(dd.mozilla)
Flags: needinfo?(dd.mozilla)
Regressions: 1727995
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: