Fix CreateNetworks to stop it from signaling duplicate networks changed events

webrtc/base/network.cc

Index: webrtc/base/network.cc
diff --git a/webrtc/base/network.cc b/webrtc/base/network.cc
index 879c1e4529d9f560e0b550795dc7f4826a5ae9d1..264300ba36b544a2261289b0968dfcd2f343eb1e 100644
--- a/webrtc/base/network.cc
+++ b/webrtc/base/network.cc
@@ -230,20 +230,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;
@@ -275,6 +267,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;
@@ -289,17 +283,34 @@ 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);
-      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.
@@ -417,6 +428,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();

[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 {
@@ -573,6 +585,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 {
@@ -748,21 +761,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()
-                   << ((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" : "");
   }
 }