Fix two problems in network.cc:

1. It signals network changed events whenever there are more than one IP address in a network.
2. It does not signal network changed events if a network disconnects and connects again.
Also changed DumpNetworks for better debugging.

BUG=webrtc:5096

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

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

webrtc/base/network.cc

@@ -242,20 +242,12 @@ void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks,
 void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks,
                                           bool* changed,
                                           NetworkManager::Stats* stats) {
+  *changed = false;
   // AddressList in this map will track IP addresses for all Networks
   // with the same key.
   std::map<std::string, AddressList> consolidated_address_list;
   NetworkList list(new_networks);
-
-  // Result of Network merge. Element in this list should have unique key.
-  NetworkList merged_list;
   std::sort(list.begin(), list.end(), CompareNetworks);
-
-  *changed = false;
-
-  if (networks_.size() != list.size())
-    *changed = true;
-
   // First, build a set of network-keys to the ipaddresses.
   for (Network* network : list) {
     bool might_add_to_merged_list = false;
@@ -287,6 +279,8 @@ void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks,
   }
 
   // Next, look for existing network objects to re-use.
+  // Result of Network merge. Element in this list should have unique key.
+  NetworkList merged_list;
   for (const auto& kv : consolidated_address_list) {
     const std::string& key = kv.first;
     Network* net = kv.second.net;
@@ -301,17 +295,36 @@ void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks,
       *changed = true;
     } else {
       // This network exists in the map already. Reset its IP addresses.
-      *changed = existing->second->SetIPs(kv.second.ips, *changed);
+      Network* existing_net = existing->second;
-      merged_list.push_back(existing->second);
+      *changed = existing_net->SetIPs(kv.second.ips, *changed);
-      if (existing->second != net) {
+      merged_list.push_back(existing_net);
+      // If the existing network was not active, networks have changed.
+      if (!existing_net->active()) {
+        *changed = true;
+      }
+      ASSERT(net->active());
+      if (existing_net != net) {
         delete net;
       }
     }
   }
-  networks_ = merged_list;
+  // It may still happen that the merged list is a subset of |networks_|.
+  // To detect this change, we compare their sizes.
+  if (merged_list.size() != networks_.size()) {
+    *changed = true;
+  }
 
-  // If the network lists changes, we resort it.
+  // If the network list changes, we re-assign |networks_| to the merged list
+  // and re-sort it.
   if (*changed) {
+    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);
+    }
     std::sort(networks_.begin(), networks_.end(), SortNetworks);
     // Now network interfaces are sorted, we should set the preference value
     // for each of the interfaces we are planning to use.
@@ -450,6 +463,7 @@ void BasicNetworkManager::ConvertIfAddrs(struct ifaddrs* interfaces,
       network->AddIP(ip);
       network->set_ignored(IsIgnoredNetwork(*network));
       if (include_ignored || !network->ignored()) {
+        current_networks[key] = network.get();
         networks->push_back(network.release());
       }
     } else {
@@ -604,6 +618,7 @@ bool BasicNetworkManager::CreateNetworks(bool include_ignored,
           bool ignored = IsIgnoredNetwork(*network);
           network->set_ignored(ignored);
           if (include_ignored || !network->ignored()) {
+            current_networks[key] = network.get();
             networks->push_back(network.release());
           }
         } else {
@@ -801,21 +816,14 @@ void BasicNetworkManager::UpdateNetworksContinually() {
   thread_->PostDelayed(kNetworksUpdateIntervalMs, this, kUpdateNetworksMessage);
 }
 
-void BasicNetworkManager::DumpNetworks(bool include_ignored) {
+void BasicNetworkManager::DumpNetworks() {
   NetworkList list;
-  CreateNetworks(include_ignored, &list);
+  GetNetworks(&list);
   LOG(LS_INFO) << "NetworkManager detected " << list.size() << " networks:";
   for (const Network* network : list) {
-    if (!network->ignored() || include_ignored) {
+    LOG(LS_INFO) << network->ToString() << ": " << network->description()
-      LOG(LS_INFO) << network->ToString() << ": "
+                 << ", active ? " << network->active()
-                   << network->description()
+                 << ((network->ignored()) ? ", Ignored" : "");
-                   << ((network->ignored()) ? ", Ignored" : "");
-    }
-  }
-  // Release the network list created previously.
-  // Do this in a seperated for loop for better readability.
-  for (Network* network : list) {
-    delete network;
   }
 }
 

webrtc/base/network.h

@@ -111,11 +111,10 @@ class NetworkManager : public DefaultLocalAddressProvider {
   // TODO(guoweis): remove this body when chromium implements this.
   virtual void GetAnyAddressNetworks(NetworkList* networks) {}
 
+  // Dumps the current list of networks in the network manager.
+  virtual void DumpNetworks() {}
   bool GetDefaultLocalAddress(int family, IPAddress* ipaddr) const override;
 
-  // Dumps a list of networks available to LS_INFO.
-  virtual void DumpNetworks(bool include_ignored) {}
-
   struct Stats {
     int ipv4_network_count;
     int ipv6_network_count;
@@ -195,8 +194,7 @@ class BasicNetworkManager : public NetworkManagerBase,
   void StartUpdating() override;
   void StopUpdating() override;
 
-  // Logs the available networks.
+  void DumpNetworks() override;
-  void DumpNetworks(bool include_ignored) override;
 
   // MessageHandler interface.
   void OnMessage(Message* msg) override;
@@ -359,6 +357,12 @@ class Network {
   int preference() const { return preference_; }
   void set_preference(int preference) { preference_ = preference; }
 
+  // When we enumerate networks and find a previously-seen network is missing,
+  // 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; }
+
   // Debugging description of this network
   std::string ToString() const;
 
@@ -374,6 +378,7 @@ class Network {
   bool ignored_;
   AdapterType type_;
   int preference_;
+  bool active_ = true;
 
   friend class NetworkManager;
 };

webrtc/base/network_unittest.cc

@@ -91,6 +91,46 @@ class NetworkTest : public testing::Test, public sigslot::has_slots<>  {
                                  NetworkManager::NetworkList* networks) {
     network_manager.ConvertIfAddrs(interfaces, include_ignored, networks);
   }
+
+  struct sockaddr_in6* CreateIpv6Addr(const std::string& ip_string,
+                                      uint32_t scope_id) {
+    struct sockaddr_in6* ipv6_addr = new struct sockaddr_in6;
+    memset(ipv6_addr, 0, sizeof(struct sockaddr_in6));
+    ipv6_addr->sin6_family = AF_INET6;
+    ipv6_addr->sin6_scope_id = scope_id;
+    IPAddress ip;
+    IPFromString(ip_string, &ip);
+    ipv6_addr->sin6_addr = ip.ipv6_address();
+    return ipv6_addr;
+  }
+
+  // Pointers created here need to be released via ReleaseIfAddrs.
+  struct ifaddrs* AddIpv6Address(struct ifaddrs* list,
+                                 char* if_name,
+                                 const std::string& ipv6_address,
+                                 const std::string& ipv6_netmask,
+                                 uint32_t scope_id) {
+    struct ifaddrs* if_addr = new struct ifaddrs;
+    memset(if_addr, 0, sizeof(struct ifaddrs));
+    if_addr->ifa_name = if_name;
+    if_addr->ifa_addr = reinterpret_cast<struct sockaddr*>(
+        CreateIpv6Addr(ipv6_address, scope_id));
+    if_addr->ifa_netmask =
+        reinterpret_cast<struct sockaddr*>(CreateIpv6Addr(ipv6_netmask, 0));
+    if_addr->ifa_next = list;
+    return if_addr;
+  }
+
+  void ReleaseIfAddrs(struct ifaddrs* list) {
+    struct ifaddrs* if_addr = list;
+    while (if_addr != nullptr) {
+      struct ifaddrs* next_addr = if_addr->ifa_next;
+      delete if_addr->ifa_addr;
+      delete if_addr->ifa_netmask;
+      delete if_addr;
+      if_addr = next_addr;
+    }
+  }
 #endif  // defined(WEBRTC_POSIX)
 
  protected:
@@ -590,10 +630,13 @@ TEST_F(NetworkTest, TestMultiplePublicNetworksOnOneInterfaceMerge) {
   }
 }
 
-// Test that DumpNetworks works.
+// Test that DumpNetworks does not crash.
-TEST_F(NetworkTest, TestDumpNetworks) {
+TEST_F(NetworkTest, TestCreateAndDumpNetworks) {
   BasicNetworkManager manager;
-  manager.DumpNetworks(true);
+  NetworkManager::NetworkList list = GetNetworks(manager, true);
+  bool changed;
+  MergeNetworkList(manager, list, &changed);
+  manager.DumpNetworks();
 }
 
 // Test that we can toggle IPv6 on and off.
@@ -700,6 +743,25 @@ TEST_F(NetworkTest, TestConvertIfAddrsNoAddress) {
   CallConvertIfAddrs(manager, &list, true, &result);
   EXPECT_TRUE(result.empty());
 }
+
+// Verify that if there are two addresses on one interface, only one network
+// is generated.
+TEST_F(NetworkTest, TestConvertIfAddrsMultiAddressesOnOneInterface) {
+  char if_name[20] = "rmnet0";
+  ifaddrs* list = nullptr;
+  list = AddIpv6Address(list, if_name, "1000:2000:3000:4000:0:0:0:1",
+                        "FFFF:FFFF:FFFF:FFFF::", 0);
+  list = AddIpv6Address(list, if_name, "1000:2000:3000:4000:0:0:0:2",
+                        "FFFF:FFFF:FFFF:FFFF::", 0);
+  NetworkManager::NetworkList result;
+  BasicNetworkManager manager;
+  CallConvertIfAddrs(manager, list, true, &result);
+  EXPECT_EQ(1U, result.size());
+  bool changed;
+  // This ensures we release the objects created in CallConvertIfAddrs.
+  MergeNetworkList(manager, result, &changed);
+  ReleaseIfAddrs(list);
+}
 #endif  // defined(WEBRTC_POSIX)
 
 #if defined(WEBRTC_LINUX) && !defined(WEBRTC_ANDROID)
@@ -783,6 +845,49 @@ TEST_F(NetworkTest, TestMergeNetworkList) {
   EXPECT_EQ(list2[0]->GetIPs()[1], ip2);
 }
 
+// Test that MergeNetworkList successfully detects the change if
+// a network becomes inactive and then active again.
+TEST_F(NetworkTest, TestMergeNetworkListWithInactiveNetworks) {
+  BasicNetworkManager manager;
+  Network network1("test_wifi", "Test Network Adapter 1",
+                   IPAddress(0x12345600U), 24);
+  Network network2("test_eth0", "Test Network Adapter 2",
+                   IPAddress(0x00010000U), 16);
+  network1.AddIP(IPAddress(0x12345678));
+  network2.AddIP(IPAddress(0x00010004));
+  NetworkManager::NetworkList list;
+  Network* net1 = new Network(network1);
+  list.push_back(net1);
+  bool changed;
+  MergeNetworkList(manager, list, &changed);
+  EXPECT_TRUE(changed);
+  list.clear();
+  manager.GetNetworks(&list);
+  ASSERT_EQ(1U, list.size());
+  EXPECT_EQ(net1, list[0]);
+
+  list.clear();
+  Network* net2 = new Network(network2);
+  list.push_back(net2);
+  MergeNetworkList(manager, list, &changed);
+  EXPECT_TRUE(changed);
+  list.clear();
+  manager.GetNetworks(&list);
+  ASSERT_EQ(1U, list.size());
+  EXPECT_EQ(net2, list[0]);
+
+  // Now network1 is inactive. Try to merge it again.
+  list.clear();
+  list.push_back(new Network(network1));
+  MergeNetworkList(manager, list, &changed);
+  EXPECT_TRUE(changed);
+  list.clear();
+  manager.GetNetworks(&list);
+  ASSERT_EQ(1U, list.size());
+  EXPECT_TRUE(list[0]->active());
+  EXPECT_EQ(net1, list[0]);
+}
+
 // Test that the filtering logic follows the defined ruleset in network.h.
 TEST_F(NetworkTest, TestIPv6Selection) {
   InterfaceAddress ip;