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.

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

believe Randell and Nils are investigating how often this is an issue.

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

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.

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

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

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

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

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

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.

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?

Please give feedback on the direction of nICEr portion of the patch.

Builds on windows

Maire - Would be great to have the WebRTC team take a look at patch Ryan posted.

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.

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.

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.

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?!

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

Please consider complete patch with NrSocket integration and unittests

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|

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.

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.

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)

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.

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

Interdiff between v3 and v6 here: https://reviewboard.mozilla.org/r/1489/diff/1-2/

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

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

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

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.

Your care and attention have absolutely increased the quality of the patch!

Update description since I'm about to add a part 2.

Factor out DummySocket into its own header file.

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.

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

Fix comparison warning bustage.

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.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=54a0557abc09 try non-unified: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4c08d25d010d

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

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

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.

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"

Please verify this addresses @ekr review comments

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.

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

(In reply to ryan from comment #72) > I have no idea how to do that. https://wiki.mozilla.org/Auto-tools/Projects/MozReview http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview.html

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

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

Interdiff here: https://reviewboard.mozilla.org/r/1489/diff/5-6/

Updated review at reviewboard. Not sure why it's not notifying bugzilla/

Still some comments remain open pending feedback.

/r/1821 - Bug 949703 - "Use HTTP proxy for WebRTC ICE-TCP, TURN/TCP, TURN/TLS" Pull down this commit: hg pull review -r 4f0706d08de1e9e71b29b3c96bc73cc6c753d995

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

MT, shouldn't this implement: https://tools.ietf.org/html/draft-ietf-httpbis-tunnel-protocol-00

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.

I believe this should address remaining concerns including the design concern.

See comments in reviewboard.

Lucky 13?

See ReviewBoard comments.

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?

Lots of compile errors here. Please fix.

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

You need to execute the call to |NrIceCtx::SetProxyServer| on STS; this was being done in an earlier patch, why the change?

This is just the original patch. Do we need to apply your Part 2 as well? If so, it needs a rebase

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

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?

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?

(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); ^~~~~~~~

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; ^

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.

I think I scheduled too much... https://treeherder.mozilla.org/#/jobs?repo=try&revision=a10344b751fc

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.

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

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

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

Unrot.

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

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

When the time comes, I can land this, so don't set checkin-needed.

Looks like we're ready for that r+

Byron, can you please put r19 up on RB so I can double-check?

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.

Here goes... https://hg.mozilla.org/integration/mozilla-inbound/rev/7933aeabf6bd https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5a7adab265

This wraps up several fixes from Byron

/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 :(

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

Backed out both patches in https://hg.mozilla.org/integration/mozilla-inbound/rev/b1b953d374c8 for mochitest-e10s-3 orange: https://treeherder.mozilla.org/logviewer.html#?job_id=5733016&repo=mozilla-inbound

That's really strange, had a try push where I retriggered those tests 5 times each, and all green.

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.

Catch up with API movement, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=17d692e1ae07

Move various fixes into part 1.

Comment on attachment 8553261 [details] [diff] [review] Part 1: Use HTTP proxy for WebRTC TURN/TCP Carry forward r=ekr

Comment on attachment 8553262 [details] [diff] [review] Part 2: Consolidate the two copies of DummySocket we have floating around Carry forward r=drno

Check back on https://treeherder.mozilla.org/#/jobs?repo=try&revision=af1a5e9b7118

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f21df91d5e2 https://hg.mozilla.org/integration/mozilla-inbound/rev/e18607ef7021

https://hg.mozilla.org/mozilla-central/rev/4f21df91d5e2 https://hg.mozilla.org/mozilla-central/rev/e18607ef7021

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>

This has already landed. If it's creating a problem for you, please file a patch with this change

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

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-

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.

Bug 1251542 is filed.