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 212 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 (existing_net != net) {
         delete net;
       }
+      // If the existing network was not active, networks have changed.
+      if (!existing_net->active()) {
+        *changed = true;
+      }

[pthatcher1, 2015/12/08 19:40:23]

Is kv.second.active() always true?  If not, should this be "if
(existing_net->active() != kv.second.active())"?  If so, should we have an
ASSERT(kv.second.active())?

[honghaiz3, 2015/12/10 19:26:18]

It should be always active because active_ is true by default.

     }
   }
-  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 and resort it.

[pthatcher1, 2015/12/08 19:40:23]

By re-assign you mean reset the active state?  If so, please just state that.

[honghaiz3, 2015/12/10 19:26:18]

I meant re-assign networks_ to the merged list. Changed the comments.

   if (*changed) {
+    networks_ = merged_list;
+    // Reset the active states of all networks.
+    for (const auto& kv : networks_map_) {
+      kv.second->set_active(false);

[pthatcher1, 2015/12/08 19:40:23]

So, we leave it in the map even if it's not active?  Is that what we did before?

[honghaiz3, 2015/12/10 19:26:18]

Yes. 
Previously it keeps all networks in the map and it does not update its active
state even if a network was gone and come back again.

+    }
+    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 97 matching lines @@
       // 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_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/08 19:40:23]

Can you explain why this is necessary, here and below?  And maybe leave a
comment explaining.  It's not immediately obvious.

[honghaiz3, 2015/12/10 19:26:18]

If you look at the if condition one-level outside, 
"if (existing_network == current_networks.end()) "
It is fairly obvious to add this because it tried to check whether this key is
in the map but never added any key to the map. 
I just added the key to map so that next time if the same key is seen, it will
find this is an existing network. 

         networks->push_back(network.release());
       }
     } else {
       (*existing_network).second->AddIP(ip);
     }
   }
 }
 
 bool BasicNetworkManager::CreateNetworks(bool include_ignored,
                                          NetworkList* networks) const {
@@ skipping 136 matching lines @@
           scoped_ptr<Network> network(new Network(name,
                                                   description,
                                                   prefix,
                                                   prefix_length,
                                                   adapter_type));
           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 155 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),
       prefix_length_(prefix_length),
       key_(MakeNetworkKey(name, prefix, prefix_length)), scope_id_(0),
       ignored_(false), type_(ADAPTER_TYPE_UNKNOWN), preference_(0) {
 }
@@ skipping 69 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