diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 623085f9a8..3ac291fa1f 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/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(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(); diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index 9efb96c42d..f3211e4a53 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -161,12 +161,15 @@ class P2PTransportChannel : public TransportChannelImpl, // Public for unit tests. const std::vector& 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 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 OptionMap; OptionMap options_; diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 37cda7c661..5ab3b051c4 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/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()); +}