Remove candidates when doing continual gathering

webrtc/p2p/base/p2ptransportchannel_unittest.cc

 /*
  *  Copyright 2009 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 80 matching lines @@
                                    "TESTICEUFRAG0002", "TESTICEUFRAG0003"};
 // Based on ICE_PWD_LENGTH
 static const char* kIcePwd[4] = {"TESTICEPWD00000000000000",
                                  "TESTICEPWD00000000000001",
                                  "TESTICEPWD00000000000002",
                                  "TESTICEPWD00000000000003"};
 
 static const uint64_t kTiebreaker1 = 11111;
 static const uint64_t kTiebreaker2 = 22222;
 
-enum {
-  MSG_CANDIDATE
-};
+enum { MSG_CANDIDATE, MSG_CANDIDATE_REMOVALS };

[pthatcher1, 2016/03/09 18:00:53]

Can you call these MSG_ADD_CANDIDATES and MSG_REMOVE_CANDIDATES?

[honghaiz3, 2016/03/10 02:34:16]

Done.

 
 static cricket::IceConfig CreateIceConfig(int receiving_timeout,
                                           bool gather_continually,
                                           int backup_ping_interval = -1) {
   cricket::IceConfig config;
   config.receiving_timeout = receiving_timeout;
   config.gather_continually = gather_continually;
   config.backup_connection_ping_interval = backup_ping_interval;
   return config;
 }
@@ skipping 95 matching lines @@
         ch_packets_.pop_front();
       }
       return ret;
     }
 
     std::string name_;  // TODO - Currently not used.
     std::list<std::string> ch_packets_;
     rtc::scoped_ptr<cricket::P2PTransportChannel> ch_;
   };
 
-  struct CandidateData : public rtc::MessageData {
-    CandidateData(cricket::TransportChannel* ch, const cricket::Candidate& c)
-        : channel(ch), candidate(c) {
-    }
+  struct CandidatesData : public rtc::MessageData {
+    CandidatesData(cricket::TransportChannel* ch, const cricket::Candidate& c)
+        : channel(ch), candidates(1, c) {}
+    CandidatesData(cricket::TransportChannel* ch,
+                   const std::vector<cricket::Candidate>& cc)
+        : channel(ch), candidates(cc) {}
     cricket::TransportChannel* channel;
-    cricket::Candidate candidate;
+    cricket::Candidates candidates;
   };
 
   struct Endpoint {
     Endpoint()
         : role_(cricket::ICEROLE_UNKNOWN),
           tiebreaker_(0),
           role_conflict_(false),
           save_candidates_(false) {}
     bool HasChannel(cricket::TransportChannel* ch) {
       return (ch == cd1_.ch_.get() || ch == cd2_.ch_.get());
@@ skipping 20 matching lines @@
     }
 
     rtc::FakeNetworkManager network_manager_;
     rtc::scoped_ptr<cricket::BasicPortAllocator> allocator_;
     ChannelData cd1_;
     ChannelData cd2_;
     cricket::IceRole role_;
     uint64_t tiebreaker_;
     bool role_conflict_;
     bool save_candidates_;
-    std::vector<CandidateData*> saved_candidates_;
+    std::vector<CandidatesData*> saved_candidates_;
   };
 
   ChannelData* GetChannelData(cricket::TransportChannel* channel) {
     if (ep1_.HasChannel(channel))
       return ep1_.GetChannelData(channel);
     else
       return ep2_.GetChannelData(channel);
   }
 
   void CreateChannels(int num) {
@@ skipping 27 matching lines @@
   cricket::P2PTransportChannel* CreateChannel(
       int endpoint,
       int component,
       const std::string& local_ice_ufrag,
       const std::string& local_ice_pwd,
       const std::string& remote_ice_ufrag,
       const std::string& remote_ice_pwd) {
     cricket::P2PTransportChannel* channel = new cricket::P2PTransportChannel(
         "test content name", component, GetAllocator(endpoint));
     channel->SignalCandidateGathered.connect(
         this, &P2PTransportChannelTestBase::OnCandidate);

[pthatcher1, 2016/03/09 18:00:53]

Can you rename this OnCandidateGathered?

[honghaiz3, 2016/03/10 02:34:16]

Done.

+    channel->SignalCandidatesRemoved.connect(
+        this, &P2PTransportChannelTestBase::OnCandidatesRemoved);
     channel->SignalReadPacket.connect(
         this, &P2PTransportChannelTestBase::OnReadPacket);
     channel->SignalRoleConflict.connect(
         this, &P2PTransportChannelTestBase::OnRoleConflict);
     channel->SetIceCredentials(local_ice_ufrag, local_ice_pwd);
     if (clear_remote_candidates_ufrag_pwd_) {
       // This only needs to be set if we're clearing them from the
       // candidates.  Some unit tests rely on this not being set.
       channel->SetRemoteIceCredentials(remote_ice_ufrag, remote_ice_pwd);
     }
@@ skipping 320 matching lines @@
     TestSendRecv(1);
   }
 
   // We pass the candidates directly to the other side.
   void OnCandidate(cricket::TransportChannelImpl* ch,
                    const cricket::Candidate& c) {
     if (force_relay_ && c.type() != cricket::RELAY_PORT_TYPE)
       return;
 
     if (GetEndpoint(ch)->save_candidates_) {
-      GetEndpoint(ch)->saved_candidates_.push_back(new CandidateData(ch, c));
+      GetEndpoint(ch)->saved_candidates_.push_back(new CandidatesData(ch, c));
     } else {
-      main_->Post(this, MSG_CANDIDATE, new CandidateData(ch, c));
+      main_->Post(this, MSG_CANDIDATE, new CandidatesData(ch, c));
     }
   }
 
   void PauseCandidates(int endpoint) {
     GetEndpoint(endpoint)->save_candidates_ = true;
   }
 
+  void OnCandidatesRemoved(cricket::TransportChannelImpl* ch,
+                           const std::vector<cricket::Candidate>& candidates) {
+    // Candidate removals are not paused.
+    CandidatesData* candidates_data = new CandidatesData(ch, candidates);
+    main_->Post(this, MSG_CANDIDATE_REMOVALS, candidates_data);
+  }
+
   // Tcp candidate verification has to be done when they are generated.
   void VerifySavedTcpCandidates(int endpoint, const std::string& tcptype) {
     for (auto& data : GetEndpoint(endpoint)->saved_candidates_) {
-      EXPECT_EQ(data->candidate.protocol(), cricket::TCP_PROTOCOL_NAME);
-      EXPECT_EQ(data->candidate.tcptype(), tcptype);
-      if (data->candidate.tcptype() == cricket::TCPTYPE_ACTIVE_STR) {
-        EXPECT_EQ(data->candidate.address().port(), cricket::DISCARD_PORT);
-      } else if (data->candidate.tcptype() == cricket::TCPTYPE_PASSIVE_STR) {
-        EXPECT_NE(data->candidate.address().port(), cricket::DISCARD_PORT);
+      cricket::Candidate& candidate = data->candidates[0];

[pthatcher1, 2016/03/09 18:00:53]

Why not just make this a for loop?

[honghaiz3, 2016/03/10 02:34:16]

Done.

+      EXPECT_EQ(candidate.protocol(), cricket::TCP_PROTOCOL_NAME);
+      EXPECT_EQ(candidate.tcptype(), tcptype);
+      if (candidate.tcptype() == cricket::TCPTYPE_ACTIVE_STR) {
+        EXPECT_EQ(candidate.address().port(), cricket::DISCARD_PORT);
+      } else if (candidate.tcptype() == cricket::TCPTYPE_PASSIVE_STR) {
+        EXPECT_NE(candidate.address().port(), cricket::DISCARD_PORT);
       } else {
-        FAIL() << "Unknown tcptype: " << data->candidate.tcptype();
+        FAIL() << "Unknown tcptype: " << candidate.tcptype();
       }
     }
   }
 
   void ResumeCandidates(int endpoint) {
     Endpoint* ed = GetEndpoint(endpoint);
-    std::vector<CandidateData*>::iterator it = ed->saved_candidates_.begin();
+    std::vector<CandidatesData*>::iterator it = ed->saved_candidates_.begin();
     for (; it != ed->saved_candidates_.end(); ++it) {
       main_->Post(this, MSG_CANDIDATE, *it);
     }
     ed->saved_candidates_.clear();
     ed->save_candidates_ = false;
   }
 
   void OnMessage(rtc::Message* msg) {
     switch (msg->message_id) {
       case MSG_CANDIDATE: {
-        rtc::scoped_ptr<CandidateData> data(
-            static_cast<CandidateData*>(msg->pdata));
+        rtc::scoped_ptr<CandidatesData> data(
+            static_cast<CandidatesData*>(msg->pdata));
         cricket::P2PTransportChannel* rch = GetRemoteChannel(data->channel);
-        cricket::Candidate c = data->candidate;
+        // Candidates are currently added one at a time.
+        cricket::Candidate c = data->candidates[0];

[pthatcher1, 2016/03/09 18:00:53]

Why not make this a for loop?

[honghaiz3, 2016/03/10 02:34:16]

Done.

         if (clear_remote_candidates_ufrag_pwd_) {
           c.set_username("");
           c.set_password("");
         }
         LOG(LS_INFO) << "Candidate(" << data->channel->component() << "->"
                      << rch->component() << "): " << c.ToString();
         rch->AddRemoteCandidate(c);
         break;
       }
+      case MSG_CANDIDATE_REMOVALS: {
+        rtc::scoped_ptr<CandidatesData> data(
+            static_cast<CandidatesData*>(msg->pdata));
+        cricket::P2PTransportChannel* rch = GetRemoteChannel(data->channel);
+        for (cricket::Candidate& c : data->candidates) {
+          LOG(LS_INFO) << "Removed remote candidate " << c.ToString();
+          rch->RemoveRemoteCandidate(c);
+        }
+        break;
+      }
     }
   }
   void OnReadPacket(cricket::TransportChannel* channel, const char* data,
                     size_t len, const rtc::PacketTime& packet_time,
                     int flags) {
     std::list<std::string>& packets = GetPacketList(channel);
     packets.push_front(std::string(data, len));
   }
   void OnRoleConflict(cricket::TransportChannelImpl* channel) {
     GetEndpoint(channel)->OnRoleConflict(true);
@@ skipping 1050 matching lines @@
   // Create channels and let them go writable, as usual.
   CreateChannels(1);
 
   // Both transport channels will reach STATE_COMPLETED quickly.
   EXPECT_EQ_WAIT(cricket::TransportChannelState::STATE_COMPLETED,
                  ep1_ch1()->GetState(), 1000);
   EXPECT_EQ_WAIT(cricket::TransportChannelState::STATE_COMPLETED,
                  ep2_ch1()->GetState(), 1000);
 }
 
-// Tests that when a network interface becomes inactive, the ports associated
-// with that network will be removed from the port list of the channel if
-// and only if Continual Gathering is enabled.
+// Tests that when a network interface becomes inactive, if and only if
+// Continual Gathering is enabled, the ports associated with that network
+// will be removed from the port list of the channel, and the respective
+// remote candidates on the other participant will be removed eventually.
 TEST_F(P2PTransportChannelMultihomedTest, TestNetworkBecomesInactive) {
   AddAddress(0, kPublicAddrs[0]);
   AddAddress(1, kPublicAddrs[1]);
   // Create channels and let them go writable, as usual.
   CreateChannels(1);
   ep1_ch1()->SetIceConfig(CreateIceConfig(2000, true));
   ep2_ch1()->SetIceConfig(CreateIceConfig(2000, false));
 
   SetAllocatorFlags(0, kOnlyLocalPorts);
   SetAllocatorFlags(1, kOnlyLocalPorts);
   EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() &&
                               ep2_ch1()->receiving() && ep2_ch1()->writable(),
                           1000, 1000);
   // More than one port has been created.
   EXPECT_LE(1U, ep1_ch1()->ports().size());
   // Endpoint 1 enabled continual gathering; the port will be removed
   // when the interface is removed.
   RemoveAddress(0, kPublicAddrs[0]);
-  EXPECT_EQ(0U, ep1_ch1()->ports().size());
+  EXPECT_TRUE(ep1_ch1()->ports().empty());
+  // The remote candidates will be removed eventually.
+  EXPECT_TRUE_WAIT(ep2_ch1()->remote_candidates().empty(), 1000);
 
   size_t num_ports = ep2_ch1()->ports().size();
   EXPECT_LE(1U, num_ports);
+  size_t num_remote_candidates = ep1_ch1()->remote_candidates().size();
   // Endpoint 2 did not enable continual gathering; the port will not be removed
-  // when the interface is removed.
+  // when the interface is removed and neither the remote candidates on the
+  // other participant.
   RemoveAddress(1, kPublicAddrs[1]);
+  rtc::Thread::Current()->ProcessMessages(500);
   EXPECT_EQ(num_ports, ep2_ch1()->ports().size());
+  EXPECT_EQ(num_remote_candidates, ep1_ch1()->remote_candidates().size());
 }
 
 /*
 
 TODO(pthatcher): Once have a way to handle network interfaces changes
 without signalling an ICE restart, put a test like this back.  In the
 mean time, this test only worked for GICE.  With ICE, it's currently
 not possible without an ICE restart.
 
 // Test that we can switch links in a coordinated fashion.
@@ skipping 876 matching lines @@
 
   // TCP Relay/Relay is the next.
   VerifyNextPingableConnection(cricket::RELAY_PORT_TYPE,
                                cricket::RELAY_PORT_TYPE,
                                cricket::TCP_PROTOCOL_NAME);
 
   // Finally, Local/Relay will be pinged.
   VerifyNextPingableConnection(cricket::LOCAL_PORT_TYPE,
                                cricket::RELAY_PORT_TYPE);
 }