Bug 1298991: only create candidate pairs from STUN response when needed. r=bwc draft
authorNils Ohlmeier [:drno] <drno@ohlmeier.org>
Mon, 29 Aug 2016 16:34:22 -0700
changeset 407741 8bde1830fa0a37205f647a4d594583e4641d041a
parent 406838 acfb2c3ac6ae0a704e2756184815296ac1314f89
child 529944 3dcc1cd8302543186c9deeca024853c8fd393eee
push id28038
push userdrno@ohlmeier.org
push dateTue, 30 Aug 2016 23:47:54 +0000
reviewersbwc
bugs1298991
milestone51.0a1
Bug 1298991: only create candidate pairs from STUN response when needed. r=bwc MozReview-Commit-ID: KpqNyi0FVb3
media/mtransport/test/ice_unittest.cpp
media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
media/mtransport/third_party/nICEr/src/ice/ice_media_stream.h
--- a/media/mtransport/test/ice_unittest.cpp
+++ b/media/mtransport/test/ice_unittest.cpp
@@ -758,17 +758,17 @@ class IceTestPeer : public sigslot::has_
         RefPtr<NrIceMediaStream> aStream = ice_ctx_->ctx()->GetStream(i);
         if (!aStream || aStream->HasParsedAttributes()) {
           continue;
         }
         std::vector<std::string> candidates =
             remote->GetCandidates(i);
 
         for (size_t j=0; j<candidates.size(); ++j) {
-          std::cerr << name_ << " Candidate: " + candidates[j] << std::endl;
+          std::cerr << name_ << " Adding remote candidate: " + candidates[j] << std::endl;
         }
         res = aStream->ParseAttributes(candidates);
         ASSERT_TRUE(NS_SUCCEEDED(res));
       }
     } else {
       // Parse empty attributes and then trickle them out later
       for (size_t i=0; i<ice_ctx_->ctx()->GetStreamCount(); ++i) {
         RefPtr<NrIceMediaStream> aStream = ice_ctx_->ctx()->GetStream(i);
--- a/media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
+++ b/media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
@@ -176,17 +176,18 @@ int nr_ice_candidate_pair_unfreeze(nr_ic
     nr_ice_candidate_pair_set_state(pctx,pair,NR_ICE_PAIR_STATE_WAITING);
 
     return(0);
   }
 
 static void nr_ice_candidate_pair_stun_cb(NR_SOCKET s, int how, void *cb_arg)
   {
     int r,_status;
-    nr_ice_cand_pair *pair=cb_arg,*orig_pair;
+    nr_ice_cand_pair *pair=cb_arg;
+    nr_ice_cand_pair *actual_pair=0;
     nr_ice_candidate *cand=0;
     nr_stun_message *sres;
     nr_transport_addr *request_src;
     nr_transport_addr *request_dst;
     nr_transport_addr *response_src;
     nr_transport_addr response_dst;
     nr_stun_message_attribute *attr;
 
@@ -251,58 +252,65 @@ static void nr_ice_candidate_pair_stun_c
           nr_ice_candidate_pair_set_state(pair->pctx,pair,NR_ICE_PAIR_STATE_SUCCEEDED);
         }
         else if(pair->stun_client->state == NR_STUN_CLIENT_STATE_DONE) {
           /* OK, this didn't correspond to a pair on the check list, but
              it probably matches one of our candidates */
 
           cand=TAILQ_FIRST(&pair->local->component->candidates);
           while(cand){
-            if(!nr_transport_addr_cmp(&cand->addr,&pair->stun_client->results.ice_binding_response.mapped_addr,NR_TRANSPORT_ADDR_CMP_MODE_ALL))
+            if(!nr_transport_addr_cmp(&cand->addr,&pair->stun_client->results.ice_binding_response.mapped_addr,NR_TRANSPORT_ADDR_CMP_MODE_ALL)) {
+              r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): found pre-existing local candidate of type %d for mapped address %s", pair->pctx->label,cand->type,cand->addr.as_string);
+              assert(cand->type != HOST);
               break;
+            }
 
             cand=TAILQ_NEXT(cand,entry_comp);
           }
 
-          /* OK, nothing found, must be peer reflexive */
           if(!cand) {
+            /* OK, nothing found, must be a new peer reflexive */
             if (pair->pctx->ctx->flags & NR_ICE_CTX_FLAGS_RELAY_ONLY) {
               /* Any STUN response with a reflexive address in it is unwanted
                  when we'll send on relay only. Bail since cand is used below. */
               goto done;
             }
             if(r=nr_ice_candidate_create(pair->pctx->ctx,
               pair->local->component,pair->local->isock,pair->local->osock,
               PEER_REFLEXIVE,pair->local->tcp_type,0,pair->local->component->component_id,&cand))
               ABORT(r);
             if(r=nr_transport_addr_copy(&cand->addr,&pair->stun_client->results.ice_binding_response.mapped_addr))
               ABORT(r);
             cand->state=NR_ICE_CAND_STATE_INITIALIZED;
             TAILQ_INSERT_TAIL(&pair->local->component->candidates,cand,entry_comp);
+          } else {
+            /* Check if we have a pair for this candidate already. */
+            if(r=nr_ice_media_stream_find_pair(pair->remote->stream, cand, pair->remote, &actual_pair)) {
+              r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): no pair exists for %s and %s", pair->pctx->label,cand->addr.as_string, pair->remote->addr.as_string);
+            }
           }
 
-          /* Note: we stomp the existing pair! */
-          orig_pair=pair;
-          if(r=nr_ice_candidate_pair_create(pair->pctx,cand,pair->remote,
-            &pair))
-            ABORT(r);
+          if(!actual_pair) {
+            if(r=nr_ice_candidate_pair_create(pair->pctx,cand,pair->remote, &actual_pair))
+              ABORT(r);
 
-          nr_ice_candidate_pair_set_state(pair->pctx,pair,NR_ICE_PAIR_STATE_SUCCEEDED);
+            if(r=nr_ice_component_insert_pair(actual_pair->remote->component,actual_pair))
+              ABORT(r);
 
-          if(r=nr_ice_component_insert_pair(pair->remote->component,pair))
-            ABORT(r);
+            /* If the original pair was nominated, make us nominated too. */
+            if(pair->peer_nominated)
+              actual_pair->peer_nominated=1;
 
-          /* If the original pair was nominated, make us nominated,
-             since we replace him*/
-          if(orig_pair->peer_nominated)
-            pair->peer_nominated=1;
+            /* Now mark the orig pair failed */
+            nr_ice_candidate_pair_set_state(pair->pctx,pair,NR_ICE_PAIR_STATE_FAILED);
+          }
 
-
-          /* Now mark the orig pair failed */
-          nr_ice_candidate_pair_set_state(orig_pair->pctx,orig_pair,NR_ICE_PAIR_STATE_FAILED);
+          assert(actual_pair);
+          nr_ice_candidate_pair_set_state(actual_pair->pctx,actual_pair,NR_ICE_PAIR_STATE_SUCCEEDED);
+          pair=actual_pair;
 
         }
 
         /* Should we set nominated? */
         if(pair->pctx->controlling){
           if(pair->pctx->ctx->flags & NR_ICE_CTX_FLAGS_AGGRESSIVE_NOMINATION)
             pair->nominated=1;
         }
--- a/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
+++ b/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ -908,8 +908,26 @@ void nr_ice_media_stream_role_change(nr_
     /* Re-insert into the check list */
     TAILQ_FOREACH_SAFE(pair,&old_checklist,check_queue_entry,temp_pair) {
       TAILQ_REMOVE(&old_checklist,pair,check_queue_entry);
       nr_ice_candidate_pair_role_change(pair);
       nr_ice_candidate_pair_insert(&stream->check_list,pair);
     }
   }
 
+int nr_ice_media_stream_find_pair(nr_ice_media_stream *str, nr_ice_candidate *lcand, nr_ice_candidate *rcand, nr_ice_cand_pair **pair)
+  {
+    nr_ice_cand_pair_head *head = &str->check_list;
+    nr_ice_cand_pair *c1;
+
+    c1=TAILQ_FIRST(head);
+    while(c1){
+      if(c1->local == lcand &&
+         c1->remote == rcand) {
+        *pair=c1;
+        return(0);
+      }
+
+      c1=TAILQ_NEXT(c1,check_queue_entry);
+    }
+
+    return(R_NOT_FOUND);
+  }
--- a/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.h
+++ b/media/mtransport/third_party/nICEr/src/ice/ice_media_stream.h
@@ -87,16 +87,17 @@ int nr_ice_media_stream_unfreeze_pairs_f
 int nr_ice_media_stream_dump_state(nr_ice_peer_ctx *pctx, nr_ice_media_stream *stream,FILE *out);
 int nr_ice_media_stream_component_nominated(nr_ice_media_stream *stream,nr_ice_component *component);
 int nr_ice_media_stream_component_failed(nr_ice_media_stream *stream,nr_ice_component *component);
 int nr_ice_media_stream_set_state(nr_ice_media_stream *str, int state);
 int nr_ice_media_stream_get_best_candidate(nr_ice_media_stream *str, int component, nr_ice_candidate **candp);
 int nr_ice_media_stream_send(nr_ice_peer_ctx *pctx, nr_ice_media_stream *str, int component, UCHAR *data, int len);
 int nr_ice_media_stream_get_active(nr_ice_peer_ctx *pctx, nr_ice_media_stream *str, int component, nr_ice_candidate **local, nr_ice_candidate **remote);
 int nr_ice_media_stream_find_component(nr_ice_media_stream *str, int comp_id, nr_ice_component **compp);
+int nr_ice_media_stream_find_pair(nr_ice_media_stream *str, nr_ice_candidate *local, nr_ice_candidate *remote, nr_ice_cand_pair **pair);
 int nr_ice_media_stream_addrs(nr_ice_peer_ctx *pctx, nr_ice_media_stream *str, int component, nr_transport_addr *local, nr_transport_addr *remote);
 int
 nr_ice_peer_ctx_parse_media_stream_attribute(nr_ice_peer_ctx *pctx, nr_ice_media_stream *stream, char *attr);
 int nr_ice_media_stream_get_consent_status(nr_ice_media_stream *stream, int component_id, int *can_send, struct timeval *ts);
 int nr_ice_media_stream_disable_component(nr_ice_media_stream *stream, int component_id);
 int nr_ice_media_stream_pair_new_trickle_candidate(nr_ice_peer_ctx *pctx, nr_ice_media_stream *pstream, nr_ice_candidate *cand);
 void nr_ice_media_stream_role_change(nr_ice_media_stream *stream);