Don't SetNeedsIceRestartFlag if widening candidate filter when surface_ice_candidates_on_ice_transport_type_changed
This patch fixes a minor bug in the implementation of
surface_ice_candidates_on_ice_transport_type_changed. The existing
implementation correctly handles the surfacing, but accidentally also
set the SetNeedsIceRestartFlag, which made _next_ offer contain
a ice restart.
Modified existing testcase to verify this.
Bug: webrtc:8939
Change-Id: If566e3249296467668627e5941495f6036cbd903
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176127
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31363}
diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc
index a3cf937..ea8d66f 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -5546,6 +5546,76 @@
DestroyChannels();
}
+// Verify that things break unless
+// - both parties use the surface_ice_candidates_on_ice_transport_type_changed
+// - both parties loosen candidate filter at the same time (approx.).
+//
+// i.e surface_ice_candidates_on_ice_transport_type_changed requires
+// coordination outside of webrtc to function properly.
+TEST_F(P2PTransportChannelTest, SurfaceRequiresCoordination) {
+ webrtc::test::ScopedFieldTrials field_trials(
+ "WebRTC-IceFieldTrials/skip_relay_to_non_relay_connections:true/");
+ 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_ALL);
+ // Enable continual gathering and also resurfacing gathered candidates upon
+ // the candidate filter changed in the ICE configuration.
+ IceConfig ice_config = CreateIceConfig(1000, GATHER_CONTINUALLY);
+ ice_config.surface_ice_candidates_on_ice_transport_type_changed = true;
+ // Pause candidates gathering 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(ice_config, ice_config);
+
+ // On the caller we only have relay,
+ // on the callee we have host, srflx and relay.
+ EXPECT_TRUE_SIMULATED_WAIT(ep1->saved_candidates_.size() == 1u,
+ kDefaultTimeout, clock);
+ EXPECT_TRUE_SIMULATED_WAIT(ep2->saved_candidates_.size() == 3u,
+ kDefaultTimeout, clock);
+
+ ResumeCandidates(0);
+ ResumeCandidates(1);
+ ASSERT_TRUE_SIMULATED_WAIT(
+ ep1_ch1()->selected_connection() != nullptr &&
+ RELAY_PORT_TYPE ==
+ ep1_ch1()->selected_connection()->local_candidate().type() &&
+ ep2_ch1()->selected_connection() != nullptr &&
+ RELAY_PORT_TYPE ==
+ ep1_ch1()->selected_connection()->remote_candidate().type(),
+ kDefaultTimeout, clock);
+ ASSERT_TRUE_SIMULATED_WAIT(ep2_ch1()->selected_connection() != nullptr,
+ kDefaultTimeout, clock);
+
+ // Wait until the callee discards it's candidates
+ // since they don't manage to connect.
+ SIMULATED_WAIT(false, 300000, clock);
+
+ // And then loosen caller candidate filter.
+ ep1->allocator_->SetCandidateFilter(CF_ALL);
+
+ SIMULATED_WAIT(false, kDefaultTimeout, clock);
+
+ // No p2p connection will be made, it will remain on relay.
+ EXPECT_TRUE(ep1_ch1()->selected_connection() != nullptr &&
+ RELAY_PORT_TYPE ==
+ ep1_ch1()->selected_connection()->local_candidate().type() &&
+ ep2_ch1()->selected_connection() != nullptr &&
+ RELAY_PORT_TYPE ==
+ ep1_ch1()->selected_connection()->remote_candidate().type());
+
+ DestroyChannels();
+}
+
TEST_F(P2PTransportChannelPingTest, TestInitialSelectDampening0) {
webrtc::test::ScopedFieldTrials field_trials(
"WebRTC-IceFieldTrials/initial_select_dampening:0/");
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 05e7b95..967fd27 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -690,6 +690,26 @@
std::function<void()> operation_complete_callback_;
};
+// Check if the changes of IceTransportsType motives an ice restart.
+bool NeedIceRestart(bool surface_ice_candidates_on_ice_transport_type_changed,
+ PeerConnectionInterface::IceTransportsType current,
+ PeerConnectionInterface::IceTransportsType modified) {
+ if (current == modified) {
+ return false;
+ }
+
+ if (!surface_ice_candidates_on_ice_transport_type_changed) {
+ return true;
+ }
+
+ auto current_filter = ConvertIceTransportTypeToCandidateFilter(current);
+ auto modified_filter = ConvertIceTransportTypeToCandidateFilter(modified);
+
+ // If surface_ice_candidates_on_ice_transport_type_changed is true and we
+ // extend the filter, then no ice restart is needed.
+ return (current_filter & modified_filter) != current_filter;
+}
+
} // namespace
// Used by parameterless SetLocalDescription() to create an offer or answer.
@@ -4089,7 +4109,9 @@
// candidate policy must set a "needs-ice-restart" bit so that the next offer
// triggers an ICE restart which will pick up the changes.
if (modified_config.servers != configuration_.servers ||
- modified_config.type != configuration_.type ||
+ NeedIceRestart(
+ configuration_.surface_ice_candidates_on_ice_transport_type_changed,
+ configuration_.type, modified_config.type) ||
modified_config.GetTurnPortPrunePolicy() !=
configuration_.GetTurnPortPrunePolicy()) {
transport_controller_->SetNeedsIceRestartFlag();
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index 2fa4fb6..f947fe5 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -6134,6 +6134,23 @@
callee()->pc()->SetConfiguration(callee_config);
EXPECT_EQ_WAIT(cricket::LOCAL_PORT_TYPE,
callee()->last_candidate_gathered().type(), kDefaultTimeout);
+
+ // Create an offer and verify that it does not contain an ICE restart (i.e new
+ // ice credentials).
+ std::string caller_ufrag_pre_offer = caller()
+ ->pc()
+ ->local_description()
+ ->description()
+ ->transport_infos()[0]
+ .description.ice_ufrag;
+ caller()->CreateAndSetAndSignalOffer();
+ std::string caller_ufrag_post_offer = caller()
+ ->pc()
+ ->local_description()
+ ->description()
+ ->transport_infos()[0]
+ .description.ice_ufrag;
+ EXPECT_EQ(caller_ufrag_pre_offer, caller_ufrag_post_offer);
}
TEST_P(PeerConnectionIntegrationTest, OnIceCandidateError) {
