diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index a109b0ae96..a5f65376ce 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -15,7 +15,6 @@ #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" @@ -149,14 +148,8 @@ P2PTransportChannel::P2PTransportChannel( webrtc::BasicRegatheringController::Config regathering_config( config_.regather_all_networks_interval_range, config_.regather_on_failed_networks_interval_or_default()); - 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); + regathering_controller_.reset(new webrtc::BasicRegatheringController( + regathering_config, this, network_thread_)); ice_event_log_.set_event_log(event_log); } @@ -1006,14 +999,6 @@ 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 83716467b3..467ef258b5 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -292,7 +292,6 @@ 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 f61d21fcc0..a92f4ff6aa 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -50,7 +50,6 @@ 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. @@ -1607,8 +1606,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_->SetCandidateFilter(CF_RELAY); - GetEndpoint(1)->allocator_->SetCandidateFilter(CF_RELAY); + GetEndpoint(0)->allocator_->set_candidate_filter(CF_RELAY); + GetEndpoint(1)->allocator_->set_candidate_filter(CF_RELAY); // Setting this allows us to control when SetRemoteIceParameters is called. set_remote_ice_parameter_source(FROM_CANDIDATE); CreateChannels(); @@ -4588,8 +4587,7 @@ TEST(P2PTransportChannelResolverTest, HostnameCandidateIsResolved) { EXPECT_CALL(mock_async_resolver_factory, Create()) .WillOnce(Return(&mock_async_resolver)); - FakePortAllocator allocator(rtc::Thread::Current(), nullptr); - P2PTransportChannel channel("tn", 0, &allocator, + P2PTransportChannel channel("tn", 0, /*allocator*/ nullptr, &mock_async_resolver_factory); Candidate hostname_candidate; SocketAddress hostname_address("fake.test", 1000); @@ -4890,191 +4888,4 @@ 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 62287912ed..4dee0fa5e0 100644 --- a/p2p/base/port_allocator.cc +++ b/p2p/base/port_allocator.cc @@ -290,16 +290,6 @@ 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 d0605b6bb6..e0cc775290 100644 --- a/p2p/base/port_allocator.h +++ b/p2p/base/port_allocator.h @@ -528,22 +528,10 @@ class RTC_EXPORT PortAllocator : public sigslot::has_slots<> { return candidate_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); } + void set_candidate_filter(uint32_t filter) { + CheckRunOnValidThreadIfInitialized(); + candidate_filter_ = filter; + } bool prune_turn_ports() const { CheckRunOnValidThreadIfInitialized(); @@ -577,10 +565,6 @@ 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 9d6b4dd3df..a9edbeccaf 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_->SetCandidateFilter(cricket::CF_RELAY); + allocator_->set_candidate_filter(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_->SetCandidateFilter(cricket::CF_RELAY); + allocator_->set_candidate_filter(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 4f418ee7f6..83c8bf2b95 100644 --- a/p2p/client/basic_port_allocator.cc +++ b/p2p/client/basic_port_allocator.cc @@ -31,7 +31,6 @@ using rtc::CreateRandomId; -namespace cricket { namespace { enum { @@ -113,37 +112,9 @@ 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; @@ -336,57 +307,20 @@ void BasicPortAllocatorSession::SetCandidateFilter(uint32_t filter) { if (filter == candidate_filter_) { return; } - uint32_t prev_filter = candidate_filter_; + // We assume the filter will only change from "ALL" to something else. + RTC_DCHECK(candidate_filter_ == CF_ALL); candidate_filter_ = filter; - for (PortData& port_data : ports_) { - if (port_data.error() || port_data.pruned()) { + for (PortData& port : ports_) { + if (!port.has_pairable_candidate()) { 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. - // - // 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); + if (absl::c_none_of(port.port()->Candidates(), + [this, &port](const Candidate& candidate) { + return CandidatePairable(candidate, port.port()); + })) { + port.set_has_pairable_candidate(false); } } } @@ -663,7 +597,6 @@ void BasicPortAllocatorSession::UpdateIceParametersInternal() { void BasicPortAllocatorSession::GetPortConfigurations() { RTC_DCHECK_RUN_ON(network_thread_); - PortConfiguration* config = new PortConfiguration(allocator_->stun_servers(), username(), password()); @@ -700,7 +633,7 @@ void BasicPortAllocatorSession::OnConfigStop() { if (it->inprogress()) { // Updating port state to error, which didn't finish allocating candidates // yet. - it->set_state(PortData::STATE_ERROR); + it->set_error(); send_signal = true; } } @@ -1092,7 +1025,7 @@ void BasicPortAllocatorSession::OnPortComplete(Port* port) { } // Moving to COMPLETE state. - data->set_state(PortData::STATE_COMPLETE); + data->set_complete(); // Send candidate allocation complete signal if this was the last port. MaybeSignalCandidatesAllocationDone(); } @@ -1110,7 +1043,7 @@ void BasicPortAllocatorSession::OnPortError(Port* port) { // SignalAddressError is currently sent from StunPort/TurnPort. // But this signal itself is generic. - data->set_state(PortData::STATE_ERROR); + data->set_error(); // Send candidate allocation complete signal if this was the last port. MaybeSignalCandidatesAllocationDone(); } @@ -1118,7 +1051,34 @@ void BasicPortAllocatorSession::OnPortError(Port* port) { bool BasicPortAllocatorSession::CheckCandidateFilter(const Candidate& c) const { RTC_DCHECK_RUN_ON(network_thread_); - return IsAllowedByCandidateFilter(c, candidate_filter_); + 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; } bool BasicPortAllocatorSession::CandidatePairable(const Candidate& c, @@ -1299,39 +1259,25 @@ 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.pruned() && p.port()->Network() == network_ && + return p.port()->Network() == network_ && p.port()->GetProtocol() == PROTO_UDP && - p.port()->Type() == LOCAL_PORT_TYPE && !p.error(); + !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.pruned() && p.port()->Network() == network_ && + return p.port()->Network() == network_ && p.port()->GetProtocol() == PROTO_TCP && - p.port()->Type() == LOCAL_PORT_TYPE && !p.error(); + !p.error(); })) { *flags |= PORTALLOCATOR_DISABLE_TCP; } if (config_ && config) { - // 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)) { + if (config_->StunServers() == config->StunServers()) { // 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 26eea1ef52..edc6b87cb0 100644 --- a/p2p/client/basic_port_allocator.h +++ b/p2p/client/basic_port_allocator.h @@ -125,15 +125,6 @@ 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; @@ -167,14 +158,6 @@ 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) {} @@ -182,7 +165,6 @@ 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; } @@ -205,12 +187,20 @@ class RTC_EXPORT BasicPortAllocatorSession : public PortAllocatorSession, } has_pairable_candidate_ = has_pairable_candidate; } - void set_state(State state) { - RTC_DCHECK(state != STATE_ERROR || state_ == STATE_INPROGRESS); - state_ = state; + void set_complete() { state_ = STATE_COMPLETE; } + void set_error() { + RTC_DCHECK(state_ == STATE_INPROGRESS); + state_ = STATE_ERROR; } 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 067e757e48..1682e9dffc 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().SetCandidateFilter(CF_RELAY); + allocator().set_candidate_filter(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().SetCandidateFilter(CF_RELAY); + allocator().set_candidate_filter(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().SetCandidateFilter(CF_RELAY); + allocator().set_candidate_filter(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().SetCandidateFilter(CF_HOST); + allocator().set_candidate_filter(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().SetCandidateFilter(CF_REFLEXIVE); + allocator().set_candidate_filter(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().SetCandidateFilter(CF_REFLEXIVE); + allocator().set_candidate_filter(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_->SetCandidateFilter(CF_RELAY); + allocator_->set_candidate_filter(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,145 +2157,6 @@ 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 1b011a3320..129eed6654 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_->SetCandidateFilter( + port_allocator_->set_candidate_filter( 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_->SetCandidateFilter( + port_allocator_->set_candidate_filter( 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 c4988aa078..8aa76754bf 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -560,10 +560,6 @@ 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) {} @@ -946,7 +942,6 @@ 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 { @@ -977,7 +972,6 @@ 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. @@ -5069,70 +5063,6 @@ 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,