Implement websockets over http/2

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P2
normal
RESOLVED FIXED
a year ago
2 months ago

People

(Reporter: u408661, Assigned: u408661)

Tracking

(Depends on 1 bug, {dev-doc-needed})

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(4 attachments)

Assignee

Description

a year ago
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.
Assignee

Comment 1

8 months ago
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.
Assignee

Comment 2

8 months ago
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!
Assignee

Comment 3

8 months ago
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)
Assignee

Comment 5

7 months ago
Posted file 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.)
Attachment #9019099 - Flags: feedback?(dd.mozilla)
Assignee

Comment 6

7 months ago
(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?
Assignee

Comment 7

7 months ago
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)
Assignee

Comment 9

7 months ago
(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+

Comment 12

7 months ago
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31c304e56e00
Implement websockets over http/2 - RFC 8441 r=michal,dragana

Comment 13

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/31c304e56e00
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Depends on: 1505388
Depends on: 1505834
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
You need to log in before you can comment on or make changes to this bug.