Don't send STUN pings if we don't have a remote ufrag and pwd.

BUG=4495 [email protected] Review URL: https://webrtc-codereview.appspot.com/44029004 Cr-Commit-Position: refs/heads/master@{#8926}
ReadyTalk · Apr 2, 2015 · 7351f46 · 7351f46
1 parent bc4b934
commit 7351f46
Show file tree

Hide file tree

Showing 3 changed files with 39 additions and 2 deletions.
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -1163,6 +1163,14 @@ void P2PTransportChannel::OnPing() {
 
 // Is the connection in a state for us to even consider pinging the other side?
 bool P2PTransportChannel::IsPingable(Connection* conn) {
+  const Candidate& remote = conn->remote_candidate();
+  // We should never get this far with an empty remote ufrag.
+  ASSERT(!remote.username().empty());
+  if (remote.username().empty() || remote.password().empty()) {
+    // If we don't have an ICE ufrag and pwd, there's no way we can ping.
+    return false;
+  }
+
   // An unconnected connection cannot be written to at all, so pinging is out
   // of the question.
   if (!conn->connected())

diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h
@@ -154,6 +154,9 @@ class P2PTransportChannel : public TransportChannelImpl,
   // Helper method used only in unittest.
   rtc::DiffServCodePoint DefaultDscpValue() const;
 
+  // Public for unit tests.
+  Connection* FindNextPingableConnection();
+
  private:
   rtc::Thread* thread() { return worker_thread_; }
   PortAllocatorSession* allocator_session() {
@@ -182,7 +185,6 @@ class P2PTransportChannel : public TransportChannelImpl,
   void RememberRemoteCandidate(const Candidate& remote_candidate,
                                PortInterface* origin_port);
   bool IsPingable(Connection* conn);
-  Connection* FindNextPingableConnection();
   void PingConnection(Connection* conn);
   void AddAllocatorSession(PortAllocatorSession* session);
   void AddConnection(Connection* connection);

diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1267,7 +1267,11 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignaling) {
                      PORTALLOCATOR_ENABLE_SHARED_UFRAG,
                      kDefaultStepDelay, kDefaultStepDelay,
                      cricket::ICEPROTO_RFC5245);
+  // Emulate no remote credentials coming in.
+  set_clear_remote_candidates_ufrag_pwd(false);
   CreateChannels(1);
+  // Only have remote credentials come in for ep2, not ep1.
+  ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]);
 
   // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive
   // candidate.
@@ -1279,11 +1283,20 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignaling) {
   WAIT((best_connection = ep1_ch1()->best_connection()) != NULL, 2000);
   EXPECT_EQ("prflx", ep1_ch1()->best_connection()->remote_candidate().type());
 
+  // Because we don't have a remote pwd, we don't ping yet.
+  EXPECT_EQ(kIceUfrag[1],
+            ep1_ch1()->best_connection()->remote_candidate().username());
+  EXPECT_EQ("", ep1_ch1()->best_connection()->remote_candidate().password());
+  EXPECT_TRUE(nullptr == ep1_ch1()->FindNextPingableConnection());
+
   ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]);
   ResumeCandidates(1);
 
-  WAIT(ep2_ch1()->best_connection() != NULL, 2000);
+  EXPECT_EQ(kIcePwd[1],
+            ep1_ch1()->best_connection()->remote_candidate().password());
+  EXPECT_TRUE(nullptr != ep1_ch1()->FindNextPingableConnection());
 
+  WAIT(ep2_ch1()->best_connection() != NULL, 2000);
   // Verify ep1's best connection is updated to use the 'local' candidate.
   EXPECT_EQ_WAIT(
       "local",
@@ -1301,7 +1314,11 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) {
                      PORTALLOCATOR_ENABLE_SHARED_UFRAG,
                      kDefaultStepDelay, kDefaultStepDelay,
                      cricket::ICEPROTO_RFC5245);
+  // Emulate no remote credentials coming in.
+  set_clear_remote_candidates_ufrag_pwd(false);
   CreateChannels(1);
+  // Only have remote credentials come in for ep2, not ep1.
+  ep2_ch1()->SetRemoteIceCredentials(kIceUfrag[3], kIcePwd[3]);
   // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive
   // candidate.
   PauseCandidates(1);
@@ -1311,9 +1328,19 @@ TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) {
   WAIT(ep1_ch1()->best_connection() != NULL, 2000);
   EXPECT_EQ("prflx", ep1_ch1()->best_connection()->remote_candidate().type());
 
+  // Because we don't have a remote pwd, we don't ping yet.
+  EXPECT_EQ(kIceUfrag[1],
+            ep1_ch1()->best_connection()->remote_candidate().username());
+  EXPECT_EQ("", ep1_ch1()->best_connection()->remote_candidate().password());
+  EXPECT_TRUE(nullptr == ep1_ch1()->FindNextPingableConnection());
+
   ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]);
   ResumeCandidates(1);
 
+  EXPECT_EQ(kIcePwd[1],
+            ep1_ch1()->best_connection()->remote_candidate().password());
+  EXPECT_TRUE(nullptr != ep1_ch1()->FindNextPingableConnection());
+
   const cricket::Connection* best_connection = NULL;
   WAIT((best_connection = ep2_ch1()->best_connection()) != NULL, 2000);