Closed Bug 1125292 Opened 5 years ago Closed 4 years ago

Support Tunnel-Protocol for WebRTC

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
Blocking Flags:
backlog parking-lot

People

(Reporter: mt, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

When using HTTP CONNECT, we should implement the spec.

http://httpwg.github.io/http-extensions/tunnel-protocol.html
Is this important?
Blocks: webrtc_spec
backlog: --- → parking-lot
Flags: needinfo?(martin.thomson)
Well, it's not relevant until we support HTTP CONNECT.
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.
Flags: needinfo?(martin.thomson)
Bug 1125292 - Sending ALPN header field for WebRTC calls, r?bwc
Attachment #8653120 - Flags: review?(docfaraday)
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
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 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.
Attachment #8653120 - Flags: review?(docfaraday)
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
Attachment #8653120 - Flags: review?(docfaraday)
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.
Attachment #8653120 - Flags: review?(docfaraday) → review+
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
https://hg.mozilla.org/mozilla-central/rev/5ee49f580513
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.