Use HTTP proxy for WebRTC ICE-TCP, TURN/TCP, TURN/TLS

RESOLVED FIXED in mozilla38

Status

()

defect
P2
normal
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: bryandonnovan, Assigned: bwc)

Tracking

(Blocks 1 bug)

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 31 obsolete attachments)

5.24 KB, patch
Details | Diff | Splinter Review
66.67 KB, patch
bwc
: review+
Details | Diff | Splinter Review
16.01 KB, patch
bwc
: review+
Details | Diff | Splinter Review
39 bytes, text/x-review-board-request
Details
39 bytes, text/x-review-board-request
Details
Reporter

Description

6 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.63 Safari/537.36

Steps to reproduce:

This is an enhancement request to use a proxy server, if configured, to establish TCP connections for WebRTC's ICE-TCP, TURN/TCP and TURN/TLS.

In some network environments, outbound TCP connections are only possible using a proxy server, and it seems reasonable that if Firefox can load pages and establish webSocket connections via proxy, then it could also establish WebRTC connections using an HTTP proxy's CONNECT method.

At the time of submission, only TURN/TCP is implemented (issue 906968) and open issue 891551 exists for ICE-TCP.
Reporter

Updated

6 years ago
Component: Untriaged → WebRTC: Networking
OS: Mac OS X → All
Product: Firefox → Core
Hardware: x86 → All

Updated

5 years ago
Blocks: 970426
Status: UNCONFIRMED → NEW
Ever confirmed: true
No longer blocks: 970426

Comment 1

5 years ago
Nils is checking with Ekr if he's talked with Justin on what C is doing.  Randell checking with network guys on if there's info on % of folks getting network settings through proxy.  can we estimate for Turn/TCP and determine based on that priority?  depending on effort would impact timing.
The answer from C is:

We support ICE-TCP, TURN/TCP and TURN/TLS, either direct or through SOCKS5/HTTPS.
We don't tunnel any media in HTTP/HTTPS (yet)
Some data:

[14:47]	jesup		repeat for mcmanus: Does anyone have any idea what percentage of users are behind networks needing TCP proxy access with UDP blocked? Trying to figure out the priority of proxy support for TURN/TCP etc for webrtc
[14:48]	jesup		Or even percent needing/using HTTP proxies?
[14:48]	mayhemer	jesup: as I remember our research from allpeers (old and not perfect :)) most users are allowed UDP
[14:49]	mcmanus		jesup we have proxy numbers, but they are lowballs because most proxies are transparent to us
[14:49]	mayhemer	jesup: according ability to only go out via a proxy (you probably want to ssl tunnel, right?) that was more then expected, I might remember some 15%...
[14:49]	mayhemer	michal: don't you rememer some more precise data here?
[14:53]	jesup		I believed there are a fair number of locked-down firewalls at corporations that only allow traffic via proxies. However, "fair number" is hard to quantify.
[14:57]	mayhemer	jesup: we had a central server that was listening on 443 that people behind proxies was able to use to tunnel out, and IIRC it was some 15% of our users (50000+)
[15:01]	mayhemer	jesup: one more info: it was in times we didn't have any UDP support (we never had...) so hard to say if those people were somehow able to tunnel out via UDP or not, anyway they were forbidden direct TCP, so direct UDP was disabled too IMO
[15:01]	jesup		mayhemer: "we" == who? 15% would be big
[15:02]	mayhemer	jesup: AllPeers
[15:02]	mayhemer	jesup: me, michal novotny and some other folks
[15:03]	mayhemer	jesup: I believe it was at least 10%, but I think more.
[15:03]	jesup		thanks
[15:08]	jesup		mayhemer: what sort of userbase would this be? (Don't know allpeers, sorry)
[15:08]	mayhemer	jesup: AllPeers was a personal content sharing P2P extension for firefox
[15:09]	mayhemer	jesup: http://en.wikipedia.org/wiki/AllPeers

Comment 4

5 years ago
believe Randell and Nils are investigating how often this is an issue.
Priority: -- → P2
Target Milestone: --- → mozilla33
(In reply to sescalante from comment #4)
> believe Randell and Nils are investigating how often this is an issue.

I think Randell and me are done. The high level summary is that at least 10% of the AllPeers extension were using proxies (not sure how that translates into % of all of our users), and Google Chrome has support for proxies.
So to me that sounds like this feature would have some impact.
I'd like to investigate the effort required and how much it would help (perhaps talking with the Chrome guys).
Assignee: nobody → docfaraday
The place to start would be by measuring what fraction of calls are supplied with TURN-TCP servers and don't manage to gather a full complement of relayed candidates. To a first approximation, that is the upper bound on people who will be helped by this. Initial measurements suggest that it's a relatively small fraction of the current failure rate, probably ~2% of the failures we are currently experiencing. Until the vast majority of failed calls get no public candidates or we have other strong evidence that this is the main problem, we should deprioritize this task
Agreed. We have bigger fish to fry wrt ICE failures right now.
Target Milestone: mozilla33 → mozilla35

Comment 9

5 years ago
ekr: in the last 2.5 weeks ~4% of all talky.io clients were able to connect on TCP/80 and unable to connect to udp/3478.

Unfortunately that is hard to correlate with the ice failures currently, implementing http://lists.w3.org/Archives/Public/public-webrtc/2014May/0115.html might be helpful for measuring this.

I doubt proxy support is used much in chrome, otherwise http://code.google.com/p/webrtc/issues/detail?id=3384 would have been found earlier.
I think that supporting HTTP/SOCKS proxies for ICE TCP or TURN TCP/TLS is a good idea.
There may be some TURN / ICE servers that can only be reached through a proxy (e.g. because they are on the TOR network).

Also, users will assume that everything TCP will go through the configured SOCKS proxy. If you end up in a scenario where HTTP/HTTPS goes through the proxy (and a TOR exit node) but TURN TCP goes directly then the user can be deanonymized very easily (especially when you can use the ICE credentials as a session key).
(In reply to Philipp Hancke from comment #9)
> ekr: in the last 2.5 weeks ~4% of all talky.io clients were able to connect
> on TCP/80 and unable to connect to udp/3478.

Are you serving TURN-TCP over 80?


> Unfortunately that is hard to correlate with the ice failures currently,
> implementing
> http://lists.w3.org/Archives/Public/public-webrtc/2014May/0115.html might be
> helpful for measuring this.
> 
> I doubt proxy support is used much in chrome, otherwise
> http://code.google.com/p/webrtc/issues/detail?id=3384 would have been found
> earlier.
Klaus,

We do intend to do this. it's just a priority issue. We'd be happy to receive a patch

Comment 13

5 years ago
Tokbox engineering is developing a patch. Hope to have something for review soon.

Comment 14

5 years ago
Looking at modifying NrSocket::connect() to insert proxy resolution for all TCP addresses, which would overwrite the destination address with the result of the HTTP proxy (if available).

Comment 15

5 years ago
@Rob Hainer, how about the status about this bug? It is important to network with http proxy.

Comment 16

5 years ago
Still working on a solution. We hope to have something to submit for review in a few days.

Comment 17

5 years ago
We're discussing with upstream a means of adding this change to the nICEr library, and when that is accepted and merged into central, we can land the changes to Firefox to use the functionality.

Comment 18

5 years ago
The work is split into 2 separate parts:
- nICEr changes
- Firefox integration

We're only submitting the nICEr to have it solidified first, but :ekr has asked that we review the first patch here. It's a first-pass, and there are some TODOs and unit tests are missing but it is functional and we would like to get some feedback so we can iterate.

- We added the concept of socket wrapper that sits between the nr_socket_buffered layer and the base nr_socket_local layer.  It is simple but limited (only TCP, only 1 wrapper and always in the same position in the stack).  Should we try something more complex?

- We added a wrapped socket called proxy_tunnel with a basic HTTP CONNECT implementation.  It could be rewritten by applications using a full HTTP stack (libcurl or Necko)

- We are not able to use NR_ASYNC_WAIT within the proxy_tunnel because the implementation in NrSocketBase::async_wait only allows one callback on NR_ASYNC_WAIT_WRITE on that fd (and nr_socket_buffered is already subscribed). We implemented a workaround but it's not ideal. Suggestions?

Comment 19

5 years ago
Posted patch nicer_connect_patch_v0.diff (obsolete) — Splinter Review
Please give feedback on the direction of nICEr portion of the patch.
Attachment #8532644 - Flags: feedback+

Comment 20

5 years ago
Posted patch nicer_connect_patch_v1.diff (obsolete) — Splinter Review
Builds on windows

Comment 21

5 years ago
Maire - Would be great to have the WebRTC team take a look at patch Ryan posted.
Flags: needinfo?(mreavy)
Comment on attachment 8532760 [details] [diff] [review]
nicer_connect_patch_v1.diff

Review of attachment 8532760 [details] [diff] [review]:
-----------------------------------------------------------------

Byron, Randell -- Can you provide feedback on this patch?  (The author wants to validate the approach.  Please also provide tips for getting it "ready to review" since the author is a first-time Firefox contributor.)  Thanks.
Attachment #8532760 - Flags: feedback?(rjesup)
Attachment #8532760 - Flags: feedback?(docfaraday)
I'll look at this as soon as I land bug 1091242 today. Need to get it landed before it rots again, and jib wants to checkin his promise work on a different day so we don't have two major rewrites landing at the same time.
Comment on attachment 8532760 [details] [diff] [review]
nicer_connect_patch_v1.diff

Review of attachment 8532760 [details] [diff] [review]:
-----------------------------------------------------------------

The general architecture looks on-target with what was discussed over mail, although it would be nice if we did not wait until the first write to send our HTTP CONNECT request. Not totally necessary though, since it is an optimization.

I would say it is necessary to have a test-case for this code. Given that the CONNECT logic is extremely simple, it probably would not be hard to cook up a test-case that verified the expected output (HTTP CONNECT, followed by some known data), and also verified that error responses were handled appropriately. This is the biggest part that is missing right now.

Obviously, we also have to wire in the new wrapper stuff too, based on settings, but that is probably not much code (don't know off the top of my head how to get the currently configured http proxy).

Once you are ready for code-review, I can give it a closer look.
Attachment #8532760 - Flags: feedback?(docfaraday) → feedback+

Comment 25

5 years ago
The reason we're waiting for the first write to send CONNECT is because the implementation of async_wait in NrSocketBase only allows waiting for one wait per fd per direction, and the stun layer is already waiting ahead of us. This can be fixed in the implementation, but we didn't want to broaden scope too much.
(In reply to ryan from comment #25)
> The reason we're waiting for the first write to send CONNECT is because the
> implementation of async_wait in NrSocketBase only allows waiting for one
> wait per fd per direction, and the stun layer is already waiting ahead of
> us. This can be fixed in the implementation, but we didn't want to broaden
> scope too much.

Yeah, this is what I figured.
I'll try to take a look at this this week.
Duplicate of this bug: 1056934
I guess we not only want a unit test of some kind for this, but some actual test setup, e.g. in the QA lab?!

Comment 30

5 years ago
@nils, We have a very simple setup in our QA Lab (squid proxy with default configuration) + we have asked at least one partner with proxy connectivity issues to test the patch.   Let us know if there is any way we can help with the testing.

Comment 31

5 years ago
Please consider complete patch with NrSocket integration and unittests
Attachment #8534680 - Flags: review+
Attachment #8534680 - Flags: review+ → review?(docfaraday)
Attachment #8532644 - Attachment is obsolete: true
Attachment #8532760 - Attachment is obsolete: true
Attachment #8532760 - Flags: feedback?(rjesup)
Comment on attachment 8534680 [details] [diff] [review]
proxy-tunnel-complete-patch-v2.diff

Review of attachment 8534680 [details] [diff] [review]:
-----------------------------------------------------------------

Sufficient architectural concerns to kick this back early, so you can start fixing without waiting for me to review the other files.

::: media/mtransport/nricectx.cpp
@@ +529,4 @@
>  
>  NrIceCtx::~NrIceCtx() {
>    MOZ_MTLOG(ML_DEBUG, "Destroying ICE ctx '" << name_ <<"'");
> +  nr_proxy_tunnel_destroy(&proxy_tunnel_);

We probably want to destroy this last, since it won't be holding onto references to any of this other stuff.

@@ +625,5 @@
>    return NS_OK;
>  }
>  
> +nsresult NrIceCtx::SetProxy(const char *host, uint16_t port) {
> +  int r;

Let's make sure to call this on STS.

@@ +635,5 @@
> +  // opentok
> +  nr_socket_wrapper *wrapper;
> +  nr_socket_wrapper_vtbl *wrapper_vtbl = new nr_socket_wrapper_vtbl();
> +  wrapper_vtbl->create = nr_proxy_tunnel_socket_create;
> +  nr_socket_wrapper_create_int(proxy_tunnel_, wrapper_vtbl, &wrapper);

Check return value here.

@@ +639,5 @@
> +  nr_socket_wrapper_create_int(proxy_tunnel_, wrapper_vtbl, &wrapper);
> +
> +  r = nr_ice_ctx_set_socket_wrapper(ctx_, wrapper);
> +  if (r) {
> +    MOZ_MTLOG(ML_ERROR, "Couldn't set proxy for '" << name_ << "'");

Please log the value of |r| as well.

@@ +650,5 @@
> +class ProtocolProxyQueryHandler :
> +  public nsIProtocolProxyCallback
> +{
> + public:
> +  ProtocolProxyQueryHandler(NrIceCtx *context) :

Single arg c'tors should generally be marked explicit.

@@ +654,5 @@
> +  ProtocolProxyQueryHandler(NrIceCtx *context) :
> +    context_(context) {
> +  }
> +
> +  NS_IMETHODIMP OnProxyAvailable(nsICancelable *aRequest, nsIURI *aURI, nsIProxyInfo *aProxyInfo, nsresult aStatus) {

Add 'MOZ_OVERRIDE' just before the opening '{'

@@ +676,5 @@
> +
> +  NS_DECL_ISUPPORTS
> +
> + private:
> +  NrIceCtx *context_;

Watch out; if PeerConnectionMedia is destroyed while this is pending, we'll UAF.

@@ +686,2 @@
>  nsresult NrIceCtx::StartGathering() {
> +  MOZ_ASSERT(NS_IsMainThread());

We don't want to be doing main thread stuff here. Additionally, since we're adding another async step that could complete after the PeerConnection goes away, we need to be very careful about our memory management.

I would suggest having all of this be done in PeerConnectionMedia, and using the nsICancelable arg on AsyncResolve. If the PeerConnection is destroyed, this will ensure that the callback does not pop after PeerConnectionMedia is freed. There's an example of this pattern in nsHttpChannel:

Setting up the cancelable |mProxyRequest|: http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2047

Canceling |mProxyRequest|: http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#4670
http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2047)

@@ +689,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsIProtocolProxyService> pps =
> +    do_GetService(NS_PROTOCOLPROXYSERVICE_CONTRACTID, &rv);
> +  if (NS_FAILED(rv)) {
> +    MOZ_MTLOG(ML_ERROR, "Failed to get proxy service");

Please log |rv|

@@ +694,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIURI> given;
> +  if (NS_FAILED(rv = NS_NewURI(getter_AddRefs(given), "https://example.com"))) {

So we're asking the proxy service what proxy to use to reach https://example.com here? Maybe rename |given| to something like |fakeDestination| to make this clearer?

@@ +695,5 @@
> +  }
> +
> +  nsCOMPtr<nsIURI> given;
> +  if (NS_FAILED(rv = NS_NewURI(getter_AddRefs(given), "https://example.com"))) {
> +    MOZ_MTLOG(ML_ERROR, "Failed to set URI");

Please log |rv|

@@ +705,5 @@
> +  if (NS_FAILED(rv = pps->AsyncResolve(given,
> +      nsIProtocolProxyService::RESOLVE_PREFER_HTTPS_PROXY |
> +      nsIProtocolProxyService::RESOLVE_ALWAYS_TUNNEL,
> +      handler, getter_AddRefs(request)))) {
> +    MOZ_MTLOG(ML_ERROR, "Failed to resolve protocol proxy");

Please log |rv|
Attachment #8534680 - Flags: review?(docfaraday) → review-

Comment 33

5 years ago

Comment 34

5 years ago
I believe I've address all raised issues
Comment on attachment 8535946 [details] [diff] [review]
proxy-tunnel-complete-patch-v3.diff

Review of attachment 8535946 [details] [diff] [review]:
-----------------------------------------------------------------

I'll give this a look first thing Monday.
Attachment #8535946 - Flags: review?(docfaraday)
This patch does not apply cleanly, and wiggle can't figure out how to apply it either. I'll try by hand, but you need to update your working copy.
Flags: needinfo?(ryan)
Ok, I see the cause of the conflicts, and should provide some guidance on how to resolve them. Since stuff like renegotiation support is coming up, we now have an EnsureIceGathering_s() function that could be called multiple times.

http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp?from=EnsureIceGathering_s#462

With respect to setting up this proxy stuff, we are already going to be forced into keeping some state (either to ensure we don't do this more than once, or in the case where we do it exactly once when initting the PCMedia, keeping track of whether it has finished). So I recommend doing this when PCMedia is constructed, and caching the result. When it comes time to start gathering, we check if we have the result in yet, and if not we set a flag that tells us to start gathering once the result is in.

I'll go ahead and start reviewing the rest of the code, since we're now pretty close to what we want architecturally.
Comment on attachment 8535946 [details] [diff] [review]
proxy-tunnel-complete-patch-v3.diff

Review of attachment 8535946 [details] [diff] [review]:
-----------------------------------------------------------------

No architecture concerns other than those already raised in comment 37.

Once you get through the review comments, I can push to try on your behalf.

::: media/mtransport/test/proxy_tunnel_socket_unittest.cpp
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: ekr@rtfm.com

Give yourself some credit here.

@@ +45,5 @@
> +const std::string kHelloMessage = "HELLO";
> +const std::string kConnectMessage = "CONNECT %s:%d HTTP/1.0\r\n\r\n";
> +const std::string kConnectHelloMessage = kConnectMessage + kHelloMessage;
> +const std::string kConnectResponseMessage = "HTTP/1.0 %d\r\n\r\n";
> +const std::string kConnectResponseHelloMessage = kConnectResponseMessage + kHelloMessage;

Hmm, this makes it kinda hard for either a human or a compiler to sanity-check that the format strings are being used correctly/safely. Maybe what we want instead is a function that builds these strings?

@@ +49,5 @@
> +const std::string kConnectResponseHelloMessage = kConnectResponseMessage + kHelloMessage;
> +
> +DataBuffer *merge(DataBuffer *a, DataBuffer *b) {
> +  ScopedDeletePtr<DataBuffer> aptr(a);
> +  ScopedDeletePtr<DataBuffer> bptr(b);

This ownership behavior is surprising given the function signature. It looks like nsAutoPtr/UniquePtr is the right thing to use here for the params/return. You could use nsRefPtr too, I suppose. Also, I would mark this function static, but it is in a unit-test so not a huge deal.

@@ +68,5 @@
> +
> +  if (b && b->len()) {
> +    return bptr.forget();
> +  } 
> +  

Some trailing whitespace scattered around these patches; they are highlighted in red when you [review] the patch.

@@ +73,5 @@
> +  return nullptr;
> +}
> +
> +class DummySocket : public NrSocketBase {
> + public:

This seems to be a copy/paste with some fixes (which look good to me) from another unit-test. Probably makes sense to move it to its own header file.

@@ +328,5 @@
> +
> + protected:
> +  nsRefPtr<DummySocket> dummy_;
> +  DummyResolver fake_;
> +  nr_socket *socket_;

The naming of these three could be better.

@@ +344,5 @@
> +  int r = nr_socket_connect(socket_, &remote_addr_);
> +  ASSERT_EQ(0, r);
> +
> +  ASSERT_EQ(nr_transport_addr_cmp(dummy_->get_connect_addr(), &proxy_addr_, 
> +        NR_TRANSPORT_ADDR_CMP_MODE_ALL), 0);

These macros work best when the expected value is the first arg.

@@ +354,5 @@
> +  ASSERT_EQ(0, r);
> +
> +  size_t written = 0;
> +  r = nr_socket_write(socket_, kHelloMessage.c_str(), kHelloMessage.size(), &written, 0);
> +  ASSERT_EQ(r, 0);

Switch arg order here. Same in a bunch of other places.

@@ +417,5 @@
> +  ASSERT_EQ(0, r);
> +
> +  char buf[4096];
> +  int len = sprintf(buf, kConnectResponseHelloMessage.c_str(), 200);
> +  dummy_->SetReadBuffer(reinterpret_cast<uint8_t *>(buf), len);

Since we've already done this above, maybe we should test the case where just the connect response (no HELLO yet) comes in here? I'm guessing trying to read should result in R_WOULDBLOCK, right?

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +310,4 @@
>        for(j=0;j<ctx->turn_server_ct;j++){
>          nr_transport_addr addr;
>          nr_socket *sock;
> +        nr_socket *wrappered_sock;

Hmm. Now we have three different nr_socket* floating around being passed to various init API. Let's just overwrite |sock| with the wrapper-generated sock if we end up using it (the only other place I see |sock| being used is in |nr_stun_server_ctx_create|, which calls getaddr, which we safely pass through to the inner socket).

@@ +330,5 @@
>          }
> +
> +        nr_socket_wrapper *wrapper = ctx->socket_wrapper;
> +
> +        r_log(LOG_ICE,LOG_DEBUG,"nr_ice_component_initialize_tcp create %p", wrapper);

We should probably have this read something like "nr_ice_component_initialize_tcp (wrapper = %p)", to make it clear what that pointer actually refers to.

::: media/mtransport/third_party/nICEr/src/ice/ice_ctx.h
@@ +133,4 @@
>  
>    nr_resolver *resolver;                      /* The resolver to use */
>    nr_interface_prioritizer *interface_prioritizer;  /* Priority decision logic */
> +  nr_socket_wrapper *socket_wrapper;          /* The socket wrapper to use */

I'd call this turn_tcp_socket_wrapper, since that is the only place it is used.

@@ +175,4 @@
>  int nr_ice_ctx_set_turn_servers(nr_ice_ctx *ctx,nr_ice_turn_server *servers, int ct);
>  int nr_ice_ctx_set_resolver(nr_ice_ctx *ctx, nr_resolver *resolver);
>  int nr_ice_ctx_set_interface_prioritizer(nr_ice_ctx *ctx, nr_interface_prioritizer *prioritizer);
> +int nr_ice_ctx_set_socket_wrapper(nr_ice_ctx *ctx, nr_socket_wrapper *wrapper);

Similar naming concern here.

::: media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c
@@ +1,1 @@
> +/*

Copy some modelines in here.

@@ +39,5 @@
> +
> +#include "nr_proxy_tunnel.h"
> +
> +#define MAX_ADDRESS_SIZE 256  // TODO
> +#define MAX_BUFFER_SIZE 1024  // TODO

These values look sane I suppose, so the TODOs are probably not needed. Although maybe we should name it MAX_HTTP_CONNECT_RESPONSE_SIZE.

@@ +80,5 @@
> +static int send_http_connect(nr_proxy_tunnel_socket *sock)
> +{
> +  r_log(LOG_GENERIC,LOG_DEBUG,"send_http_connect %p", sock);
> +
> +  int r;

Variable decls need to go at the top, or the windows builds will break (this is for .c files only). Same thing in several other places.

@@ +84,5 @@
> +  int r;
> +  int port;
> +  char connect_addr[MAX_ADDRESS_SIZE];
> +  nr_transport_addr_get_port(&sock->remote_addr, &port);
> +  nr_transport_addr_get_addrstring(&sock->remote_addr, connect_addr, sizeof(connect_addr));

We probably should check that we didn't truncate here.

@@ +90,5 @@
> +  char http_connect[MAX_ADDRESS_SIZE + 64];
> +  snprintf(http_connect, sizeof(http_connect), "CONNECT %s:%d HTTP/1.0%s", connect_addr, port, ENDLN);
> +
> +  size_t bytes_sent;
> +  r = nr_socket_write(sock->inner, http_connect, strlen(http_connect), &bytes_sent, 0);

We need to check for errors here, and return appropriately.

@@ +93,5 @@
> +  size_t bytes_sent;
> +  r = nr_socket_write(sock->inner, http_connect, strlen(http_connect), &bytes_sent, 0);
> +
> +  if (bytes_sent < strlen(http_connect)) {
> +    // TODO: buffering and wait for

Isn't sock->inner going to be buffering for us (this is a nr_socket_buffered_stun, right)?

@@ +94,5 @@
> +  r = nr_socket_write(sock->inner, http_connect, strlen(http_connect), &bytes_sent, 0);
> +
> +  if (bytes_sent < strlen(http_connect)) {
> +    // TODO: buffering and wait for
> +    r_log(LOG_GENERIC,LOG_DEBUG,"send_http_connect should be buffering %zu", bytes_sent);

Watch out; MSVC does not support %z (yes, _really_: http://msdn.microsoft.com/en-us/library/tcxf1dw6.aspx)

@@ +105,5 @@
> +
> +static int parse_response(char* response, size_t len, unsigned int *http_status)
> +{
> +  char data[MAX_BUFFER_SIZE + 1];
> +  memcpy(data, response, len);

We need some bounds-checking here. We could do that and also find the '\n' with a memchr call.

@@ +111,5 @@
> +
> +  char *token = NULL;
> +  token = strtok(data, "\n");
> +  if (token == NULL) {
> +    r_log(LOG_GENERIC,LOG_DEBUG,"parse_response end of line not found ");

We assume that the caller has already found \r\n\r\n, right? If so, this needs to be an error log, and we should probably make the log line explicitly say that this is the caller's bug.

@@ +115,5 @@
> +    r_log(LOG_GENERIC,LOG_DEBUG,"parse_response end of line not found ");
> +    return R_BAD_DATA;
> +  }
> +
> +  if (sscanf(token, "HTTP/%*u.%*u %u", http_status) != 1) {

Seems like token will be pointing at some http (including an '\n'), a bunch of uninitialized data, and then finally the '\0' we add above. The only thing that saves us is verifying that there is a '\n' present, which will stop the sscanf, because it isn't in the format string. However, the moment some unfortunate programmer decides to add a %s to catch the http reason, bad things will happen. Here's what I recommend:

Find '\r' in |response| with memchr.
Copy just the stuff before the '\r' into a temp buffer, and null-terminate.
Then pass the temp buffer to sscanf.

@@ +116,5 @@
> +    return R_BAD_DATA;
> +  }
> +
> +  if (sscanf(token, "HTTP/%*u.%*u %u", http_status) != 1) {
> +    r_log(LOG_GENERIC,LOG_DEBUG,"parse_response sscanf failed %s", token);

I'd say this should be a WARNING.

The rubric that I've been trying to use for log-levels in the nICEr codebase is as follows:

ERROR: Anything that is a bug in our code, or dooms ICE to failure.
WARNING: Stuff that will degrade ICE, but not necessarily completely doom it, as long as it isn't a bug in our code (see above).
NOTICE: Stuff that is anomalous, but not particularly harmful.
INFO: Generally important events in ICE overall (eg; started sending checks).

@@ +143,5 @@
> +
> +  // /* Cancel waiting on the socket */
> +  // if (sock->inner && !nr_socket_getfd(sock->inner, &fd)) {
> +  //   NR_ASYNC_CANCEL(fd, NR_ASYNC_WAIT_WRITE);
> +  // }

Pretty sure we can remove this.

@@ +169,5 @@
> +
> +  return nr_socket_getaddr(sock->inner, addrp);
> +}
> +
> +static int socket_connect(nr_proxy_tunnel_socket *sock, nr_transport_addr *addr)

This function seems to boil down to nr_socket_connect(sock->inner, addr), let's just do that at the callsites.

@@ +190,5 @@
> +abort:
> +  return(_status);
> +}
> +
> +static int nr_proxy_tunnel_socket_resolved_cb(void *obj, nr_transport_addr *addr)

Maybe call this |proxy_addr| for readability?

@@ +201,5 @@
> +  // Mark the socket resolver as completed
> +  sock->resolver_handle = 0;
> +
> +  if (addr) {
> +    r_log(LOG_GENERIC,LOG_DEBUG,"Resolved candidate address %s", addr->as_string);

"Resolved http proxy %s -> %s" or similar.

@@ +204,5 @@
> +  if (addr) {
> +    r_log(LOG_GENERIC,LOG_DEBUG,"Resolved candidate address %s", addr->as_string);
> +  }
> +  else {
> +    r_log(LOG_GENERIC,LOG_WARNING,"Failed to resolve candidate.");

"Could not resolve http proxy %s" or similar

@@ +208,5 @@
> +    r_log(LOG_GENERIC,LOG_WARNING,"Failed to resolve candidate.");
> +    ABORT(R_NOT_FOUND);
> +  }
> +
> +  if ((r=socket_connect(sock, addr))) {

This is just nr_socket_connect(sock->inner, addr)

@@ +225,5 @@
> +  nr_proxy_tunnel *module = sock->module;
> +
> +  nr_transport_addr_copy(&sock->remote_addr, addr);
> +
> +  r_log(LOG_GENERIC,LOG_DEBUG,"nr_proxy_tunnel_socket_connect %p %p %s %d",

%d and uint16_t don't line up. Recommend using %u and casting |proxy_port| to an unsigned.

@@ +228,5 @@
> +
> +  r_log(LOG_GENERIC,LOG_DEBUG,"nr_proxy_tunnel_socket_connect %p %p %s %d",
> +        sock, module->resolver, module->proxy_host, module->proxy_port);
> +
> +  // Check if the proxy_host is already an IP address

c++ comment, please use /* */ in c code. Same in other places.

@@ +234,5 @@
> +  int has_addr = nr_ip4_str_port_to_transport_addr(module->proxy_host,
> +    module->proxy_port, IPPROTO_TCP, &proxy_addr) == 0;
> +
> +  if (!has_addr && !module->resolver) {
> +    ABORT(R_NOT_FOUND);

Log an error here, since this is misconfiguration.

@@ +237,5 @@
> +  if (!has_addr && !module->resolver) {
> +    ABORT(R_NOT_FOUND);
> +  }
> +
> +  // TODO: we could cache this DNS resolution

I doubt it would get us much of a performance boost, since it is no doubt being cached elsewhere.

@@ +238,5 @@
> +    ABORT(R_NOT_FOUND);
> +  }
> +
> +  // TODO: we could cache this DNS resolution
> +  if (!has_addr && module->proxy_host && module->proxy_port) {

There's no reason to check |proxy_host| down here, since we use it above. We should probably just sanity-check these earlier in this function.

@@ +242,5 @@
> +  if (!has_addr && module->proxy_host && module->proxy_port) {
> +      nr_resolver_resource resource;
> +      resource.domain_name=module->proxy_host;
> +      resource.port=module->proxy_port;
> +      //TODO: resource.stun_turn=protocol;

We probably want to hard-code this to TURN. The only time we would ever do TCP STUN is in preparation for ICE TCP, which we do not support yet, and would probably be impossible to do through a relay anyhow.

@@ +249,5 @@
> +      r_log(LOG_GENERIC,LOG_DEBUG,"nr_proxy_tunnel_socket_connect; nr_resolver_resolve");
> +      if ((r=nr_resolver_resolve(module->resolver, &resource,
> +          nr_proxy_tunnel_socket_resolved_cb, (void *)sock, &sock->resolver_handle))) {
> +
> +      r_log(LOG_GENERIC,LOG_DEBUG,"Could not invoke DNS resolver");

Indentation seems messed up here. Also, I'd say this is an ERROR, since the resolver flaked out on us.

@@ +256,5 @@
> +
> +    ABORT(R_WOULDBLOCK);
> +  }
> +
> +  _status=socket_connect(sock, &proxy_addr);

This is just nr_socket_connect(sock->inner, &proxy_addr);

@@ +274,5 @@
> +  } else {
> +    send_http_connect(sock);
> +
> +    return nr_socket_write(sock->inner, msg, len, written, 0);
> +  }

if (!sock->connect_requested) {
  send_http_request(sock);
}

return nr_socket_write(...);

@@ +291,5 @@
> +    return nr_socket_read(sock->inner, buf, maxlen, len, 0);
> +  }
> +
> +  if (sock->buffered_bytes >= sizeof(sock->buffer))
> +    ABORT(R_BAD_DATA);

This seems like a bug worthy of an assert, and some ERROR logging. Also, this is probably an R_INTERNAL error.

@@ +305,5 @@
> +  }
> +
> +  sock->buffered_bytes += bytes_read;
> +
> +  char *ptr = strstr(sock->buffer, ENDLN);

This data is not null-terminated, nor can it be.

@@ +313,5 @@
> +      ABORT(r);
> +    }
> +
> +    if (http_status != 200) {
> +      r_log(LOG_GENERIC,LOG_ERR,"nr_proxy_tunnel_socket_read unable to connect %u", http_status);

Let's allow any 2xx response:

http://tools.ietf.org/html/rfc2817#section-5.3

@@ +344,5 @@
> +
> +  // /* Cancel waiting on the socket */
> +  // if (sock->inner && !nr_socket_getfd(sock->inner, &fd)) {
> +  //   NR_ASYNC_CANCEL(fd, NR_ASYNC_WAIT_WRITE);
> +  // }

Maybe cancel our resolver here, just in case?

@@ +384,5 @@
> +
> +int nr_proxy_tunnel_set_proxy(nr_proxy_tunnel *proxy_tunnel, const char* host, UINT2 port) {
> +  r_log(LOG_GENERIC,LOG_DEBUG,"nr_proxy_tunnel_set_proxy %p %s %d", proxy_tunnel, host, port);
> +
> +  proxy_tunnel->proxy_host = r_strdup(host);

Maybe assert that this isn't already set?

@@ +424,5 @@
> +  _status=0;
> +abort:
> +  if (_status) {
> +    void *sock_v = sock;
> +    sock->inner = 0;  /* Give up ownership so we don't destroy */

This derefs null if we failed to alloc |sock|. Maybe set |inner| after all possible failures?

::: media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.h
@@ +1,1 @@
> +/*

Needs modelines.

::: media/mtransport/third_party/nICEr/src/net/nr_socket_wrapper.c
@@ +1,1 @@
> +/*

Needs modelines.

@@ +68,5 @@
> +
> +  wrapper = *wrapperp;
> +  *wrapperp = 0;
> +
> +  nr_socket_destroy((nr_socket **)&wrapper->obj);

I'm pretty sure wrapper->obj is an nr_proxy_tunnel, not an nr_socket of any type. This gets cleaned up in ~NrIceCtx() right now. I guess you could instead add a destroy function to the vtbl.

::: media/mtransport/third_party/nICEr/src/net/nr_socket_wrapper.h
@@ +1,1 @@
> +/*

Needs modelines.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +232,5 @@
> +  explicit ProtocolProxyQueryHandler(NrIceCtx *context) :
> +    context_(context) {
> +  }
> +
> +  NS_IMETHODIMP OnProxyAvailable(nsICancelable *aRequest, nsIURI *aURI, nsIProxyInfo *aProxyInfo, nsresult aStatus) MOZ_OVERRIDE {

Outside of third_party, we need to make sure to wrap lines to 80 columns. Basic code conventions can be found here:

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Naming_and_Formatting_code

@@ +233,5 @@
> +    context_(context) {
> +  }
> +
> +  NS_IMETHODIMP OnProxyAvailable(nsICancelable *aRequest, nsIURI *aURI, nsIProxyInfo *aProxyInfo, nsresult aStatus) MOZ_OVERRIDE {
> +    CSFLogInfo(logTag, "%s: Proxy Available: %d", __FUNCTION__, (uint32_t)aStatus);

Cast |aStatus| to an int, since that matches the format string.

@@ +348,5 @@
>  
> +  nsCOMPtr<nsIProtocolProxyService> pps =
> +    do_GetService(NS_PROTOCOLPROXYSERVICE_CONTRACTID, &rv);
> +  if (NS_FAILED(rv)) {
> +    CSFLogError(logTag, "%s: Failed to get proxy service: %d", __FUNCTION__, (uint32_t)rv);

Cast to int to match format string. Two other places in this function.

@@ +353,5 @@
> +    return rv;
> +  }
> +
> +  nsCOMPtr<nsIURI> fakeHttpsLocation;
> +  if (NS_FAILED(rv = NS_NewURI(getter_AddRefs(fakeHttpsLocation), "https://example.com"))) {

I find this less readable than assigning rv outside of the if statement.

@@ +359,5 @@
> +    return rv;
> +  }
> +
> +  nsRefPtr<ProtocolProxyQueryHandler> handler = new ProtocolProxyQueryHandler(mIceCtx);
> +  if (NS_FAILED(rv = pps->AsyncResolve(fakeHttpsLocation,

Same here.

@@ +474,5 @@
>    CSFLogDebug(logTag, "%s: ", __FUNCTION__);
>  
> +  nsresult status;
> +  if (mProxyRequest) 
> +    mProxyRequest->Cancel(status);

Place add braces (nICEr is third-party and gets away with it)
Attachment #8535946 - Flags: review?(docfaraday) → review-

Comment 39

5 years ago
Flags: needinfo?(ryan)
Attachment #8534680 - Attachment is obsolete: true
Comment on attachment 8536854 [details] [diff] [review]
proxy-tunnel-complete-patch-v4.diff

Review of attachment 8536854 [details] [diff] [review]:
-----------------------------------------------------------------

Feedback on new architecture, looks just about right, needs a race cleaned up.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +236,5 @@
>        mUuidGen(MakeUnique<PCUuidGenerator>()),
>        mMainThread(mParent->GetMainThread()),
> +      mSTSThread(mParent->GetSTSThread()), 
> +      mGatheringReady(false),
> +      mProxyResolveCompleted(false) {

Let's init mHttpsProxyPort too.

@@ +513,5 @@
>  
>  void
>  PeerConnectionMedia::EnsureIceGathering_s() {
> +  if (mIceCtx->gathering_state() == NrIceCtx::ICE_CTX_GATHER_INIT &&
> +      mGatheringReady && mProxyResolveCompleted) {

Checking |mGatheringReady| and |mProxyResolveCompleted| on STS is going to be racy, since they're set on main. I'd add a PeerConnectionMedia::GatherIfReady() that checks these, and then thunks out to EnsureIceGathering_s() if both are set.
Attachment #8536854 - Flags: feedback+

Comment 41

5 years ago
I believe I've answered all of your concerns (outside of two), however it's a long comment list so I may have missed something in error.

1) I haven't added modelines to nICEr code since they are not there in existing files.

2) I have not renamed socket_wrapper variables or functions to make them less generic, since the wrapper mechanism was designed to be generic (even if there's only one use now).

Let me know if you would like to insist on either of these points.
(In reply to ryan from comment #41)
> Created attachment 8538041 [details] [diff] [review]
> proxy-tunnel-complete-patch-v5.diff
> 
> I believe I've answered all of your concerns (outside of two), however it's
> a long comment list so I may have missed something in error.
> 
> 1) I haven't added modelines to nICEr code since they are not there in
> existing files.

  Ok, this is fine.

> 
> 2) I have not renamed socket_wrapper variables or functions to make them
> less generic, since the wrapper mechanism was designed to be generic (even
> if there's only one use now).
> 

   If our current usage is not generic, we should not choose names that suggest it is. If/when we apply this more generically, then yes we can change the naming (although to do this we would need to have |nr_socket_wrapper_create| take a candidate type and protocol).

Comment 43

5 years ago
Comment on attachment 8538116 [details] [diff] [review]
proxy-tunnel-complete-patch-v6.diff

Review of attachment 8538116 [details] [diff] [review]:
-----------------------------------------------------------------

Since reviewboard displays interdiff far better (and allows you to review an interdiff at all), I did my re-review over there:

https://reviewboard.mozilla.org/r/1489/#issue-summary
Attachment #8538116 - Flags: review-

Comment 46

5 years ago
Brown paper bag not included.
Some issues still remain. If you want to push back on any of them, feel free, there is supposed to be some give-and-take here. The credentials for reviewboard are the same as your bugzilla credentials.

https://reviewboard.mozilla.org/r/1489/#issue-summary

Comment 48

5 years ago
This does not include factoring DummySocket or DummyResolver out -- although worth while goal, I don't believe it should hold back this patch.
Comment on attachment 8538730 [details] [diff] [review]
proxy-tunnel-complete-patch-v8.diff

Review of attachment 8538730 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I can factor DummySocket out in a subsequent patch. We're still not quite out of the woods yet, just a couple of easy bugfixes are left.

https://reviewboard.mozilla.org/r/1489/#issue-summary
Attachment #8538730 - Flags: review-

Comment 50

5 years ago
Thank you for your patience in reviewing this patch
Comment on attachment 8538881 [details] [diff] [review]
proxy-tunnel-complete-patch-v9.diff

Review of attachment 8538881 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, that's about all I can find here. Patch for factoring out DummySocket coming soon.
Attachment #8538881 - Flags: review+

Comment 52

5 years ago
Your care and attention have absolutely increased the quality of the patch!
Update description since I'm about to add a part 2.
Attachment #8538881 - Attachment is obsolete: true
Factor out DummySocket into its own header file.
Attachment #8535946 - Attachment is obsolete: true
Attachment #8536854 - Attachment is obsolete: true
Attachment #8538041 - Attachment is obsolete: true
Attachment #8538116 - Attachment is obsolete: true
Attachment #8538180 - Attachment is obsolete: true
Attachment #8538730 - Attachment is obsolete: true
Attachment #8538887 - Flags: review+
Comment on attachment 8538889 [details] [diff] [review]
Part 2: Consolidate the two copies of DummySocket we have floating around.

Review of attachment 8538889 [details] [diff] [review]:
-----------------------------------------------------------------

The version of DummySocket in proxy_tunnel_socket_unittest.cpp was copied from buffered_stun_socket_unittest.cpp, and enhanced slightly. I have copied just the enhanced version into its own header file.
Attachment #8538889 - Flags: review?(drno)
I halfway expect missing include bustage in the new dummysocket.h on some platform, but here are the try runs.

try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=eaf7ccdedd9b

non-unified try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e85bcc5e77b5
Attachment #8538889 - Attachment is obsolete: true
Attachment #8538889 - Flags: review?(drno)
Attachment #8539334 - Flags: review?(drno)
Getting closer, although seeing signs that we've messed the lifecycle of PCMedia up. Investigating.

https://tbpl.mozilla.org/?tree=Try&rev=ef2e0c2cbb2f
Fix some lifecycle problems. Also, it seems that Cancel() expects to be passed some error result; NS_ERROR_ABORT seems to be commonly used for this.
Attachment #8539334 - Attachment is obsolete: true
Attachment #8539334 - Flags: review?(drno)
Attachment #8539447 - Flags: review?(drno)
Comment on attachment 8539447 [details] [diff] [review]
Part 2: Consolidate the two copies of DummySocket we have floating around.

Review of attachment 8539447 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8539447 - Flags: review?(drno) → review+
Comment on attachment 8538887 [details] [diff] [review]
Part 1: Use HTTP proxy for WebRTC ICE-TCP, TURN/TCP, TURN/TLS

Review of attachment 8538887 [details] [diff] [review]:
-----------------------------------------------------------------

Adding myself so I can review it this week. Please do not land without my r+.
Attachment #8538887 - Flags: review?(ekr)
Comment on attachment 8538887 [details] [diff] [review]
Part 1: Use HTTP proxy for WebRTC ICE-TCP, TURN/TCP, TURN/TLS

Review of attachment 8538887 [details] [diff] [review]:
-----------------------------------------------------------------

Byron, can you please put this up on rb?
Part 1 is already up here: https://reviewboard.mozilla.org/r/1489/

Part 2 is probably straightforward enough that it can be reviewed on splinter.
I have reviewed the integration piece on Reviewboard. It needs at least one more pass
I just reviewed the proxy handling piece. It also needs another pass.
Attachment #8538887 - Flags: review?(ekr) → review-

Comment 67

5 years ago
Tokbox is eager to get this uplifted into FF36. Let us know if there is anything we can do facilitate another pass or the review process.
(In reply to Rob Hainer from comment #67)
> Tokbox is eager to get this uplifted into FF36. Let us know if there is
> anything we can do facilitate another pass or the review process.

Sorry I wasn't clear. "another pass" in this context means "Please submit a revised patch that addresses the review comments"

Comment 69

5 years ago
Please verify this addresses @ekr review comments

Comment 70

5 years ago
I believe I've addressed all comments in a timely fashion. Given the importance of the patch, I would like if we can apply with some urgency and address non-critical concerns post commit.

As Rob mentioned, we would also like to uplift to 36 very much.
(In reply to ryan from comment #69)
> Created attachment 8541893 [details] [diff] [review]
> proxy-tunnel-complete-patch-v10.diff
> 
> Please verify this addresses @ekr review comments

Please upload a new patch to ReviewBoard and I'll try to get to it
today.

Comment 72

5 years ago
I have no idea how to do that. The usability of review board is suspect.

Comment 74

5 years ago
(In reply to Eric Rescorla (:ekr) from comment #73)

> https://wiki.mozilla.org/Auto-tools/Projects/MozReview
> http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html

This requires I have ssh access to hg.mozilla.org, which I do not believe I have. Byron put this on review board originally.
You should apply for Level 1 commit access. See:

https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/

CC Byron or myself on the bug and we can vouch for you.

Comment 76

5 years ago
(In reply to Eric Rescorla (:ekr) from comment #75)
> You should apply for Level 1 commit access. See:

https://bugzilla.mozilla.org/show_bug.cgi?id=1116318

Thank you for expediting this as much as possible.
Turnaround for level 1 access can take some time, give me a minute to put the latest patch up on reviewboard.
Updated review at reviewboard. Not sure why it's not notifying bugzilla/

Comment 80

5 years ago
Still some comments remain open pending feedback.

Comment 81

5 years ago

Comment 82

5 years ago
/r/1821 - Bug 949703 - "Use HTTP proxy for WebRTC ICE-TCP, TURN/TCP, TURN/TLS"

Pull down this commit:

hg pull review -r 4f0706d08de1e9e71b29b3c96bc73cc6c753d995

Comment 83

5 years ago
I tried to get this to push to the existing review, but perhaps I don't know the correct reviewid?
(In reply to ryan from comment #80)
> Created attachment 8542792 [details] [diff] [review]
> proxy-tunnel-complete-patch-v11.diff
> 
> Still some comments remain open pending feedback.

Additional feedback in the review

Updated

5 years ago
Blocks: 1116583
MT, shouldn't this implement:
https://tools.ietf.org/html/draft-ietf-httpbis-tunnel-protocol-00
Flags: needinfo?(martin.thomson)
Yep.  That's pretty easy to do right now.  The Tunnel-Protocol header field should be set to "webrtc" as defined in https://tools.ietf.org/html/draft-ietf-rtcweb-alpn-00

I suppose that since you've not provided me with a review on bug 996238, I'm going to have to be the one that cleans up the mess regarding "c-webrtc" though.
Flags: needinfo?(martin.thomson)

Updated

4 years ago
Blocks: 1115934

Updated

4 years ago
Blocks: 1117984

Comment 87

4 years ago
I believe this should address remaining concerns including the design concern.
See comments in reviewboard.

Comment 89

4 years ago
Lucky 13?

Updated

4 years ago
Attachment #8544954 - Flags: review?(ekr)
See ReviewBoard comments.

Comment 91

4 years ago

Comment 92

4 years ago

Comment 93

4 years ago

Comment 94

4 years ago
Posted patch Latest version (obsolete) — Splinter Review
Attachment #8541893 - Attachment is obsolete: true
Attachment #8542792 - Attachment is obsolete: true
Attachment #8542797 - Attachment is obsolete: true
Attachment #8544212 - Attachment is obsolete: true
Attachment #8544954 - Attachment is obsolete: true
Attachment #8549336 - Attachment is obsolete: true
Attachment #8549920 - Attachment is obsolete: true
Attachment #8550370 - Attachment is obsolete: true
Attachment #8544954 - Flags: review?(ekr)
Comment on attachment 8550372 [details] [diff] [review]
Latest version

Review of attachment 8550372 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed the patch and it LGTM with nits in ReviewBoard.
I haven't reviewed the unit tests, but I understand they have
Byron's r+.

However, I just ran the unit tests and they crashed on my machine,
which seems problematic.

The problem seems to be:

   nr_proxy_tunnel_config_create(&config_);
   nr_proxy_tunnel_config_set_resolver(config_, nr_resolver_);

You don't seem to be setting the host.

Can you please fix the unit tests?

Comment 96

4 years ago
Attachment #8550372 - Attachment is obsolete: true

Comment 97

4 years ago
Attachment #8550640 - Attachment is obsolete: true
Lots of compile errors here. Please fix.

Comment 100

4 years ago
It looks like really only one error. is -Werror the only difference in flags between ./mach build and the build server?

Do you have any idea what  b2g_emulator_vm try opt test mochitest-2

DeviceRunner TEST-UNEXPECTED-FAIL | dom/bindings/test/test_forOf.html | application timed out after 330.0 seconds with no output

PROCESS-CRASH | dom/bindings/test/test_forOf.html | application crashed [@ mozilla::NrIceCtx::SetProxyServer] 

Means?
I do not know. Needinfoing Byron
Flags: needinfo?(docfaraday)
You need to execute the call to |NrIceCtx::SetProxyServer| on STS; this was being done in an earlier patch, why the change?
Flags: needinfo?(docfaraday)
This is just the original patch. Do we need to apply your Part 2 as well? If so, it needs a rebase
Flags: needinfo?(docfaraday)

Comment 104

4 years ago
StartGathering was done in EnsureIceGathering_s but SetProxyServer got lost in the shuffle.

Comment 105

4 years ago
Setting .mozconfig: mk_add_options MOZ_MAKE_FLAGS="-s -Wsign-compare -Werror -j8"

doesn't appear to cause my local build to fail (although I see the warning). What flags to I need to ensure the build server will pass if my local build passes?

Comment 106

4 years ago
Addresses the SetProxyServer threading issue and the signed compare issue.

AddressSanitizer complained about PeerConnectionMedia.cpp:239 but I'm not sure how that could be a heap use after free -- could it be spurious?
Attachment #8550730 - Attachment is obsolete: true
Attachment #8539447 - Attachment is obsolete: true
Assignee: docfaraday → ekr
Status: NEW → ASSIGNED
(In reply to ryan from comment #106)
> Created attachment 8550919 [details] [diff] [review]
> proxy-tunnel-complete-patch-v18.diff
> 
> Addresses the SetProxyServer threading issue and the signed compare issue.
> 
> AddressSanitizer complained about PeerConnectionMedia.cpp:239 but I'm not
> sure how that could be a heap use after free -- could it be spurious?

Seems unlikely, but perhaps it needs Byron's patch.

I rebased Byron's patch (see c107) and pushed it to try:

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e32840e8efa

However, in the process I noticed a number of compile warnings in your new
code. Can you please check and fix?

Warning: -Wsign-compare in /Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c: comparison of integers of different signs: 'int' and 'unsigned long'
/Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:103:30: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
  if (printed < 0 || printed >= sizeof(mesg)) {
                     ~~~~~~~ ^  ~~~~~~~~~~~~
Warning: -Wsign-compare in /Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c: comparison of integers of different signs: 'long' and 'size_t' (aka 'unsigned long')
/Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:131:20: warning: comparison of integers of different signs: 'long' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
    if (end - term >= N && memcmp(term, ENDLN, N) == 0) {
        ~~~~~~~~~~ ^  ~
Warning: -Wmissing-prototypes in /Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c: no previous prototype for function 'nr_proxy_tunnel_config_copy'
/Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:466:5: warning: no previous prototype for function 'nr_proxy_tunnel_config_copy' [-Wmissing-prototypes]
int nr_proxy_tunnel_config_copy(nr_proxy_tunnel_config *config, nr_proxy_tunnel_config **copypp)
    ^
Warning: -Wincompatible-pointer-types in /Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c: incompatible pointer types passing 'nr_socket_proxy_tunnel **' (aka 'struct nr_socket_proxy_tunnel_ **') to parameter of type 'void **'
/Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:522:36: warning: incompatible pointer types passing 'nr_socket_proxy_tunnel **' (aka 'struct nr_socket_proxy_tunnel_ **') to parameter of type 'void **' [-Wincompatible-pointer-types]
    nr_socket_proxy_tunnel_destroy(&sock);
                                   ^~~~~
/Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:165:50: note: passing argument to parameter 'objpp' here
static int nr_socket_proxy_tunnel_destroy(void **objpp)
                                                 ^
Warning: -Wmissing-prototypes in /Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c: no previous prototype for function 'nr_socket_wrapper_factory_proxy_tunnel_wrap'
/Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:527:5: warning: no previous prototype for function 'nr_socket_wrapper_factory_proxy_tunnel_wrap' [-Wmissing-prototypes]
int nr_socket_wrapper_factory_proxy_tunnel_wrap(void *obj,
    ^
Warning: -Wmissing-prototypes in /Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c: no previous prototype for function 'nr_socket_wrapper_factory_proxy_tunnel_destroy'
/Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:539:5: warning: no previous prototype for function 'nr_socket_wrapper_factory_proxy_tunnel_destroy' [-Wmissing-prototypes]
int nr_socket_wrapper_factory_proxy_tunnel_destroy(void **objpp) {
    ^
Warning: -Wincompatible-pointer-types in /Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c: incompatible pointer types passing 'nr_socket_wrapper_factory_proxy_tunnel **' (aka 'struct nr_socket_wrapper_factory_proxy_tunnel_ **') to parameter of type 'void **'
/Users/ekr/dev/mozilla-inbound/media/mtransport/third_party/nICEr/src/net/nr_proxy_tunnel.c:580:52: warning: incompatible pointer types passing 'nr_socket_wrapper_factory_proxy_tunnel **' (aka 'struct nr_socket_wrapper_factory_proxy_tunnel_ **') to parameter of type 'void **' [-Wincompatible-pointer-types]
    nr_socket_wrapper_factory_proxy_tunnel_destroy(&wrapper);
                                                   ^~~~~~~~

Comment 109

4 years ago
What's wrong with the incompatible pointer types? What's the solution? A cast?
(In reply to ryan from comment #109)
> What's wrong with the incompatible pointer types? What's the solution? A
> cast?

The issue is that the dtor takes a void * because it is called from the base class,
but you are here passing a T * (which gets cast to a void * when passed to the
outparam). Usually, I create a void *temporary, like so:


  if (_status) {
    void *wrapperv = wrapper;

    nr_socket_wrapper_factory_proxy_tunnel_destroy(&wrapperv);
  }
P.S.


Warning: -Wuninitialized in /Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp: variable 'status' is uninitialized when used here
/Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:745:27: warning: variable 'status' is uninitialized when used here [-Wuninitialized]
    mProxyRequest->Cancel(status);
                          ^~~~~~
/Users/ekr/dev/mozilla-inbound/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:743:3: note: variable 'status' is declared here
  nsresult status;
  ^

Comment 112

4 years ago
Still not sure why my local build won't error on warnings...
(In reply to ryan from comment #112)
> Created attachment 8550955 [details] [diff] [review]
> proxy-tunnel-complete-patch-v19.diff
> 
> Still not sure why my local build won't error on warnings...

I think it's a configure flag. Anyway, you should be able to push to
try as a double check.
ac_add_options --enable-warnings-as-errors
(In reply to Eric Rescorla (:ekr) from comment #103)
> This is just the original patch. Do we need to apply your Part 2 as well? If
> so, it needs a rebase

Yes, part 2 will be necessary to fix some things.
Flags: needinfo?(docfaraday)
(In reply to ryan from comment #106)
> Created attachment 8550919 [details] [diff] [review]
> proxy-tunnel-complete-patch-v18.diff
> 
> Addresses the SetProxyServer threading issue and the signed compare issue.
> 
> AddressSanitizer complained about PeerConnectionMedia.cpp:239 but I'm not
> sure how that could be a heap use after free -- could it be spurious?

   That's one of the things the part 2 fixes; |ProtocolProxyQueryHandler| wasn't retaining a ref to the PeerConnectionMedia to keep it alive. It also fixed the compiler warning about the Cancel call.
Come to think of it, maybe fixing the Cancel call will fix both of those problems...
I am attempting another try push with a variant on part 2 that doesn't include making |ProtocolProxyQueryHandler::pcm_| an nsRefPtr; we were UAFing that, but I think that was because the Cancel call wasn't working (which I fixed later on).
Byron, you and Ryan seem to disagree about the status value to send to
Cancel.

patch-19 passes NS_OK.
Byron's patch passes NS_ERROR_ABORT.

If one of you tells me what's needed, I'll finish the rebase of Byron's patch.
Patch #19 seems to properly incorporate my code review comments.

To follow up here, it seems like here's what's needed:

1. Byron and Ryan to settle on the Cancel argument.
2. A consolidated set of patches (or a merger) that reflects that.
3. A green try push.

Once we have that, I'll r+ and someone can mark it checkin-needed for
the sheriffs to land.
The arg passed to Cancel has to be a failure, or else it is a no-op:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsProtocolProxyService.cpp?from=netwerk/base/src/nsProtocolProxyService.cpp&case=true#148

Other than that, it doesn't look all that important what error is passed. I've seen NS_ERROR_ABORT, and NS_ERROR_FAILURE, but it seemed like NS_ERROR_ABORT was a good enough fit:

https://dxr.mozilla.org/mozilla-central/search?q=Cancel%28NS_ERROR&case=true
Attachment #8550933 - Attachment is obsolete: true
Assignee: ekr → docfaraday

Comment 126

4 years ago
What do those 3 failures mean?
The bustage on OS X 10.8 debug static analysis seems to either be infra or buildsystem-related (probably just that my working copy is at a bad revision), nothing to worry about.

The other two are known intermittents that don't involve this patch-set.
Comment on attachment 8552509 [details] [diff] [review]
Part 2: Consolidate the two copies of DummySocket we have floating around

Review of attachment 8552509 [details] [diff] [review]:
-----------------------------------------------------------------

Carry forward r=drno
Attachment #8552509 - Flags: review+
Attachment #8550919 - Attachment is obsolete: true
Attachment #8538887 - Attachment is obsolete: true
When the time comes, I can land this, so don't set checkin-needed.
Looks like we're ready for that r+
Flags: needinfo?(ekr)
Byron, can you please put r19 up on RB so I can double-check?
Flags: needinfo?(ekr)
Done.
Comment on attachment 8550955 [details] [diff] [review]
proxy-tunnel-complete-patch-v19.diff

Review of attachment 8550955 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with the changes indicated in ReviewBoard.
Attachment #8550955 - Flags: review+

Comment 136

4 years ago
This wraps up several fixes from Byron
Attachment #8550955 - Attachment is obsolete: true
Attachment #8552755 - Flags: review?(ekr)
Posted file MozReview Request: bz://949703/bwc (obsolete) —
/r/2827 - Bug 949703 - Part 3: Some nit fixes.

Pull down this commit:

hg pull review -r 381ce47cdd327b6517d708346309518ea5e5fe19
(In reply to Byron Campen [:bwc] from comment #134)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7933aeabf6bd

Looks like something went awry in the commit information for this :(
Target Milestone: mozilla35 → ---
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #138)
> (In reply to Byron Campen [:bwc] from comment #134)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/7933aeabf6bd
> 
> Looks like something went awry in the commit information for this :(

Maybe "<ryan>" as the committer name has confused something, which has interpreted it as an html tag? Ryan, go ahead and set up your .hgrc to use something like:

[ui]
username = Byron Campen [:bwc] <docfaraday@gmail.com>

You can also run ./mach mercurial-setup, and it should ask you for this info among other things.
That's really strange, had a try push where I retriggered those tests 5 times each, and all green.
Flags: needinfo?(docfaraday)
rebase and try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86c8a8758771

I suspect that we either conflicted with something that just landed, or that something else caused this intermittent and we just got unlucky.
Comment on attachment 8552758 [details] [diff] [review]
proxy-tunnel-complete-patch-v20.diff

Review of attachment 8552758 [details] [diff] [review]:
-----------------------------------------------------------------

Ryan,

Thanks for revising your patch. This LGTM.

Byron, because Ryan fixed the Cancel argument, you will need to adjust your patch.

::: media/mtransport/nricectx.h
@@ +181,5 @@
> +  NrIceProxyServer(const std::string& host, uint16_t port) :
> +    host_(host), port_(port) {
> +  }
> +
> +  NrIceProxyServer& operator=(const NrIceProxyServer& that) {

You no longer need the operator= constructor and I believe also the 0-argument constructor.
Try is fine.
Maybe hg messed up the rebase? Here's a try push of exactly the same revision that was pushed to inbound yesterday:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d4f411b7fbc
And here's another try push with some consolidation of stuff:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2714100a7ec0
Ugh, bug Bug 436344 has just shifted the API out from under us. Give me a bit.
Attachment #8552758 - Attachment is obsolete: true
Attachment #8552509 - Attachment is obsolete: true
Comment on attachment 8553261 [details] [diff] [review]
Part 1: Use HTTP proxy for WebRTC TURN/TCP

Carry forward r=ekr
Attachment #8553261 - Flags: review+
Comment on attachment 8553262 [details] [diff] [review]
Part 2: Consolidate the two copies of DummySocket we have floating around

Carry forward r=drno
Attachment #8553262 - Flags: review+
Blocks: 1125292
https://hg.mozilla.org/mozilla-central/rev/4f21df91d5e2
https://hg.mozilla.org/mozilla-central/rev/e18607ef7021
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38

Comment 155

4 years ago
Comment on attachment 8553261 [details] [diff] [review]
Part 1: Use HTTP proxy for WebRTC TURN/TCP

Review of attachment 8553261 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/mtransport/third_party/nICEr/src/net/nr_socket_wrapper.c
@@ +70,5 @@
> +
> +  wrapper = *wrapperp;
> +  *wrapperp = 0;
> +
> +  assert(wrapper->vtbl);

Missing #include <assert.h>

Updated

4 years ago
Depends on: 1125588
This has already landed. If it's creating a problem for you, please file a patch with this change
Depends on: 1126036
Flags: needinfo?(docfaraday)

Comment 157

4 years ago
This bug is blocking roll-out for a large customer with a restrictive environment (all traffic is proxied).

We have provided for them a build of Firefox 34 with an early version of the patch which they have used internally for testing and which works for them. We have also tested the latest Nightly internally in a simulated environment and it is working (with all ports blocked except to the proxy). Lastly they will be beginning more testing using a version of Nightly to verify our results.

Comment 158

4 years ago
Comment on attachment 8553261 [details] [diff] [review]
Part 1: Use HTTP proxy for WebRTC TURN/TCP

Approval Request Comment
[Feature/regressing bug #]: 949703
[User impact if declined]: Users behind restrictive networks will not be able to use TURN-TCP for WebRTC from behind a proxy
[Describe test coverage new/current, TreeHerder]: Internal testing has been performed in live customer networks. Simulation can be done by starting a WebRTC session behind a firewall where all traffic is blocked except a proxy.
[Risks and why]: E10S or other threading changes may expose thread-safety issues in previously single-threaded code.
[String/UUID change made/needed]: No.
Attachment #8553261 - Flags: approval-mozilla-aurora?
Also, if this is uplifted, bug 1126036 must come with it.
Comment on attachment 8553261 [details] [diff] [review]
Part 1: Use HTTP proxy for WebRTC TURN/TCP

This is a fair size code change and new feature work. Given the state of the branch and the shortened 37 cycle, I don't think we should take this change in 37. I understand that this is blocking some clients from using WebRTC/Hello. Can this functionality be packaged as an add-on that can be deployed until 38 ships?

I'm happy to discuss this further if there is additional justification for uplift.

Aurora-
Attachment #8553261 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Flags: needinfo?(mreavy)
Attachment #8552755 - Flags: review?(ekr)
Attachment #8552762 - Attachment is obsolete: true

Comment 164

3 years ago
HTTP proxy is not used in version 44.0.2. My FF is under a http proxy. https://apprtc.appspot.com/ is used as WebRTC application. By FF 44, relay candidate is not gathered and cannot setup video communication with others. By FF 39, everything is OK.

Would you please check and confirm this? Thanks.
(In reply to detour from comment #164)
> HTTP proxy is not used in version 44.0.2. My FF is under a http proxy.
> https://apprtc.appspot.com/ is used as WebRTC application. By FF 44, relay
> candidate is not gathered and cannot setup video communication with others.
> By FF 39, everything is OK.
> 
> Would you please check and confirm this? Thanks.

If you are experiencing problems, please open a new bug report so we can collect debug information in that ticket.

Comment 166

3 years ago
Bug 1251542 is filed.
You need to log in before you can comment on or make changes to this bug.