Bugzilla

Assignee

Comment 2

•

11 years ago

Attached patch WIP for TURN TCP (obsolete) — Details — Splinter Review

Added

Assignee

Updated

•

11 years ago

Attachment #817601 - Attachment is obsolete: true

Updated

•

11 years ago

Blocks: 928060

Updated

•

11 years ago

No longer blocks: 928060

Depends on: 928060

Updated

•

11 years ago

Blocks: 928060

No longer depends on: 928060

Comment 3

•

11 years ago

Please note bug 906968 comment 9.

Comment 4

•

11 years ago

Er, bug 928060 comment 9.

Nathan Froyd [:froydnj]

Updated

•

11 years ago

Blocks: 928930

Assignee

Comment 5

•

11 years ago

Attached patch WIP for TURN TCP (obsolete) — Details — Splinter Review

Added

Assignee

Comment 6

•

11 years ago

Comment on attachment 820415 [details] [diff] [review]
WIP for TURN TCP

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

Adam,

This is an untested and unreviewed WIP, but should be enough give you an idea of where
I am going.

Attachment #820415 - Flags: feedback?(adam)

Assignee

Comment 7

•

11 years ago

Attached patch WIP for TURN TCP (obsolete) — Details — Splinter Review

Added

Assignee

Updated

•

11 years ago

Attachment #820415 - Attachment is obsolete: true

Attachment #820415 - Flags: feedback?(adam)

Assignee

Comment 8

•

11 years ago

This patch needs to be rebased to remove the log changes I accidentally reverted.

Assignee

Comment 9

•

11 years ago

Comment on attachment 820781 [details] [diff] [review]
WIP for TURN TCP

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

Review comments from myself.

::: media/mtransport/nr_socket_prsock.cpp
@@ +463,5 @@
> +  if ((r=nr_transport_addr_copy(&remote_addr_, addr)))
> +    ABORT(r);
> +
> +  if(fd_==nullptr)
> +    ABORT(R_EOD);

Move this up.

@@ +483,5 @@
> +}
> +
> +
> +int NrSocket::write(const void *msg, size_t len, size_t *written) {
> +  ASSERT_ON_THREAD(ststhread_);

Check for connected_

@@ +502,5 @@
> +  return _status;
> +}
> +
> +int NrSocket::read(void* buf, size_t maxlen, size_t *len) {
> +  ASSERT_ON_THREAD(ststhread_);

Check for connected_

::: media/mtransport/nricectx.cpp
@@ +182,5 @@
>  
>  
>  
> +nsresult NrIceStunServer::ToNicerStunStruct(nr_ice_stun_server *server,
> +                                            std::string transport) const {

Can this be a const &

@@ +231,5 @@
> +  } else {
> +    MOZ_ASSERT(false);
> +    return NS_ERROR_FAILURE;
> +  }
> +  

Trailing whitespace.

::: media/mtransport/test/stunserver.cpp
@@ -154,3 @@
>  }
>  
>  static nr_socket_vtbl nr_socket_wrapped_vtbl = {

Revert

::: media/mtransport/test/stunserver.h
@@ -11,3 @@
>  
>  #include <map>
>  #include <string>

Revert

::: media/mtransport/test/turn_unittest.cpp
@@ +120,2 @@
>      ASSERT_EQ(0, r);
> +    

Whitespace.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +211,5 @@
>      cand->stream=comp->stream;
>  
>  
> +    r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): creating candidate %s with type %d",
> +      ctx->label,label,ctype);

Revert log

@@ +660,4 @@
>    }
>  #endif /* USE_TURN */
>  
>  static void nr_ice_srvrflx_stun_finished_cb(NR_SOCKET sock, int how, void *cb_arg)

revert log

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
@@ +84,5 @@
>      else{
>        o_priority=rcand->priority;
>        a_priority=lcand->priority;
>      }
>      pair->priority=(MIN(o_priority, a_priority))<<32 |

This file is only log changes. Revert.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +229,5 @@
>        for(j=0;j<ctx->turn_server_ct;j++){
> +        /* Skip non-UDP */
> +        if (ctx->turn_servers[j].transport != IPPROTO_UDP)
> +          continue;
> +            

Fix whitespace.

@@ +311,5 @@
> +        nr_socket *sock;
> +        nr_socket *buffered_sock;
> +        nr_socket *turn_sock;
> +
> +        /* Skip non-UDP */

non-tcp

@@ +326,5 @@
> +          r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): couldn't create socket for address %s",ctx->label,addr.as_string);
> +          continue;
> +        }
> +        /* Wrap it */
> +        if((r=nr_socket_buffered_stun_create(sock, 32000, &buffered_sock)))

Let's make this a constant somewhere.

@@ +342,5 @@
> +        if(r=nr_ice_candidate_create(ctx,component,
> +          isock,turn_sock,RELAYED,
> +          &ctx->turn_servers[j].turn_server,component->component_id,&cand))
> +           ABORT(r);
> +        cand->u.relayed.srvflx_candidate=NULL;

Comment on why no srvflx.

@@ +354,5 @@
> +        if(r=nr_stun_server_ctx_create(label,sock,&isock->stun_server))
> +          ABORT(r);
> +        if(r=nr_ice_socket_register_stun_server(isock,isock->stun_server,&isock->stun_server_handle))
> +          ABORT(r);
> +        

Whitespace.

@@ +404,5 @@
> +
> +    /* Initialize the UDP candidates */
> +    if (r=nr_ice_component_initialize_udp(ctx, component, addrs, addr_ct, lufrag, &pwd))
> +      ABORT(r);
> +    /* And the TCP candidates */

add ws

@@ +487,5 @@
>        c2=TAILQ_NEXT(c2,entry_comp);
>      }
>  
>      if (tmp) {
> +      r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): Removing redundant candidate %s",

REvert log messages.

::: media/mtransport/third_party/nICEr/src/ice/ice_socket.c
@@ +34,5 @@
>  
>  static char *RCSSTRING __UNUSED__="$Id: ice_socket.c,v 1.2 2008/04/28 17:59:01 ekr Exp $";
>  
>  #include <assert.h>
> +#include <string.h>

remove?

@@ +215,5 @@
> +    }
> +    else {
> +      assert(addr.protocol == IPPROTO_TCP);
> +      sock->type = NR_ICE_SOCKET_TYPE_STREAM;
> +    }

assert for else?

::: media/mtransport/third_party/nICEr/src/net/nr_socket.h
@@ +59,5 @@
>      nr_transport_addr *addr);
>    int (*getfd)(void *obj, NR_SOCKET *fd);
>    int (*getaddr)(void *obj, nr_transport_addr *addrp);
> +  int (*connect)(void *obj, nr_transport_addr *addr);
> +  int (*swrite)(void *obj,const void *msg, size_t len, size_t *written);

Space

@@ +60,5 @@
>    int (*getfd)(void *obj, NR_SOCKET *fd);
>    int (*getaddr)(void *obj, nr_transport_addr *addrp);
> +  int (*connect)(void *obj, nr_transport_addr *addr);
> +  int (*swrite)(void *obj,const void *msg, size_t len, size_t *written);
> +  int (*sread)(void *obj,void * restrict buf, size_t maxlen, size_t *len);

Space.

@@ +82,5 @@
>  int nr_socket_getaddr(nr_socket *sock, nr_transport_addr *addrp);
>  int nr_socket_close(nr_socket *sock);
> +int nr_socket_connect(nr_socket *sock, nr_transport_addr *addr);
> +int nr_socket_write(nr_socket *sock,const void *msg, size_t len, size_t *written, int flags);
> +int nr_socket_read(nr_socket *sock, void * restrict buf, size_t maxlen, size_t *len, int flags);

Get rid of flags here.

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
@@ +1,2 @@
> +/*
> +Copyright (c) 2007, Adobe Systems, Incorporated

This is complicated enough that it deserves a standalone test. Please add one.

@@ +42,5 @@
> +#include "p_buf.h"
> +#include "nr_socket.h"
> +#include "stun.h"
> +#include "nr_socket_buffered_stun.h"
> +

Extra whitespace.

@@ +62,5 @@
> +  /* Write state */
> +  nr_p_buf_ctx *p_bufs;
> +  nr_p_buf_head pending_writes;
> +  size_t pending;
> +  size_t max_pending;

Let's rename some stuff in here for clarity.

@@ +86,5 @@
> +  nr_socket_buffered_stun_getfd,
> +  nr_socket_buffered_stun_getaddr,
> +  nr_socket_buffered_stun_connect,
> +  nr_socket_buffered_stun_write,
> +  nr_socket_buffered_stun_read,

Can we remove the read and write? I don't think they are ever called externally. and they don't match the packet discipline.

@@ +103,5 @@
> +
> +  if ((r=nr_ip4_port_to_transport_addr(INADDR_ANY, 0, NR_IPV4, &sock->remote_addr)))
> +    ABORT(r);
> +
> +  /* TODO(ekr@rtfm.com): Check this */

Check that this max is right for STUN TCP.

@@ +115,5 @@
> +  if ((r=nr_p_buf_ctx_create(4096, &sock->p_bufs)))
> +    ABORT(r);
> +  sock->max_pending=max_pending;
> +
> +

Extra whitespace.

@@ +165,5 @@
> +  int r, _status;
> +  size_t written;
> +
> +  /* Check that we are writing to the connected address if
> +     connected */

Is there ever a case where we are not connected?

@@ +184,5 @@
> +}
> +
> +static void nr_socket_buffered_stun_failed(nr_socket_buffered_stun *sock)
> +  {
> +    sock->read_state = NR_ICE_SOCKET_READ_FAILED;

Does this need to be an extra fxn?

@@ +194,5 @@
> +  int r, _status;
> +  size_t bytes_read;
> +  nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
> +
> +reread:

Check for the failed state and fast fail here rather than later.

@@ +208,5 @@
> +
> +  /* Unfinished */
> +  if (sock->bytes_needed)
> +    ABORT(R_WOULDBLOCK);
> +          

whitespace.

@@ +210,5 @@
> +  if (sock->bytes_needed)
> +    ABORT(R_WOULDBLOCK);
> +          
> +  /* No more bytes expeected */
> +  if (sock->read_state == NR_ICE_SOCKET_READ_NONE) {

This means we are in header state. Add coment.

@@ +228,5 @@
> +
> +    goto reread;
> +  }
> +    
> +  if (maxlen < sock->bytes_needed)

Comment that this means we are in the body state.

@@ +248,5 @@
> +abort:
> +  if (_status && (_status != R_WOULDBLOCK)) {
> +    nr_socket_buffered_stun_failed(sock);
> +  }
> +      

Whitespace.

@@ +269,5 @@
> +
> +static int nr_socket_buffered_stun_close(void *obj)
> +{
> +  /* No-op */
> +  return 0;

This should probably call the sub-socket close fxn.

@@ +317,5 @@
> +      r_log(LOG_GENERIC, LOG_INFO, "Buffers full");
> +      ABORT(R_IO_ERROR);
> +    }
> +
> +    if (len) {

This is redundant.

@@ +360,5 @@
> +      ABORT(r);
> +    }
> +
> +    n1->r_offset += written;
> +    if (n1->r_offset <= n1->length) {

< not <=

@@ +373,5 @@
> +
> +  _status=0;
> +abort:
> +  if (_status && _status != R_WOULDBLOCK) {
> +    /* TODO(ekr@rtfm.com): Mark the socket as failed */

Do this TODO.

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.h
@@ +50,3 @@
>  
>  #endif
> +

Extra whitespace.

::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c
@@ +29,5 @@
>  (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>  OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>  */
>  
>  

Whitespace only change.

@@ +90,5 @@
> +       transmits to 1 and the timeout to maximum timeout*/
> +    if (ctx->my_addr.protocol == IPPROTO_TCP) {
> +      /* TODO(ekr@rtfm.com): check against 5766 */
> +      ctx->timeout_ms = ctx->final_retransmit_backoff_ms;
> +      ctx->maximum_transmits = 1;

The spec suggests using the total of all timeouts. That seems crazy long, though.

@@ +292,5 @@
>  
>      if (ctx->state != NR_STUN_CLIENT_STATE_RUNNING)
>          ABORT(R_NOT_PERMITTED);
>  
> +    r_log(NR_LOG_STUN,LOG_DEBUG,"STUN-CLIENT(%s): Sending(my_addr=%s,peer_addr=%s)",ctx->label,ctx->my_addr.as_string,ctx->peer_addr.as_string);

Revert these log message changes. They are just merge errors on my part.

::: media/mtransport/third_party/nICEr/src/stun/stun_hint.c
@@ +240,5 @@
> +  
> +  hdr = (nr_stun_message_header *)buf;
> +
> +  *length = ntohs(hdr->length);
> +  

Fix whitespace.

::: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c
@@ +86,5 @@
>  static int nr_turn_permission_find(nr_turn_client_ctx *ctx,
>                                     nr_transport_addr *addr,
>                                     nr_turn_permission **permp);
> +static int nr_turn_client_write_tcp(nr_turn_client_ctx *ctx,
> +                                    const UCHAR *buffer, int len);

Cruft.

@@ +404,5 @@
> +    ABORT(R_INTERNAL);
> +
> +  r = nr_socket_connect(ctx->sock, &ctx->turn_server_addr);
> +
> +  if (r == R_WOULDBLOCK) {

Remove whitespace.

@@ +433,5 @@
> +  /* Assume we connected successfully */
> +  if (ctx->state == NR_TURN_CLIENT_STATE_ALLOCATION_WAIT) {
> +    if ((r=nr_turn_stun_ctx_start(STAILQ_FIRST(&ctx->stun_ctxs))))
> +      ABORT(r);
> +    ctx->state = NR_TURN_CLIENT_STATE_ALLOCATING;

This could use a comment

@@ +970,5 @@
>    }
>    return(_status);
>  }
>  
> +

Extra whitespace.

::: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.h
@@ +83,4 @@
>  
>    char *label;
>    nr_socket *sock;
> +

whitespace only change

Assignee

Comment 10

•

11 years ago

Attached patch WIP for TURN TCP (obsolete) — Details — Splinter Review

Added
WIP for TURN TCP

Added

Assignee

Updated

•

11 years ago

Attachment #820781 - Attachment is obsolete: true

Assignee

Comment 11

•

11 years ago

Attached patch WIP for TURN TCP (obsolete) — Details — Splinter Review

Added
WIP for TURN TCP

Added

Assignee

Updated

•

11 years ago

Attachment #821009 - Attachment is obsolete: true

Comment 13

•

11 years ago

Comment on attachment 821054 [details] [diff] [review]
WIP for TURN TCP

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

Not done looking over the whole patch, but I wanted to get some quick comments to you on the buffered_stun code.

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
@@ +90,5 @@
> +  nr_socket_buffered_stun_read,
> +  nr_socket_buffered_stun_close
> +};
> +
> +int nr_socket_buffered_stun_create(nr_socket *inner, int max_pending, nr_socket **sockp)

The inner sock ownership contract here isn't clear when the call to create() fails. For example, if the initial RCALLOC doesn't work, then the inner socket survives; any failure after that point means that it is deleted. The easiest solution here would be to tear down the inner sock if the first RCALLOC fails.

@@ +111,5 @@
> +  sock->buffer_size = NR_STUN_MAX_MESSAGE_SIZE;
> +  sock->bytes_needed = sizeof(nr_stun_message_header);
> +
> +  STAILQ_INIT(&sock->pending_writes);
> +  if ((r=nr_p_buf_ctx_create(4096, &sock->p_bufs)))

Why 4096? Shouldn't this just be NR_STUN_MAX_MESSAGE_SIZE? If not, please define a new constant.

@@ +168,5 @@
> +  /* Check that we are writing to the connected address if
> +     connected */
> +  if (!nr_transport_addr_is_wildcard(&sock->remote_addr)) {
> +    if (nr_transport_addr_cmp(&sock->remote_addr, to, NR_TRANSPORT_ADDR_CMP_MODE_ALL))
> +      ABORT(R_BAD_DATA);

Maybe log here? This would be tricky to track down.

@@ +174,5 @@
> +
> +  if ((r=nr_socket_buffered_stun_write(obj, msg, len, &written)))
> +    ABORT(r);
> +
> +  if (len != written)

I don't think this is right: nr_socket_buffered_stun_write appears to set written to the number of bytes actually handed off to the kernel, not the number of bytes that it has taken possession of. So, e.g., if you try to send 1000 bytes, and only 800 make it down to the kernel, then it looks like we'll queue the remaining 200 and return "800" in "written". Then len != written, and even though everything appears to be working just fine, we're going to return an error here.

@@ +195,5 @@
> +  size_t bytes_read;
> +  nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
> +
> +reread:
> +  /* Read all the expected bytes */

The buffer handling in here looks pretty clean by casual inspection, but it might be worth doing an
assert(sock->bytes_needed <= sock->buffer_size - sock->bytes_read);

Yes, I recognize that this is kind of paranoid.

@@ +268,5 @@
> +}
> +
> +static int nr_socket_buffered_stun_close(void *obj)
> +{
> +  /* No-op */

Why don't we close the inner FD here?

@@ +317,5 @@
> +      r_log(LOG_GENERIC, LOG_INFO, "Buffers full");
> +      ABORT(R_IO_ERROR);
> +    }
> +
> +    if (len) {

Why are we checking for len again? We're inside an if (len) block already, and I don't see any way len could have changed...

@@ +335,5 @@
> +
> +    NR_ASYNC_WAIT(fd, NR_ASYNC_WAIT_WRITE, nr_socket_buffered_stun_writable_cb, sock);
> +  }
> +
> +  *written = written2;

There's a path through here where the nr_socket_write succeeds but only writes part of the buffer, and then something else fails later. Admittedly, in the current code, any such failures are pretty catastrophic; but it still seems that such a situation should correctly reflect the bytes successfully written, right? Is the contract here supposed to be that any out-values remain untouched if the return code is not success?

@@ +381,5 @@
> +static int nr_socket_buffered_stun_read(void *obj,void * restrict buf, size_t maxlen, size_t *len)
> +{
> +  nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
> +
> +  return nr_socket_read(sock->inner, buf, maxlen, len, 0);

Shouldn't this call through to the code that does the framing? It's a bit surprising that this socket would return discrete messages with recvfrom, but raw bytes with read. If nothing else, this is a footgun, sitting around for someone to completely hose framing with. I would strongly suggest either asserting in here, or making it call into the recvfrom code and discard the address.

Assignee

Comment 14

•

11 years ago

Comment on attachment 821054 [details] [diff] [review]
WIP for TURN TCP

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

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
@@ +90,5 @@
> +  nr_socket_buffered_stun_read,
> +  nr_socket_buffered_stun_close
> +};
> +
> +int nr_socket_buffered_stun_create(nr_socket *inner, int max_pending, nr_socket **sockp)

Good catch.

@@ +111,5 @@
> +  sock->buffer_size = NR_STUN_MAX_MESSAGE_SIZE;
> +  sock->bytes_needed = sizeof(nr_stun_message_header);
> +
> +  STAILQ_INIT(&sock->pending_writes);
> +  if ((r=nr_p_buf_ctx_create(4096, &sock->p_bufs)))

That would be fine too. This code was written before I had quite figured out what I wanted.

@@ +168,5 @@
> +  /* Check that we are writing to the connected address if
> +     connected */
> +  if (!nr_transport_addr_is_wildcard(&sock->remote_addr)) {
> +    if (nr_transport_addr_cmp(&sock->remote_addr, to, NR_TRANSPORT_ADDR_CMP_MODE_ALL))
> +      ABORT(R_BAD_DATA);

Yeah. Or assert.

@@ +174,5 @@
> +
> +  if ((r=nr_socket_buffered_stun_write(obj, msg, len, &written)))
> +    ABORT(r);
> +
> +  if (len != written)

Good catch. As I said in my comments, this needs a unit test, which would have caught this. :(

@@ +195,5 @@
> +  size_t bytes_read;
> +  nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
> +
> +reread:
> +  /* Read all the expected bytes */

Not at all.

@@ +268,5 @@
> +}
> +
> +static int nr_socket_buffered_stun_close(void *obj)
> +{
> +  /* No-op */

Yep. That's in my review.

@@ +317,5 @@
> +      r_log(LOG_GENERIC, LOG_INFO, "Buffers full");
> +      ABORT(R_IO_ERROR);
> +    }
> +
> +    if (len) {

Yep. that's in my review too.

@@ +335,5 @@
> +
> +    NR_ASYNC_WAIT(fd, NR_ASYNC_WAIT_WRITE, nr_socket_buffered_stun_writable_cb, sock);
> +  }
> +
> +  *written = written2;

I am planning to remove the raw write code and make it just an internal, so that will clean this up.

@@ +381,5 @@
> +static int nr_socket_buffered_stun_read(void *obj,void * restrict buf, size_t maxlen, size_t *len)
> +{
> +  nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
> +
> +  return nr_socket_read(sock->inner, buf, maxlen, len, 0);

I am planning to make these inaccessible. See my review.

Ivan Tsay (:ITsay)

Comment 15

•

11 years ago

Plus it for v1.3 since it blocks the committed feature of webRTC audio P2P

blocking-b2g: --- → 1.3+

Whiteboard: [webrtc] → [webrtc][ft:webrtc]

Assignee

Comment 16

•

11 years ago

Attached patch WIP for TURN TCP (obsolete) — Details — Splinter Review

Added
WIP for TURN TCP

Added

Assignee

Updated

•

11 years ago

Attachment #821054 - Attachment is obsolete: true

Comment 17

•

11 years ago

Comment on attachment 821054 [details] [diff] [review]
WIP for TURN TCP

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

Here's my feedback about the n-1 patch. I'm leaving off comments about excessive userspace buffering, as I think we've already hashed those out. I'm also not commenting on unit test coverage for the new buffered socket, as I know you're already working something out for that.

I think we want some ICE unittests that check that TURN UDP is preferred over TURN TCP when both are available.

::: media/mtransport/nr_socket_prsock.cpp
@@ +247,5 @@
>  
>      switch(netaddr->raw.family) {
>        case AF_INET:
>          if ((r = nr_ip4_port_to_transport_addr(ntohl(netaddr->inet.ip),
>                                                 ntohs(netaddr->inet.port),

The "protocol" parameter isn't used in this function -- I suspect that the call to nr_ip4_port_to_transport_addr should pass it in instead of a hardcoded IPPROTO_UDP, right?

@@ +462,5 @@
> +
> +  if ((r=nr_transport_addr_copy(&remote_addr_, addr)))
> +    ABORT(r);
> +
> +  if(fd_==nullptr)

if (!fd_)

::: media/mtransport/nricectx.cpp
@@ +231,5 @@
> +  } else {
> +    MOZ_ASSERT(false);
> +    return NS_ERROR_FAILURE;
> +  }
> +  

ws

::: media/mtransport/nricectx.h
@@ +140,5 @@
>    static NrIceTurnServer *Create(const std::string& addr, uint16_t port,
>                                   const std::string& username,
>                                   const std::vector<unsigned char>& password,
>                                   const std::string& transport = kNrIceTransportUdp) {
>      // TODO: Bug 906968 - Support TCP

Remove comment

::: media/mtransport/test/turn_unittest.cpp
@@ +120,2 @@
>      ASSERT_EQ(0, r);
> +    

WS

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +441,5 @@
>          break;
>  #ifdef USE_TURN
>        case RELAYED:
>          protocol=NR_RESOLVE_PROTOCOL_TURN;
> +        protocol=cand->u.relayed.server->transport;

s/protocol/transport/

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +229,5 @@
>        for(j=0;j<ctx->turn_server_ct;j++){
> +        /* Skip non-UDP */
> +        if (ctx->turn_servers[j].transport != IPPROTO_UDP)
> +          continue;
> +            

WS

@@ +282,5 @@
> +    return(_status);
> +  }
> +
> +
> +static int nr_ice_component_initialize_tcp(struct nr_ice_ctx_ *ctx,nr_ice_component *component, nr_local_addr *addrs, int addr_ct, char *lufrag, Data *pwd)

This seems very copy-paste-ish as compared to the corresponding UDP function. Is there some reason we can't consolidate the shared functionality?

@@ +326,5 @@
> +          r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): couldn't create socket for address %s",ctx->label,addr.as_string);
> +          continue;
> +        }
> +        /* Wrap it */
> +        if((r=nr_socket_buffered_stun_create(sock, 32000, &buffered_sock)))

Replace 32000 with a constant.

@@ +354,5 @@
> +        if(r=nr_stun_server_ctx_create(label,sock,&isock->stun_server))
> +          ABORT(r);
> +        if(r=nr_ice_socket_register_stun_server(isock,isock->stun_server,&isock->stun_server_handle))
> +          ABORT(r);
> +        

WS

::: media/mtransport/third_party/nICEr/src/ice/ice_socket.c
@@ +65,5 @@
>      if(r=nr_socket_recvfrom(sock->sock,buf,sizeof(buf),&len_s,0,&addr)){
>        r_log(LOG_ICE,LOG_ERR,"ICE(%s): Error reading from socket",sock->ctx->label);
> +      if (r != R_WOULDBLOCK && (sock->type != NR_ICE_SOCKET_TYPE_DGRAM)) {
> +        r_log(LOG_ICE,LOG_ERR,"ICE(%s): Error on reliable socket. Abandoning.",sock->ctx->label);
> +      

WS

@@ +66,5 @@
>        r_log(LOG_ICE,LOG_ERR,"ICE(%s): Error reading from socket",sock->ctx->label);
> +      if (r != R_WOULDBLOCK && (sock->type != NR_ICE_SOCKET_TYPE_DGRAM)) {
> +        r_log(LOG_ICE,LOG_ERR,"ICE(%s): Error on reliable socket. Abandoning.",sock->ctx->label);
> +      
> +        NR_ASYNC_CANCEL(s, NR_ASYNC_WAIT_READ);

What is the logic for this socket being harvested and/or restarted? It seems we should set the state to failed and have some "finished" callback into the app here to let them know that the socket has gone bad.

@@ +208,5 @@
>      sock->component=comp;
>  
> +    if(r=nr_socket_getaddr(nsock, &addr))
> +      ABORT(r);
> +    

WS

@@ +221,3 @@
>      TAILQ_INIT(&sock->candidates);
>      TAILQ_INIT(&sock->stun_ctxs);
> +    

WS

::: media/mtransport/third_party/nICEr/src/net/nr_socket.c
@@ +50,5 @@
> +    /* Check for the last thing in the vtbl to check for versioning
> +       issues */
> +    assert(vtbl->close);
> +    if (!vtbl->close)
> +       ABORT(R_INTERNAL);

I'm not sure I follow how this is supposed to protect against versioning issues.

For example, if this code is called by an older library, then this code's notion of "close" lives beyond the end of the vtbl structure that they've passed us, and may or may not contain a null (but my money's on "not").

If you really want to maintain some kind of ABI versioning, I would suggest you make it explicit: add an integer "version" field at the top of the socket_vtbl structure. In terms of existing code, I think it's safe enough to assume (a) function pointers will always be >= 256, and (b) version numbers for this ABI will always be < 256.

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_turn.c
@@ +70,5 @@
>    nr_socket_turn_getfd,
>    nr_socket_turn_getaddr,
> +  0,
> +  0,
> +  0,

Just to be clear: if someone tries to call the new methods here, the desired behavior is a null deref in nr_socket_connect and friends?

::: media/mtransport/third_party/nICEr/src/stun/stun_client_ctx.c
@@ +88,5 @@
> +    /* If we are doing TCP, compute the maximum timeout as if
> +       we retransmitted and then set the maximum number of
> +       transmits to 1 and the timeout to maximum timeout*/
> +    if (ctx->my_addr.protocol == IPPROTO_TCP) {
> +      /* TODO(ekr@rtfm.com): check against 5766 */

AFAICT from reading 5766, there's no rexmission on TCP, so I think the code below is correct.

::: media/mtransport/third_party/nICEr/src/stun/stun_hint.c
@@ +230,5 @@
>     return 1;
>  }
>  
> +int
> +nr_stun_message_length(UCHAR *buf, int len, int *length)

This is a first: I don't think I've ever gotten lost in a six-line function before. Can we rename these "buffer_length" and "stun_message_length" or some serviceable abbreviations of those?

::: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c
@@ +339,5 @@
>  
> +  if (addr->protocol == IPPROTO_UDP) {
> +    ctx->state = NR_TURN_CLIENT_STATE_CONNECTED;
> +  }
> +  else {

assert(addr->protocol == IPPROTO_TCP);

@@ +380,5 @@
> +    NR_SOCKET fd;
> +    int r;
> +
> +    r = nr_socket_getfd(ctx->sock, &fd);
> +    assert(!r);

Are you sure? The contract with getfd is that it can't fail? This seems brittle. I'd log it and move on.

@@ +609,5 @@
>      ABORT(r);
>    stun->stun->params.allocate_request.lifetime_secs =
>        TURN_LIFETIME_REQUEST_SECONDS;
>  
> +  if (ctx->state == NR_TURN_CLIENT_STATE_INITTED) {

switch (ctx->state) ?

@@ +970,5 @@
>    }
>    return(_status);
>  }
>  
> +

ws-only

::: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.h
@@ +81,2 @@
>  #define NR_TURN_CLIENT_STATE_FAILED          6
> +#define NR_TURN_CLIENT_STATE_CANCELLED       7

You seemed to be concerned about ABI issues in nr_socket. Reassigning meanings for constants is kind of taking things the opposite way.

Attachment #821054 - Attachment is obsolete: false

Updated

•

11 years ago

Attachment #821054 - Attachment is obsolete: true

Assignee

Comment 18

•

11 years ago

Attached patch WIP for TURN TCP (obsolete) — Details — Splinter Review

Added
WIP for TURN TCP

Added
* * *
Disable long-running signaling test to avoid timeouts

Randell Jesup [:jesup] (needinfo me)

Assignee

Updated

•

11 years ago

Attachment #825364 - Attachment is obsolete: true

Reporter

Comment 19

•

11 years ago

This is planned for 28, however this should not block shipping webrtc for B2G, just as we shipped desktop and Android without TURN TCP.

No longer blocks: 923363

blocking-b2g: 1.3+ → 1.3?

Maire Reavy [:mreavy]

Updated

•

11 years ago

Target Milestone: mozilla27 → mozilla28

Jason Smith [:jsmith]

Comment 20

•

11 years ago

(In reply to Randell Jesup [:jesup] from comment #19)
> This is planned for 28, however this should not block shipping webrtc for
> B2G, just as we shipped desktop and Android without TURN TCP.

Sounds good. Moving to blocking-

blocking-b2g: 1.3? → -

Assignee

Comment 21

•

11 years ago

Attached patch Add support for TURN TCP (obsolete) — Details — Splinter Review

Assignee

Comment 22

•

11 years ago

Comment on attachment 8341714 [details] [diff] [review]
Add support for TURN TCP

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

Adam, here is a new version. Need to re-review myself, but this should be enough for you to get started.

Try at:  https://tbpl.mozilla.org/?tree=Try&rev=24e738605e67

Attachment #8341714 - Flags: review?(adam)

Updated

•

11 years ago

Attachment #826006 - Attachment is obsolete: true

Comment 23

•

11 years ago

Attached patch Add support for TURN TCP (obsolete) — Details — Splinter Review

Updated

•

11 years ago

Attachment #8341714 - Attachment is obsolete: true

Attachment #8341714 - Flags: review?(adam)

Updated

•

11 years ago

Assignee: ekr → adam

Status: NEW → ASSIGNED

Updated

•

11 years ago

Assignee: adam → ekr

Comment 24

•

11 years ago

Comment on attachment 8341908 [details] [diff] [review]
Add support for TURN TCP

Unrotting patch. I didn't fix the compile error of "unused 'r'" and "treat warnings as errors".

Attachment #8341908 - Flags: review?(adam)

Comment 25

•

11 years ago

Attached patch Interdiff between my earlier review and current patch (obsolete) — Details — Splinter Review

The interdiff tool was just having fits, so I dropped an interdiff in here to help with my review. Note that my unrotting of the old patch was sloppy, so the interdiff may be less than 100% accurate for nr_socket_prsock.cpp, nr_socket_prsock.h, and ice_component.c

Comment 26

•

11 years ago

Comment on attachment 8341908 [details] [diff] [review]
Add support for TURN TCP

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

r- for failure handling in nr_ice_socket_readable_cb(). I think there's also a functional flaw in TCP connection timeout handling.

All other comments are stylistic.

Whitespace nits:

buffered_stun_socket_unittest.cpp:53: trailing whitespace
buffered_stun_socket_unittest.cpp:76: trailing whitespace
buffered_stun_socket_unittest.cpp:87: trailing whitespace
buffered_stun_socket_unittest.cpp:106: trailing whitespace
buffered_stun_socket_unittest.cpp:116: trailing whitespace
buffered_stun_socket_unittest.cpp:124: trailing whitespace
buffered_stun_socket_unittest.cpp:138: trailing whitespace
buffered_stun_socket_unittest.cpp:162: trailing whitespace
buffered_stun_socket_unittest.cpp:236: trailing whitespace
buffered_stun_socket_unittest.cpp:238: trailing whitespace
buffered_stun_socket_unittest.cpp:318: trailing whitespace
buffered_stun_socket_unittest.cpp:328: trailing whitespace
buffered_stun_socket_unittest.cpp:345: trailing whitespace
buffered_stun_socket_unittest.cpp:377: trailing whitespace
buffered_stun_socket_unittest.cpp:396: trailing whitespace
buffered_stun_socket_unittest.cpp:416: trailing whitespace
buffered_stun_socket_unittest.cpp:420: trailing whitespace
ice_socket.c:207: trailing whitespace
ice_socket.c:218: trailing whitespace
nr_socket_buffered_stun.c:222: trailing whitespace
nr_socket_buffered_stun.c:241: trailing whitespace
nr_socket_buffered_stun.c:262: trailing whitespace
MediaPipeline.cpp:424: trailing whitespace

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +172,5 @@
>    }
>  
> +
> +
> +

Four blank lines seems rather excessive as a visual break...

::: media/mtransport/third_party/nICEr/src/ice/ice_socket.c
@@ +65,5 @@
>      if(r=nr_socket_recvfrom(sock->sock,buf,sizeof(buf),&len_s,0,&addr)){
> +      if (r != R_WOULDBLOCK && (sock->type != NR_ICE_SOCKET_TYPE_DGRAM)) {
> +        r_log(LOG_ICE,LOG_ERR,"ICE(%s): Error on reliable socket. Abandoning.",sock->ctx->label);
> +        NR_ASYNC_CANCEL(s, NR_ASYNC_WAIT_READ);
> +      }

We need to return here (or set len_s=0) so that we don't fall through to normal processing below with len_s set to garbage.

Also, since we currently don't let the application know about this failure, and don't do anything to revive the socket, we need to either add a callback that lets the application know about the problem; or, minimally, add a "TODO" and open a new bug for informing the application of this condition.

::: media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
@@ +75,5 @@
> +static int nr_socket_buffered_stun_getaddr(void *obj, nr_transport_addr *addrp);
> +static int nr_socket_buffered_stun_close(void *obj);
> +static int nr_socket_buffered_stun_connect(void *sock, nr_transport_addr *addr);
> +static int nr_socket_buffered_stun_write(void *obj,const void *msg, size_t len, size_t *written);
> +static int nr_socket_buffered_stun_read(void *obj,void * restrict buf, size_t maxlen, size_t *len);

Remove the buffered_stun_read forward declaration.

@@ +278,5 @@
> +}
> +
> +static int nr_socket_buffered_stun_close(void *obj)
> +{
> +  /* No-op */

Expand comment to explain why the inner socket is not closed when the buffered socket is closed.

::: media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c
@@ +421,5 @@
> +
> +static void nr_turn_client_connect_timeout_cb(NR_SOCKET s, int how, void *cb_arg)
> +{
> +  nr_turn_client_ctx *ctx = (nr_turn_client_ctx *)cb_arg;
> +  NR_SOCKET fd;

fd doesn't appear to be used at the moment, although I think it should be (see below).

@@ +426,5 @@
> +
> +  r_log(NR_LOG_TURN, LOG_INFO, "TURN(%s): connect timeout", ctx->label);
> +
> +  ctx->connected_timer_handle = 0;
> +

Shouldn't we "NR_ASYNC_CANCEL(fd, NR_ASYNC_WAIT_WRITE);" here also? If the timeout fires and then the connection succeeds, we'll end up transitioning to FAILED (with all of the machinery that entails) followed by a transition to CONNECTED.

I think we want to close the underlying fd as well so that the OS can release its resources now that we've given up.

Attachment #8341908 - Flags: review?(adam) → review-

Comment 27

•

11 years ago

Also, I have some line-length nits in non-nICEr code:

nr_socket_prsock.cpp:1062: Line is 89 characters long; please wrap to 80
nr_socket_prsock.cpp:1063: Line is 91 characters long; please wrap to 80
nr_socket_prsock.cpp:1165: Line is 91 characters long; please wrap to 80
nr_socket_prsock.cpp:1171: Line is 93 characters long; please wrap to 80
turn_unittest.cpp:125: Line is 82 characters long; please wrap to 80

Assignee

Comment 28

•

11 years ago

Comment on attachment 8341936 [details] [diff] [review]
Interdiff between my earlier review and current patch

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

Some nits and one material thing.. (E10S)

::: mozilla-inbound-interdiff-before/media/mtransport/nr_socket_prsock.cpp
@@ +219,5 @@
>    return r;
>  }
>  
>  // Helper functions for addresses
> +int nr_transport_addr_to_praddr(nr_transport_addr *addr,

Does this need to be public?

@@ +589,5 @@
> +  if ((r=nr_transport_addr_to_praddr(addr, &naddr)))
> +    ABORT(r);
> +
> +  if ((r=nr_transport_addr_copy(&remote_addr_, addr)))
> +    ABORT(r);

remote_addr_ seems unused. Can we remove?

@@ +616,5 @@
> +  ASSERT_ON_THREAD(ststhread_);
> +  int _status;
> +  int32_t status;
> +
> +  status = PR_Write(fd_, msg, len);

This should check for connected_

@@ +631,5 @@
> +  return _status;
> +}
> +
> +int NrSocket::read(void* buf, size_t maxlen, size_t *len) {
> +  ASSERT_ON_THREAD(ststhread_);

This should check for connected_

@@ +654,4 @@
>  // NrSocketIpc Implementation
>  NS_IMPL_ISUPPORTS1(NrSocketIpc, nsIUDPSocketInternal)
>  
>  NrSocketIpc::NrSocketIpc(const nsCOMPtr<nsIEventTarget> &main_thread)

We need to disable TURN TCP for E10S.

::: mozilla-inbound-interdiff-before/media/mtransport/nricectx.h
@@ +155,5 @@
>  
>   private:
>    NrIceTurnServer(const std::string& username,
>                    const std::vector<unsigned char>& password,
> +                  const std::string &transport) :

unnecessary

::: mozilla-inbound-interdiff-before/media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +322,5 @@
>          addr.protocol = IPPROTO_TCP;
>          if ((r=nr_transport_addr_fmt_addr_string(&addr)))
>            ABORT(r);
>          if((r=nr_socket_local_create(&addr, &sock))){
> +          r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): couldn't create socket for address %s",ctx->label,addr.as_string);

This seems like it should be WATNING

@@ +340,5 @@
>  
>          /* Attach ourselves to it */
>          if(r=nr_ice_candidate_create(ctx,component,
> +                                     isock,turn_sock,RELAYED,
> +                                     &ctx->turn_servers[j].turn_server,component->component_id,&cand))

Reindent to match nICEr style

::: mozilla-inbound-interdiff-before/media/mtransport/third_party/nICEr/src/ice/ice_socket.c
@@ +68,2 @@
>          NR_ASYNC_CANCEL(s, NR_ASYNC_WAIT_READ);
>        }

This needs a return (adam noted this).

::: mozilla-inbound-interdiff-before/media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c
@@ +427,5 @@
> +  r_log(NR_LOG_TURN, LOG_INFO, "TURN(%s): connect timeout", ctx->label);
> +
> +  ctx->connected_timer_handle = 0;
> +
> +  nr_turn_client_failed(ctx);

Add a comment about cancelling

::: mozilla-inbound-interdiff-before/media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.h
@@ +103,2 @@
>    void *refresh_timer_handle;
> +

Extra whitespace.

Assignee

Comment 29

•

11 years ago

Attached patch Add support for TURN TCP — Details — Splinter Review

Assignee

Comment 30

•

11 years ago

Comment on attachment 8343291 [details] [diff] [review]
Add support for TURN TCP

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

Adam, I haven't double-checked this, but depending on your schedule.

Attachment #8343291 - Flags: review?(adam)

Updated

•

11 years ago

Attachment #8341908 - Attachment is obsolete: true

Updated

•

11 years ago

Attachment #8341936 - Attachment is obsolete: true

Comment 31

•

11 years ago

Attached patch New interdiff for latest changes (obsolete) — Details — Splinter Review

Comment 32

•

11 years ago

Attached patch New interdiff for latest changes, ignoring WS changes — Details — Splinter Review

Attachment #8343373 - Attachment is obsolete: true

Comment 33

•

11 years ago

Comment on attachment 8343291 [details] [diff] [review]
Add support for TURN TCP

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

I think this is ready to ship. Minor nits below; take 'em or leave 'em.

::: media/mtransport/nr_socket_prsock.cpp
@@ +592,5 @@
> +  if(!fd_)
> +    ABORT(R_EOD);
> +
> +  // Note: this just means we tried to connect, not that we
> +  // are actually live.

I might suggest changing the name to "connect_invoked_" or similar, then.

::: media/mtransport/test/buffered_stun_socket_unittest.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-pl */

I think you might have inadvertently added a "pl" to the end of this Mode line.

@@ +5,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Original author: ekr@rtfm.com
> +
> +#include <iostream>

Interesting. I don't find any streaming symbols in this file, nor do I see any stream operators. Are we fixing one of the header files below? If so, would it be preferable to fix the header itself?

Attachment #8343291 - Flags: review?(adam) → review+

Assignee

Comment 34

•

11 years ago

Attached patch Turn off TURN TCP for gonk — Details — Splinter Review

Randell Jesup [:jesup] (needinfo me)

Assignee

Comment 35

•

11 years ago

Comment on attachment 8343779 [details] [diff] [review]
Turn off TURN TCP for gonk

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

Minor technical change after ABR's reveiw.

Attachment #8343779 - Flags: review?(rjesup)

Reporter

Comment 36

•

11 years ago

Comment on attachment 8343779 [details] [diff] [review]
Turn off TURN TCP for gonk

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

r+ with nit

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +646,5 @@
>        NS_ConvertUTF16toUTF8 credential(server.mCredential);
>        NS_ConvertUTF16toUTF8 username(server.mUsername);
>  
> +#ifdef MOZ_WIDGET_GONK
> +      if (transport.get == kNrIceTransportTcp)

.get()

Attachment #8343779 - Flags: review?(rjesup) → review+

Assignee

Comment 37

•

11 years ago

Attached patch Add support for TURN TCP. — Details — Splinter Review

Assignee

Comment 38

•

11 years ago

Comment on attachment 8343851 [details] [diff] [review]
Add support for TURN TCP.

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

carry forward r+ from abr

Attachment #8343851 - Flags: review+