From dacdd9403d30cdb13ab2de645841edd2ae76950d Mon Sep 17 00:00:00 2001 From: "jiayl@webrtc.org" Date: Fri, 23 Jan 2015 17:33:34 +0000 Subject: [PATCH] Reland r7980: Accept incoming pings before remote answer is set, to reduce connection latency. Set ICE connection state to 'checking' after setting the remote answer, so that it can transition into 'connected' if the peer reflexive connection is up before any remote candidate is set. See more details in crbug/446908 BUG=4068, crbug/446908 R=juberti@webrtc.org, pthatcher@webrtc.org Review URL: https://webrtc-codereview.appspot.com/38709004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@8141 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../src/org/webrtc/PeerConnectionTest.java | 38 ++- talk/app/webrtc/webrtcsession.cc | 13 ++ talk/app/webrtc/webrtcsession_unittest.cc | 6 +- webrtc/p2p/base/p2ptransportchannel.cc | 30 +-- .../p2p/base/p2ptransportchannel_unittest.cc | 216 ++++++++++++------ webrtc/p2p/base/port.cc | 24 +- webrtc/p2p/base/port.h | 10 + webrtc/p2p/base/portallocator.cc | 3 + webrtc/p2p/client/portallocator_unittest.cc | 6 +- 9 files changed, 235 insertions(+), 111 deletions(-) diff --git a/talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java b/talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java index b04885822e..c3ec72c4c7 100644 --- a/talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java +++ b/talk/app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java @@ -600,26 +600,6 @@ public class PeerConnectionTest { sdpLatch = new SdpObserverLatch(); offeringExpectations.expectSignalingChange(SignalingState.STABLE); offeringExpectations.expectAddStream("answeredMediaStream"); - offeringPC.setRemoteDescription(sdpLatch, answerSdp); - assertTrue(sdpLatch.await()); - assertNull(sdpLatch.getSdp()); - - offeringExpectations.waitForAllExpectationsToBeSatisfied(); - answeringExpectations.waitForAllExpectationsToBeSatisfied(); - - assertEquals(offeringPC.getLocalDescription().type, offerSdp.type); - assertEquals(offeringPC.getRemoteDescription().type, answerSdp.type); - assertEquals(answeringPC.getLocalDescription().type, answerSdp.type); - assertEquals(answeringPC.getRemoteDescription().type, offerSdp.type); - - if (!RENDER_TO_GUI) { - // Wait for at least some frames to be delivered at each end (number - // chosen arbitrarily). - offeringExpectations.expectFramesDelivered(10); - answeringExpectations.expectFramesDelivered(10); - offeringExpectations.expectSetSize(); - answeringExpectations.expectSetSize(); - } offeringExpectations.expectIceConnectionChange( IceConnectionState.CHECKING); @@ -635,6 +615,24 @@ public class PeerConnectionTest { answeringExpectations.expectIceConnectionChange( IceConnectionState.CONNECTED); + offeringPC.setRemoteDescription(sdpLatch, answerSdp); + assertTrue(sdpLatch.await()); + assertNull(sdpLatch.getSdp()); + + assertEquals(offeringPC.getLocalDescription().type, offerSdp.type); + assertEquals(offeringPC.getRemoteDescription().type, answerSdp.type); + assertEquals(answeringPC.getLocalDescription().type, answerSdp.type); + assertEquals(answeringPC.getRemoteDescription().type, offerSdp.type); + + if (!RENDER_TO_GUI) { + // Wait for at least some frames to be delivered at each end (number + // chosen arbitrarily). + offeringExpectations.expectFramesDelivered(10); + answeringExpectations.expectFramesDelivered(10); + offeringExpectations.expectSetSize(); + answeringExpectations.expectSetSize(); + } + offeringExpectations.expectStateChange(DataChannel.State.OPEN); // See commentary about SCTP DataChannels above for why this is here. answeringExpectations.expectDataChannel("offeringDC"); diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 54018f223a..27b936bf06 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -837,6 +837,19 @@ bool WebRtcSession::SetRemoteDescription(SessionDescriptionInterface* desc, if (error() != cricket::BaseSession::ERROR_NONE) { return BadRemoteSdp(desc->type(), GetSessionErrorMsg(), err_desc); } + + // Set the the ICE connection state to connecting since the connection may + // become writable with peer reflexive candidates before any remote candidate + // is signaled. + // TODO(pthatcher): This is a short-term solution for crbug/446908. A real fix + // is to have a new signal the indicates a change in checking state from the + // transport and expose a new checking() member from transport that can be + // read to determine the current checking state. The existing SignalConnecting + // actually means "gathering candidates", so cannot be be used here. + if (desc->type() != SessionDescriptionInterface::kOffer && + ice_connection_state_ == PeerConnectionInterface::kIceConnectionNew) { + SetIceConnectionState(PeerConnectionInterface::kIceConnectionChecking); + } return true; } diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index 4b52e77769..d0fb805a50 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -354,7 +354,8 @@ class WebRtcSessionTest : public testing::Test { SocketAddress(), SocketAddress(), SocketAddress())); allocator_->set_flags(cricket::PORTALLOCATOR_DISABLE_TCP | cricket::PORTALLOCATOR_DISABLE_RELAY | - cricket::PORTALLOCATOR_ENABLE_BUNDLE); + cricket::PORTALLOCATOR_ENABLE_BUNDLE | + cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG); EXPECT_TRUE(channel_manager_->Init()); desc_factory_->set_add_legacy_streams(false); allocator_->set_step_delay(cricket::kMinimumStepDelay); @@ -1212,7 +1213,8 @@ class WebRtcSessionTest : public testing::Test { allocator_->AddRelay(relay_server); allocator_->set_step_delay(cricket::kMinimumStepDelay); allocator_->set_flags(cricket::PORTALLOCATOR_DISABLE_TCP | - cricket::PORTALLOCATOR_ENABLE_BUNDLE); + cricket::PORTALLOCATOR_ENABLE_BUNDLE | + cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG); } cricket::FakeMediaEngine* media_engine_; diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 9afaca6d93..4bbe9cf239 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -303,6 +303,12 @@ void P2PTransportChannel::SetRemoteIceCredentials(const std::string& ice_ufrag, remote_ice_ufrag_ = ice_ufrag; remote_ice_pwd_ = ice_pwd; + // We need to update the credentials for any peer reflexive candidates. + std::vector::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 @@ void P2PTransportChannel::OnUnknownAddress( } 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 @@ void P2PTransportChannel::OnUnknownAddress( } } - 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; @@ -573,6 +569,8 @@ void P2PTransportChannel::OnUnknownAddress( return; } + LOG(LS_INFO) << "Adding connection from peer reflexive candidate: " + << new_remote_candidate.ToString(); AddConnection(connection); connection->ReceivedPing(); @@ -720,6 +718,8 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port, // 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/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index bc24b6b8bd..e5df2d7e8c 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/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 char* kIcePwd[4] = {"TESTICEPWD00000000000000", 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 @@ class P2PTransportChannelTestBase : public testing::Test, rtc::scoped_ptr 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 @@ class P2PTransportChannelTestBase : public testing::Test, 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 @@ class P2PTransportChannelTestBase : public testing::Test, rtc::scoped_ptr 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 saved_candidates_; }; ChannelData* GetChannelData(cricket::TransportChannel* channel) { @@ -277,11 +285,11 @@ class P2PTransportChannelTestBase : public testing::Test, 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 @@ class P2PTransportChannelTestBase : public testing::Test, 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 @@ class P2PTransportChannelTestBase : public testing::Test, 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 data( - static_cast(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)); + } + } + + void PauseCandidates(int endpoint) { + GetEndpoint(endpoint)->save_candidates_ = true; + } + + void ResumeCandidates(int endpoint) { + Endpoint* ed = GetEndpoint(endpoint); + std::vector::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 data( + static_cast(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; + } } - LOG(LS_INFO) << "Candidate(" << data->channel->component() << "->" - << rch->component() << "): " << c.type() << ", " << c.protocol() - << ", " << c.address().ToString() << ", " << c.username() - << ", " << c.generation(); - rch->OnCandidate(c); } void OnReadPacket(cricket::TransportChannel* channel, const char* data, size_t len, const rtc::PacketTime& packet_time, @@ -822,6 +848,10 @@ class P2PTransportChannelTest : public P2PTransportChannelTestBase { 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_F(P2PTransportChannelTest, HandleUfragPwdChangeAsIce) { // 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_F(P2PTransportChannelTest, HandleUfragPwdChangeAsGice) { // 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 @@ TEST_F(P2PTransportChannelTest, GetStats) { 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, TestTcpConnectionsFromActiveToPassive) { 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 @@ TEST_F(P2PTransportChannelTest, TestBundleAllocatorToNonBundleAllocator) { 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, TestIceRoleConflictWithoutBundle) { 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/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index cdef0fc55e..0472b1698d 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -418,7 +418,8 @@ bool Port::GetStunMessage(const char* data, size_t size, 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 @@ void Connection::HandleRoleConflictFromPeer() { 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/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 4a5c89985f..6d7d6d5437 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -535,6 +535,16 @@ class Connection : public rtc::MessageHandler, 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/webrtc/p2p/base/portallocator.cc b/webrtc/p2p/base/portallocator.cc index 86133474d6..5ac58ea9de 100644 --- a/webrtc/p2p/base/portallocator.cc +++ b/webrtc/p2p/base/portallocator.cc @@ -28,6 +28,9 @@ PortAllocatorSession::PortAllocatorSession(const std::string& content_name, // 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/webrtc/p2p/client/portallocator_unittest.cc b/webrtc/p2p/client/portallocator_unittest.cc index 0cee3bde7c..6f83beb6ca 100644 --- a/webrtc/p2p/client/portallocator_unittest.cc +++ b/webrtc/p2p/client/portallocator_unittest.cc @@ -644,7 +644,8 @@ TEST_F(PortAllocatorTest, TestCandidateFilterWithReflexiveOnlyAndNoNAT) { 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 session1( CreateSession("session1", cricket::ICE_CANDIDATE_COMPONENT_RTP)); @@ -671,7 +672,8 @@ TEST_F(PortAllocatorTest, TestBasicMuxFeatures) { // 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 session1( CreateSession("session1", kContentName,