Surface ICE candidates that match an updated candidate filter.
After this change an ICE agent can surface candidates that do not match
the previous filter but are allowed by the updated one. The candidate
filter, as part of the internal implementation in the ICE transport,
manifests the RTCIceTransportPolicy field in RTCConfiguration.
This new feature would allow an ICE agent to gather new candidates when
the transport policy changes from e.g. 'relay' to 'all' without an ICE
restart.
A caveat in the current implementation remains, and a candidate can
surface multiple times if the transport policy, or the candidate filter
directly, performs multiple transitions from a value that disallows to
one that allows the underlying candidate type. For example, if the
transport policy is updated by 'all' -> 'relay' -> 'all', the same host
candidate can surface after the second update.
Bug: webrtc:8939
Change-Id: I92c2e07dafab225c702c5de28f47958a0d3270cc
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/132282
Commit-Queue: Qingsi Wang <qingsi@webrtc.org>
Reviewed-by: Jeroen de Borst <jeroendb@webrtc.org>
Reviewed-by: Seth Hampson <shampson@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27674}diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index a5f6537..a109b0a 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -15,6 +15,7 @@
#include <utility>
#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 @@
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<webrtc::BasicRegatheringController>(
+ 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 @@
"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 467ef25..8371646 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -292,6 +292,7 @@
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 a92f4ff..f61d21f 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -50,6 +50,7 @@
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 @@
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 @@
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 @@
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 4dee0fa..6228791 100644
--- a/p2p/base/port_allocator.cc
+++ b/p2p/base/port_allocator.cc
@@ -290,6 +290,16 @@
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 e0cc775..d0605b6 100644
--- a/p2p/base/port_allocator.h
+++ b/p2p/base/port_allocator.h
@@ -528,10 +528,22 @@
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 @@
// Return IceParameters of the pooled sessions.
std::vector<IceParameters> GetPooledIceCredentials();
+ // Fired when |candidate_filter_| changes.
+ sigslot::signal2<uint32_t /* prev_filter */, uint32_t /* cur_filter */>
+ 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 a9edbec..9d6b4dd 100644
--- a/p2p/base/port_allocator_unittest.cc
+++ b/p2p/base/port_allocator_unittest.cc
@@ -100,7 +100,7 @@
// 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 @@
// 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 83c8bf2..4f418ee 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 @@
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 @@
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::GetPortConfigurations() {
RTC_DCHECK_RUN_ON(network_thread_);
+
PortConfiguration* config =
new PortConfiguration(allocator_->stun_servers(), username(), password());
@@ -633,7 +700,7 @@
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 @@
}
// 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 @@
// 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 @@
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 @@
// 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 edc6b87..26eea1e 100644
--- a/p2p/client/basic_port_allocator.h
+++ b/p2p/client/basic_port_allocator.h
@@ -125,6 +125,15 @@
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 @@
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 @@
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 @@
}
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 1682e9d..067e757 100644
--- a/p2p/client/basic_port_allocator_unittest.cc
+++ b/p2p/client/basic_port_allocator_unittest.cc
@@ -1266,7 +1266,7 @@
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 @@
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 @@
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, 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 @@
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, 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 @@
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 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 129eed6..1b011a3 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -5420,7 +5420,7 @@
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 @@
webrtc::TurnCustomizer* turn_customizer,
absl::optional<int> 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 8aa7675..c4988aa 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -560,6 +560,10 @@
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 @@
return;
}
SendIceMessage(candidate->sdp_mid(), candidate->sdp_mline_index(), ice_sdp);
+ last_candidate_gathered_ = candidate->candidate();
}
void OnDataChannel(
rtc::scoped_refptr<DataChannelInterface> data_channel) override {
@@ -972,6 +977,7 @@
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 @@
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,
