diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index a5f65376ce..a109b0ae96 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -15,6 +15,7 @@ #include #include "absl/algorithm/container.h" +#include "absl/memory/memory.h" #include "api/candidate.h" #include "logging/rtc_event_log/ice_logger.h" #include "p2p/base/candidate_pair_interface.h" @@ -148,8 +149,14 @@ P2PTransportChannel::P2PTransportChannel( webrtc::BasicRegatheringController::Config regathering_config( config_.regather_all_networks_interval_range, config_.regather_on_failed_networks_interval_or_default()); - regathering_controller_.reset(new webrtc::BasicRegatheringController( - regathering_config, this, network_thread_)); + regathering_controller_ = + absl::make_unique( + regathering_config, this, network_thread_); + RTC_DCHECK(allocator_ != nullptr); + // We populate the change in the candidate filter to the session taken by + // the transport. + allocator_->SignalCandidateFilterChanged.connect( + this, &P2PTransportChannel::OnCandidateFilterChanged); ice_event_log_.set_event_log(event_log); } @@ -999,6 +1006,14 @@ void P2PTransportChannel::OnUnknownAddress( "a new candidate pair created from an unknown remote address"); } +void P2PTransportChannel::OnCandidateFilterChanged(uint32_t prev_filter, + uint32_t cur_filter) { + if (prev_filter == cur_filter || allocator_session() == nullptr) { + return; + } + allocator_session()->SetCandidateFilter(cur_filter); +} + void P2PTransportChannel::OnRoleConflict(PortInterface* port) { SignalRoleConflict(this); // STUN ping will be sent when SetRole is called // from Transport. diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index 467ef258b5..83716467b3 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -292,6 +292,7 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { IceMessage* stun_msg, const std::string& remote_username, bool port_muxed); + void OnCandidateFilterChanged(uint32_t prev_filter, uint32_t cur_filter); // When a port is destroyed, remove it from both lists |ports_| // and |pruned_ports_|. diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index a92f4ff6aa..f61d21fcc0 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -50,6 +50,7 @@ using ::testing::InvokeWithoutArgs; using ::testing::NiceMock; using ::testing::Return; using ::testing::SetArgPointee; +using ::testing::SizeIs; // Default timeout for tests in this file. // Should be large enough for slow buildbots to run the tests reliably. @@ -1606,8 +1607,8 @@ TEST_F(P2PTransportChannelTest, kDefaultPortAllocatorFlags); // Only gather relay candidates, so that when the prflx candidate arrives // it's prioritized above the current candidate pair. - GetEndpoint(0)->allocator_->set_candidate_filter(CF_RELAY); - GetEndpoint(1)->allocator_->set_candidate_filter(CF_RELAY); + GetEndpoint(0)->allocator_->SetCandidateFilter(CF_RELAY); + GetEndpoint(1)->allocator_->SetCandidateFilter(CF_RELAY); // Setting this allows us to control when SetRemoteIceParameters is called. set_remote_ice_parameter_source(FROM_CANDIDATE); CreateChannels(); @@ -4587,7 +4588,8 @@ TEST(P2PTransportChannelResolverTest, HostnameCandidateIsResolved) { EXPECT_CALL(mock_async_resolver_factory, Create()) .WillOnce(Return(&mock_async_resolver)); - P2PTransportChannel channel("tn", 0, /*allocator*/ nullptr, + FakePortAllocator allocator(rtc::Thread::Current(), nullptr); + P2PTransportChannel channel("tn", 0, &allocator, &mock_async_resolver_factory); Candidate hostname_candidate; SocketAddress hostname_address("fake.test", 1000); @@ -4888,4 +4890,191 @@ TEST_F(P2PTransportChannelTest, DestroyChannels(); } +// Test that after changing the candidate filter from relay-only to allowing all +// types of candidates when doing continual gathering, we can gather without ICE +// restart the other types of candidates that are now enabled and form candidate +// pairs. Also, we verify that the relay candidates gathered previously are not +// removed and are still usable for necessary route switching. +TEST_F(P2PTransportChannelTest, + SurfaceHostCandidateOnCandidateFilterChangeFromRelayToAll) { + rtc::ScopedFakeClock clock; + + ConfigureEndpoints( + OPEN, OPEN, + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET, + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET); + auto* ep1 = GetEndpoint(0); + auto* ep2 = GetEndpoint(1); + ep1->allocator_->SetCandidateFilter(CF_RELAY); + ep2->allocator_->SetCandidateFilter(CF_RELAY); + IceConfig continual_gathering_config = + CreateIceConfig(1000, GATHER_CONTINUALLY); + CreateChannels(continual_gathering_config, continual_gathering_config); + ASSERT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection() != nullptr, + kDefaultTimeout, clock); + ASSERT_TRUE_SIMULATED_WAIT(ep2_ch1()->selected_connection() != nullptr, + kDefaultTimeout, clock); + EXPECT_EQ(RELAY_PORT_TYPE, + ep1_ch1()->selected_connection()->local_candidate().type()); + EXPECT_EQ(RELAY_PORT_TYPE, + ep2_ch1()->selected_connection()->local_candidate().type()); + + // Loosen the candidate filter at ep1. + ep1->allocator_->SetCandidateFilter(CF_ALL); + EXPECT_TRUE_SIMULATED_WAIT( + ep1_ch1()->selected_connection() != nullptr && + ep1_ch1()->selected_connection()->local_candidate().type() == + LOCAL_PORT_TYPE, + kDefaultTimeout, clock); + EXPECT_EQ(RELAY_PORT_TYPE, + ep1_ch1()->selected_connection()->remote_candidate().type()); + + // Loosen the candidate filter at ep2. + ep2->allocator_->SetCandidateFilter(CF_ALL); + EXPECT_TRUE_SIMULATED_WAIT( + ep2_ch1()->selected_connection() != nullptr && + ep2_ch1()->selected_connection()->local_candidate().type() == + LOCAL_PORT_TYPE, + kDefaultTimeout, clock); + // We have migrated to a host-host candidate pair. + EXPECT_EQ(LOCAL_PORT_TYPE, + ep2_ch1()->selected_connection()->remote_candidate().type()); + + // Block the traffic over non-relay-to-relay routes and expect a route change. + fw()->AddRule(false, rtc::FP_ANY, kPublicAddrs[0], kPublicAddrs[1]); + fw()->AddRule(false, rtc::FP_ANY, kPublicAddrs[1], kPublicAddrs[0]); + fw()->AddRule(false, rtc::FP_ANY, kPublicAddrs[0], kTurnUdpExtAddr); + fw()->AddRule(false, rtc::FP_ANY, kPublicAddrs[1], kTurnUdpExtAddr); + // We should be able to reuse the previously gathered relay candidates. + EXPECT_EQ_SIMULATED_WAIT( + RELAY_PORT_TYPE, + ep1_ch1()->selected_connection()->local_candidate().type(), + kDefaultTimeout, clock); + EXPECT_EQ(RELAY_PORT_TYPE, + ep1_ch1()->selected_connection()->remote_candidate().type()); +} + +// A similar test as SurfaceHostCandidateOnCandidateFilterChangeFromRelayToAll, +// and we should surface server-reflexive candidates that are enabled after +// changing the candidate filter. +TEST_F(P2PTransportChannelTest, + SurfaceSrflxCandidateOnCandidateFilterChangeFromRelayToNoHost) { + rtc::ScopedFakeClock clock; + // We need an actual NAT so that the host candidate is not equivalent to the + // srflx candidate; otherwise, the host candidate would still surface even + // though we disable it via the candidate filter below. This is a result of + // the following limitation in the current implementation: + // 1. We don't generate the srflx candidate when we have public IP. + // 2. We keep the host candidate in this case in CheckCandidateFilter even + // though we intend to filter them. + ConfigureEndpoints( + NAT_FULL_CONE, NAT_FULL_CONE, + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET, + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET); + auto* ep1 = GetEndpoint(0); + auto* ep2 = GetEndpoint(1); + ep1->allocator_->SetCandidateFilter(CF_RELAY); + ep2->allocator_->SetCandidateFilter(CF_RELAY); + IceConfig continual_gathering_config = + CreateIceConfig(1000, GATHER_CONTINUALLY); + CreateChannels(continual_gathering_config, continual_gathering_config); + ASSERT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection() != nullptr, + kDefaultTimeout, clock); + ASSERT_TRUE_SIMULATED_WAIT(ep2_ch1()->selected_connection() != nullptr, + kDefaultTimeout, clock); + const uint32_t kCandidateFilterNoHost = CF_ALL & ~CF_HOST; + // Loosen the candidate filter at ep1. + ep1->allocator_->SetCandidateFilter(kCandidateFilterNoHost); + EXPECT_TRUE_SIMULATED_WAIT( + ep1_ch1()->selected_connection() != nullptr && + ep1_ch1()->selected_connection()->local_candidate().type() == + STUN_PORT_TYPE, + kDefaultTimeout, clock); + EXPECT_EQ(RELAY_PORT_TYPE, + ep1_ch1()->selected_connection()->remote_candidate().type()); + + // Loosen the candidate filter at ep2. + ep2->allocator_->SetCandidateFilter(kCandidateFilterNoHost); + EXPECT_TRUE_SIMULATED_WAIT( + ep2_ch1()->selected_connection() != nullptr && + ep2_ch1()->selected_connection()->local_candidate().type() == + STUN_PORT_TYPE, + kDefaultTimeout, clock); + // We have migrated to a srflx-srflx candidate pair. + EXPECT_EQ(STUN_PORT_TYPE, + ep2_ch1()->selected_connection()->remote_candidate().type()); + + // Block the traffic over non-relay-to-relay routes and expect a route change. + fw()->AddRule(false, rtc::FP_ANY, kPrivateAddrs[0], kPublicAddrs[1]); + fw()->AddRule(false, rtc::FP_ANY, kPrivateAddrs[1], kPublicAddrs[0]); + fw()->AddRule(false, rtc::FP_ANY, kPrivateAddrs[0], kTurnUdpExtAddr); + fw()->AddRule(false, rtc::FP_ANY, kPrivateAddrs[1], kTurnUdpExtAddr); + // We should be able to reuse the previously gathered relay candidates. + EXPECT_EQ_SIMULATED_WAIT( + RELAY_PORT_TYPE, + ep1_ch1()->selected_connection()->local_candidate().type(), + kDefaultTimeout, clock); + EXPECT_EQ(RELAY_PORT_TYPE, + ep1_ch1()->selected_connection()->remote_candidate().type()); +} + +// Test that when the candidate filter is updated to be more restrictive, +// candidates that 1) have already been gathered and signaled 2) but no longer +// match the filter, are not removed. +TEST_F(P2PTransportChannelTest, + RestrictingCandidateFilterDoesNotRemoveRegatheredCandidates) { + rtc::ScopedFakeClock clock; + + ConfigureEndpoints( + OPEN, OPEN, + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET, + kDefaultPortAllocatorFlags | PORTALLOCATOR_ENABLE_SHARED_SOCKET); + auto* ep1 = GetEndpoint(0); + auto* ep2 = GetEndpoint(1); + ep1->allocator_->SetCandidateFilter(CF_ALL); + ep2->allocator_->SetCandidateFilter(CF_ALL); + IceConfig continual_gathering_config = + CreateIceConfig(1000, GATHER_CONTINUALLY); + // Pause candidates so we can gather all types of candidates. See + // P2PTransportChannel::OnConnectionStateChange, where we would stop the + // gathering when we have a strongly connected candidate pair. + PauseCandidates(0); + PauseCandidates(1); + CreateChannels(continual_gathering_config, continual_gathering_config); + + // We have gathered host, srflx and relay candidates. + EXPECT_TRUE_SIMULATED_WAIT(ep1->saved_candidates_.size() == 3u, + kDefaultTimeout, clock); + ResumeCandidates(0); + ResumeCandidates(1); + ASSERT_TRUE_SIMULATED_WAIT(ep1_ch1()->selected_connection() != nullptr, + kDefaultTimeout, clock); + ASSERT_TRUE_SIMULATED_WAIT(ep2_ch1()->selected_connection() != nullptr, + kDefaultTimeout, clock); + // Test that we have a host-host candidate pair selected and the number of + // candidates signaled to the remote peer stays the same. + auto test_invariants = [this]() { + EXPECT_EQ(LOCAL_PORT_TYPE, + ep1_ch1()->selected_connection()->local_candidate().type()); + EXPECT_EQ(LOCAL_PORT_TYPE, + ep1_ch1()->selected_connection()->remote_candidate().type()); + EXPECT_THAT(ep2_ch1()->remote_candidates(), SizeIs(3)); + }; + + test_invariants(); + + // Set a more restrictive candidate filter at ep1. + ep1->allocator_->SetCandidateFilter(CF_HOST | CF_REFLEXIVE); + SIMULATED_WAIT(false, kDefaultTimeout, clock); + test_invariants(); + + ep1->allocator_->SetCandidateFilter(CF_HOST); + SIMULATED_WAIT(false, kDefaultTimeout, clock); + test_invariants(); + + ep1->allocator_->SetCandidateFilter(CF_NONE); + SIMULATED_WAIT(false, kDefaultTimeout, clock); + test_invariants(); +} + } // namespace cricket diff --git a/p2p/base/port_allocator.cc b/p2p/base/port_allocator.cc index 4dee0fa5e0..62287912ed 100644 --- a/p2p/base/port_allocator.cc +++ b/p2p/base/port_allocator.cc @@ -290,6 +290,16 @@ void PortAllocator::DiscardCandidatePool() { pooled_sessions_.clear(); } +void PortAllocator::SetCandidateFilter(uint32_t filter) { + CheckRunOnValidThreadIfInitialized(); + if (candidate_filter_ == filter) { + return; + } + uint32_t prev_filter = candidate_filter_; + candidate_filter_ = filter; + SignalCandidateFilterChanged(prev_filter, filter); +} + void PortAllocator::GetCandidateStatsFromPooledSessions( CandidateStatsList* candidate_stats_list) { CheckRunOnValidThreadAndInitialized(); diff --git a/p2p/base/port_allocator.h b/p2p/base/port_allocator.h index e0cc775290..d0605b6bb6 100644 --- a/p2p/base/port_allocator.h +++ b/p2p/base/port_allocator.h @@ -528,10 +528,22 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { return candidate_filter_; } - void set_candidate_filter(uint32_t filter) { - CheckRunOnValidThreadIfInitialized(); - candidate_filter_ = filter; - } + // The new filter value will be populated to future allocation sessions, when + // they are created via CreateSession, and also pooled sessions when one is + // taken via TakePooledSession. + // + // A change in the candidate filter also fires a signal + // |SignalCandidateFilterChanged|, so that objects subscribed to this signal + // can, for example, update the candidate filter for sessions created by this + // allocator and already taken by the object. + // + // Specifically for the session taken by the ICE transport, we currently do + // not support removing candidate pairs formed with local candidates from this + // session that are disabled by the new candidate filter. + void SetCandidateFilter(uint32_t filter); + // Deprecated. + // TODO(qingsi): Remove this after Chromium migrates to the new method. + void set_candidate_filter(uint32_t filter) { SetCandidateFilter(filter); } bool prune_turn_ports() const { CheckRunOnValidThreadIfInitialized(); @@ -565,6 +577,10 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { // Return IceParameters of the pooled sessions. std::vector GetPooledIceCredentials(); + // Fired when |candidate_filter_| changes. + sigslot::signal2 + SignalCandidateFilterChanged; + protected: virtual PortAllocatorSession* CreateSessionInternal( const std::string& content_name, diff --git a/p2p/base/port_allocator_unittest.cc b/p2p/base/port_allocator_unittest.cc index a9edbeccaf..9d6b4dd3df 100644 --- a/p2p/base/port_allocator_unittest.cc +++ b/p2p/base/port_allocator_unittest.cc @@ -100,7 +100,7 @@ TEST_F(PortAllocatorTest, TestDefaults) { // Call CreateSession and verify that the parameters passed in and the // candidate filter are applied as expected. TEST_F(PortAllocatorTest, CreateSession) { - allocator_->set_candidate_filter(cricket::CF_RELAY); + allocator_->SetCandidateFilter(cricket::CF_RELAY); auto session = CreateSession(kContentName, 1, kIceUfrag, kIcePwd); ASSERT_NE(nullptr, session); EXPECT_EQ(cricket::CF_RELAY, session->candidate_filter()); @@ -258,7 +258,7 @@ TEST_F(PortAllocatorTest, TakePooledSessionUpdatesIceParameters) { // session is taken. So a pooled session should gather candidates // unfiltered until it's returned by TakePooledSession. TEST_F(PortAllocatorTest, TakePooledSessionUpdatesCandidateFilter) { - allocator_->set_candidate_filter(cricket::CF_RELAY); + allocator_->SetCandidateFilter(cricket::CF_RELAY); SetConfigurationWithPoolSize(1); auto peeked_session = GetPooledSession(); ASSERT_NE(nullptr, peeked_session); diff --git a/p2p/client/basic_port_allocator.cc b/p2p/client/basic_port_allocator.cc index 83c8bf2b95..4f418ee7f6 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -31,6 +31,7 @@ using rtc::CreateRandomId; +namespace cricket { namespace { enum { @@ -112,9 +113,37 @@ void FilterNetworks(NetworkList* networks, NetworkFilter filter) { networks->erase(start_to_remove, networks->end()); } +bool IsAllowedByCandidateFilter(const Candidate& c, uint32_t filter) { + // When binding to any address, before sending packets out, the getsockname + // returns all 0s, but after sending packets, it'll be the NIC used to + // send. All 0s is not a valid ICE candidate address and should be filtered + // out. + if (c.address().IsAnyIP()) { + return false; + } + + if (c.type() == RELAY_PORT_TYPE) { + return ((filter & CF_RELAY) != 0); + } else if (c.type() == STUN_PORT_TYPE) { + return ((filter & CF_REFLEXIVE) != 0); + } else if (c.type() == LOCAL_PORT_TYPE) { + if ((filter & CF_REFLEXIVE) && !c.address().IsPrivateIP()) { + // We allow host candidates if the filter allows server-reflexive + // candidates and the candidate is a public IP. Because we don't generate + // server-reflexive candidates if they have the same IP as the host + // candidate (i.e. when the host candidate is a public IP), filtering to + // only server-reflexive candidates won't work right when the host + // candidates have public IPs. + return true; + } + + return ((filter & CF_HOST) != 0); + } + return false; +} + } // namespace -namespace cricket { const uint32_t DISABLE_ALL_PHASES = PORTALLOCATOR_DISABLE_UDP | PORTALLOCATOR_DISABLE_TCP | PORTALLOCATOR_DISABLE_STUN | PORTALLOCATOR_DISABLE_RELAY; @@ -307,20 +336,57 @@ void BasicPortAllocatorSession::SetCandidateFilter(uint32_t filter) { if (filter == candidate_filter_) { return; } - // We assume the filter will only change from "ALL" to something else. - RTC_DCHECK(candidate_filter_ == CF_ALL); + uint32_t prev_filter = candidate_filter_; candidate_filter_ = filter; - for (PortData& port : ports_) { - if (!port.has_pairable_candidate()) { + for (PortData& port_data : ports_) { + if (port_data.error() || port_data.pruned()) { continue; } + PortData::State cur_state = port_data.state(); + bool found_signalable_candidate = false; + bool found_pairable_candidate = false; + cricket::Port* port = port_data.port(); + for (const auto& c : port->Candidates()) { + if (!IsStopped() && !IsAllowedByCandidateFilter(c, prev_filter) && + IsAllowedByCandidateFilter(c, filter)) { + // This candidate was not signaled because of not matching the previous + // filter (see OnCandidateReady below). Let the Port to fire the signal + // again. + // + // Note that + // 1) we would need the Port to enter the state of in-progress of + // gathering to have candidates signaled; + // + // 2) firing the signal would also let the session set the port ready + // if needed, so that we could form candidate pairs with candidates + // from this port; + // + // * See again OnCandidateReady below for 1) and 2). + // + // 3) we only try to resurface candidates if we have not stopped + // getting ports, which is always true for the continual gathering. + if (!found_signalable_candidate) { + found_signalable_candidate = true; + port_data.set_state(PortData::STATE_INPROGRESS); + } + port->SignalCandidateReady(port, c); + } + + if (CandidatePairable(c, port)) { + found_pairable_candidate = true; + } + } + // Restore the previous state. + port_data.set_state(cur_state); // Setting a filter may cause a ready port to become non-ready // if it no longer has any pairable candidates. - if (absl::c_none_of(port.port()->Candidates(), - [this, &port](const Candidate& candidate) { - return CandidatePairable(candidate, port.port()); - })) { - port.set_has_pairable_candidate(false); + // + // Note that we only set for the negative case here, since a port would be + // set to have pairable candidates when it signals a ready candidate, which + // requires the port is still in the progress of gathering/surfacing + // candidates, and would be done in the firing of the signal above. + if (!found_pairable_candidate) { + port_data.set_has_pairable_candidate(false); } } } @@ -597,6 +663,7 @@ void BasicPortAllocatorSession::UpdateIceParametersInternal() { void BasicPortAllocatorSession::GetPortConfigurations() { RTC_DCHECK_RUN_ON(network_thread_); + PortConfiguration* config = new PortConfiguration(allocator_->stun_servers(), username(), password()); @@ -633,7 +700,7 @@ void BasicPortAllocatorSession::OnConfigStop() { if (it->inprogress()) { // Updating port state to error, which didn't finish allocating candidates // yet. - it->set_error(); + it->set_state(PortData::STATE_ERROR); send_signal = true; } } @@ -1025,7 +1092,7 @@ void BasicPortAllocatorSession::OnPortComplete(Port* port) { } // Moving to COMPLETE state. - data->set_complete(); + data->set_state(PortData::STATE_COMPLETE); // Send candidate allocation complete signal if this was the last port. MaybeSignalCandidatesAllocationDone(); } @@ -1043,7 +1110,7 @@ void BasicPortAllocatorSession::OnPortError(Port* port) { // SignalAddressError is currently sent from StunPort/TurnPort. // But this signal itself is generic. - data->set_error(); + data->set_state(PortData::STATE_ERROR); // Send candidate allocation complete signal if this was the last port. MaybeSignalCandidatesAllocationDone(); } @@ -1051,34 +1118,7 @@ void BasicPortAllocatorSession::OnPortError(Port* port) { bool BasicPortAllocatorSession::CheckCandidateFilter(const Candidate& c) const { RTC_DCHECK_RUN_ON(network_thread_); - uint32_t filter = candidate_filter_; - - // When binding to any address, before sending packets out, the getsockname - // returns all 0s, but after sending packets, it'll be the NIC used to - // send. All 0s is not a valid ICE candidate address and should be filtered - // out. - if (c.address().IsAnyIP()) { - return false; - } - - if (c.type() == RELAY_PORT_TYPE) { - return ((filter & CF_RELAY) != 0); - } else if (c.type() == STUN_PORT_TYPE) { - return ((filter & CF_REFLEXIVE) != 0); - } else if (c.type() == LOCAL_PORT_TYPE) { - if ((filter & CF_REFLEXIVE) && !c.address().IsPrivateIP()) { - // We allow host candidates if the filter allows server-reflexive - // candidates and the candidate is a public IP. Because we don't generate - // server-reflexive candidates if they have the same IP as the host - // candidate (i.e. when the host candidate is a public IP), filtering to - // only server-reflexive candidates won't work right when the host - // candidates have public IPs. - return true; - } - - return ((filter & CF_HOST) != 0); - } - return false; + return IsAllowedByCandidateFilter(c, candidate_filter_); } bool BasicPortAllocatorSession::CandidatePairable(const Candidate& c, @@ -1259,25 +1299,39 @@ void AllocationSequence::DisableEquivalentPhases(rtc::Network* network, // This can happen if, say, there's a network change event right before an // application-triggered ICE restart. Hopefully this problem will just go // away if we get rid of the gathering "phases" though, which is planned. + // + // + // PORTALLOCATOR_DISABLE_UDP is used to disable a Port from gathering the host + // candidate (and srflx candidate if Port::SharedSocket()), and we do not want + // to disable the gathering of these candidates just becaue of an existing + // Port over PROTO_UDP, namely a TurnPort over UDP. if (absl::c_any_of(session_->ports_, [this](const BasicPortAllocatorSession::PortData& p) { - return p.port()->Network() == network_ && + return !p.pruned() && p.port()->Network() == network_ && p.port()->GetProtocol() == PROTO_UDP && - !p.error(); + p.port()->Type() == LOCAL_PORT_TYPE && !p.error(); })) { *flags |= PORTALLOCATOR_DISABLE_UDP; } + // Similarly we need to check both the protocol used by an existing Port and + // its type. if (absl::c_any_of(session_->ports_, [this](const BasicPortAllocatorSession::PortData& p) { - return p.port()->Network() == network_ && + return !p.pruned() && p.port()->Network() == network_ && p.port()->GetProtocol() == PROTO_TCP && - !p.error(); + p.port()->Type() == LOCAL_PORT_TYPE && !p.error(); })) { *flags |= PORTALLOCATOR_DISABLE_TCP; } if (config_ && config) { - if (config_->StunServers() == config->StunServers()) { + // We need to regather srflx candidates if either of the following + // conditions occurs: + // 1. The STUN servers are different from the previous gathering. + // 2. We will regather host candidates, hence possibly inducing new NAT + // bindings. + if (config_->StunServers() == config->StunServers() && + (*flags & PORTALLOCATOR_DISABLE_UDP)) { // Already got this STUN servers covered. *flags |= PORTALLOCATOR_DISABLE_STUN; } diff --git a/p2p/client/basic_port_allocator.h b/p2p/client/basic_port_allocator.h index edc6b87cb0..26eea1ef52 100644 --- a/p2p/client/basic_port_allocator.h +++ b/p2p/client/basic_port_allocator.h @@ -125,6 +125,15 @@ class RTC_EXPORT BasicPortAllocatorSession : public PortAllocatorSession, rtc::Thread* network_thread() { return network_thread_; } rtc::PacketSocketFactory* socket_factory() { return socket_factory_; } + // If the new filter allows new types of candidates compared to the previous + // filter, gathered candidates that were discarded because of not matching the + // previous filter will be signaled if they match the new one. + // + // We do not perform any regathering since the port allocator flags decide + // the type of candidates to gather and the candidate filter only controls the + // signaling of candidates. As a result, with the candidate filter changed + // alone, all newly allowed candidates for signaling should already be + // gathered by the respective cricket::Port. void SetCandidateFilter(uint32_t filter) override; void StartGettingPorts() override; void StopGettingPorts() override; @@ -158,6 +167,14 @@ class RTC_EXPORT BasicPortAllocatorSession : public PortAllocatorSession, private: class PortData { public: + enum State { + STATE_INPROGRESS, // Still gathering candidates. + STATE_COMPLETE, // All candidates allocated and ready for process. + STATE_ERROR, // Error in gathering candidates. + STATE_PRUNED // Pruned by higher priority ports on the same network + // interface. Only TURN ports may be pruned. + }; + PortData() {} PortData(Port* port, AllocationSequence* seq) : port_(port), sequence_(seq) {} @@ -165,6 +182,7 @@ class RTC_EXPORT BasicPortAllocatorSession : public PortAllocatorSession, Port* port() const { return port_; } AllocationSequence* sequence() const { return sequence_; } bool has_pairable_candidate() const { return has_pairable_candidate_; } + State state() const { return state_; } bool complete() const { return state_ == STATE_COMPLETE; } bool error() const { return state_ == STATE_ERROR; } bool pruned() const { return state_ == STATE_PRUNED; } @@ -187,20 +205,12 @@ class RTC_EXPORT BasicPortAllocatorSession : public PortAllocatorSession, } has_pairable_candidate_ = has_pairable_candidate; } - void set_complete() { state_ = STATE_COMPLETE; } - void set_error() { - RTC_DCHECK(state_ == STATE_INPROGRESS); - state_ = STATE_ERROR; + void set_state(State state) { + RTC_DCHECK(state != STATE_ERROR || state_ == STATE_INPROGRESS); + state_ = state; } private: - enum State { - STATE_INPROGRESS, // Still gathering candidates. - STATE_COMPLETE, // All candidates allocated and ready for process. - STATE_ERROR, // Error in gathering candidates. - STATE_PRUNED // Pruned by higher priority ports on the same network - // interface. Only TURN ports may be pruned. - }; Port* port_ = nullptr; AllocationSequence* sequence_ = nullptr; bool has_pairable_candidate_ = false; diff --git a/p2p/client/basic_port_allocator_unittest.cc b/p2p/client/basic_port_allocator_unittest.cc index 1682e9dffc..067e757e48 100644 --- a/p2p/client/basic_port_allocator_unittest.cc +++ b/p2p/client/basic_port_allocator_unittest.cc @@ -1266,7 +1266,7 @@ TEST_F(BasicPortAllocatorTest, TestGetAllPortsNoAdapters) { TEST_F(BasicPortAllocatorTest, TestDisableAdapterEnumerationWithoutNatRelayTransportOnly) { ResetWithStunServerNoNat(kStunAddr); - allocator().set_candidate_filter(CF_RELAY); + allocator().SetCandidateFilter(CF_RELAY); // Expect to see no ports and no candidates. CheckDisableAdapterEnumeration(0U, rtc::IPAddress(), rtc::IPAddress(), rtc::IPAddress(), rtc::IPAddress()); @@ -1527,7 +1527,7 @@ TEST_F(BasicPortAllocatorTest, TestSessionUsesOwnCandidateFilter) { AddInterface(kClientAddr); ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); // Set candidate filter *after* creating the session. Should have no effect. - allocator().set_candidate_filter(CF_RELAY); + allocator().SetCandidateFilter(CF_RELAY); session_->StartGettingPorts(); // 7 candidates and 4 ports is what we would normally get (see the // TestGetAllPorts* tests). @@ -1546,7 +1546,7 @@ TEST_F(BasicPortAllocatorTest, TestCandidateFilterWithRelayOnly) { AddInterface(kClientAddr); // GTURN is not configured here. ResetWithTurnServersNoNat(kTurnUdpIntAddr, rtc::SocketAddress()); - allocator().set_candidate_filter(CF_RELAY); + allocator().SetCandidateFilter(CF_RELAY); ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); session_->StartGettingPorts(); EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, @@ -1565,7 +1565,7 @@ TEST_F(BasicPortAllocatorTest, TestCandidateFilterWithRelayOnly) { TEST_F(BasicPortAllocatorTest, TestCandidateFilterWithHostOnly) { AddInterface(kClientAddr); allocator().set_flags(PORTALLOCATOR_ENABLE_SHARED_SOCKET); - allocator().set_candidate_filter(CF_HOST); + allocator().SetCandidateFilter(CF_HOST); ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); session_->StartGettingPorts(); EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, @@ -1583,7 +1583,7 @@ TEST_F(BasicPortAllocatorTest, TestCandidateFilterWithReflexiveOnly) { ResetWithStunServerAndNat(kStunAddr); allocator().set_flags(PORTALLOCATOR_ENABLE_SHARED_SOCKET); - allocator().set_candidate_filter(CF_REFLEXIVE); + allocator().SetCandidateFilter(CF_REFLEXIVE); ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); session_->StartGettingPorts(); EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, @@ -1602,7 +1602,7 @@ TEST_F(BasicPortAllocatorTest, TestCandidateFilterWithReflexiveOnly) { TEST_F(BasicPortAllocatorTest, TestCandidateFilterWithReflexiveOnlyAndNoNAT) { AddInterface(kClientAddr); allocator().set_flags(PORTALLOCATOR_ENABLE_SHARED_SOCKET); - allocator().set_candidate_filter(CF_REFLEXIVE); + allocator().SetCandidateFilter(CF_REFLEXIVE); ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); session_->StartGettingPorts(); EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, @@ -2133,7 +2133,7 @@ TEST_F(BasicPortAllocatorTest, TestSetCandidateFilterAfterCandidatesGathered) { kDefaultAllocationTimeout, fake_clock); size_t initial_candidates_size = peeked_session->ReadyCandidates().size(); size_t initial_ports_size = peeked_session->ReadyPorts().size(); - allocator_->set_candidate_filter(CF_RELAY); + allocator_->SetCandidateFilter(CF_RELAY); // Assume that when TakePooledSession is called, the candidate filter will be // applied to the pooled session. This is tested by PortAllocatorTest. session_ = @@ -2157,6 +2157,145 @@ TEST_F(BasicPortAllocatorTest, TestSetCandidateFilterAfterCandidatesGathered) { } } +// Test that candidates that do not match a previous candidate filter can be +// surfaced if they match the new one after setting the filter value. +TEST_F(BasicPortAllocatorTest, + SurfaceNewCandidatesAfterSetCandidateFilterToAddCandidateTypes) { + // We would still surface a host candidate if the IP is public, even though it + // is disabled by the candidate filter. See + // BasicPortAllocatorSession::CheckCandidateFilter. Use the private address so + // that the srflx candidate is not equivalent to the host candidate. + AddInterface(kPrivateAddr); + ResetWithStunServerAndNat(kStunAddr); + + AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress()); + + allocator_->set_flags(allocator().flags() | + PORTALLOCATOR_ENABLE_SHARED_SOCKET | + PORTALLOCATOR_DISABLE_TCP); + + allocator_->SetCandidateFilter(CF_NONE); + ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); + session_->StartGettingPorts(); + EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, + kDefaultAllocationTimeout, fake_clock); + EXPECT_TRUE(candidates_.empty()); + EXPECT_TRUE(ports_.empty()); + + // Surface the relay candidate previously gathered but not signaled. + session_->SetCandidateFilter(CF_RELAY); + ASSERT_EQ_SIMULATED_WAIT(1u, candidates_.size(), kDefaultAllocationTimeout, + fake_clock); + EXPECT_EQ(RELAY_PORT_TYPE, candidates_.back().type()); + EXPECT_EQ(1u, ports_.size()); + + // Surface the srflx candidate previously gathered but not signaled. + session_->SetCandidateFilter(CF_RELAY | CF_REFLEXIVE); + ASSERT_EQ_SIMULATED_WAIT(2u, candidates_.size(), kDefaultAllocationTimeout, + fake_clock); + EXPECT_EQ(STUN_PORT_TYPE, candidates_.back().type()); + EXPECT_EQ(2u, ports_.size()); + + // Surface the srflx candidate previously gathered but not signaled. + session_->SetCandidateFilter(CF_ALL); + ASSERT_EQ_SIMULATED_WAIT(3u, candidates_.size(), kDefaultAllocationTimeout, + fake_clock); + EXPECT_EQ(LOCAL_PORT_TYPE, candidates_.back().type()); + EXPECT_EQ(2u, ports_.size()); +} + +// This is a similar test as +// SurfaceNewCandidatesAfterSetCandidateFilterToAddCandidateTypes, and we +// test the transitions for which the new filter value is not a super set of the +// previous value. +TEST_F( + BasicPortAllocatorTest, + SurfaceNewCandidatesAfterSetCandidateFilterToAllowDifferentCandidateTypes) { + // We would still surface a host candidate if the IP is public, even though it + // is disabled by the candidate filter. See + // BasicPortAllocatorSession::CheckCandidateFilter. Use the private address so + // that the srflx candidate is not equivalent to the host candidate. + AddInterface(kPrivateAddr); + ResetWithStunServerAndNat(kStunAddr); + + AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress()); + + allocator_->set_flags(allocator().flags() | + PORTALLOCATOR_ENABLE_SHARED_SOCKET | + PORTALLOCATOR_DISABLE_TCP); + + allocator_->SetCandidateFilter(CF_NONE); + ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); + session_->StartGettingPorts(); + EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, + kDefaultAllocationTimeout, fake_clock); + EXPECT_TRUE(candidates_.empty()); + EXPECT_TRUE(ports_.empty()); + + // Surface the relay candidate previously gathered but not signaled. + session_->SetCandidateFilter(CF_RELAY); + EXPECT_EQ_SIMULATED_WAIT(1u, candidates_.size(), kDefaultAllocationTimeout, + fake_clock); + EXPECT_EQ(RELAY_PORT_TYPE, candidates_.back().type()); + EXPECT_EQ(1u, ports_.size()); + + // Surface the srflx candidate previously gathered but not signaled. + session_->SetCandidateFilter(CF_REFLEXIVE); + EXPECT_EQ_SIMULATED_WAIT(2u, candidates_.size(), kDefaultAllocationTimeout, + fake_clock); + EXPECT_EQ(STUN_PORT_TYPE, candidates_.back().type()); + EXPECT_EQ(2u, ports_.size()); + + // Surface the host candidate previously gathered but not signaled. + session_->SetCandidateFilter(CF_HOST); + EXPECT_EQ_SIMULATED_WAIT(3u, candidates_.size(), kDefaultAllocationTimeout, + fake_clock); + EXPECT_EQ(LOCAL_PORT_TYPE, candidates_.back().type()); + // We use a shared socket and cricket::UDPPort handles the srflx candidate. + EXPECT_EQ(2u, ports_.size()); +} + +// Test that after an allocation session has stopped getting ports, changing the +// candidate filter to allow new types of gathered candidates does not surface +// any candidate. +TEST_F(BasicPortAllocatorTest, + NoCandidateSurfacedWhenUpdatingCandidateFilterIfSessionStopped) { + AddInterface(kPrivateAddr); + ResetWithStunServerAndNat(kStunAddr); + + AddTurnServers(kTurnUdpIntAddr, rtc::SocketAddress()); + + allocator_->set_flags(allocator().flags() | + PORTALLOCATOR_ENABLE_SHARED_SOCKET | + PORTALLOCATOR_DISABLE_TCP); + + allocator_->SetCandidateFilter(CF_NONE); + ASSERT_TRUE(CreateSession(ICE_CANDIDATE_COMPONENT_RTP)); + session_->StartGettingPorts(); + EXPECT_TRUE_SIMULATED_WAIT(candidate_allocation_done_, + kDefaultAllocationTimeout, fake_clock); + auto test_invariants = [this]() { + EXPECT_TRUE(candidates_.empty()); + EXPECT_TRUE(ports_.empty()); + }; + + test_invariants(); + + session_->StopGettingPorts(); + + session_->SetCandidateFilter(CF_RELAY); + SIMULATED_WAIT(false, kDefaultAllocationTimeout, fake_clock); + test_invariants(); + + session_->SetCandidateFilter(CF_RELAY | CF_REFLEXIVE); + SIMULATED_WAIT(false, kDefaultAllocationTimeout, fake_clock); + test_invariants(); + + session_->SetCandidateFilter(CF_ALL); + SIMULATED_WAIT(false, kDefaultAllocationTimeout, fake_clock); + test_invariants(); +} + TEST_F(BasicPortAllocatorTest, SetStunKeepaliveIntervalForPorts) { const int pool_size = 1; const int expected_stun_keepalive_interval = 123; diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 129eed6654..1b011a3320 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -5420,7 +5420,7 @@ PeerConnection::InitializePortAllocator_n( port_allocator_->set_flags(port_allocator_flags); // No step delay is used while allocating ports. port_allocator_->set_step_delay(cricket::kMinimumStepDelay); - port_allocator_->set_candidate_filter( + port_allocator_->SetCandidateFilter( ConvertIceTransportTypeToCandidateFilter(configuration.type)); port_allocator_->set_max_ipv6_networks(configuration.max_ipv6_networks); @@ -5450,7 +5450,7 @@ bool PeerConnection::ReconfigurePortAllocator_n( webrtc::TurnCustomizer* turn_customizer, absl::optional stun_candidate_keepalive_interval, bool have_local_description) { - port_allocator_->set_candidate_filter( + port_allocator_->SetCandidateFilter( ConvertIceTransportTypeToCandidateFilter(type)); // According to JSEP, after setLocalDescription, changing the candidate pool // size is not allowed, and changing the set of ICE servers will not result diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 8aa76754bf..c4988aa078 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -560,6 +560,10 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, return event_log_factory_; } + const cricket::Candidate& last_candidate_gathered() const { + return last_candidate_gathered_; + } + private: explicit PeerConnectionWrapper(const std::string& debug_name) : debug_name_(debug_name) {} @@ -942,6 +946,7 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, return; } SendIceMessage(candidate->sdp_mid(), candidate->sdp_mline_index(), ice_sdp); + last_candidate_gathered_ = candidate->candidate(); } void OnDataChannel( rtc::scoped_refptr data_channel) override { @@ -972,6 +977,7 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, SignalingMessageReceiver* signaling_message_receiver_ = nullptr; int signaling_delay_ms_ = 0; bool signal_ice_candidates_ = true; + cricket::Candidate last_candidate_gathered_; // Store references to the video sources we've created, so that we can stop // them, if required. @@ -5063,6 +5069,70 @@ TEST_P(PeerConnectionIntegrationTest, EXPECT_LT(0, callee_ice_event_count); } +TEST_P(PeerConnectionIntegrationTest, RegatherAfterChangingIceTransportType) { + static const rtc::SocketAddress turn_server_internal_address{"88.88.88.0", + 3478}; + static const rtc::SocketAddress turn_server_external_address{"88.88.88.1", 0}; + + CreateTurnServer(turn_server_internal_address, turn_server_external_address); + + webrtc::PeerConnectionInterface::IceServer ice_server; + ice_server.urls.push_back("turn:88.88.88.0:3478"); + ice_server.username = "test"; + ice_server.password = "test"; + + PeerConnectionInterface::RTCConfiguration caller_config; + caller_config.servers.push_back(ice_server); + caller_config.type = webrtc::PeerConnectionInterface::kRelay; + caller_config.continual_gathering_policy = PeerConnection::GATHER_CONTINUALLY; + + PeerConnectionInterface::RTCConfiguration callee_config; + callee_config.servers.push_back(ice_server); + callee_config.type = webrtc::PeerConnectionInterface::kRelay; + callee_config.continual_gathering_policy = PeerConnection::GATHER_CONTINUALLY; + + ASSERT_TRUE( + CreatePeerConnectionWrappersWithConfig(caller_config, callee_config)); + + // Do normal offer/answer and wait for ICE to complete. + ConnectFakeSignaling(); + caller()->AddAudioVideoTracks(); + callee()->AddAudioVideoTracks(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + // Since we are doing continual gathering, the ICE transport does not reach + // kIceGatheringComplete (see + // P2PTransportChannel::OnCandidatesAllocationDone), and consequently not + // kIceConnectionComplete. + EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionConnected, + caller()->ice_connection_state(), kDefaultTimeout); + EXPECT_EQ_WAIT(webrtc::PeerConnectionInterface::kIceConnectionConnected, + callee()->ice_connection_state(), kDefaultTimeout); + // Note that we cannot use the metric + // |WebRTC.PeerConnection.CandidatePairType_UDP| in this test since this + // metric is only populated when we reach kIceConnectionComplete in the + // current implementation. + EXPECT_EQ(cricket::RELAY_PORT_TYPE, + caller()->last_candidate_gathered().type()); + EXPECT_EQ(cricket::RELAY_PORT_TYPE, + callee()->last_candidate_gathered().type()); + + // Loosen the caller's candidate filter. + caller_config = caller()->pc()->GetConfiguration(); + caller_config.type = webrtc::PeerConnectionInterface::kAll; + caller()->pc()->SetConfiguration(caller_config); + // We should have gathered a new host candidate. + EXPECT_EQ_WAIT(cricket::LOCAL_PORT_TYPE, + caller()->last_candidate_gathered().type(), kDefaultTimeout); + + // Loosen the callee's candidate filter. + callee_config = callee()->pc()->GetConfiguration(); + callee_config.type = webrtc::PeerConnectionInterface::kAll; + callee()->pc()->SetConfiguration(callee_config); + EXPECT_EQ_WAIT(cricket::LOCAL_PORT_TYPE, + callee()->last_candidate_gathered().type(), kDefaultTimeout); +} + INSTANTIATE_TEST_SUITE_P(PeerConnectionIntegrationTest, PeerConnectionIntegrationTest, Values(SdpSemantics::kPlanB,