Revert "Surface ICE candidates that match an updated candidate filter."
This reverts commit cd8d1cf68e4eeed71fba51c97006a91bfd41813d.
Reason for revert: breaks an internal project
Original change's description:
> 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}
TBR=shampson@webrtc.org,qingsi@webrtc.org,jeroendb@webrtc.org,sukhanov@webrtc.org
Change-Id: Idd51a640e55a612b42fe8b69e05dff57a22d021a
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: webrtc:8939
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/133581
Reviewed-by: Qingsi Wang <qingsi@webrtc.org>
Commit-Queue: Qingsi Wang <qingsi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27677}diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index a109b0a..a5f6537 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -15,7 +15,6 @@
#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"
@@ -149,14 +148,8 @@
webrtc::BasicRegatheringController::Config regathering_config(
config_.regather_all_networks_interval_range,
config_.regather_on_failed_networks_interval_or_default());
- 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);
+ regathering_controller_.reset(new webrtc::BasicRegatheringController(
+ regathering_config, this, network_thread_));
ice_event_log_.set_event_log(event_log);
}
@@ -1006,14 +999,6 @@
"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 8371646..467ef25 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -292,7 +292,6 @@
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 f61d21f..a92f4ff 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -50,7 +50,6 @@
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 @@
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 @@
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 @@
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 6228791..4dee0fa 100644
--- a/p2p/base/port_allocator.cc
+++ b/p2p/base/port_allocator.cc
@@ -290,16 +290,6 @@
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 d0605b6..e0cc775 100644
--- a/p2p/base/port_allocator.h
+++ b/p2p/base/port_allocator.h
@@ -528,22 +528,10 @@
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 @@
// 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 9d6b4dd..a9edbec 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_->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 @@
// 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 4f418ee..83c8bf2 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 @@
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 @@
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::GetPortConfigurations() {
RTC_DCHECK_RUN_ON(network_thread_);
-
PortConfiguration* config =
new PortConfiguration(allocator_->stun_servers(), username(), password());
@@ -700,7 +633,7 @@
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 @@
}
// 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 @@
// 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 @@
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 @@
// 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 26eea1e..edc6b87 100644
--- a/p2p/client/basic_port_allocator.h
+++ b/p2p/client/basic_port_allocator.h
@@ -125,15 +125,6 @@
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 @@
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 @@
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 @@
}
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 067e757..1682e9d 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().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 @@
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 @@
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, 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 @@
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, 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 @@
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 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 1b011a3..129eed6 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_->SetCandidateFilter(
+ port_allocator_->set_candidate_filter(
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_->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 c4988aa..8aa7675 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -560,10 +560,6 @@
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 @@
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 {
@@ -977,7 +972,6 @@
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 @@
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,
