Bug 822159 - Fix trickle ICE to start checking when new candidates come in. r=abr

media/mtransport/nricemediastream.cpp

@@ -146,11 +146,6 @@ nsresult NrIceMediaStream::ParseTrickleCandidate(const std::string& candidate) {
     }
   }
 
-  if (ctx_->state() == NrIceCtx::ICE_CTX_GATHERED) {
-    // Try to start checks if they are not already started
-    return ctx_->StartChecks();
-  }
-
   return NS_OK;
 }
 

media/mtransport/test/ice_unittest.cpp

@@ -51,7 +51,8 @@ class IceTestPeer : public sigslot::has_slots<> {
       ready_ct_(0),
       ice_complete_(false),
       received_(0),
-      sent_(0) {
+      sent_(0),
+      remote_(nullptr) {
     ice_ctx_->SignalGatheringCompleted.connect(this,
                                               &IceTestPeer::GatheringComplete);
     ice_ctx_->SignalCompleted.connect(this, &IceTestPeer::IceCompleted);
@@ -92,6 +93,9 @@ class IceTestPeer : public sigslot::has_slots<> {
 
   bool gathering_complete() { return gathering_complete_; }
   int ready_ct() { return ready_ct_; }
+  bool is_ready(size_t stream) {
+    return streams_[stream]->state() == NrIceMediaStream::ICE_OPEN;
+  }
   bool ice_complete() { return ice_complete_; }
   size_t received() { return received_; }
   size_t sent() { return sent_; }
@@ -100,6 +104,8 @@ class IceTestPeer : public sigslot::has_slots<> {
   void Connect(IceTestPeer *remote, TrickleMode trickle_mode, bool start = true) {
     nsresult res;
 
+    remote_ = remote;
+
     test_utils->sts_target()->Dispatch(
       WrapRunnableRet(ice_ctx_,
         &NrIceCtx::ParseGlobalAttributes, remote->GetGlobalAttributes(), &res),
@@ -135,25 +141,29 @@ class IceTestPeer : public sigslot::has_slots<> {
         NS_DISPATCH_SYNC);
       ASSERT_TRUE(NS_SUCCEEDED(res));
     }
+  }
 
-    if (trickle_mode == TRICKLE_DEFERRED) {
+  void DoTrickle(size_t stream) {
+    std::cerr << "Doing trickle for stream " << stream << std::endl;
     // If we are in trickle deferred mode, now trickle in the candidates
-      // after ICE has started
-      for (size_t i=0; i<streams_.size(); ++i) {
+    // for |stream}
+    nsresult res;
+
+    ASSERT_GT(remote_->streams_.size(), stream);
+
     std::vector<std::string> candidates =
-            remote->GetCandidates(remote->streams_[i]->name());
+      remote_->GetCandidates(remote_->streams_[stream]->name());
 
     for (size_t j=0; j<candidates.size(); j++) {
       test_utils->sts_target()->Dispatch(
-              WrapRunnableRet(streams_[i], &NrIceMediaStream::ParseTrickleCandidate,
+        WrapRunnableRet(streams_[stream],
+                        &NrIceMediaStream::ParseTrickleCandidate,
                         candidates[j],
                         &res), NS_DISPATCH_SYNC);
 
       ASSERT_TRUE(NS_SUCCEEDED(res));
     }
   }
-    }
-  }
 
   void Close() {
     ice_ctx_->destroy_peer_ctx();
@@ -213,6 +223,7 @@ class IceTestPeer : public sigslot::has_slots<> {
   bool ice_complete_;
   size_t received_;
   size_t sent_;
+  IceTestPeer *remote_;
 };
 
 class IceTest : public ::testing::Test {
@@ -254,14 +265,29 @@ class IceTest : public ::testing::Test {
     return true;
   }
 
-  void Connect(TrickleMode trickle_mode = TRICKLE_NONE) {
-    p1_->Connect(p2_, trickle_mode);
-    p2_->Connect(p1_, trickle_mode);
+  void Connect() {
+    p1_->Connect(p2_, TRICKLE_NONE);
+    p2_->Connect(p1_, TRICKLE_NONE);
 
     ASSERT_TRUE_WAIT(p1_->ready_ct() == 1 && p2_->ready_ct() == 1, 5000);
     ASSERT_TRUE_WAIT(p1_->ice_complete() && p2_->ice_complete(), 5000);
   }
 
+  void ConnectTrickle() {
+    p1_->Connect(p2_, TRICKLE_DEFERRED);
+    p2_->Connect(p1_, TRICKLE_DEFERRED);
+  }
+
+  void DoTrickle(size_t stream) {
+    p1_->DoTrickle(stream);
+    p2_->DoTrickle(stream);
+    ASSERT_TRUE_WAIT(p1_->is_ready(stream), 5000);
+    ASSERT_TRUE_WAIT(p2_->is_ready(stream), 5000);
+  }
+
+  void VerifyConnected() {
+  }
+
   void CloseP1() {
     p1_->Close();
   }
@@ -320,12 +346,27 @@ TEST_F(IceTest, TestConnectAutoPrioritize) {
   Connect();
 }
 
-TEST_F(IceTest, TestConnectTrickle) {
+TEST_F(IceTest, TestConnectTrickleOneStreamOneComponent) {
   AddStream("first", 1);
   ASSERT_TRUE(Gather(true));
-  Connect(TRICKLE_DEFERRED);
+  ConnectTrickle();
+  DoTrickle(0);
+  ASSERT_TRUE_WAIT(p1_->ice_complete(), 1000);
+  ASSERT_TRUE_WAIT(p2_->ice_complete(), 1000);
 }
 
+TEST_F(IceTest, TestConnectTrickleTwoStreamsOneComponent) {
+  AddStream("first", 1);
+  AddStream("second", 1);
+  ASSERT_TRUE(Gather(true));
+  ConnectTrickle();
+  DoTrickle(0);
+  DoTrickle(1);
+  ASSERT_TRUE_WAIT(p1_->ice_complete(), 1000);
+  ASSERT_TRUE_WAIT(p2_->ice_complete(), 1000);
+}
+
+
 TEST_F(IceTest, TestSendReceive) {
   AddStream("first", 1);
   ASSERT_TRUE(Gather(true));

media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c

@@ -333,8 +333,10 @@ static void nr_ice_media_stream_check_timer_cb(NR_SOCKET s, int h, void *cb_arg)
       nr_ice_candidate_pair_start(pair->pctx,pair); /* Ignore failures */
       NR_ASYNC_TIMER_SET(timer_val,nr_ice_media_stream_check_timer_cb,cb_arg,&stream->timer);
     }
-    /* TODO(ekr@rtfm.com): Report on the special case where there are no checks to
-       run at all */
+    else {
+      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): no pairs for %s",stream->pctx->label,stream->label);
+    }
+
     _status=0;
   abort:
     return;
@@ -344,7 +346,6 @@ static void nr_ice_media_stream_check_timer_cb(NR_SOCKET s, int h, void *cb_arg)
 /* Start checks for this media stream (aka check list) */
 int nr_ice_media_stream_start_checks(nr_ice_peer_ctx *pctx, nr_ice_media_stream *stream)
   {
-    assert(stream->ice_state==NR_ICE_MEDIA_STREAM_CHECKS_FROZEN);
     nr_ice_media_stream_set_state(stream,NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE);
     nr_ice_media_stream_check_timer_cb(0,0,stream);
 
@@ -500,18 +501,23 @@ int nr_ice_media_stream_dump_state(nr_ice_peer_ctx *pctx, nr_ice_media_stream *s
 
 int nr_ice_media_stream_set_state(nr_ice_media_stream *str, int state)
   {
-    // removed, per EKR:  assert(state!=str->ice_state);
+    /* Make no-change a no-op */
+    if (state == str->ice_state)
+      return 0;
 
     r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): stream %s state %s->%s",
       str->pctx->label,str->label,
       nr_ice_media_stream_states[str->ice_state],
       nr_ice_media_stream_states[state]);
     
-    if(str->ice_state != NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE)
+    if(state == NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE)
       str->pctx->active_streams++;
     if(str->ice_state == NR_ICE_MEDIA_STREAM_CHECKS_ACTIVE)
       str->pctx->active_streams--;
 
+    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s): %d active streams",
+      str->pctx->label, str->pctx->active_streams);
+
     str->ice_state=state;
     
     return(0);

media/mtransport/third_party/nICEr/src/ice/ice_peer_ctx.c

@@ -247,6 +247,26 @@ int nr_ice_peer_ctx_parse_trickle_candidate(nr_ice_peer_ctx *pctx, nr_ice_media_
       }
     }
 
+    /* Start checks if this stream is not checking yet or if it has checked
+       all the available candidates but not had a completed check for all
+       components.
+
+       Note that this is not compliant with RFC 5245, but consistent with
+       the libjingle trickle ICE behavior. Note that we will not restart
+       checks if either (a) the stream has failed or (b) all components
+       have a successful pair because the switch statement above jumps
+       will in both states.
+
+       TODO(ekr@rtfm.com): restart checks.
+       TODO(ekr@rtfm.com): update when the trickle ICE RFC is published
+    */
+    if (!pstream->timer) {
+      if(r=nr_ice_media_stream_start_checks(pctx, pstream)) {
+        r_log(LOG_ICE,LOG_ERR,"ICE(%s): peer (%s), stream(%s) failed to start checks",pctx->ctx->label,pctx->label,stream->label);
+        ABORT(r);
+      }
+    }
+
     _status=0;
  abort:
     return(_status);