Fix CreateNetworks to stop it from signaling duplicate networks changed events

webrtc/base/network.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 224 matching lines @@
 
 void NetworkManagerBase::MergeNetworkList(const NetworkList& new_networks,
                                           bool* changed) {
   NetworkManager::Stats stats;
   MergeNetworkList(new_networks, changed, &stats);
 }
 
 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;
     std::string key = MakeNetworkKey(network->name(),
                                      network->prefix(),
                                      network->prefix_length());
     if (consolidated_address_list.find(key) ==
         consolidated_address_list.end()) {
       AddressList addrlist;
       addrlist.net = network;
@@ skipping 11 matching lines @@
       if (current_list.ips[0].family() == AF_INET) {
         stats->ipv4_network_count++;
       } else {
         ASSERT(current_list.ips[0].family() == AF_INET6);
         stats->ipv6_network_count++;
       }
     }
   }
 
   // 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;
     auto existing = networks_map_.find(key);
     if (existing == networks_map_.end()) {
       // This network is new. Place it in the network map.
       merged_list.push_back(net);
       networks_map_[key] = net;
       // Also, we might have accumulated IPAddresses from the first
       // step, set it here.
       net->SetIPs(kv.second.ips, true);
       *changed = true;
     } else {
       // This network exists in the map already. Reset its IP addresses.
-      *changed = existing->second->SetIPs(kv.second.ips, *changed);
-      merged_list.push_back(existing->second);
-      if (existing->second != net) {
+      Network* existing_net = existing->second;
+      *changed = existing_net->SetIPs(kv.second.ips, *changed);
+      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.
     // Preference order of network interfaces might have changed from previous
     // sorting due to addition of higher preference network interface.
     // Since we have already sorted the network interfaces based on our
     // requirements, we will just assign a preference value starting with 127,
     // in decreasing order.
     int pref = kHighestNetworkPreference;
     for (Network* network : networks_) {
@@ skipping 118 matching lines @@
 #endif
       // TODO(phoglund): Need to recognize other types as well.
       scoped_ptr<Network> network(new Network(cursor->ifa_name,
                                               cursor->ifa_name, prefix,
                                               prefix_length, adapter_type));
       network->set_default_local_address_provider(this);
       network->set_scope_id(scope_id);
       network->AddIP(ip);
       network->set_ignored(IsIgnoredNetwork(*network));
       if (include_ignored || !network->ignored()) {
+        current_networks[key] = network.get();

[pthatcher1, 2015/12/15 08:13:09]

OK, I understand this now.  But it makes me think that you're fixing a bug where
we currently won't group IPs of the same network into the same network.  If you
are fixing this bug, and it looks like you are, can you add a unit test that
fails with the current code but passes with your fixed code?  It appears our
test coverage is lacking if this has been here incorrect for so long.

[honghaiz3, 2015/12/15 20:04:53]

I have a test there already. 
Note that this does not cause big problems though because later in
MergeNetworkList, they combine the networks with the same interface, except that
it will signal a network change event. 
We can simply the code MergeNetworkList once this is landed as that is not
harmful (and less change now).   

         networks->push_back(network.release());
       }
     } else {
       (*existing_network).second->AddIP(ip);
     }
   }
 }
 
 bool BasicNetworkManager::CreateNetworks(bool include_ignored,
                                          NetworkList* networks) const {
@@ skipping 134 matching lines @@
             adapter_type = ADAPTER_TYPE_LOOPBACK;
           }
           scoped_ptr<Network> network(new Network(name, description, prefix,
                                                   prefix_length, adapter_type));
           network->set_default_local_address_provider(this);
           network->set_scope_id(scope_id);
           network->AddIP(ip);
           bool ignored = IsIgnoredNetwork(*network);
           network->set_ignored(ignored);
           if (include_ignored || !network->ignored()) {
+            current_networks[key] = network.get();
             networks->push_back(network.release());
           }
         } else {
           (*existing_network).second->AddIP(ip);
         }
       }
       // Count is per-adapter - all 'Networks' created from the same
       // adapter need to have the same name.
       ++count;
     }
@@ skipping 177 matching lines @@
       sent_first_update_ = true;
     }
   }
 }
 
 void BasicNetworkManager::UpdateNetworksContinually() {
   UpdateNetworksOnce();
   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()
-                   << ((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;
+    LOG(LS_INFO) << network->ToString() << ": " << network->description()
+                 << ", active ? " << network->active()
+                 << ((network->ignored()) ? ", Ignored" : "");
   }
 }
 
 Network::Network(const std::string& name,
                  const std::string& desc,
                  const IPAddress& prefix,
                  int prefix_length)
     : name_(name),
       description_(desc),
       prefix_(prefix),
@@ skipping 82 matching lines @@
   std::stringstream ss;
   // Print out the first space-terminated token of the network desc, plus
   // the IP address.
   ss << "Net[" << description_.substr(0, description_.find(' '))
      << ":" << prefix_.ToSensitiveString() << "/" << prefix_length_
      << ":" << AdapterTypeToString(type_) << "]";
   return ss.str();
 }
 
 }  // namespace rtc