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
diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc
index 7aa87fb..388fff7 100644
--- a/talk/app/webrtc/webrtcsession_unittest.cc
+++ b/talk/app/webrtc/webrtcsession_unittest.cc
@@ -309,7 +309,8 @@
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 @@
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_;
diff --git a/talk/p2p/base/p2ptransportchannel_unittest.cc b/talk/p2p/base/p2ptransportchannel_unittest.cc
index 0b7f882..9bfa84f 100644
--- a/talk/p2p/base/p2ptransportchannel_unittest.cc
+++ b/talk/p2p/base/p2ptransportchannel_unittest.cc
@@ -139,7 +139,7 @@
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 @@
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_;
diff --git a/talk/p2p/base/port_unittest.cc b/talk/p2p/base/port_unittest.cc
index 04becfc..a44a12e 100644
--- a/talk/p2p/base/port_unittest.cc
+++ b/talk/p2p/base/port_unittest.cc
@@ -343,7 +343,7 @@
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 @@
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_;
diff --git a/talk/p2p/base/stunport.cc b/talk/p2p/base/stunport.cc
index a178eb4..a185a35 100644
--- a/talk/p2p/base/stunport.cc
+++ b/talk/p2p/base/stunport.cc
@@ -389,10 +389,12 @@
}
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
- // address then discarding the stun address.
- // For STUN related address is local socket address.
+ // 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,
+ // then discarding the stun 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 @@
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
diff --git a/talk/p2p/base/stunport.h b/talk/p2p/base/stunport.h
index b3b6d5b..1fc1f64 100644
--- a/talk/p2p/base/stunport.h
+++ b/talk/p2p/base/stunport.h
@@ -194,6 +194,8 @@
// 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_;
diff --git a/talk/p2p/base/stunport_unittest.cc b/talk/p2p/base/stunport_unittest.cc
index 6506181..c5abc48 100644
--- a/talk/p2p/base/stunport_unittest.cc
+++ b/talk/p2p/base/stunport_unittest.cc
@@ -61,9 +61,9 @@
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 @@
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 @@
// No crash is success.
}
-// Test that candidates can be allocated for multiple STUN servers.
-TEST_F(StunPortTest, TestMultipleGoodStunServers) {
+// Test that the same address is added only once if two STUN servers are in use.
+TEST_F(StunPortTest, TestNoDuplicatedAddressWithTwoStunServers) {
ServerAddresses stun_servers;
stun_servers.insert(kStunAddr1);
stun_servers.insert(kStunAddr2);
@@ -262,7 +269,7 @@
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 @@
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());
+}
diff --git a/talk/p2p/base/stunserver.cc b/talk/p2p/base/stunserver.cc
index d9633f0..df428f9 100644
--- a/talk/p2p/base/stunserver.cc
+++ b/talk/p2p/base/stunserver.cc
@@ -68,19 +68,7 @@
void StunServer::OnBindingRequest(
StunMessage* msg, const rtc::SocketAddress& remote_addr) {
StunMessage response;
- response.SetType(STUN_BINDING_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);
-
+ GetStunBindReqponse(msg, remote_addr, &response);
SendResponse(response, remote_addr);
}
@@ -108,4 +96,21 @@
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
diff --git a/talk/p2p/base/stunserver.h b/talk/p2p/base/stunserver.h
index 1e14408..a141e5b 100644
--- a/talk/p2p/base/stunserver.h
+++ b/talk/p2p/base/stunserver.h
@@ -51,7 +51,7 @@
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 @@
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_;
};
diff --git a/talk/p2p/base/teststunserver.h b/talk/p2p/base/teststunserver.h
index 6f85009..840150b 100644
--- a/talk/p2p/base/teststunserver.h
+++ b/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,
- const rtc::SocketAddress& addr)
- : socket_(thread->socketserver()->CreateAsyncSocket(addr.family(),
- SOCK_DGRAM)),
- udp_socket_(rtc::AsyncUDPSocket::Create(socket_, addr)),
- server_(udp_socket_) {
+ static TestStunServer* Create(rtc::Thread* thread,
+ const rtc::SocketAddress& addr) {
+ rtc::AsyncSocket* socket =
+ thread->socketserver()->CreateAsyncSocket(addr.family(), SOCK_DGRAM);
+ rtc::AsyncUDPSocket* 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_;
- rtc::AsyncUDPSocket* udp_socket_;
- cricket::StunServer server_;
+ explicit TestStunServer(rtc::AsyncUDPSocket* socket) : StunServer(socket) {}
+
+ 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
diff --git a/talk/p2p/client/portallocator_unittest.cc b/talk/p2p/client/portallocator_unittest.cc
index e793064..b1feca2 100644
--- a/talk/p2p/client/portallocator_unittest.cc
+++ b/talk/p2p/client/portallocator_unittest.cc
@@ -112,7 +112,8 @@
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 @@
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_;
