Accept incoming pings before remote answer is set to reduce connection latency.
BUG=4068
R=juberti@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/33509004
git-svn-id: http://webrtc.googlecode.com/svn/trunk/webrtc@7980 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/p2p/base/p2ptransportchannel.cc b/p2p/base/p2ptransportchannel.cc
index 8073f59..2ef88be 100644
--- a/p2p/base/p2ptransportchannel.cc
+++ b/p2p/base/p2ptransportchannel.cc
@@ -303,6 +303,12 @@
remote_ice_ufrag_ = ice_ufrag;
remote_ice_pwd_ = ice_pwd;
+ // We need to update the credentials for any peer reflexive candidates.
+ std::vector<Connection*>::iterator it = connections_.begin();
+ for (; it != connections_.end(); ++it) {
+ (*it)->MaybeSetRemoteIceCredentials(ice_ufrag, ice_pwd);
+ }
+
if (ice_restart) {
// |candidate.generation()| is not signaled in ICEPROTO_RFC5245.
// Therefore we need to keep track of the remote ice restart so
@@ -450,12 +456,10 @@
}
const Candidate* candidate = NULL;
- bool known_username = false;
std::string remote_password;
for (it = remote_candidates_.begin(); it != remote_candidates_.end(); ++it) {
if (it->username() == remote_username) {
remote_password = it->password();
- known_username = true;
if (ufrag_per_port ||
(it->address() == address &&
it->protocol() == ProtoToString(proto))) {
@@ -467,19 +471,11 @@
}
}
- if (!known_username) {
- if (port_muxed) {
- // When Ports are muxed, SignalUnknownAddress is delivered to all
- // P2PTransportChannel belong to a session. Return from here will
- // save us from sending stun binding error message from incorrect channel.
- return;
- }
- // Don't know about this username, the request is bogus
- // This sometimes happens if a binding response comes in before the ACCEPT
- // message. It is totally valid; the retry state machine will try again.
- port->SendBindingErrorResponse(stun_msg, address,
- STUN_ERROR_STALE_CREDENTIALS, STUN_ERROR_REASON_STALE_CREDENTIALS);
- return;
+ // The STUN binding request may arrive after setRemoteDescription and before
+ // adding remote candidate, so we need to set the password to the shared
+ // password if the user name matches.
+ if (remote_password.empty() && remote_username == remote_ice_ufrag_) {
+ remote_password = remote_ice_pwd_;
}
Candidate new_remote_candidate;
@@ -569,6 +565,8 @@
return;
}
+ LOG(LS_INFO) << "Adding connection from peer reflexive candidate: "
+ << new_remote_candidate.ToString();
AddConnection(connection);
connection->ReceivedPing();
@@ -716,6 +714,8 @@
// found, then we can create a new connection for this address.
Connection* connection = port->GetConnection(remote_candidate.address());
if (connection != NULL) {
+ connection->MaybeUpdatePeerReflexiveCandidate(remote_candidate);
+
// It is not legal to try to change any of the parameters of an existing
// connection; however, the other side can send a duplicate candidate.
if (!remote_candidate.IsEquivalent(connection->remote_candidate())) {
diff --git a/p2p/base/p2ptransportchannel_unittest.cc b/p2p/base/p2ptransportchannel_unittest.cc
index bc24b6b..e5df2d7 100644
--- a/p2p/base/p2ptransportchannel_unittest.cc
+++ b/p2p/base/p2ptransportchannel_unittest.cc
@@ -31,6 +31,7 @@
using cricket::kDefaultPortAllocatorFlags;
using cricket::kMinimumStepDelay;
using cricket::kDefaultStepDelay;
+using cricket::PORTALLOCATOR_ENABLE_BUNDLE;
using cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG;
using cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET;
using cricket::ServerAddresses;
@@ -96,6 +97,10 @@
static const uint64 kTiebreaker1 = 11111;
static const uint64 kTiebreaker2 = 22222;
+enum {
+ MSG_CANDIDATE
+};
+
// This test simulates 2 P2P endpoints that want to establish connectivity
// with each other over various network topologies and conditions, which can be
// specified in each individial test.
@@ -199,10 +204,21 @@
rtc::scoped_ptr<cricket::P2PTransportChannel> ch_;
};
+ struct CandidateData : public rtc::MessageData {
+ CandidateData(cricket::TransportChannel* ch, const cricket::Candidate& c)
+ : channel(ch), candidate(c) {
+ }
+ cricket::TransportChannel* channel;
+ cricket::Candidate candidate;
+ };
+
struct Endpoint {
- Endpoint() : signaling_delay_(0), role_(cricket::ICEROLE_UNKNOWN),
- tiebreaker_(0), role_conflict_(false),
- protocol_type_(cricket::ICEPROTO_GOOGLE) {}
+ Endpoint()
+ : role_(cricket::ICEROLE_UNKNOWN),
+ tiebreaker_(0),
+ role_conflict_(false),
+ save_candidates_(false),
+ protocol_type_(cricket::ICEPROTO_GOOGLE) {}
bool HasChannel(cricket::TransportChannel* ch) {
return (ch == cd1_.ch_.get() || ch == cd2_.ch_.get());
}
@@ -213,7 +229,6 @@
else
return &cd2_;
}
- void SetSignalingDelay(int delay) { signaling_delay_ = delay; }
void SetIceRole(cricket::IceRole role) { role_ = role; }
cricket::IceRole ice_role() { return role_; }
@@ -236,19 +251,12 @@
rtc::scoped_ptr<cricket::BasicPortAllocator> allocator_;
ChannelData cd1_;
ChannelData cd2_;
- int signaling_delay_;
cricket::IceRole role_;
uint64 tiebreaker_;
bool role_conflict_;
+ bool save_candidates_;
cricket::IceProtocolType protocol_type_;
- };
-
- struct CandidateData : public rtc::MessageData {
- CandidateData(cricket::TransportChannel* ch, const cricket::Candidate& c)
- : channel(ch), candidate(c) {
- }
- cricket::TransportChannel* channel;
- cricket::Candidate candidate;
+ std::vector<CandidateData*> saved_candidates_;
};
ChannelData* GetChannelData(cricket::TransportChannel* channel) {
@@ -277,11 +285,11 @@
std::string ice_ufrag_ep2_cd2_ch = kIceUfrag[3];
std::string ice_pwd_ep2_cd2_ch = kIcePwd[3];
// In BUNDLE each endpoint must share common ICE credentials.
- if (ep1_.allocator_->flags() & cricket::PORTALLOCATOR_ENABLE_BUNDLE) {
+ if (ep1_.allocator_->flags() & PORTALLOCATOR_ENABLE_BUNDLE) {
ice_ufrag_ep1_cd2_ch = ice_ufrag_ep1_cd1_ch;
ice_pwd_ep1_cd2_ch = ice_pwd_ep1_cd1_ch;
}
- if (ep2_.allocator_->flags() & cricket::PORTALLOCATOR_ENABLE_BUNDLE) {
+ if (ep2_.allocator_->flags() & PORTALLOCATOR_ENABLE_BUNDLE) {
ice_ufrag_ep2_cd2_ch = ice_ufrag_ep2_cd1_ch;
ice_pwd_ep2_cd2_ch = ice_pwd_ep2_cd1_ch;
}
@@ -380,9 +388,6 @@
void SetAllocatorFlags(int endpoint, int flags) {
GetAllocator(endpoint)->set_flags(flags);
}
- void SetSignalingDelay(int endpoint, int delay) {
- GetEndpoint(endpoint)->SetSignalingDelay(delay);
- }
void SetIceProtocol(int endpoint, cricket::IceProtocolType type) {
GetEndpoint(endpoint)->SetIceProtocolType(type);
}
@@ -629,23 +634,44 @@
if (force_relay_ && c.type() != cricket::RELAY_PORT_TYPE)
return;
- main_->PostDelayed(GetEndpoint(ch)->signaling_delay_, this, 0,
- new CandidateData(ch, c));
- }
- void OnMessage(rtc::Message* msg) {
- rtc::scoped_ptr<CandidateData> data(
- static_cast<CandidateData*>(msg->pdata));
- cricket::P2PTransportChannel* rch = GetRemoteChannel(data->channel);
- cricket::Candidate c = data->candidate;
- if (clear_remote_candidates_ufrag_pwd_) {
- c.set_username("");
- c.set_password("");
+ if (GetEndpoint(ch)->save_candidates_) {
+ GetEndpoint(ch)->saved_candidates_.push_back(new CandidateData(ch, c));
+ } else {
+ main_->Post(this, MSG_CANDIDATE, new CandidateData(ch, c));
}
- LOG(LS_INFO) << "Candidate(" << data->channel->component() << "->"
- << rch->component() << "): " << c.type() << ", " << c.protocol()
- << ", " << c.address().ToString() << ", " << c.username()
- << ", " << c.generation();
- rch->OnCandidate(c);
+ }
+
+ void PauseCandidates(int endpoint) {
+ GetEndpoint(endpoint)->save_candidates_ = true;
+ }
+
+ void ResumeCandidates(int endpoint) {
+ Endpoint* ed = GetEndpoint(endpoint);
+ std::vector<CandidateData*>::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));
+ cricket::P2PTransportChannel* rch = GetRemoteChannel(data->channel);
+ cricket::Candidate c = data->candidate;
+ if (clear_remote_candidates_ufrag_pwd_) {
+ c.set_username("");
+ c.set_password("");
+ }
+ LOG(LS_INFO) << "Candidate(" << data->channel->component() << "->"
+ << rch->component() << "): " << c.ToString();
+ rch->OnCandidate(c);
+ break;
+ }
+ }
}
void OnReadPacket(cricket::TransportChannel* channel, const char* data,
size_t len, const rtc::PacketTime& packet_time,
@@ -822,6 +848,10 @@
SetIceProtocol(1, type);
SetAllocatorFlags(1, allocator_flags2);
SetAllocationStepDelay(1, delay2);
+
+ if (type == cricket::ICEPROTO_RFC5245) {
+ set_clear_remote_candidates_ufrag_pwd(true);
+ }
}
void ConfigureEndpoint(int endpoint, Config config) {
switch (config) {
@@ -1163,14 +1193,12 @@
// Test that we restart candidate allocation when local ufrag&pwd changed.
// Standard Ice protocol is used.
TEST_F(P2PTransportChannelTest, HandleUfragPwdChangeBundleAsIce) {
- ConfigureEndpoints(OPEN, OPEN,
- PORTALLOCATOR_ENABLE_SHARED_UFRAG,
- PORTALLOCATOR_ENABLE_SHARED_UFRAG,
- kMinimumStepDelay, kMinimumStepDelay,
- cricket::ICEPROTO_RFC5245);
- SetAllocatorFlags(0, cricket::PORTALLOCATOR_ENABLE_BUNDLE);
- SetAllocatorFlags(1, cricket::PORTALLOCATOR_ENABLE_BUNDLE);
-
+ ConfigureEndpoints(
+ OPEN, OPEN,
+ PORTALLOCATOR_ENABLE_BUNDLE | PORTALLOCATOR_ENABLE_SHARED_UFRAG,
+ PORTALLOCATOR_ENABLE_BUNDLE | PORTALLOCATOR_ENABLE_SHARED_UFRAG,
+ kMinimumStepDelay, kMinimumStepDelay,
+ cricket::ICEPROTO_RFC5245);
CreateChannels(2);
TestHandleIceUfragPasswordChanged();
DestroyChannels();
@@ -1192,14 +1220,12 @@
// Test that ICE restart works when bundle is enabled.
// Google Ice protocol is used.
TEST_F(P2PTransportChannelTest, HandleUfragPwdChangeBundleAsGice) {
- ConfigureEndpoints(OPEN, OPEN,
- PORTALLOCATOR_ENABLE_SHARED_UFRAG,
- PORTALLOCATOR_ENABLE_SHARED_UFRAG,
- kDefaultStepDelay, kDefaultStepDelay,
- cricket::ICEPROTO_GOOGLE);
- SetAllocatorFlags(0, cricket::PORTALLOCATOR_ENABLE_BUNDLE);
- SetAllocatorFlags(1, cricket::PORTALLOCATOR_ENABLE_BUNDLE);
-
+ ConfigureEndpoints(
+ OPEN, OPEN,
+ PORTALLOCATOR_ENABLE_BUNDLE | PORTALLOCATOR_ENABLE_SHARED_UFRAG,
+ PORTALLOCATOR_ENABLE_BUNDLE | PORTALLOCATOR_ENABLE_SHARED_UFRAG,
+ kDefaultStepDelay, kDefaultStepDelay,
+ cricket::ICEPROTO_GOOGLE);
CreateChannels(2);
TestHandleIceUfragPasswordChanged();
DestroyChannels();
@@ -1233,25 +1259,68 @@
DestroyChannels();
}
-// Test that we properly handle getting a STUN error due to slow signaling.
-TEST_F(P2PTransportChannelTest, DISABLED_SlowSignaling) {
- ConfigureEndpoints(OPEN, NAT_SYMMETRIC,
- kDefaultPortAllocatorFlags,
- kDefaultPortAllocatorFlags,
+// Test that we properly create a connection on a STUN ping from unknown address
+// when the signaling is slow.
+TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignaling) {
+ ConfigureEndpoints(OPEN, OPEN,
+ PORTALLOCATOR_ENABLE_SHARED_UFRAG,
+ PORTALLOCATOR_ENABLE_SHARED_UFRAG,
kDefaultStepDelay, kDefaultStepDelay,
- cricket::ICEPROTO_GOOGLE);
- // Make signaling from the callee take 500ms, so that the initial STUN pings
- // from the callee beat the signaling, and so the caller responds with a
- // unknown username error. We should just eat that and carry on; mishandling
- // this will instead cause all the callee's connections to be discarded.
- SetSignalingDelay(1, 1000);
+ cricket::ICEPROTO_RFC5245);
CreateChannels(1);
+
+ // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive
+ // candidate.
+ PauseCandidates(1);
+
+ // The caller should have the best connection connected to the peer reflexive
+ // candidate.
const cricket::Connection* best_connection = NULL;
- // Wait until the callee's connections are created.
- WAIT((best_connection = ep2_ch1()->best_connection()) != NULL, 1000);
- // Wait to see if they get culled; they shouldn't.
- WAIT(ep2_ch1()->best_connection() != best_connection, 1000);
- EXPECT_TRUE(ep2_ch1()->best_connection() == best_connection);
+ WAIT((best_connection = ep1_ch1()->best_connection()) != NULL, 2000);
+ EXPECT_EQ("prflx", ep1_ch1()->best_connection()->remote_candidate().type());
+
+ ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]);
+ ResumeCandidates(1);
+
+ WAIT(ep2_ch1()->best_connection() != NULL, 2000);
+
+ // Verify ep1's best connection is updated to use the 'local' candidate.
+ EXPECT_EQ_WAIT(
+ "local",
+ ep1_ch1()->best_connection()->remote_candidate().type(),
+ 2000);
+ EXPECT_EQ(best_connection, ep1_ch1()->best_connection());
+ DestroyChannels();
+}
+
+// Test that we properly create a connection on a STUN ping from unknown address
+// when the signaling is slow and the end points are behind NAT.
+TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateBeforeSignalingWithNAT) {
+ ConfigureEndpoints(OPEN, NAT_SYMMETRIC,
+ PORTALLOCATOR_ENABLE_SHARED_UFRAG,
+ PORTALLOCATOR_ENABLE_SHARED_UFRAG,
+ kDefaultStepDelay, kDefaultStepDelay,
+ cricket::ICEPROTO_RFC5245);
+ CreateChannels(1);
+ // Pause sending ep2's candidates to ep1 until ep1 receives the peer reflexive
+ // candidate.
+ PauseCandidates(1);
+
+ // The caller should have the best connection connected to the peer reflexive
+ // candidate.
+ WAIT(ep1_ch1()->best_connection() != NULL, 2000);
+ EXPECT_EQ("prflx", ep1_ch1()->best_connection()->remote_candidate().type());
+
+ ep1_ch1()->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]);
+ ResumeCandidates(1);
+
+ const cricket::Connection* best_connection = NULL;
+ WAIT((best_connection = ep2_ch1()->best_connection()) != NULL, 2000);
+
+ // Wait to verify the connection is not culled.
+ WAIT(ep1_ch1()->writable(), 2000);
+ EXPECT_EQ(ep2_ch1()->best_connection(), best_connection);
+ EXPECT_EQ("prflx", ep1_ch1()->best_connection()->remote_candidate().type());
DestroyChannels();
}
@@ -1359,8 +1428,10 @@
TEST_F(P2PTransportChannelTest, TestBundleAllocatorToBundleAllocator) {
AddAddress(0, kPublicAddrs[0]);
AddAddress(1, kPublicAddrs[1]);
- SetAllocatorFlags(0, cricket::PORTALLOCATOR_ENABLE_BUNDLE);
- SetAllocatorFlags(1, cricket::PORTALLOCATOR_ENABLE_BUNDLE);
+ SetAllocatorFlags(
+ 0, PORTALLOCATOR_ENABLE_BUNDLE | PORTALLOCATOR_ENABLE_SHARED_UFRAG);
+ SetAllocatorFlags(
+ 1, PORTALLOCATOR_ENABLE_BUNDLE | PORTALLOCATOR_ENABLE_SHARED_UFRAG);
CreateChannels(2);
@@ -1385,7 +1456,8 @@
AddAddress(0, kPublicAddrs[0]);
AddAddress(1, kPublicAddrs[1]);
// Enable BUNDLE flag at one side.
- SetAllocatorFlags(0, cricket::PORTALLOCATOR_ENABLE_BUNDLE);
+ SetAllocatorFlags(
+ 0, PORTALLOCATOR_ENABLE_BUNDLE | PORTALLOCATOR_ENABLE_SHARED_UFRAG);
CreateChannels(2);
@@ -1418,8 +1490,10 @@
TEST_F(P2PTransportChannelTest, TestIceRoleConflictWithBundle) {
AddAddress(0, kPublicAddrs[0]);
AddAddress(1, kPublicAddrs[1]);
- SetAllocatorFlags(0, cricket::PORTALLOCATOR_ENABLE_BUNDLE);
- SetAllocatorFlags(1, cricket::PORTALLOCATOR_ENABLE_BUNDLE);
+ SetAllocatorFlags(
+ 0, PORTALLOCATOR_ENABLE_BUNDLE | PORTALLOCATOR_ENABLE_SHARED_UFRAG);
+ SetAllocatorFlags(
+ 1, PORTALLOCATOR_ENABLE_BUNDLE | PORTALLOCATOR_ENABLE_SHARED_UFRAG);
TestSignalRoleConflict();
}
diff --git a/p2p/base/port.cc b/p2p/base/port.cc
index cdef0fc..0472b16 100644
--- a/p2p/base/port.cc
+++ b/p2p/base/port.cc
@@ -418,7 +418,8 @@
if (IsStandardIce() &&
!stun_msg->ValidateMessageIntegrity(data, size, password_)) {
LOG_J(LS_ERROR, this) << "Received STUN request with bad M-I "
- << "from " << addr.ToSensitiveString();
+ << "from " << addr.ToSensitiveString()
+ << ", password_=" << password_;
SendBindingErrorResponse(stun_msg.get(), addr, STUN_ERROR_UNAUTHORIZED,
STUN_ERROR_REASON_UNAUTHORIZED);
return true;
@@ -1348,6 +1349,27 @@
port_->SignalRoleConflict(port_);
}
+void Connection::MaybeSetRemoteIceCredentials(const std::string& ice_ufrag,
+ const std::string& ice_pwd) {
+ if (remote_candidate_.username() == ice_ufrag &&
+ remote_candidate_.password().empty()) {
+ remote_candidate_.set_password(ice_pwd);
+ }
+}
+
+void Connection::MaybeUpdatePeerReflexiveCandidate(
+ const Candidate& new_candidate) {
+ if (remote_candidate_.type() == PRFLX_PORT_TYPE &&
+ new_candidate.type() != PRFLX_PORT_TYPE &&
+ remote_candidate_.protocol() == new_candidate.protocol() &&
+ remote_candidate_.address() == new_candidate.address() &&
+ remote_candidate_.username() == new_candidate.username() &&
+ remote_candidate_.password() == new_candidate.password() &&
+ remote_candidate_.generation() == new_candidate.generation()) {
+ remote_candidate_ = new_candidate;
+ }
+}
+
void Connection::OnMessage(rtc::Message *pmsg) {
ASSERT(pmsg->message_id == MSG_DELETE);
diff --git a/p2p/base/port.h b/p2p/base/port.h
index b78c2c5..0547418 100644
--- a/p2p/base/port.h
+++ b/p2p/base/port.h
@@ -535,6 +535,16 @@
IceMode remote_ice_mode() const { return remote_ice_mode_; }
+ // Update the ICE password of the remote candidate if |ice_ufrag| matches
+ // the candidate's ufrag, and the candidate's passwrod has not been set.
+ void MaybeSetRemoteIceCredentials(const std::string& ice_ufrag,
+ const std::string& ice_pwd);
+
+ // If |remote_candidate_| is peer reflexive and is equivalent to
+ // |new_candidate| except the type, update |remote_candidate_| to
+ // |new_candidate|.
+ void MaybeUpdatePeerReflexiveCandidate(const Candidate& new_candidate);
+
protected:
// Constructs a new connection to the given remote port.
Connection(Port* port, size_t index, const Candidate& candidate);
diff --git a/p2p/base/portallocator.cc b/p2p/base/portallocator.cc
index 8613347..5ac58ea 100644
--- a/p2p/base/portallocator.cc
+++ b/p2p/base/portallocator.cc
@@ -28,6 +28,9 @@
// by itself.
username_(flags_ & PORTALLOCATOR_ENABLE_SHARED_UFRAG ? ice_ufrag : ""),
password_(flags_ & PORTALLOCATOR_ENABLE_SHARED_UFRAG ? ice_pwd : "") {
+ // If bundle is enabled, shared ufrag must be enabled too.
+ ASSERT((!(flags_ & PORTALLOCATOR_ENABLE_BUNDLE)) ||
+ (flags_ & PORTALLOCATOR_ENABLE_SHARED_UFRAG));
}
PortAllocator::~PortAllocator() {
diff --git a/p2p/client/portallocator_unittest.cc b/p2p/client/portallocator_unittest.cc
index 5dcc8f9..5590e70 100644
--- a/p2p/client/portallocator_unittest.cc
+++ b/p2p/client/portallocator_unittest.cc
@@ -644,7 +644,8 @@
TEST_F(PortAllocatorTest, TestBasicMuxFeatures) {
AddInterface(kClientAddr);
- allocator().set_flags(cricket::PORTALLOCATOR_ENABLE_BUNDLE);
+ allocator().set_flags(cricket::PORTALLOCATOR_ENABLE_BUNDLE |
+ cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG);
// Session ID - session1.
rtc::scoped_ptr<cricket::PortAllocatorSession> session1(
CreateSession("session1", cricket::ICE_CANDIDATE_COMPONENT_RTP));
@@ -671,7 +672,8 @@
// set of candidates.
TEST_F(PortAllocatorTest, TestBundleIceRestart) {
AddInterface(kClientAddr);
- allocator().set_flags(cricket::PORTALLOCATOR_ENABLE_BUNDLE);
+ allocator().set_flags(cricket::PORTALLOCATOR_ENABLE_BUNDLE |
+ cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG);
// Session ID - session1.
rtc::scoped_ptr<cricket::PortAllocatorSession> session1(
CreateSession("session1", kContentName,
