From e3c6c827179e9a340154338aad182604d4546079 Mon Sep 17 00:00:00 2001 From: honghaiz Date: Wed, 17 Feb 2016 13:00:28 -0800 Subject: [PATCH] When doing continual gathering, remove the local ports when a corresponding network is dropped. BUG= Review URL: https://codereview.webrtc.org/1696933003 Cr-Commit-Position: refs/heads/master@{#11660} --- webrtc/base/network.cc | 9 +++--- webrtc/base/network.h | 12 ++++++- webrtc/base/network_unittest.cc | 17 ++++++++++ webrtc/p2p/base/p2ptransportchannel.cc | 27 ++++++++++++++-- webrtc/p2p/base/p2ptransportchannel.h | 1 + .../p2p/base/p2ptransportchannel_unittest.cc | 31 +++++++++++++++++++ webrtc/p2p/base/port.cc | 13 ++++++-- webrtc/p2p/base/port.h | 2 ++ webrtc/p2p/base/portinterface.h | 3 ++ 9 files changed, 104 insertions(+), 11 deletions(-) diff --git a/webrtc/base/network.cc b/webrtc/base/network.cc index 6b04f891a1..4f3b91981e 100644 --- a/webrtc/base/network.cc +++ b/webrtc/base/network.cc @@ -317,10 +317,11 @@ void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks, networks_ = merged_list; // Reset the active states of all networks. for (const auto& kv : networks_map_) { - kv.second->set_active(false); - } - for (Network* network : networks_) { - network->set_active(true); + Network* network = kv.second; + // If |network| is in the newly generated |networks_|, it is active. + bool found = std::find(networks_.begin(), networks_.end(), network) != + networks_.end(); + network->set_active(found); } std::sort(networks_.begin(), networks_.end(), SortNetworks); // Now network interfaces are sorted, we should set the preference value diff --git a/webrtc/base/network.h b/webrtc/base/network.h index 10ce6f0e01..a81bcabf66 100644 --- a/webrtc/base/network.h +++ b/webrtc/base/network.h @@ -265,6 +265,8 @@ class Network { AdapterType type); ~Network(); + sigslot::signal1 SignalInactive; + const DefaultLocalAddressProvider* default_local_address_provider() { return default_local_address_provider_; } @@ -342,7 +344,15 @@ class Network { // we do not remove it (because it may be used elsewhere). Instead, we mark // it inactive, so that we can detect network changes properly. bool active() const { return active_; } - void set_active(bool active) { active_ = active; } + void set_active(bool active) { + if (active_ == active) { + return; + } + active_ = active; + if (!active) { + SignalInactive(this); + } + } // Debugging description of this network std::string ToString() const; diff --git a/webrtc/base/network_unittest.cc b/webrtc/base/network_unittest.cc index 0f7d6db8ff..7ad45a37af 100644 --- a/webrtc/base/network_unittest.cc +++ b/webrtc/base/network_unittest.cc @@ -58,6 +58,16 @@ class NetworkTest : public testing::Test, public sigslot::has_slots<> { callback_called_ = true; } + void listenToNetworkInactive(BasicNetworkManager& network_manager) { + BasicNetworkManager::NetworkList networks; + network_manager.GetNetworks(&networks); + for (Network* network : networks) { + network->SignalInactive.connect(this, &NetworkTest::OnNetworkInactive); + } + } + + void OnNetworkInactive(const Network* network) { num_networks_inactive_++; } + NetworkManager::Stats MergeNetworkList( BasicNetworkManager& network_manager, const NetworkManager::NetworkList& list, @@ -147,6 +157,8 @@ class NetworkTest : public testing::Test, public sigslot::has_slots<> { protected: bool callback_called_; + // Number of networks that become inactive. + int num_networks_inactive_ = 0; }; class TestBasicNetworkManager : public BasicNetworkManager { @@ -280,6 +292,7 @@ TEST_F(NetworkTest, TestBasicMergeNetworkList) { EXPECT_TRUE(changed); EXPECT_EQ(stats.ipv6_network_count, 0); EXPECT_EQ(stats.ipv4_network_count, 1); + listenToNetworkInactive(manager); list.clear(); manager.GetNetworks(&list); @@ -294,7 +307,9 @@ TEST_F(NetworkTest, TestBasicMergeNetworkList) { EXPECT_TRUE(changed); EXPECT_EQ(stats.ipv6_network_count, 0); EXPECT_EQ(stats.ipv4_network_count, 1); + EXPECT_EQ(1, num_networks_inactive_); list.clear(); + num_networks_inactive_ = 0; manager.GetNetworks(&list); EXPECT_EQ(1U, list.size()); @@ -309,6 +324,7 @@ TEST_F(NetworkTest, TestBasicMergeNetworkList) { EXPECT_TRUE(changed); EXPECT_EQ(stats.ipv6_network_count, 0); EXPECT_EQ(stats.ipv4_network_count, 2); + EXPECT_EQ(0, num_networks_inactive_); list.clear(); // Verify that we get previous instances of Network objects. @@ -326,6 +342,7 @@ TEST_F(NetworkTest, TestBasicMergeNetworkList) { EXPECT_FALSE(changed); EXPECT_EQ(stats.ipv6_network_count, 0); EXPECT_EQ(stats.ipv4_network_count, 2); + EXPECT_EQ(0, num_networks_inactive_); list.clear(); // Verify that we get previous instances of Network objects. diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 87a50bc93a..74f1392b1b 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -12,13 +12,15 @@ #include #include -#include "webrtc/p2p/base/common.h" -#include "webrtc/p2p/base/relayport.h" // For RELAY_PORT_TYPE. -#include "webrtc/p2p/base/stunport.h" // For STUN_PORT_TYPE. + #include "webrtc/base/common.h" #include "webrtc/base/crc32.h" #include "webrtc/base/logging.h" #include "webrtc/base/stringencode.h" +#include "webrtc/p2p/base/candidate.h" +#include "webrtc/p2p/base/common.h" +#include "webrtc/p2p/base/relayport.h" // For RELAY_PORT_TYPE. +#include "webrtc/p2p/base/stunport.h" // For STUN_PORT_TYPE. #include "webrtc/system_wrappers/include/field_trial.h" namespace { @@ -464,6 +466,8 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession *session, port->SignalUnknownAddress.connect( this, &P2PTransportChannel::OnUnknownAddress); port->SignalDestroyed.connect(this, &P2PTransportChannel::OnPortDestroyed); + port->SignalNetworkInactive.connect( + this, &P2PTransportChannel::OnPortNetworkInactive); port->SignalRoleConflict.connect( this, &P2PTransportChannel::OnRoleConflict); port->SignalSentPacket.connect(this, &P2PTransportChannel::OnSentPacket); @@ -1415,6 +1419,23 @@ void P2PTransportChannel::OnPortDestroyed(PortInterface* port) { << static_cast(ports_.size()) << " remaining"; } +void P2PTransportChannel::OnPortNetworkInactive(PortInterface* port) { + // If it does not gather continually, the port will be removed from the list + // when ICE restarts. + if (!gather_continually_) { + return; + } + auto it = std::find(ports_.begin(), ports_.end(), port); + // Don't need to do anything if the port has been deleted from the port list. + if (it == ports_.end()) { + return; + } + ports_.erase(it); + LOG(INFO) << "Removed port due to inactive networks: " << ports_.size() + << " remaining"; + // TODO(honghaiz): Signal candidate removals to the remote side. +} + // We data is available, let listeners know void P2PTransportChannel::OnReadPacket(Connection* connection, const char* data, diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index f2e9315343..fde00263d0 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -227,6 +227,7 @@ class P2PTransportChannel : public TransportChannelImpl, const std::string& remote_username, bool port_muxed); void OnPortDestroyed(PortInterface* port); + void OnPortNetworkInactive(PortInterface* port); void OnRoleConflict(PortInterface* port); void OnConnectionStateChange(Connection* connection); diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index ab17cfb55d..38f12fa7ff 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1770,6 +1770,37 @@ TEST_F(P2PTransportChannelMultihomedTest, TestGetState) { ep2_ch1()->GetState(), 1000); } +// Tests that when a network interface becomes inactive, the ports associated +// with that network will be removed from the port list of the channel if +// and only if Continual Gathering is enabled. +TEST_F(P2PTransportChannelMultihomedTest, TestNetworkBecomesInactive) { + AddAddress(0, kPublicAddrs[0]); + AddAddress(1, kPublicAddrs[1]); + // Create channels and let them go writable, as usual. + CreateChannels(1); + ep1_ch1()->SetIceConfig(CreateIceConfig(2000, true)); + ep2_ch1()->SetIceConfig(CreateIceConfig(2000, false)); + + SetAllocatorFlags(0, kOnlyLocalPorts); + SetAllocatorFlags(1, kOnlyLocalPorts); + EXPECT_TRUE_WAIT_MARGIN(ep1_ch1()->receiving() && ep1_ch1()->writable() && + ep2_ch1()->receiving() && ep2_ch1()->writable(), + 1000, 1000); + // More than one port has been created. + EXPECT_LE(1U, ep1_ch1()->ports().size()); + // Endpoint 1 enabled continual gathering; the port will be removed + // when the interface is removed. + RemoveAddress(0, kPublicAddrs[0]); + EXPECT_EQ(0U, ep1_ch1()->ports().size()); + + size_t num_ports = ep2_ch1()->ports().size(); + EXPECT_LE(1U, num_ports); + // Endpoint 2 did not enable continual gathering; the port will not be removed + // when the interface is removed. + RemoveAddress(1, kPublicAddrs[1]); + EXPECT_EQ(num_ports, ep2_ch1()->ports().size()); +} + /* TODO(pthatcher): Once have a way to handle network interfaces changes diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index 4d95279bba..ed26fe494c 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -20,6 +20,7 @@ #include "webrtc/base/helpers.h" #include "webrtc/base/logging.h" #include "webrtc/base/messagedigest.h" +#include "webrtc/base/network.h" #include "webrtc/base/scoped_ptr.h" #include "webrtc/base/stringencode.h" #include "webrtc/base/stringutils.h" @@ -195,6 +196,7 @@ void Port::Construct() { ice_username_fragment_ = rtc::CreateRandomString(ICE_UFRAG_LENGTH); password_ = rtc::CreateRandomString(ICE_PWD_LENGTH); } + network_->SignalInactive.connect(this, &Port::OnNetworkInactive); // TODO(honghaiz): Make it configurable from user setting. network_cost_ = (network_->type() == rtc::ADAPTER_TYPE_CELLULAR) ? kMaxNetworkCost : 0; @@ -633,11 +635,16 @@ void Port::OnMessage(rtc::Message *pmsg) { } } +void Port::OnNetworkInactive(const rtc::Network* network) { + ASSERT(network == network_); + SignalNetworkInactive(this); +} + std::string Port::ToString() const { std::stringstream ss; - ss << "Port[" << content_name_ << ":" << component_ - << ":" << generation_ << ":" << type_ - << ":" << network_->ToString() << "]"; + ss << "Port[" << std::hex << this << std::dec << ":" << content_name_ << ":" + << component_ << ":" << generation_ << ":" << type_ << ":" + << network_->ToString() << "]"; return ss.str(); } diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 95abe86464..75b786fd0c 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -358,6 +358,8 @@ class Port : public PortInterface, public rtc::MessageHandler, return ice_role_ == ICEROLE_CONTROLLED && connections_.empty(); } + void OnNetworkInactive(const rtc::Network* network); + rtc::Thread* thread_; rtc::PacketSocketFactory* factory_; std::string type_; diff --git a/webrtc/p2p/base/portinterface.h b/webrtc/p2p/base/portinterface.h index e83879f3b7..e73861965e 100644 --- a/webrtc/p2p/base/portinterface.h +++ b/webrtc/p2p/base/portinterface.h @@ -104,6 +104,9 @@ class PortInterface { // any usefulness. sigslot::signal1 SignalDestroyed; + // Signaled when the network used by this port becomes inactive. + sigslot::signal1 SignalNetworkInactive; + // Signaled when Port discovers ice role conflict with the peer. sigslot::signal1 SignalRoleConflict;