From a186f42077d2a36b52f97511151d057455c26807 Mon Sep 17 00:00:00 2001
From: Jonas Oreland <jonaso@webrtc.org>
Date: Mon, 23 Nov 2020 14:31:26 +0100
Subject: [PATCH] p2p: Fix bug causing old candidates on ice restart

This patch fixes a bug where old candidates was
generated if doing GATHER_CONTINUALLY.

The problem was that the old port allocator session
was never stopped, and when the new sessio is created
it will attach to the network that will signal OnNetworkChanged().

The patch adds explicit stop of old sessions.

The problem was not possible to trigger using fake_network
as this "incorrectly" called SignalNetworkChanged directly
rather than after a Thread->Post() like network.cc does it.

Bug: webrtc:12210
Change-Id: Ief3f961bd97f06f4c4194ecbc3200c635ba63cf6
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/194961
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32675}
---
 p2p/base/p2p_transport_channel.cc          |  7 +++++
 p2p/base/p2p_transport_channel_unittest.cc | 32 ++++++++++++++++++++++
 rtc_base/fake_network.h                    | 18 ++++++++++--
 3 files changed, 54 insertions(+), 3 deletions(-)

diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index ad002aedbb..f511fb915a 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -843,6 +843,13 @@ void P2PTransportChannel::MaybeStartGathering() {
                                 static_cast<int>(IceRestartState::MAX_VALUE));
     }
 
+    for (const auto& session : allocator_sessions_) {
+      if (session->IsStopped()) {
+        continue;
+      }
+      session->StopGettingPorts();
+    }
+
     // Time for a new allocator.
     std::unique_ptr<PortAllocatorSession> pooled_session =
         allocator_->TakePooledSession(transport_name(), component(),
diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc
index 2af2a7412e..0b9e1baa0a 100644
--- a/p2p/base/p2p_transport_channel_unittest.cc
+++ b/p2p/base/p2p_transport_channel_unittest.cc
@@ -6108,4 +6108,36 @@ INSTANTIATE_TEST_SUITE_P(GatherAfterConnectedTest,
                          GatherAfterConnectedTest,
                          ::testing::Values(true, false));
 
+// Tests no candidates are generated with old ice ufrag/passwd after an ice
+// restart even if continual gathering is enabled.
+TEST_F(P2PTransportChannelTest, TestIceNoOldCandidatesAfterIceRestart) {
+  rtc::ScopedFakeClock clock;
+  AddAddress(0, kAlternateAddrs[0]);
+  ConfigureEndpoints(OPEN, OPEN, kDefaultPortAllocatorFlags,
+                     kDefaultPortAllocatorFlags);
+
+  // gathers continually.
+  IceConfig config = CreateIceConfig(1000, GATHER_CONTINUALLY);
+  CreateChannels(config, config);
+
+  EXPECT_TRUE_SIMULATED_WAIT(CheckConnected(ep1_ch1(), ep2_ch1()),
+                             kDefaultTimeout, clock);
+
+  PauseCandidates(0);
+
+  ep1_ch1()->SetIceParameters(kIceParams[3]);
+  ep1_ch1()->MaybeStartGathering();
+
+  EXPECT_TRUE_SIMULATED_WAIT(GetEndpoint(0)->saved_candidates_.size() > 0,
+                             kDefaultTimeout, clock);
+
+  for (const auto& cd : GetEndpoint(0)->saved_candidates_) {
+    for (const auto& c : cd->candidates) {
+      EXPECT_EQ(c.username(), kIceUfrag[3]);
+    }
+  }
+
+  DestroyChannels();
+}
+
 }  // namespace cricket
diff --git a/rtc_base/fake_network.h b/rtc_base/fake_network.h
index 8bd50b69f0..1bbdd460a0 100644
--- a/rtc_base/fake_network.h
+++ b/rtc_base/fake_network.h
@@ -70,10 +70,11 @@ class FakeNetworkManager : public NetworkManagerBase,
     ++start_count_;
     if (start_count_ == 1) {
       sent_first_update_ = false;
-      rtc::Thread::Current()->Post(RTC_FROM_HERE, this);
+      rtc::Thread::Current()->Post(RTC_FROM_HERE, this, kUpdateNetworksMessage);
     } else {
       if (sent_first_update_) {
-        SignalNetworksChanged();
+        rtc::Thread::Current()->Post(RTC_FROM_HERE, this,
+                                     kSignalNetworksMessage);
       }
     }
   }
@@ -81,7 +82,15 @@ class FakeNetworkManager : public NetworkManagerBase,
   void StopUpdating() override { --start_count_; }
 
   // MessageHandler interface.
-  void OnMessage(Message* msg) override { DoUpdateNetworks(); }
+  void OnMessage(Message* msg) override {
+    if (msg->message_id == kUpdateNetworksMessage) {
+      DoUpdateNetworks();
+    } else if (msg->message_id == kSignalNetworksMessage) {
+      SignalNetworksChanged();
+    } else {
+      RTC_CHECK(false);
+    }
+  }
 
   using NetworkManagerBase::set_default_local_addresses;
   using NetworkManagerBase::set_enumeration_permission;
@@ -129,6 +138,9 @@ class FakeNetworkManager : public NetworkManagerBase,
   int start_count_ = 0;
   bool sent_first_update_ = false;
 
+  static constexpr uint32_t kUpdateNetworksMessage = 1;
+  static constexpr uint32_t kSignalNetworksMessage = 2;
+
   std::unique_ptr<webrtc::MdnsResponderInterface> mdns_responder_;
 };