1125292 - Support Tunnel-Protocol for WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Martin Thomson [:mt:]]

When using HTTP CONNECT, we should implement the spec.

http://httpwg.github.io/http-extensions/tunnel-protocol.html

Comment 1

[Maire Reavy [:mreavy]]

Is this important?

Comment 2

[Eric Rescorla (:ekr)]

Well, it's not relevant until we support HTTP CONNECT.

Comment 3

[Martin Thomson [:mt:]]

We support connect, at least that is what I understand this code to be doing, at least in part:
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp#240-245

To answer Maire's question, as it stands, no one expects this header field.  But this is going to the RFC editor, and WebRTC is starting to be deployed into enterprise environments more.  Expect to see some demand.  Note that this is pretty trivial to add.

Comment 4

[Martin Thomson [:mt:]]

Attachment: MozReview Request: Bug 1125292 - Sending ALPN header field for WebRTC calls, r?bwc

Bug 1125292 - Sending ALPN header field for WebRTC calls, r?bwc

Comment 5

[Martin Thomson [:mt:]]

Comment on attachment 8653120 [details]
MozReview Request: Bug 1125292 - Sending ALPN header field for WebRTC calls, r?bwc

Bug 1125292 - Sending ALPN header field for WebRTC calls, r?bwc

Comment 6

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/17371/#review15561

::: media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:133
(Diff revision 1)
> +  strncpy(mesg + offset, END_HEADERS, sizeof(END_HEADERS));
> +  offset += sizeof(END_HEADERS) - 1;

I think I would use strlen and memcpy here, to avoid the off-by-one business.

::: media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:501
(Diff revision 1)
> +  if (alpn && strlen(alpn) > MAX_ALPN_LENGTH) {

Add some parens here.

::: media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:512
(Diff revision 1)
> +    char * alpndup = r_strdup(alpn);

Nit: char \*alpndup

Comment 7

[Byron Campen [:bwc]]

Comment on attachment 8653120 [details]
MozReview Request: Bug 1125292 - Sending ALPN header field for WebRTC calls, r?bwc

https://reviewboard.mozilla.org/r/17373/#review15525

::: media/mtransport/nricectx.h:181
(Diff revision 1)
>    NrIceProxyServer(const std::string& host, uint16_t port) :
> -    host_(host), port_(port) {
> +    host_(host), port_(port), alpn_() {
> +  }
> +
> +  NrIceProxyServer(const std::string& host, uint16_t port,
> +                   const std::string& alpn) :
> +    host_(host), port_(port), alpn_(alpn) {
>    }

It seems like these two could be rolled into one without any confusion.

Comment 8

[Martin Thomson [:mt:]]

Comment on attachment 8653120 [details]
MozReview Request: Bug 1125292 - Sending ALPN header field for WebRTC calls, r?bwc

Bug 1125292 - Sending ALPN header field for WebRTC calls, r?bwc

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8653120 [details]
MozReview Request: Bug 1125292 - Sending ALPN header field for WebRTC calls, r?bwc

https://reviewboard.mozilla.org/r/17373/#review16913

::: media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:133
(Diff revisions 1 - 2)
> -  strncpy(mesg + offset, END_HEADERS, sizeof(END_HEADERS));
> +  memcpy(mesg + offset, END_HEADERS, sizeof(END_HEADERS));

This would read a little better if we used strlen(END_HEADERS) here too.

Comment 10

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/17373/#review15525

> It seems like these two could be rolled into one without any confusion.

Ooh, check it out, our buildconfig now supports delegated c'tors:

https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ee49f580513

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5ee49f580513