If gather_continually is set to true, keep the last port allocator session running while stopping all existing process of getting ports (when p2ptransportchannel first becomes writable).

BUG=5034

Review URL: https://codereview.webrtc.org/1359363003

Cr-Commit-Position: refs/heads/master@{#10110}

7 files changed, 95 insertions, 6 deletions

diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index 15ac006872..cce20f4abb 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -1070,12 +1070,21 @@ void P2PTransportChannel::UpdateChannelState() {
 // was writable, go into the writable state.
 void P2PTransportChannel::HandleWritable() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
-  if (!writable()) {
-    for (uint32 i = 0; i < allocator_sessions_.size(); ++i) {
-      if (allocator_sessions_[i]->IsGettingPorts()) {
-        allocator_sessions_[i]->StopGettingPorts();
-      }
+  if (writable()) {
+    return;
+  }
+
+  for (PortAllocatorSession* session : allocator_sessions_) {
+    if (!session->IsGettingPorts()) {
+      continue;
+    }
+    // If gathering continually, keep the last session running so that it
+    // will gather candidates if the networks change.
+    if (gather_continually_ && session == allocator_sessions_.back()) {
+      session->ClearGettingPorts();
+      break;
     }
+    session->StopGettingPorts();
   }
 
   was_writable_ = true;
diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
index 8c25d82068..4add040afe 100644
--- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc
+++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc
@@ -1439,6 +1439,34 @@ TEST_F(P2PTransportChannelTest, TestForceTurn) {
   DestroyChannels();
 }
 
+// Test that if continual gathering is set to true, ICE gathering state will
+// not change to "Complete", and vice versa.
+TEST_F(P2PTransportChannelTest, TestContinualGathering) {
+  ConfigureEndpoints(OPEN, OPEN, kDefaultPortAllocatorFlags,
+                     kDefaultPortAllocatorFlags);
+  SetAllocationStepDelay(0, kDefaultStepDelay);
+  SetAllocationStepDelay(1, kDefaultStepDelay);
+  CreateChannels(1);
+  cricket::IceConfig config = CreateIceConfig(1000, true);
+  ep1_ch1()->SetIceConfig(config);
+  // By default, ep2 does not gather continually.
+
+  EXPECT_TRUE_WAIT_MARGIN(ep1_ch1() != NULL && ep2_ch1() != NULL &&
+                              ep1_ch1()->receiving() && ep1_ch1()->writable() &&
+                              ep2_ch1()->receiving() && ep2_ch1()->writable(),
+                          1000, 1000);
+  WAIT(cricket::IceGatheringState::kIceGatheringComplete ==
+           ep1_ch1()->gathering_state(),
+       1000);
+  EXPECT_EQ(cricket::IceGatheringState::kIceGatheringGathering,
+            ep1_ch1()->gathering_state());
+  // By now, ep2 should have completed gathering.
+  EXPECT_EQ(cricket::IceGatheringState::kIceGatheringComplete,
+            ep2_ch1()->gathering_state());
+
+  DestroyChannels();
+}
+
 // Test what happens when we have 2 users behind the same NAT. This can lead
 // to interesting behavior because the STUN server will only give out the
 // address of the outermost NAT.
diff --git a/webrtc/p2p/base/portallocator.h b/webrtc/p2p/base/portallocator.h
index 6e3efa8c41..69b5ed0582 100644
--- a/webrtc/p2p/base/portallocator.h
+++ b/webrtc/p2p/base/portallocator.h
@@ -91,6 +91,9 @@ class PortAllocatorSession : public sigslot::has_slots<> {
   // Starts gathering STUN and Relay configurations.
   virtual void StartGettingPorts() = 0;
   virtual void StopGettingPorts() = 0;
+  // Only stop the existing gathering process but may start new ones if needed.
+  virtual void ClearGettingPorts() = 0;
+  // Whether the process of getting ports has been stopped.
   virtual bool IsGettingPorts() = 0;
 
   sigslot::signal2<PortAllocatorSession*, PortInterface*> SignalPortReady;
diff --git a/webrtc/p2p/client/basicportallocator.cc b/webrtc/p2p/client/basicportallocator.cc
index 11d0cef847..3c77b4feab 100644
--- a/webrtc/p2p/client/basicportallocator.cc
+++ b/webrtc/p2p/client/basicportallocator.cc
@@ -192,10 +192,14 @@ void BasicPortAllocatorSession::StartGettingPorts() {
 void BasicPortAllocatorSession::StopGettingPorts() {
   ASSERT(rtc::Thread::Current() == network_thread_);
   running_ = false;
+  network_thread_->Post(this, MSG_CONFIG_STOP);
+  ClearGettingPorts();
+}
+
+void BasicPortAllocatorSession::ClearGettingPorts() {
   network_thread_->Clear(this, MSG_ALLOCATE);
   for (uint32 i = 0; i < sequences_.size(); ++i)
     sequences_[i]->Stop();
-  network_thread_->Post(this, MSG_CONFIG_STOP);
 }
 
 void BasicPortAllocatorSession::OnMessage(rtc::Message *message) {
diff --git a/webrtc/p2p/client/basicportallocator.h b/webrtc/p2p/client/basicportallocator.h
index 37ce1a0748..ac2cfbfc13 100644
--- a/webrtc/p2p/client/basicportallocator.h
+++ b/webrtc/p2p/client/basicportallocator.h
@@ -112,6 +112,7 @@ class BasicPortAllocatorSession : public PortAllocatorSession,
 
   virtual void StartGettingPorts();
   virtual void StopGettingPorts();
+  virtual void ClearGettingPorts();
   virtual bool IsGettingPorts() { return running_; }
 
  protected:
diff --git a/webrtc/p2p/client/fakeportallocator.h b/webrtc/p2p/client/fakeportallocator.h
index 9e53bc04aa..dca86f633e 100644
--- a/webrtc/p2p/client/fakeportallocator.h
+++ b/webrtc/p2p/client/fakeportallocator.h
@@ -63,6 +63,8 @@ class FakePortAllocatorSession : public PortAllocatorSession {
 
   virtual void StopGettingPorts() { running_ = false; }
   virtual bool IsGettingPorts() { return running_; }
+  virtual void ClearGettingPorts() {}
+
   int port_config_count() { return port_config_count_; }
 
   void AddPort(cricket::Port* port) {
diff --git a/webrtc/p2p/client/portallocator_unittest.cc b/webrtc/p2p/client/portallocator_unittest.cc
index 2893807575..92f995a3d7 100644
--- a/webrtc/p2p/client/portallocator_unittest.cc
+++ b/webrtc/p2p/client/portallocator_unittest.cc
@@ -1279,3 +1279,45 @@ TEST_F(PortAllocatorTest, TestEnableIPv6Addresses) {
       kClientAddr);
   EXPECT_EQ(4U, candidates_.size());
 }
+
+TEST_F(PortAllocatorTest, TestStopGettingPorts) {
+  AddInterface(kClientAddr);
+  allocator_->set_step_delay(cricket::kDefaultStepDelay);
+  EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  session_->StartGettingPorts();
+  ASSERT_EQ_WAIT(2U, candidates_.size(), 1000);
+  EXPECT_EQ(2U, ports_.size());
+  session_->StopGettingPorts();
+  EXPECT_TRUE_WAIT(candidate_allocation_done_, 1000);
+
+  // After stopping getting ports, adding a new interface will not start
+  // getting ports again.
+  candidates_.clear();
+  ports_.clear();
+  candidate_allocation_done_ = false;
+  network_manager_.AddInterface(kClientAddr2);
+  rtc::Thread::Current()->ProcessMessages(1000);
+  EXPECT_EQ(0U, candidates_.size());
+  EXPECT_EQ(0U, ports_.size());
+}
+
+TEST_F(PortAllocatorTest, TestClearGettingPorts) {
+  AddInterface(kClientAddr);
+  allocator_->set_step_delay(cricket::kDefaultStepDelay);
+  EXPECT_TRUE(CreateSession(cricket::ICE_CANDIDATE_COMPONENT_RTP));
+  session_->StartGettingPorts();
+  ASSERT_EQ_WAIT(2U, candidates_.size(), 1000);
+  EXPECT_EQ(2U, ports_.size());
+  session_->ClearGettingPorts();
+  WAIT(candidate_allocation_done_, 1000);
+  EXPECT_FALSE(candidate_allocation_done_);
+
+  // After clearing getting ports, adding a new interface will start getting
+  // ports again.
+  candidates_.clear();
+  ports_.clear();
+  candidate_allocation_done_ = false;
+  network_manager_.AddInterface(kClientAddr2);
+  ASSERT_EQ_WAIT(2U, candidates_.size(), 1000);
+  EXPECT_EQ(2U, ports_.size());
+}