Bug 891551 - Part 2: (Upliftable) Fix bugs where PR_WOULD_BLOCK_ERROR (or, in some cases, PR_NOT_CONNECTED_ERROR while a TCP socket was connecting) would cause sockets to be abandoned for no good reason (see also bug 985493 and 1001671). r=bwc
authorByron Campen [:bwc] <docfaraday@gmail.com>
Fri, 02 May 2014 10:49:00 -0700
changeset 185463 d2afe977d5e220599b4a318cf7146dac91d97fb0
parent 185462 1384a8bb2d456ea58333c9208023a9ef7db52d95
child 185464 f1ff24498536c7e60f05704100cbfaf746917a16
push id26854
push userttaubert@mozilla.com
push dateThu, 29 May 2014 06:33:11 +0000
treeherdermozilla-central@1e712b724d17 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbwc
bugs891551, 985493, 1001671
milestone32.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 891551 - Part 2: (Upliftable) Fix bugs where PR_WOULD_BLOCK_ERROR (or, in some cases, PR_NOT_CONNECTED_ERROR while a TCP socket was connecting) would cause sockets to be abandoned for no good reason (see also bug 985493 and 1001671). r=bwc
media/mtransport/nr_socket_prsock.cpp
media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.h
media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c
media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.h
--- a/media/mtransport/nr_socket_prsock.cpp
+++ b/media/mtransport/nr_socket_prsock.cpp
@@ -579,17 +579,19 @@ int NrSocket::recvfrom(void * buf, size_
                        nr_transport_addr *from) {
   ASSERT_ON_THREAD(ststhread_);
   int r,_status;
   PRNetAddr nfrom;
   int32_t status;
 
   status = PR_RecvFrom(fd_, buf, maxlen, flags, &nfrom, PR_INTERVAL_NO_WAIT);
   if (status <= 0) {
-    r_log(LOG_GENERIC,LOG_ERR,"Error in recvfrom");
+    if (PR_GetError() == PR_WOULD_BLOCK_ERROR)
+      ABORT(R_WOULDBLOCK);
+    r_log(LOG_GENERIC, LOG_INFO, "Error in recvfrom");
     ABORT(R_IO_ERROR);
   }
   *len=status;
 
   if((r=nr_praddr_to_transport_addr(&nfrom,from,my_addr_.protocol,0)))
     ABORT(r);
 
   //r_log(LOG_GENERIC,LOG_DEBUG,"Read %d bytes from %s",*len,addr->as_string);
--- a/media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
+++ b/media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.c
@@ -43,21 +43,22 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE
 #include "nr_socket.h"
 #include "stun.h"
 #include "nr_socket_buffered_stun.h"
 
 
 typedef struct nr_socket_buffered_stun_ {
   nr_socket *inner;
   nr_transport_addr remote_addr;
+  int connected;
 
   /* Read state */
   int read_state;
-#define NR_ICE_SOCKET_READ_NONE 0
-#define NR_ICE_SOCKET_READ_HDR  1
+#define NR_ICE_SOCKET_READ_NONE   0
+#define NR_ICE_SOCKET_READ_HDR    1
 #define NR_ICE_SOCKET_READ_FAILED 2
   UCHAR *buffer;
   size_t buffer_size;
   size_t bytes_needed;
   size_t bytes_read;
 
   /* Write state */
   nr_p_buf_ctx *p_bufs;
@@ -86,35 +87,52 @@ static nr_socket_vtbl nr_socket_buffered
   nr_socket_buffered_stun_getfd,
   nr_socket_buffered_stun_getaddr,
   nr_socket_buffered_stun_connect,
   0,
   0,
   nr_socket_buffered_stun_close
 };
 
+int nr_socket_buffered_set_connected_to(nr_socket *sock, nr_transport_addr *remote_addr)
+{
+  nr_socket_buffered_stun *buf_sock = (nr_socket_buffered_stun *)sock->obj;
+  int r, _status;
+
+  if ((r=nr_transport_addr_copy(&buf_sock->remote_addr, remote_addr)))
+    ABORT(r);
+
+  buf_sock->connected = 1;
+
+  _status=0;
+abort:
+  return(_status);
+}
+
 int nr_socket_buffered_stun_create(nr_socket *inner, int max_pending, nr_socket **sockp)
 {
   int r, _status;
   nr_socket_buffered_stun *sock = 0;
 
   if (!(sock = RCALLOC(sizeof(nr_socket_buffered_stun))))
     ABORT(R_NO_MEMORY);
 
   sock->inner = inner;
 
   if ((r=nr_ip4_port_to_transport_addr(INADDR_ANY, 0, IPPROTO_UDP, &sock->remote_addr)))
     ABORT(r);
 
   /* TODO(ekr@rtfm.com): Check this */
   if (!(sock->buffer = RMALLOC(NR_STUN_MAX_MESSAGE_SIZE)))
     ABORT(R_NO_MEMORY);
+
   sock->read_state = NR_ICE_SOCKET_READ_NONE;
   sock->buffer_size = NR_STUN_MAX_MESSAGE_SIZE;
   sock->bytes_needed = sizeof(nr_stun_message_header);
+  sock->connected = 0;
 
   STAILQ_INIT(&sock->pending_writes);
   if ((r=nr_p_buf_ctx_create(NR_STUN_MAX_MESSAGE_SIZE, &sock->p_bufs)))
     ABORT(r);
   sock->max_pending=max_pending;
 
 
   if ((r=nr_socket_create_int(sock, &nr_socket_buffered_stun_vtbl, sockp)))
@@ -279,30 +297,59 @@ static int nr_socket_buffered_stun_getad
   nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
 
   return nr_socket_getaddr(sock->inner, addrp);
 }
 
 static int nr_socket_buffered_stun_close(void *obj)
 {
   nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
+  NR_SOCKET fd;
+
+  /* Cancel waiting on the socket */
+  if (sock->inner && !nr_socket_getfd(sock->inner, &fd)) {
+    NR_ASYNC_CANCEL(fd, NR_ASYNC_WAIT_WRITE);
+  }
 
   return nr_socket_close(sock->inner);
 }
 
+static void nr_socket_buffered_stun_connected_cb(NR_SOCKET s, int how, void *arg)
+{
+  nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)arg;
+
+  assert(!sock->connected);
+
+  sock->connected = 1;
+  if (sock->pending)
+    nr_socket_buffered_stun_writable_cb(s, how, arg);
+}
+
 static int nr_socket_buffered_stun_connect(void *obj, nr_transport_addr *addr)
 {
   nr_socket_buffered_stun *sock = (nr_socket_buffered_stun *)obj;
   int r, _status;
 
   if ((r=nr_transport_addr_copy(&sock->remote_addr, addr)))
     ABORT(r);
 
-  if ((r=nr_socket_connect(sock->inner, addr)))
+  if ((r=nr_socket_connect(sock->inner, addr))) {
+    if (r == R_WOULDBLOCK) {
+      NR_SOCKET fd;
+
+      if ((r=nr_socket_getfd(sock->inner, &fd)))
+        ABORT(r);
+
+      NR_ASYNC_WAIT(fd, NR_ASYNC_WAIT_WRITE, nr_socket_buffered_stun_connected_cb, sock);
+      ABORT(R_WOULDBLOCK);
+    }
     ABORT(r);
+  } else {
+    sock->connected = 1;
+  }
 
   _status=0;
 abort:
   return(_status);
 }
 
 static int nr_socket_buffered_stun_write(void *obj,const void *msg, size_t len, size_t *written)
 {
@@ -315,17 +362,17 @@ static int nr_socket_buffered_stun_write
   /* Buffers are close to full, report error. Do this now so we never
      get partial writes */
   if ((sock->pending + len) > sock->max_pending) {
     r_log(LOG_GENERIC, LOG_INFO, "Write buffer for %s full", sock->remote_addr.as_string);
     ABORT(R_WOULDBLOCK);
   }
 
 
-  if (!sock->pending) {
+  if (sock->connected && !sock->pending) {
     r = nr_socket_write(sock->inner, msg, len, &written2, 0);
     if (r) {
       if (r != R_WOULDBLOCK)
         ABORT(r);
 
       written2=0;
     }
   } else {
--- a/media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.h
+++ b/media/mtransport/third_party/nICEr/src/stun/nr_socket_buffered_stun.h
@@ -43,10 +43,13 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE
    1. Writes don't block and are automatically flushed when needed.
    2. All reads are in units of STUN messages
 
    This socket takes ownership of the inner socket |sock|.
  */
 int nr_socket_buffered_stun_create(nr_socket *inner, int max_pending,
   nr_socket **sockp);
 
+int nr_socket_buffered_set_connected_to(nr_socket *sock,
+    nr_transport_addr *remote_addr);
+
 #endif
 
--- a/media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c
+++ b/media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.c
@@ -57,27 +57,23 @@ int NR_LOG_TURN = 0;
 #define TURN_LIFETIME_REQUEST_SECONDS    3600  /* One hour */
 #define TURN_USECS_PER_S                 1000000
 #define TURN_REFRESH_SLACK_SECONDS       10    /* How long before expiry to refresh
                                                   allocations/permissions. The RFC 5766
                                                   Section 7 recommendation if 60 seconds,
                                                   but this is silly since the transaction
                                                   times out after about 5. */
 #define TURN_PERMISSION_LIFETIME_SECONDS 300   /* 5 minutes. From RFC 5766 2.3 */
-#define TURN_CONNECT_TIMEOUT 1500  /* 1.5 seconds */
 
 static int nr_turn_stun_ctx_create(nr_turn_client_ctx *tctx, int type,
                                    NR_async_cb success_cb,
                                    NR_async_cb failure_cb,
                                    nr_turn_stun_ctx **ctxp);
 static int nr_turn_stun_ctx_destroy(nr_turn_stun_ctx **ctxp);
 static void nr_turn_stun_ctx_cb(NR_SOCKET s, int how, void *arg);
-static int nr_turn_client_connect(nr_turn_client_ctx *ctx);
-static void nr_turn_client_connect_timeout_cb(NR_SOCKET s, int how, void *cb_arg);
-static void nr_turn_client_connected_cb(NR_SOCKET s, int how, void *cb_arg);
 static int nr_turn_stun_set_auth_params(nr_turn_stun_ctx *ctx,
                                         char *realm, char *nonce);
 static void nr_turn_client_refresh_timer_cb(NR_SOCKET s, int how, void *arg);
 static int nr_turn_client_refresh_setup(nr_turn_client_ctx *ctx,
                                         nr_turn_stun_ctx **sctx);
 static int nr_turn_client_failed(nr_turn_client_ctx *ctx);
 static int nr_turn_client_start_refresh_timer(nr_turn_client_ctx *ctx,
                                               nr_turn_stun_ctx *sctx,
@@ -332,24 +328,19 @@ int nr_turn_client_ctx_create(const char
   if (!ctx->username)
     ABORT(R_NO_MEMORY);
 
   if ((r=r_data_create(&ctx->password, password->data, password->len)))
     ABORT(r);
   if ((r=nr_transport_addr_copy(&ctx->turn_server_addr, addr)))
     ABORT(r);
 
-  if (addr->protocol == IPPROTO_UDP) {
-    ctx->state = NR_TURN_CLIENT_STATE_CONNECTED;
-  }
-  else {
-    assert(addr->protocol == IPPROTO_TCP);
-    ctx->state = NR_TURN_CLIENT_STATE_INITTED;
-
-    if (r=nr_turn_client_connect(ctx)) {
+  ctx->state = NR_TURN_CLIENT_STATE_INITTED;
+  if (addr->protocol == IPPROTO_TCP) {
+    if ((r=nr_socket_connect(ctx->sock, &ctx->turn_server_addr))) {
       if (r != R_WOULDBLOCK)
         ABORT(r);
     }
   }
 
   *ctxp=ctx;
 
   _status=0;
@@ -402,115 +393,27 @@ nr_turn_client_ctx_destroy(nr_turn_clien
     nr_turn_permission_destroy(&perm);
   }
 
   RFREE(ctx);
 
   return(0);
 }
 
-static int nr_turn_client_connect(nr_turn_client_ctx *ctx)
-{
-  int r,_status;
-
-  if (ctx->turn_server_addr.protocol != IPPROTO_TCP)
-    ABORT(R_INTERNAL);
-
-  r = nr_socket_connect(ctx->sock, &ctx->turn_server_addr);
-
-  if (r == R_WOULDBLOCK) {
-    NR_SOCKET fd;
-
-    if (r=nr_socket_getfd(ctx->sock, &fd))
-      ABORT(r);
-
-    NR_ASYNC_WAIT(fd, NR_ASYNC_WAIT_WRITE, nr_turn_client_connected_cb, ctx);
-    NR_ASYNC_TIMER_SET(TURN_CONNECT_TIMEOUT,
-                       nr_turn_client_connect_timeout_cb, ctx,
-                       &ctx->connected_timer_handle);
-    ABORT(R_WOULDBLOCK);
-  }
-  if (r) {
-    ABORT(R_IO_ERROR);
-  }
-
-  ctx->state = NR_TURN_CLIENT_STATE_CONNECTED;
-
-  _status = 0;
-abort:
-  return(_status);
-}
-
-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;
-
-  r_log(NR_LOG_TURN, LOG_INFO, "TURN(%s): connect timeout", ctx->label);
-
-  ctx->connected_timer_handle = 0;
-
-  /* This also cancels waiting on the file descriptor */
-  nr_turn_client_failed(ctx);
-}
-
-static void nr_turn_client_connected_cb(NR_SOCKET s, int how, void *cb_arg)
-{
-  int r, _status;
-  nr_turn_client_ctx *ctx = (nr_turn_client_ctx *)cb_arg;
-
-  /* Cancel the connection failure timer */
-  NR_async_timer_cancel(ctx->connected_timer_handle);
-  ctx->connected_timer_handle=0;
-
-  /* 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;
-  }
-  else {
-    ctx->state = NR_TURN_CLIENT_STATE_CONNECTED;
-  }
-
-  _status = 0;
-abort:
-  if (_status) {
-    nr_turn_client_failed(ctx);
-  }
-}
-
 int nr_turn_client_cancel(nr_turn_client_ctx *ctx)
 {
   nr_turn_stun_ctx *stun = 0;
 
   if (ctx->state == NR_TURN_CLIENT_STATE_CANCELLED ||
       ctx->state == NR_TURN_CLIENT_STATE_FAILED)
     return 0;
 
   if (ctx->label)
     r_log(NR_LOG_TURN, LOG_INFO, "TURN(%s): cancelling", ctx->label);
 
-  /* If we are waiting for connect, we need to stop
-     waiting for writability */
-  if (ctx->state == NR_TURN_CLIENT_STATE_INITTED ||
-      ctx->state == NR_TURN_CLIENT_STATE_ALLOCATION_WAIT) {
-    NR_SOCKET fd;
-    int r;
-
-    r = nr_socket_getfd(ctx->sock, &fd);
-    if (r) {
-      /* We should be able to get the fd, but if we get an
-         error, we shouldn't cancel. */
-      r_log(NR_LOG_TURN, LOG_ERR, "TURN: Couldn't get internal fd");
-    }
-    else {
-      NR_ASYNC_CANCEL(fd, NR_ASYNC_WAIT_WRITE);
-    }
-  }
-
   /* Cancel the STUN client ctxs */
   stun = STAILQ_FIRST(&ctx->stun_ctxs);
   while (stun) {
     nr_stun_client_cancel(stun->stun);
     stun = STAILQ_NEXT(stun, entry);
   }
 
   /* Cancel the timers, if not already cancelled */
@@ -624,41 +527,34 @@ static void nr_turn_client_error_cb(NR_S
 }
 
 int nr_turn_client_allocate(nr_turn_client_ctx *ctx,
                             NR_async_cb finished_cb, void *cb_arg)
 {
   nr_turn_stun_ctx *stun = 0;
   int r,_status;
 
-  assert(ctx->state == NR_TURN_CLIENT_STATE_INITTED ||
-         ctx->state == NR_TURN_CLIENT_STATE_CONNECTED);
+  assert(ctx->state == NR_TURN_CLIENT_STATE_INITTED);
 
   ctx->finished_cb=finished_cb;
   ctx->cb_arg=cb_arg;
 
   if ((r=nr_turn_stun_ctx_create(ctx, NR_TURN_CLIENT_MODE_ALLOCATE_REQUEST,
                                  nr_turn_client_allocate_cb,
                                  nr_turn_client_error_cb,
                                  &stun)))
     ABORT(r);
   stun->stun->params.allocate_request.lifetime_secs =
       TURN_LIFETIME_REQUEST_SECONDS;
 
-  switch(ctx->state) {
-    case NR_TURN_CLIENT_STATE_INITTED:
-      /* We are waiting for connect before we can allocate */
-      ctx->state = NR_TURN_CLIENT_STATE_ALLOCATION_WAIT;
-      break;
-    case NR_TURN_CLIENT_STATE_CONNECTED:
+  if (ctx->state == NR_TURN_CLIENT_STATE_INITTED) {
       if ((r=nr_turn_stun_ctx_start(stun)))
         ABORT(r);
       ctx->state = NR_TURN_CLIENT_STATE_ALLOCATING;
-      break;
-    default:
+  } else {
       ABORT(R_ALREADY);
   }
 
   _status=0;
 abort:
   if (_status) {
     nr_turn_client_failed(ctx);
   }
--- a/media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.h
+++ b/media/mtransport/third_party/nICEr/src/stun/turn_client_ctx.h
@@ -69,22 +69,20 @@ typedef struct nr_turn_permission_ {
 typedef STAILQ_HEAD(nr_turn_permission_head_, nr_turn_permission_)
     nr_turn_permission_head;
 
 /* A single connection to a TURN server. Use one
    turn_client_ctx per socket/server pair. */
 typedef struct nr_turn_client_ctx_ {
   int state;
 #define NR_TURN_CLIENT_STATE_INITTED         1
-#define NR_TURN_CLIENT_STATE_CONNECTED       2
-#define NR_TURN_CLIENT_STATE_ALLOCATION_WAIT 3
-#define NR_TURN_CLIENT_STATE_ALLOCATING      4
-#define NR_TURN_CLIENT_STATE_ALLOCATED       5
-#define NR_TURN_CLIENT_STATE_FAILED          6
-#define NR_TURN_CLIENT_STATE_CANCELLED       7
+#define NR_TURN_CLIENT_STATE_ALLOCATING      2
+#define NR_TURN_CLIENT_STATE_ALLOCATED       3
+#define NR_TURN_CLIENT_STATE_FAILED          4
+#define NR_TURN_CLIENT_STATE_CANCELLED       5
 
   char *label;
   nr_socket *sock;
 
   char *username;
   Data *password;
   char *nonce;
   char *realm;