Stop a session when a new connection becomes writable.

We cannot do it at the end of sorting because it may stop a session too early.
Also remove was_writable_, which is not useful.
BUG=webrtc:5119

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

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

webrtc/p2p/base/p2ptransportchannel.cc

@@ -215,7 +215,6 @@ P2PTransportChannel::P2PTransportChannel(const std::string& transport_name,
       best_connection_(NULL),
       pending_best_connection_(NULL),
       sort_dirty_(false),
-      was_writable_(false),
       remote_ice_mode_(ICEMODE_FULL),
       ice_role_(ICEROLE_UNKNOWN),
       tiebreaker_(0),
@@ -241,6 +240,8 @@ P2PTransportChannel::~P2PTransportChannel() {
 // Add the allocator session to our list so that we know which sessions
 // are still active.
 void P2PTransportChannel::AddAllocatorSession(PortAllocatorSession* session) {
+  ASSERT(worker_thread_ == rtc::Thread::Current());
+
   session->set_generation(static_cast<uint32_t>(allocator_sessions_.size()));
   allocator_sessions_.push_back(session);
 
@@ -1078,11 +1079,8 @@ void P2PTransportChannel::UpdateChannelState() {
   set_receiving(receiving);
 }
 
-// We checked the status of our connections and we had at least one that
-// was writable, go into the writable state.
-void P2PTransportChannel::HandleWritable() {
-  ASSERT(worker_thread_ == rtc::Thread::Current());
-  if (writable()) {
+void P2PTransportChannel::MaybeStopPortAllocatorSessions() {
+  if (!IsGettingPorts()) {
     return;
   }
 
@@ -1098,16 +1096,20 @@ void P2PTransportChannel::HandleWritable() {
     }
     session->StopGettingPorts();
   }
+}
 
-  was_writable_ = true;
-  set_writable(true);
+// Go into the writable state and notify upper layer if it was not before.
+void P2PTransportChannel::HandleWritable() {
+  ASSERT(worker_thread_ == rtc::Thread::Current());
+  if (!writable()) {
+    set_writable(true);
+  }
 }
 
 // Notify upper layer about channel not writable state, if it was before.
 void P2PTransportChannel::HandleNotWritable() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
-  if (was_writable_) {
-    was_writable_ = false;
+  if (writable()) {
     set_writable(false);
   }
 }
@@ -1291,6 +1293,14 @@ void P2PTransportChannel::OnConnectionStateChange(Connection* connection) {
     }
   }
 
+  // May stop the allocator session when at least one connection becomes
+  // strongly connected after starting to get ports. It is not enough to check
+  // that the connection becomes weakly connected because the connection may be
+  // changing from (writable, receiving) to (writable, not receiving).
+  if (!connection->weak()) {
+    MaybeStopPortAllocatorSessions();
+  }
+
   // We have to unroll the stack before doing this because we may be changing
   // the state of connections while sorting.
   RequestSort();

webrtc/p2p/base/p2ptransportchannel.h

@@ -161,12 +161,15 @@ class P2PTransportChannel : public TransportChannelImpl,
   // Public for unit tests.
   const std::vector<Connection*>& connections() const { return connections_; }
 
- private:
-  rtc::Thread* thread() { return worker_thread_; }
+  // Public for unit tests.
   PortAllocatorSession* allocator_session() {
     return allocator_sessions_.back();
   }
 
+ private:
+  rtc::Thread* thread() { return worker_thread_; }
+  bool IsGettingPorts() { return allocator_session()->IsGettingPorts(); }
+
   // A transport channel is weak if the current best connection is either
   // not receiving or not writable, or if there is no best connection at all.
   bool weak() const;
@@ -178,6 +181,7 @@ class P2PTransportChannel : public TransportChannelImpl,
   void HandleWritable();
   void HandleNotWritable();
   void HandleAllTimedOut();
+  void MaybeStopPortAllocatorSessions();
 
   Connection* GetBestConnectionOnNetwork(rtc::Network* network) const;
   bool CreateConnections(const Candidate& remote_candidate,
@@ -239,7 +243,6 @@ class P2PTransportChannel : public TransportChannelImpl,
   Connection* pending_best_connection_;
   std::vector<RemoteCandidate> remote_candidates_;
   bool sort_dirty_;  // indicates whether another sort is needed right now
-  bool was_writable_;
   bool had_connection_ = false;  // if connections_ has ever been nonempty
   typedef std::map<rtc::Socket::Option, int> OptionMap;
   OptionMap options_;

webrtc/p2p/base/p2ptransportchannel_unittest.cc

@@ -2190,3 +2190,31 @@ TEST_F(P2PTransportChannelPingTest, TestDeleteConnectionsIfAllWriteTimedout) {
   conn3->Prune();
   EXPECT_TRUE_WAIT(ch.connections().empty(), 1000);
 }
+
+// Test that after a port allocator session is started, it will be stopped
+// when a new connection becomes writable and receiving. Also test that this
+// holds even if the transport channel did not lose the writability.
+TEST_F(P2PTransportChannelPingTest, TestStopPortAllocatorSessions) {
+  cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr);
+  cricket::P2PTransportChannel ch("test channel", 1, nullptr, &pa);
+  PrepareChannel(&ch);
+  ch.SetIceConfig(CreateIceConfig(2000, false));
+  ch.Connect();
+  ch.MaybeStartGathering();
+  ch.AddRemoteCandidate(CreateCandidate("1.1.1.1", 1, 100));
+  cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1);
+  ASSERT_TRUE(conn1 != nullptr);
+  conn1->ReceivedPingResponse();  // Becomes writable and receiving
+  EXPECT_TRUE(!ch.allocator_session()->IsGettingPorts());
+
+  // Restart gathering even if the transport channel is still writable.
+  // It should stop getting ports after a new connection becomes strongly
+  // connected.
+  ch.SetIceCredentials(kIceUfrag[1], kIcePwd[1]);
+  ch.MaybeStartGathering();
+  ch.AddRemoteCandidate(CreateCandidate("2.2.2.2", 2, 100));
+  cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2);
+  ASSERT_TRUE(conn2 != nullptr);
+  conn2->ReceivedPingResponse();  // Becomes writable and receiving
+  EXPECT_TRUE(!ch.allocator_session()->IsGettingPorts());
+}