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}

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/");

pc/peer_connection.cc

@@ -690,6 +690,26 @@ class CreateSessionDescriptionObserverOperationWrapper
   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 @@ 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();

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) {