diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index a3cf9370e2..ea8d66f890 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -5546,6 +5546,76 @@ TEST_F(P2PTransportChannelTest, 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 05e7b95591..967fd27889 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -690,6 +690,26 @@ class CreateSessionDescriptionObserverOperationWrapper std::function 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 @@ RTCError PeerConnection::SetConfiguration( // 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 2fa4fb6ade..f947fe5e77 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -6134,6 +6134,23 @@ TEST_P(PeerConnectionIntegrationTest, RegatherAfterChangingIceTransportType) { 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) {