diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc index df6e4d0e09..d25de260f3 100644 --- a/webrtc/p2p/base/dtlstransportchannel.cc +++ b/webrtc/p2p/base/dtlstransportchannel.cc @@ -138,12 +138,6 @@ DtlsTransportChannelWrapper::DtlsTransportChannelWrapper( DtlsTransportChannelWrapper::~DtlsTransportChannelWrapper() { } -void DtlsTransportChannelWrapper::Connect() { - // We should only get a single call to Connect. - ASSERT(dtls_state() == DTLS_TRANSPORT_NEW); - channel_->Connect(); -} - bool DtlsTransportChannelWrapper::SetLocalCertificate( const rtc::scoped_refptr& certificate) { if (dtls_active_) { diff --git a/webrtc/p2p/base/dtlstransportchannel.h b/webrtc/p2p/base/dtlstransportchannel.h index 6368bf9819..c8b76d3303 100644 --- a/webrtc/p2p/base/dtlstransportchannel.h +++ b/webrtc/p2p/base/dtlstransportchannel.h @@ -176,8 +176,6 @@ class DtlsTransportChannelWrapper : public TransportChannelImpl { channel_->SetRemoteIceMode(mode); } - void Connect() override; - void MaybeStartGathering() override { channel_->MaybeStartGathering(); } IceGatheringState gathering_state() const override { diff --git a/webrtc/p2p/base/faketransportcontroller.h b/webrtc/p2p/base/faketransportcontroller.h index 7b7aec5908..c96acda5d7 100644 --- a/webrtc/p2p/base/faketransportcontroller.h +++ b/webrtc/p2p/base/faketransportcontroller.h @@ -111,12 +111,6 @@ class FakeTransportChannel : public TransportChannelImpl, return true; } - void Connect() override { - if (state_ == STATE_INIT) { - state_ = STATE_CONNECTING; - } - } - void MaybeStartGathering() override { if (gathering_state_ == kIceGatheringNew) { gathering_state_ = kIceGatheringGathering; @@ -145,7 +139,7 @@ class FakeTransportChannel : public TransportChannelImpl, // If |asymmetric| is true this method only affects this FakeTransportChannel. // If false, it affects |dest| as well. void SetDestination(FakeTransportChannel* dest, bool asymmetric = false) { - if (state_ == STATE_CONNECTING && dest) { + if (state_ == STATE_INIT && dest) { // This simulates the delivery of candidates. dest_ = dest; if (local_cert_ && dest_->local_cert_) { @@ -160,7 +154,7 @@ class FakeTransportChannel : public TransportChannelImpl, } else if (state_ == STATE_CONNECTED && !dest) { // Simulates loss of connectivity, by asymmetrically forgetting dest_. dest_ = nullptr; - state_ = STATE_CONNECTING; + state_ = STATE_INIT; set_writable(false); } } @@ -314,7 +308,7 @@ class FakeTransportChannel : public TransportChannelImpl, } } - enum State { STATE_INIT, STATE_CONNECTING, STATE_CONNECTED }; + enum State { STATE_INIT, STATE_CONNECTED }; FakeTransportChannel* dest_ = nullptr; State state_ = STATE_INIT; bool async_ = false; @@ -578,7 +572,6 @@ class FakeTransportController : public TransportController { transport->SetLocalTransportDescription(faketransport_desc, cricket::CA_OFFER, nullptr); } - transport->ConnectChannels(); transport->MaybeStartGathering(); } } diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index a4bde05d80..b5d6755cee 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -27,7 +27,7 @@ namespace { // messages for queuing up work for ourselves -enum { MSG_SORT = 1, MSG_CHECK_AND_PING }; +enum { MSG_SORT_AND_UPDATE_STATE = 1, MSG_CHECK_AND_PING }; // The minimum improvement in RTT that justifies a switch. static const double kMinImprovement = 10; @@ -287,7 +287,7 @@ void P2PTransportChannel::SetRemoteIceCredentials(const std::string& ice_ufrag, static_cast(remote_ice_parameters_.size() - 1)); } // Updating the remote ICE candidate generation could change the sort order. - RequestSort(); + RequestSortAndStateUpdate(); } void P2PTransportChannel::SetRemoteIceMode(IceMode mode) { @@ -352,21 +352,10 @@ const IceConfig& P2PTransportChannel::config() const { return config_; } -// Go into the state of processing candidates, and running in general -void P2PTransportChannel::Connect() { - ASSERT(worker_thread_ == rtc::Thread::Current()); +void P2PTransportChannel::MaybeStartGathering() { if (ice_ufrag_.empty() || ice_pwd_.empty()) { - ASSERT(false); - LOG(LS_ERROR) << "P2PTransportChannel::Connect: The ice_ufrag_ and the " - << "ice_pwd_ are not set."; return; } - - // Start checking and pinging as the ports come in. - thread()->Post(RTC_FROM_HERE, this, MSG_CHECK_AND_PING); -} - -void P2PTransportChannel::MaybeStartGathering() { // Start gathering if we never started before, or if an ICE restart occurred. if (allocator_sessions_.empty() || IceCredentialsChanged(allocator_sessions_.back()->ice_ufrag(), @@ -443,7 +432,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession *session, CreateConnection(port, *iter, iter->origin_port()); } - SortConnections(); + SortConnectionsAndUpdateState(); } // A new candidate is available, let listeners know @@ -585,7 +574,7 @@ void P2PTransportChannel::OnUnknownAddress( // Update the list of connections since we just added another. We do this // after sending the response since it could (in principle) delete the // connection in question. - SortConnections(); + SortConnectionsAndUpdateState(); } void P2PTransportChannel::OnRoleConflict(PortInterface* port) { @@ -629,7 +618,7 @@ void P2PTransportChannel::OnNominated(Connection* conn) { SwitchSelectedConnection(conn); // Now that we have selected a connection, it is time to prune other // connections and update the read/write state of the channel. - RequestSort(); + RequestSortAndStateUpdate(); } void P2PTransportChannel::AddRemoteCandidate(const Candidate& candidate) { @@ -676,7 +665,7 @@ void P2PTransportChannel::AddRemoteCandidate(const Candidate& candidate) { CreateConnections(new_remote_candidate, NULL); // Resort the connections list, which may have new elements. - SortConnections(); + SortConnectionsAndUpdateState(); } void P2PTransportChannel::RemoveRemoteCandidate( @@ -942,18 +931,35 @@ void P2PTransportChannel::UpdateConnectionStates() { // We need to copy the list of connections since some may delete themselves // when we call UpdateState. - for (size_t i = 0; i < connections_.size(); ++i) - connections_[i]->UpdateState(now); + for (Connection* c : connections_) { + c->UpdateState(now); + } } // Prepare for best candidate sorting. -void P2PTransportChannel::RequestSort() { +void P2PTransportChannel::RequestSortAndStateUpdate() { if (!sort_dirty_) { - worker_thread_->Post(RTC_FROM_HERE, this, MSG_SORT); + worker_thread_->Post(RTC_FROM_HERE, this, MSG_SORT_AND_UPDATE_STATE); sort_dirty_ = true; } } +void P2PTransportChannel::MaybeStartPinging() { + if (started_pinging_) { + return; + } + + int64_t now = rtc::TimeMillis(); + if (std::any_of( + connections_.begin(), connections_.end(), + [this, now](const Connection* c) { return IsPingable(c, now); })) { + LOG_J(LS_INFO, this) << "Have a pingable connection for the first time; " + << "starting to ping."; + thread()->Post(RTC_FROM_HERE, this, MSG_CHECK_AND_PING); + started_pinging_ = true; + } +} + // Compare two connections based on their writing, receiving, and connected // states. int P2PTransportChannel::CompareConnectionStates(const Connection* a, @@ -1096,7 +1102,7 @@ bool P2PTransportChannel::PresumedWritable(const Connection* conn) const { // Sort the available connections to find the best one. We also monitor // the number of available connections and the current state. -void P2PTransportChannel::SortConnections() { +void P2PTransportChannel::SortConnectionsAndUpdateState() { ASSERT(worker_thread_ == rtc::Thread::Current()); // Make sure the connection states are up-to-date since this affects how they @@ -1165,9 +1171,15 @@ void P2PTransportChannel::SortConnections() { HandleAllTimedOut(); } - // Update the state of this channel. This method is called whenever the - // state of any connection changes, so this is a good place to do this. + // Update the state of this channel. UpdateState(); + + // Also possibly start pinging. + // We could start pinging if: + // * The first connection was created. + // * ICE credentials were provided. + // * A TCP connection became connected. + MaybeStartPinging(); } void P2PTransportChannel::PruneConnections() { @@ -1238,7 +1250,7 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) { // transport controller will get the up-to-date channel state. However it // should not be called too often; in the case that multiple connection states // change, it should be called after all the connection states have changed. For -// example, we call this at the end of SortConnections. +// example, we call this at the end of SortConnectionsAndUpdateState. void P2PTransportChannel::UpdateState() { TransportChannelState state = ComputeState(); if (state_ != state) { @@ -1358,8 +1370,8 @@ Connection* P2PTransportChannel::GetBestConnectionOnNetwork( // Handle any queued up requests void P2PTransportChannel::OnMessage(rtc::Message *pmsg) { switch (pmsg->message_id) { - case MSG_SORT: - OnSort(); + case MSG_SORT_AND_UPDATE_STATE: + SortConnectionsAndUpdateState(); break; case MSG_CHECK_AND_PING: OnCheckAndPing(); @@ -1370,12 +1382,6 @@ void P2PTransportChannel::OnMessage(rtc::Message *pmsg) { } } -// Handle queued up sort request -void P2PTransportChannel::OnSort() { - // Resort the connections based on the new statistics. - SortConnections(); -} - // Handle queued up check-and-ping request void P2PTransportChannel::OnCheckAndPing() { // Make sure the states of the connections are up-to-date (since this affects @@ -1405,7 +1411,7 @@ void P2PTransportChannel::OnCheckAndPing() { // A connection is considered a backup connection if the channel state // is completed, the connection is not the selected connection and it is active. -bool P2PTransportChannel::IsBackupConnection(Connection* conn) const { +bool P2PTransportChannel::IsBackupConnection(const Connection* conn) const { return state_ == STATE_COMPLETED && conn != selected_connection_ && conn->active(); } @@ -1413,7 +1419,8 @@ bool P2PTransportChannel::IsBackupConnection(Connection* conn) const { // Is the connection in a state for us to even consider pinging the other side? // We consider a connection pingable even if it's not connected because that's // how a TCP connection is kicked into reconnecting on the active side. -bool P2PTransportChannel::IsPingable(Connection* conn, int64_t now) { +bool P2PTransportChannel::IsPingable(const Connection* conn, + int64_t now) const { const Candidate& remote = conn->remote_candidate(); // We should never get this far with an empty remote ufrag. ASSERT(!remote.username().empty()); @@ -1471,8 +1478,9 @@ bool P2PTransportChannel::IsSelectedConnectionPingable(int64_t now) { return selected_connection_->last_ping_sent() + interval <= now; } -int P2PTransportChannel::CalculateActiveWritablePingInterval(Connection* conn, - int64_t now) { +int P2PTransportChannel::CalculateActiveWritablePingInterval( + const Connection* conn, + int64_t now) const { // Ping each connection at a higher rate at least // MIN_PINGS_AT_WEAK_PING_INTERVAL times. if (conn->num_pings_sent() < MIN_PINGS_AT_WEAK_PING_INTERVAL) { @@ -1557,7 +1565,7 @@ void P2PTransportChannel::OnConnectionStateChange(Connection* connection) { // We have to unroll the stack before doing this because we may be changing // the state of connections while sorting. - RequestSort(); + RequestSortAndStateUpdate(); } // When a connection is removed, edit it out, and then update our best @@ -1580,18 +1588,21 @@ void P2PTransportChannel::OnConnectionDestroyed(Connection* connection) { << static_cast(connections_.size()) << " remaining)"; // If this is currently the selected connection, then we need to pick a new - // one. The call to SortConnections will pick a new one. It looks at the - // current selected connection in order to avoid switching between fairly - // similar ones. Since this connection is no longer an option, we can just - // set selected to nullptr and re-choose a best assuming that there was no - // selected connection. + // one. The call to SortConnectionsAndUpdateState will pick a new one. It + // looks at the current selected connection in order to avoid switching + // between fairly similar ones. Since this connection is no longer an option, + // we can just set selected to nullptr and re-choose a best assuming that + // there was no selected connection. if (selected_connection_ == connection) { LOG(LS_INFO) << "selected connection destroyed. Will choose a new one."; SwitchSelectedConnection(nullptr); - RequestSort(); + RequestSortAndStateUpdate(); + } else { + // If a non-selected connection was destroyed, we don't need to re-sort but + // we do need to update state, because we could be switching to "failed" or + // "completed". + UpdateState(); } - - UpdateState(); } // When a port is destroyed remove it from our list of ports to use for diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index fc22b2089a..59bb773bf4 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -93,7 +93,6 @@ class P2PTransportChannel : public TransportChannelImpl, void SetRemoteIceCredentials(const std::string& ice_ufrag, const std::string& ice_pwd) override; void SetRemoteIceMode(IceMode mode) override; - void Connect() override; void MaybeStartGathering() override; IceGatheringState gathering_state() const override { return gathering_state_; @@ -215,7 +214,10 @@ class P2PTransportChannel : public TransportChannelImpl, // Returns true if it's possible to send packets on this channel. bool ReadyToSend() const; void UpdateConnectionStates(); - void RequestSort(); + void RequestSortAndStateUpdate(); + // Start pinging if we haven't already started, and we now have a connection + // that's pingable. + void MaybeStartPinging(); // The methods below return a positive value if a is preferable to b, // a negative value if b is preferable, and 0 if they're equally preferable. @@ -233,7 +235,9 @@ class P2PTransportChannel : public TransportChannelImpl, bool PresumedWritable(const cricket::Connection* conn) const; - void SortConnections(); + bool ShouldSwitchSelectedConnection(const cricket::Connection* selected, + const cricket::Connection* conn) const; + void SortConnectionsAndUpdateState(); void SwitchSelectedConnection(Connection* conn); void UpdateState(); void HandleAllTimedOut(); @@ -252,9 +256,10 @@ class P2PTransportChannel : public TransportChannelImpl, bool IsDuplicateRemoteCandidate(const Candidate& candidate); void RememberRemoteCandidate(const Candidate& remote_candidate, PortInterface* origin_port); - bool IsPingable(Connection* conn, int64_t now); + bool IsPingable(const Connection* conn, int64_t now) const; bool IsSelectedConnectionPingable(int64_t now); - int CalculateActiveWritablePingInterval(Connection* conn, int64_t now); + int CalculateActiveWritablePingInterval(const Connection* conn, + int64_t now) const; void PingConnection(Connection* conn); void AddAllocatorSession(std::unique_ptr session); void AddConnection(Connection* connection); @@ -283,14 +288,13 @@ class P2PTransportChannel : public TransportChannelImpl, void OnNominated(Connection* conn); void OnMessage(rtc::Message* pmsg) override; - void OnSort(); void OnCheckAndPing(); // Returns true if the new_connection should be selected for transmission. bool ShouldSwitchSelectedConnection(Connection* new_connection) const; void PruneConnections(); - bool IsBackupConnection(Connection* conn) const; + bool IsBackupConnection(const Connection* conn) const; Connection* FindConnectionToPing(int64_t now); Connection* FindOldestConnectionNeedingTriggeredCheck(int64_t now); @@ -366,6 +370,7 @@ class P2PTransportChannel : public TransportChannelImpl, TransportChannelState state_ = TransportChannelState::STATE_INIT; IceConfig config_; int last_sent_packet_id_ = -1; // -1 indicates no packet was sent before. + bool started_pinging_ = false; RTC_DISALLOW_COPY_AND_ASSIGN(P2PTransportChannel); }; diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index c55485a9d0..b5645007c3 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -355,7 +355,6 @@ class P2PTransportChannelTestBase : public testing::Test, } channel->SetIceRole(GetEndpoint(endpoint)->ice_role()); channel->SetIceTiebreaker(GetEndpoint(endpoint)->GetIceTiebreaker()); - channel->Connect(); return channel; } void DestroyChannels() { @@ -370,6 +369,7 @@ class P2PTransportChannelTestBase : public testing::Test, P2PTransportChannel* ep2_ch2() { return ep2_.cd2_.ch_.get(); } TestTurnServer* test_turn_server() { return &turn_server_; } + rtc::VirtualSocketServer* virtual_socket_server() { return vss_.get(); } // Common results. static const Result kLocalUdpToLocalUdp; @@ -487,8 +487,6 @@ class P2PTransportChannelTestBase : public testing::Test, // on the local and remote candidate of ep2_ch1, match. This can be // used in an EXPECT_TRUE_WAIT. bool CheckCandidate2(const Result& expected) { - const std::string& local_type = LocalCandidate(ep2_ch1())->type(); - // const std::string& remote_type = RemoteCandidate(ep2_ch1())->type(); const std::string& local_proto = LocalCandidate(ep2_ch1())->protocol(); const std::string& remote_proto = RemoteCandidate(ep2_ch1())->protocol(); // Removed remote_type comparision aginst selected connection remote @@ -498,16 +496,19 @@ class P2PTransportChannelTestBase : public testing::Test, // and in other cases like NAT -> NAT it will be LUSU. To avoid these // mismatches and we are doing comparision in different way. // i.e. when don't match its remote type is either local or stun. + // + // Update(deadbeef): Also had to remove local type comparison. There + // is currently an issue where the local type is not updated to stun. + // So one side may see local<->relay while the other sees relay<->stun. + // This mean the other side may prioritize prflx<->relay, and won't have + // a local type of relay no matter how long we wait. + // TODO(deadbeef): Fix this!! It's causing us to have very sparse test + // coverage and is a very real bug. + // // TODO(ronghuawu): Refine the test criteria. // https://code.google.com/p/webrtc/issues/detail?id=1953 return ((local_proto == expected.local_proto2 && - remote_proto == expected.remote_proto2) && - (local_type == expected.local_type2 || - // Sometimes we expect local -> prflx or prflx -> local - // and instead get prflx -> local or local -> prflx, and - // that's OK. - (IsLocalToPrflxOrTheReverse(expected) && - local_type == expected.remote_type2))); + remote_proto == expected.remote_proto2)); } // EXPECT_EQ on the approprite parts of the expected Result, based @@ -1063,20 +1064,6 @@ const P2PTransportChannelTest::Result* P2PTransportChannelTest::kMatrix[NUM_CONF P2P_TEST(x, PROXY_HTTPS) \ P2P_TEST(x, PROXY_SOCKS) -#define FLAKY_P2P_TEST_SET(x) \ - P2P_TEST(x, OPEN) \ - P2P_TEST(x, NAT_FULL_CONE) \ - P2P_TEST(x, NAT_ADDR_RESTRICTED) \ - P2P_TEST(x, NAT_PORT_RESTRICTED) \ - P2P_TEST(x, NAT_SYMMETRIC) \ - P2P_TEST(x, NAT_DOUBLE_CONE) \ - P2P_TEST(x, NAT_SYMMETRIC_THEN_CONE) \ - P2P_TEST(x, BLOCK_UDP) \ - P2P_TEST(x, BLOCK_UDP_AND_INCOMING_TCP) \ - P2P_TEST(x, BLOCK_ALL_BUT_OUTGOING_HTTP) \ - P2P_TEST(x, PROXY_HTTPS) \ - P2P_TEST(x, PROXY_SOCKS) - P2P_TEST_SET(OPEN) P2P_TEST_SET(NAT_FULL_CONE) P2P_TEST_SET(NAT_ADDR_RESTRICTED) @@ -1445,7 +1432,7 @@ TEST_F(P2PTransportChannelTest, TestIceConfigWillPassDownToPort) { const std::vector ports_after = ep1_ch1()->ports(); for (size_t i = 0; i < ports_after.size(); ++i) { EXPECT_EQ(ICEROLE_CONTROLLED, ports_before[i]->GetIceRole()); - // SetIceTiebreaker after Connect() has been called will fail. So expect the + // SetIceTiebreaker after ports have been created will fail. So expect the // original value. EXPECT_EQ(kHighTiebreaker, ports_before[i]->IceTiebreaker()); } @@ -1703,6 +1690,12 @@ TEST_F(P2PTransportChannelTest, TurnToTurnPresumedWritable) { TEST_F(P2PTransportChannelTest, TurnToPrflxPresumedWritable) { rtc::ScopedFakeClock fake_clock; + // We need to add artificial network delay to verify that the connection + // is presumed writable before it's actually writable. Without this delay + // it would become writable instantly. + virtual_socket_server()->set_delay_mean(50); + virtual_socket_server()->UpdateDelayDistribution(); + ConfigureEndpoints(NAT_SYMMETRIC, NAT_SYMMETRIC, kDefaultPortAllocatorFlags, kDefaultPortAllocatorFlags); // We want the remote TURN candidate to show up as prflx. To do this we need @@ -2291,7 +2284,6 @@ TEST_F(P2PTransportChannelPingTest, TestTriggeredChecks) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("trigger checks", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 2)); @@ -2316,7 +2308,6 @@ TEST_F(P2PTransportChannelPingTest, TestAllConnectionsPingedSufficiently) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("ping sufficiently", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 2)); @@ -2345,7 +2336,6 @@ TEST_F(P2PTransportChannelPingTest, TestStunPingIntervals) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("TestChannel", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); Connection* conn = WaitForConnectionTo(&ch, "1.1.1.1", 1); @@ -2419,11 +2409,51 @@ TEST_F(P2PTransportChannelPingTest, TestStunPingIntervals) { STABILIZING_WRITABLE_CONNECTION_PING_INTERVAL + SCHEDULING_RANGE); } +// Test that we start pinging as soon as we have a connection and remote ICE +// credentials. +TEST_F(P2PTransportChannelPingTest, PingingStartedAsSoonAsPossible) { + rtc::ScopedFakeClock clock; + + FakePortAllocator pa(rtc::Thread::Current(), nullptr); + P2PTransportChannel ch("TestChannel", 1, &pa); + ch.SetIceRole(ICEROLE_CONTROLLING); + ch.SetIceCredentials(kIceUfrag[0], kIcePwd[0]); + ch.MaybeStartGathering(); + EXPECT_EQ_WAIT(IceGatheringState::kIceGatheringComplete, ch.gathering_state(), + kDefaultTimeout); + + // Simulate a binding request being received, creating a peer reflexive + // candidate pair while we still don't have remote ICE credentials. + IceMessage request; + request.SetType(STUN_BINDING_REQUEST); + request.AddAttribute( + new StunByteStringAttribute(STUN_ATTR_USERNAME, kIceUfrag[1])); + uint32_t prflx_priority = ICE_TYPE_PREFERENCE_PRFLX << 24; + request.AddAttribute( + new StunUInt32Attribute(STUN_ATTR_PRIORITY, prflx_priority)); + Port* port = GetPort(&ch); + ASSERT_NE(nullptr, port); + port->SignalUnknownAddress(port, rtc::SocketAddress("1.1.1.1", 1), PROTO_UDP, + &request, kIceUfrag[1], false); + Connection* conn = GetConnectionTo(&ch, "1.1.1.1", 1); + ASSERT_NE(nullptr, conn); + + // Simulate waiting for a second (and change) and verify that no pings were + // sent, since we don't yet have remote ICE credentials. + SIMULATED_WAIT(conn->num_pings_sent() > 0, 1025, clock); + EXPECT_EQ(0, conn->num_pings_sent()); + + // Set remote ICE credentials. Now we should be able to ping. Ensure that + // the first ping is sent as soon as possible, within one simulated clock + // tick. + ch.SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]); + EXPECT_TRUE_SIMULATED_WAIT(conn->num_pings_sent() > 0, 1, clock); +} + TEST_F(P2PTransportChannelPingTest, TestNoTriggeredChecksWhenWritable) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("trigger checks", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 2)); @@ -2449,7 +2479,6 @@ TEST_F(P2PTransportChannelPingTest, TestFailedConnectionNotPingable) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("Do not ping failed connections", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); @@ -2467,7 +2496,6 @@ TEST_F(P2PTransportChannelPingTest, TestSignalStateChanged) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("state change", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); @@ -2488,7 +2516,6 @@ TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithVariousUfrags) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("add candidate", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); // Add a candidate with a future ufrag. ch.AddRemoteCandidate( @@ -2541,7 +2568,6 @@ TEST_F(P2PTransportChannelPingTest, ConnectionResurrection) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("connection resurrection", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); // Create conn1 and keep track of original candidate priority. @@ -2601,7 +2627,6 @@ TEST_F(P2PTransportChannelPingTest, TestReceivingStateChange) { ch.SetIceConfig(CreateIceConfig(500, false)); EXPECT_EQ(500, ch.receiving_timeout()); EXPECT_EQ(50, ch.check_receiving_interval()); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); @@ -2625,7 +2650,6 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { P2PTransportChannel ch("receiving state change", 1, &pa); PrepareChannel(&ch); ch.SetIceRole(ICEROLE_CONTROLLED); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); @@ -2704,7 +2728,6 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionFromUnknownAddress) { P2PTransportChannel ch("receiving state change", 1, &pa); PrepareChannel(&ch); ch.SetIceRole(ICEROLE_CONTROLLED); - ch.Connect(); ch.MaybeStartGathering(); // A minimal STUN message with prflx priority. IceMessage request; @@ -2785,7 +2808,6 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBasedOnMediaReceived) { P2PTransportChannel ch("receiving state change", 1, &pa); PrepareChannel(&ch); ch.SetIceRole(ICEROLE_CONTROLLED); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 10)); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); @@ -2838,7 +2860,6 @@ TEST_F(P2PTransportChannelPingTest, P2PTransportChannel ch("SwitchSelectedConnection", 1, &pa); PrepareChannel(&ch); ch.SetIceRole(ICEROLE_CONTROLLED); - ch.Connect(); ch.MaybeStartGathering(); // The connections have decreasing priority. Connection* conn1 = @@ -2878,7 +2899,6 @@ TEST_F(P2PTransportChannelPingTest, P2PTransportChannel ch("SwitchSelectedConnection", 1, &pa); PrepareChannel(&ch); ch.SetIceRole(ICEROLE_CONTROLLED); - ch.Connect(); ch.MaybeStartGathering(); // The connections have decreasing priority. Connection* conn1 = @@ -2924,7 +2944,6 @@ TEST_F(P2PTransportChannelPingTest, P2PTransportChannel ch("SwitchSelectedConnection", 1, &pa); PrepareChannel(&ch); ch.SetIceRole(ICEROLE_CONTROLLED); - ch.Connect(); ch.MaybeStartGathering(); // The connections have decreasing priority. Connection* conn1 = @@ -2963,7 +2982,6 @@ TEST_F(P2PTransportChannelPingTest, TestAddRemoteCandidateWithAddressReuse) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("candidate reuse", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); const std::string host_address = "1.1.1.1"; const int port_num = 1; @@ -3003,7 +3021,6 @@ TEST_F(P2PTransportChannelPingTest, TestDontPruneWhenWeak) { P2PTransportChannel ch("test channel", 1, &pa); PrepareChannel(&ch); ch.SetIceRole(ICEROLE_CONTROLLED); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); @@ -3041,7 +3058,6 @@ TEST_F(P2PTransportChannelPingTest, TestDontPruneHighPriorityConnections) { P2PTransportChannel ch("test channel", 1, &pa); PrepareChannel(&ch); ch.SetIceRole(ICEROLE_CONTROLLED); - ch.Connect(); ch.MaybeStartGathering(); Connection* conn1 = CreateConnectionWithCandidate(ch, clock, "1.1.1.1", 1, 100, true); @@ -3063,7 +3079,6 @@ TEST_F(P2PTransportChannelPingTest, TestGetState) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("test channel", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); EXPECT_EQ(TransportChannelState::STATE_INIT, ch.GetState()); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 100)); @@ -3090,7 +3105,6 @@ TEST_F(P2PTransportChannelPingTest, TestConnectionPrunedAgain) { P2PTransportChannel ch("test channel", 1, &pa); PrepareChannel(&ch); ch.SetIceConfig(CreateIceConfig(1000, false)); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 100)); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); @@ -3132,7 +3146,6 @@ TEST_F(P2PTransportChannelPingTest, TestDeleteConnectionsIfAllWriteTimedout) { FakePortAllocator pa(rtc::Thread::Current(), nullptr); P2PTransportChannel ch("test channel", 1, &pa); PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); // Have one connection only but later becomes write-time-out. ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 100)); @@ -3166,7 +3179,6 @@ TEST_F(P2PTransportChannelPingTest, TestStopPortAllocatorSessions) { P2PTransportChannel ch("test channel", 1, &pa); PrepareChannel(&ch); ch.SetIceConfig(CreateIceConfig(2000, false)); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 100)); Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); @@ -3204,7 +3216,6 @@ TEST_F(P2PTransportChannelPingTest, PrepareChannel(&ch); IceConfig config = CreateIceConfig(1000, true); ch.SetIceConfig(config); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); @@ -3227,7 +3238,6 @@ TEST_F(P2PTransportChannelPingTest, TestIceRoleUpdatedOnPortAfterIceRestart) { P2PTransportChannel ch("test channel", ICE_CANDIDATE_COMPONENT_DEFAULT, &pa); // Starts with ICEROLE_CONTROLLING. PrepareChannel(&ch); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); @@ -3252,7 +3262,6 @@ TEST_F(P2PTransportChannelPingTest, TestPortDestroyedAfterTimeout) { PrepareChannel(&ch); // Only a controlled channel should expect its ports to be destroyed. ch.SetIceRole(ICEROLE_CONTROLLED); - ch.Connect(); ch.MaybeStartGathering(); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "1.1.1.1", 1, 1)); @@ -3297,7 +3306,6 @@ class P2PTransportChannelMostLikelyToWorkFirstTest stable_writable_connection_ping_interval; channel_->SetIceConfig(config); PrepareChannel(channel_.get()); - channel_->Connect(); channel_->MaybeStartGathering(); return *channel_.get(); } diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index ca162b86fa..71a9e6b2b1 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -926,6 +926,7 @@ void Connection::set_connected(bool value) { if (value != old_value) { LOG_J(LS_VERBOSE, this) << "set_connected from: " << old_value << " to " << value; + SignalStateChange(this); } } @@ -1235,7 +1236,7 @@ bool Connection::dead(int64_t now) const { return now > (time_created_ms_ + MIN_CONNECTION_LIFETIME); } -bool Connection::stable(int64_t now) { +bool Connection::stable(int64_t now) const { // A connection is stable if it's RTT has converged and it isn't missing any // responses. We should send pings at a higher rate until the RTT converges // and whenever a ping response is missing (so that we can detect @@ -1507,11 +1508,11 @@ void Connection::MaybeAddPrflxCandidate(ConnectionRequest* request, SignalStateChange(this); } -bool Connection::rtt_converged() { +bool Connection::rtt_converged() const { return rtt_samples_ > (RTT_RATIO + 1); } -bool Connection::missing_responses(int64_t now) { +bool Connection::missing_responses(int64_t now) const { if (pings_since_last_response_.empty()) { return false; } diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 6cae6f8ae9..2cd264e09e 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -593,7 +593,7 @@ class Connection : public CandidatePairInterface, // response in milliseconds int64_t last_received() const; - bool stable(int64_t now); + bool stable(int64_t now) const; protected: enum { MSG_DELETE = 0, MSG_FIRST_AVAILABLE }; @@ -612,11 +612,11 @@ class Connection : public CandidatePairInterface, void OnConnectionRequestTimeout(ConnectionRequest* req); void OnConnectionRequestSent(ConnectionRequest* req); - bool rtt_converged(); + bool rtt_converged() const; // If the response is not received within 2 * RTT, the response is assumed to // be missing. - bool missing_responses(int64_t now); + bool missing_responses(int64_t now) const; // Changes the state and signals if necessary. void set_write_state(WriteState value); diff --git a/webrtc/p2p/base/transport.cc b/webrtc/p2p/base/transport.cc index 33c52776ad..9fbe9524c3 100644 --- a/webrtc/p2p/base/transport.cc +++ b/webrtc/p2p/base/transport.cc @@ -114,7 +114,6 @@ bool Transport::SetLocalTransportDescription( } if (ret) { local_description_set_ = true; - ConnectChannels(); } return ret; @@ -181,9 +180,6 @@ TransportChannelImpl* Transport::CreateChannel(int component) { if (local_description_ && remote_description_) ApplyNegotiatedTransportDescription(channel, nullptr); - if (connect_requested_) { - channel->Connect(); - } return channel; } @@ -206,21 +202,8 @@ void Transport::DestroyChannel(int component) { DestroyTransportChannel(channel); } -void Transport::ConnectChannels() { - if (connect_requested_ || channels_.empty()) - return; - - connect_requested_ = true; - - RTC_DCHECK(local_description_); - - CallChannels(&TransportChannelImpl::Connect); -} - void Transport::MaybeStartGathering() { - if (connect_requested_) { - CallChannels(&TransportChannelImpl::MaybeStartGathering); - } + CallChannels(&TransportChannelImpl::MaybeStartGathering); } void Transport::DestroyAllChannels() { diff --git a/webrtc/p2p/base/transport.h b/webrtc/p2p/base/transport.h index 1583b710db..9269891577 100644 --- a/webrtc/p2p/base/transport.h +++ b/webrtc/p2p/base/transport.h @@ -214,9 +214,6 @@ class Transport : public sigslot::has_slots<> { return local_description_set_ && remote_description_set_; } - // Returns whether the client has requested the channels to connect. - bool connect_requested() const { return connect_requested_; } - void SetIceRole(IceRole role); IceRole ice_role() const { return ice_role_; } @@ -260,9 +257,6 @@ class Transport : public sigslot::has_slots<> { ContentAction action, std::string* error_desc); - // Tells all current and future channels to start connecting. - void ConnectChannels(); - // Tells channels to start gathering candidates if necessary. // Should be called after ConnectChannels() has been called at least once, // which will happen in SetLocalTransportDescription. @@ -364,7 +358,6 @@ class Transport : public sigslot::has_slots<> { const std::string name_; PortAllocator* const allocator_; bool channels_destroyed_ = false; - bool connect_requested_ = false; IceRole ice_role_ = ICEROLE_UNKNOWN; uint64_t tiebreaker_ = 0; IceMode remote_ice_mode_ = ICEMODE_FULL; diff --git a/webrtc/p2p/base/transport_unittest.cc b/webrtc/p2p/base/transport_unittest.cc index d5c9368440..d119e83367 100644 --- a/webrtc/p2p/base/transport_unittest.cc +++ b/webrtc/p2p/base/transport_unittest.cc @@ -239,7 +239,6 @@ TEST_F(TransportTest, TestGetStats) { cricket::CONNECTIONROLE_NONE, nullptr); transport_->SetLocalTransportDescription(faketransport_desc, cricket::CA_OFFER, nullptr); - transport_->ConnectChannels(); EXPECT_TRUE(transport_->GetStats(&stats)); ASSERT_EQ(1U, stats.channel_stats.size()); EXPECT_EQ(1, stats.channel_stats[0].component); diff --git a/webrtc/p2p/base/transportchannelimpl.h b/webrtc/p2p/base/transportchannelimpl.h index 70cb4e292f..59065a10a8 100644 --- a/webrtc/p2p/base/transportchannelimpl.h +++ b/webrtc/p2p/base/transportchannelimpl.h @@ -46,7 +46,7 @@ class TransportChannelImpl : public TransportChannel { virtual void SetIceProtocolType(IceProtocolType type) {} // SetIceCredentials only need to be implemented by the ICE // transport channels. Non-ICE transport channels can just ignore. - // The ufrag and pwd should be set before the Connect() is called. + // The ufrag and pwd must be set before candidate gathering can start. virtual void SetIceCredentials(const std::string& ice_ufrag, const std::string& ice_pwd) = 0; // SetRemoteIceCredentials only need to be implemented by the ICE @@ -59,9 +59,6 @@ class TransportChannelImpl : public TransportChannel { virtual void SetIceConfig(const IceConfig& config) = 0; - // Begins the process of attempting to make a connection to the other client. - virtual void Connect() = 0; - // Start gathering candidates if not already started, or if an ICE restart // occurred. virtual void MaybeStartGathering() = 0; diff --git a/webrtc/p2p/base/transportcontroller_unittest.cc b/webrtc/p2p/base/transportcontroller_unittest.cc index 4ca481f241..68b66cf994 100644 --- a/webrtc/p2p/base/transportcontroller_unittest.cc +++ b/webrtc/p2p/base/transportcontroller_unittest.cc @@ -587,7 +587,6 @@ TEST_F(TransportControllerTest, TestSignalReceiving) { TEST_F(TransportControllerTest, TestSignalGatheringStateGathering) { FakeTransportChannel* channel = CreateChannel("audio", 1); ASSERT_NE(nullptr, channel); - channel->Connect(); channel->MaybeStartGathering(); // Should be in the gathering state as soon as any transport starts gathering. EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); @@ -602,7 +601,6 @@ TEST_F(TransportControllerTest, TestSignalGatheringStateComplete) { FakeTransportChannel* channel3 = CreateChannel("data", 1); ASSERT_NE(nullptr, channel3); - channel3->Connect(); channel3->MaybeStartGathering(); EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); EXPECT_EQ(1, gathering_state_signal_count_); @@ -615,9 +613,7 @@ TEST_F(TransportControllerTest, TestSignalGatheringStateComplete) { EXPECT_EQ(2, gathering_state_signal_count_); // Make remaining channels start and then finish gathering. - channel1->Connect(); channel1->MaybeStartGathering(); - channel2->Connect(); channel2->MaybeStartGathering(); EXPECT_EQ_WAIT(cricket::kIceGatheringGathering, gathering_state_, kTimeout); EXPECT_EQ(3, gathering_state_signal_count_);