Fix the duplicated candidate problem when using multiple STUN servers.

BUG=3723
R=pthatcher@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/26469004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@7312 4adac7df-926f-26a2-2b94-8c16560cd09d

talk/app/webrtc/webrtcsession_unittest.cc

@@ -309,7 +309,8 @@ class WebRtcSessionTest : public testing::Test {
       ss_scope_(fss_.get()),
       stun_socket_addr_(rtc::SocketAddress(kStunAddrHost,
                                                  cricket::STUN_SERVER_PORT)),
-      stun_server_(Thread::Current(), stun_socket_addr_),
+      stun_server_(cricket::TestStunServer::Create(Thread::Current(),
+                                                   stun_socket_addr_)),
       turn_server_(Thread::Current(), kTurnUdpIntAddr, kTurnUdpExtAddr),
       mediastream_signaling_(channel_manager_.get()),
       ice_type_(PeerConnectionInterface::kAll) {
@@ -1109,7 +1110,7 @@ class WebRtcSessionTest : public testing::Test {
   rtc::scoped_ptr<rtc::FirewallSocketServer> fss_;
   rtc::SocketServerScope ss_scope_;
   rtc::SocketAddress stun_socket_addr_;
-  cricket::TestStunServer stun_server_;
+  rtc::scoped_ptr<cricket::TestStunServer> stun_server_;
   cricket::TestTurnServer turn_server_;
   rtc::FakeNetworkManager network_manager_;
   rtc::scoped_ptr<cricket::BasicPortAllocator> allocator_;

talk/p2p/base/p2ptransportchannel_unittest.cc

@@ -139,7 +139,7 @@ class P2PTransportChannelTestBase : public testing::Test,
         nss_(new rtc::NATSocketServer(vss_.get())),
         ss_(new rtc::FirewallSocketServer(nss_.get())),
         ss_scope_(ss_.get()),
-        stun_server_(main_, kStunAddr),
+        stun_server_(cricket::TestStunServer::Create(main_, kStunAddr)),
         turn_server_(main_, kTurnUdpIntAddr, kTurnUdpExtAddr),
         relay_server_(main_, kRelayUdpIntAddr, kRelayUdpExtAddr,
                       kRelayTcpIntAddr, kRelayTcpExtAddr,
@@ -745,7 +745,7 @@ class P2PTransportChannelTestBase : public testing::Test,
   rtc::scoped_ptr<rtc::NATSocketServer> nss_;
   rtc::scoped_ptr<rtc::FirewallSocketServer> ss_;
   rtc::SocketServerScope ss_scope_;
-  cricket::TestStunServer stun_server_;
+  rtc::scoped_ptr<cricket::TestStunServer> stun_server_;
   cricket::TestTurnServer turn_server_;
   cricket::TestRelayServer relay_server_;
   rtc::SocksProxyServer socks_server1_;

talk/p2p/base/port_unittest.cc

@@ -343,7 +343,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> {
         nat_factory2_(ss_.get(), kNatAddr2),
         nat_socket_factory1_(&nat_factory1_),
         nat_socket_factory2_(&nat_factory2_),
-        stun_server_(main_, kStunAddr),
+        stun_server_(TestStunServer::Create(main_, kStunAddr)),
         turn_server_(main_, kTurnUdpIntAddr, kTurnUdpExtAddr),
         relay_server_(main_, kRelayUdpIntAddr, kRelayUdpExtAddr,
                       kRelayTcpIntAddr, kRelayTcpExtAddr,
@@ -617,7 +617,7 @@ class PortTest : public testing::Test, public sigslot::has_slots<> {
   rtc::NATSocketFactory nat_factory2_;
   rtc::BasicPacketSocketFactory nat_socket_factory1_;
   rtc::BasicPacketSocketFactory nat_socket_factory2_;
-  TestStunServer stun_server_;
+  scoped_ptr<TestStunServer> stun_server_;
   TestTurnServer turn_server_;
   TestRelayServer relay_server_;
   std::string username_;

talk/p2p/base/stunport.cc

@@ -389,10 +389,12 @@ void UDPPort::OnStunBindingRequestSucceeded(
   }
   bind_request_succeeded_servers_.insert(stun_server_addr);
 
-  if (!SharedSocket() || stun_reflected_addr != socket_->GetLocalAddress()) {
+  // If socket is shared and |stun_reflected_addr| is equal to local socket
-    // If socket is shared and |stun_reflected_addr| is equal to local socket
+  // address, or if the same address has been added by another STUN server,
-    // address then discarding the stun address.
+  // then discarding the stun address.
-    // For STUN related address is local socket address.
+  // For STUN, related address is the local socket address.
+  if ((!SharedSocket() || stun_reflected_addr != socket_->GetLocalAddress()) &&
+      !HasCandidateWithAddress(stun_reflected_addr)) {
     AddAddress(stun_reflected_addr, socket_->GetLocalAddress(),
                socket_->GetLocalAddress(), UDP_PROTOCOL_NAME, "",
                STUN_PORT_TYPE, ICE_TYPE_PREFERENCE_SRFLX, 0, false);
@@ -443,4 +445,14 @@ void UDPPort::OnSendPacket(const void* data, size_t size, StunRequest* req) {
     PLOG(LERROR, socket_->GetError()) << "sendto";
 }
 
+bool UDPPort::HasCandidateWithAddress(const rtc::SocketAddress& addr) const {
+  const std::vector<Candidate>& existing_candidates = Candidates();
+  std::vector<Candidate>::const_iterator it = existing_candidates.begin();
+  for (; it != existing_candidates.end(); ++it) {
+    if (it->address() == addr)
+      return true;
+  }
+  return false;
+}
+
 }  // namespace cricket

talk/p2p/base/stunport.h

@@ -194,6 +194,8 @@ class UDPPort : public Port {
   // changed to SignalPortReady.
   void MaybeSetPortCompleteOrError();
 
+  bool HasCandidateWithAddress(const rtc::SocketAddress& addr) const;
+
   ServerAddresses server_addresses_;
   ServerAddresses bind_request_succeeded_servers_;
   ServerAddresses bind_request_failed_servers_;

talk/p2p/base/stunport_unittest.cc

@@ -61,9 +61,9 @@ class StunPortTest : public testing::Test,
         ss_scope_(ss_.get()),
         network_("unittest", "unittest", rtc::IPAddress(INADDR_ANY), 32),
         socket_factory_(rtc::Thread::Current()),
-        stun_server_1_(new cricket::TestStunServer(
+        stun_server_1_(cricket::TestStunServer::Create(
           rtc::Thread::Current(), kStunAddr1)),
-        stun_server_2_(new cricket::TestStunServer(
+        stun_server_2_(cricket::TestStunServer::Create(
           rtc::Thread::Current(), kStunAddr2)),
         done_(false), error_(false), stun_keepalive_delay_(0) {
   }
@@ -150,6 +150,13 @@ class StunPortTest : public testing::Test,
     stun_keepalive_delay_ = delay;
   }
 
+  cricket::TestStunServer* stun_server_1() {
+    return stun_server_1_.get();
+  }
+  cricket::TestStunServer* stun_server_2() {
+    return stun_server_2_.get();
+  }
+
  private:
   rtc::scoped_ptr<rtc::PhysicalSocketServer> pss_;
   rtc::scoped_ptr<rtc::VirtualSocketServer> ss_;
@@ -253,8 +260,8 @@ TEST_F(StunPortTest, TestSharedSocketPrepareAddressInvalidHostname) {
   // No crash is success.
 }
 
-// Test that candidates can be allocated for multiple STUN servers.
+// Test that the same address is added only once if two STUN servers are in use.
-TEST_F(StunPortTest, TestMultipleGoodStunServers) {
+TEST_F(StunPortTest, TestNoDuplicatedAddressWithTwoStunServers) {
   ServerAddresses stun_servers;
   stun_servers.insert(kStunAddr1);
   stun_servers.insert(kStunAddr2);
@@ -262,7 +269,7 @@ TEST_F(StunPortTest, TestMultipleGoodStunServers) {
   EXPECT_EQ("stun", port()->Type());
   PrepareAddress();
   EXPECT_TRUE_WAIT(done(), kTimeoutMs);
-  EXPECT_EQ(2U, port()->Candidates().size());
+  EXPECT_EQ(1U, port()->Candidates().size());
 }
 
 // Test that candidates can be allocated for multiple STUN servers, one of which
@@ -277,3 +284,21 @@ TEST_F(StunPortTest, TestMultipleStunServersWithBadServer) {
   EXPECT_TRUE_WAIT(done(), kTimeoutMs);
   EXPECT_EQ(1U, port()->Candidates().size());
 }
+
+// Test that two candidates are allocated if the two STUN servers return
+// different mapped addresses.
+TEST_F(StunPortTest, TestTwoCandidatesWithTwoStunServersAcrossNat) {
+  const SocketAddress kStunMappedAddr1("77.77.77.77", 0);
+  const SocketAddress kStunMappedAddr2("88.77.77.77", 0);
+  stun_server_1()->set_fake_stun_addr(kStunMappedAddr1);
+  stun_server_2()->set_fake_stun_addr(kStunMappedAddr2);
+
+  ServerAddresses stun_servers;
+  stun_servers.insert(kStunAddr1);
+  stun_servers.insert(kStunAddr2);
+  CreateStunPort(stun_servers);
+  EXPECT_EQ("stun", port()->Type());
+  PrepareAddress();
+  EXPECT_TRUE_WAIT(done(), kTimeoutMs);
+  EXPECT_EQ(2U, port()->Candidates().size());
+}

talk/p2p/base/stunserver.cc

@@ -68,19 +68,7 @@ void StunServer::OnPacket(
 void StunServer::OnBindingRequest(
     StunMessage* msg, const rtc::SocketAddress& remote_addr) {
   StunMessage response;
-  response.SetType(STUN_BINDING_RESPONSE);
+  GetStunBindReqponse(msg, remote_addr, &response);
-  response.SetTransactionID(msg->transaction_id());
-
-  // Tell the user the address that we received their request from.
-  StunAddressAttribute* mapped_addr;
-  if (!msg->IsLegacy()) {
-    mapped_addr = StunAttribute::CreateAddress(STUN_ATTR_MAPPED_ADDRESS);
-  } else {
-    mapped_addr = StunAttribute::CreateXorAddress(STUN_ATTR_XOR_MAPPED_ADDRESS);
-  }
-  mapped_addr->SetAddress(remote_addr);
-  response.AddAttribute(mapped_addr);
-
   SendResponse(response, remote_addr);
 }
 
@@ -108,4 +96,21 @@ void StunServer::SendResponse(
     LOG_ERR(LS_ERROR) << "sendto";
 }
 
+void StunServer::GetStunBindReqponse(StunMessage* request,
+                                     const rtc::SocketAddress& remote_addr,
+                                     StunMessage* response) const {
+  response->SetType(STUN_BINDING_RESPONSE);
+  response->SetTransactionID(request->transaction_id());
+
+  // Tell the user the address that we received their request from.
+  StunAddressAttribute* mapped_addr;
+  if (!request->IsLegacy()) {
+    mapped_addr = StunAttribute::CreateAddress(STUN_ATTR_MAPPED_ADDRESS);
+  } else {
+    mapped_addr = StunAttribute::CreateXorAddress(STUN_ATTR_XOR_MAPPED_ADDRESS);
+  }
+  mapped_addr->SetAddress(remote_addr);
+  response->AddAttribute(mapped_addr);
+}
+
 }  // namespace cricket

talk/p2p/base/stunserver.h

@@ -51,7 +51,7 @@ class StunServer : public sigslot::has_slots<> {
       const rtc::PacketTime& packet_time);
 
   // Handlers for the different types of STUN/TURN requests:
-  void OnBindingRequest(StunMessage* msg,
+  virtual void OnBindingRequest(StunMessage* msg,
       const rtc::SocketAddress& addr);
   void OnAllocateRequest(StunMessage* msg,
       const rtc::SocketAddress& addr);
@@ -69,6 +69,11 @@ class StunServer : public sigslot::has_slots<> {
   void SendResponse(const StunMessage& msg,
        const rtc::SocketAddress& addr);
 
+  // A helper method to compose a STUN binding response.
+  void GetStunBindReqponse(StunMessage* request,
+                           const rtc::SocketAddress& remote_addr,
+                           StunMessage* response) const;
+
  private:
   rtc::scoped_ptr<rtc::AsyncUDPSocket> socket_;
 };

talk/p2p/base/teststunserver.h

@@ -35,19 +35,39 @@
 namespace cricket {
 
 // A test STUN server. Useful for unit tests.
-class TestStunServer {
+class TestStunServer : StunServer {
  public:
-  TestStunServer(rtc::Thread* thread,
+  static TestStunServer* Create(rtc::Thread* thread,
-                 const rtc::SocketAddress& addr)
+                                const rtc::SocketAddress& addr) {
-      : socket_(thread->socketserver()->CreateAsyncSocket(addr.family(),
+    rtc::AsyncSocket* socket =
-                                                          SOCK_DGRAM)),
+        thread->socketserver()->CreateAsyncSocket(addr.family(), SOCK_DGRAM);
-        udp_socket_(rtc::AsyncUDPSocket::Create(socket_, addr)),
+    rtc::AsyncUDPSocket* udp_socket =
-        server_(udp_socket_) {
+        rtc::AsyncUDPSocket::Create(socket, addr);
+
+    return new TestStunServer(udp_socket);
   }
+
+  // Set a fake STUN address to return to the client.
+  void set_fake_stun_addr(const rtc::SocketAddress& addr) {
+    fake_stun_addr_ = addr;
+  }
+
  private:
-  rtc::AsyncSocket* socket_;
+  explicit TestStunServer(rtc::AsyncUDPSocket* socket) : StunServer(socket) {}
-  rtc::AsyncUDPSocket* udp_socket_;
+
-  cricket::StunServer server_;
+  void OnBindingRequest(StunMessage* msg,
+                        const rtc::SocketAddress& remote_addr) OVERRIDE {
+    if (fake_stun_addr_.IsNil()) {
+      StunServer::OnBindingRequest(msg, remote_addr);
+    } else {
+      StunMessage response;
+      GetStunBindReqponse(msg, fake_stun_addr_, &response);
+      SendResponse(response, remote_addr);
+    }
+  }
+
+ private:
+  rtc::SocketAddress fake_stun_addr_;
 };
 
 }  // namespace cricket

talk/p2p/client/portallocator_unittest.cc

@@ -112,7 +112,8 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> {
         ss_scope_(fss_.get()),
         nat_factory_(vss_.get(), kNatAddr),
         nat_socket_factory_(&nat_factory_),
-        stun_server_(Thread::Current(), kStunAddr),
+        stun_server_(cricket::TestStunServer::Create(Thread::Current(),
+                                                     kStunAddr)),
         relay_server_(Thread::Current(), kRelayUdpIntAddr, kRelayUdpExtAddr,
                       kRelayTcpIntAddr, kRelayTcpExtAddr,
                       kRelaySslTcpIntAddr, kRelaySslTcpExtAddr),
@@ -280,7 +281,7 @@ class PortAllocatorTest : public testing::Test, public sigslot::has_slots<> {
   rtc::scoped_ptr<rtc::NATServer> nat_server_;
   rtc::NATSocketFactory nat_factory_;
   rtc::BasicPacketSocketFactory nat_socket_factory_;
-  cricket::TestStunServer stun_server_;
+  rtc::scoped_ptr<cricket::TestStunServer> stun_server_;
   cricket::TestRelayServer relay_server_;
   cricket::TestTurnServer turn_server_;
   rtc::FakeNetworkManager network_manager_;