Really fix chromium-os:14180 this time.

chrome/browser/chromeos/cros/network_library.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/cros/network_library.h"
 
 #include <algorithm>
 #include <map>
 
 #include "base/i18n/time_formatting.h"
@@ skipping 3647 matching lines @@
   void UpdateNetworkDeviceList(const ListValue* devices) {
     NetworkDeviceMap old_device_map = device_map_;
     device_map_.clear();
     VLOG(2) << "Updating Device List.";
     for (ListValue::const_iterator iter = devices->begin();
          iter != devices->end(); ++iter) {
       std::string device_path;
       (*iter)->GetAsString(&device_path);
       if (!device_path.empty()) {
         NetworkDeviceMap::iterator found = old_device_map.find(device_path);
-        if (found != old_device_map.end()) {
-          VLOG(2) << " Adding device: " << device_path;
-          device_map_[device_path] = found->second;
-          old_device_map.erase(found);
+        if (found == old_device_map.end()) {
+          VLOG(2) << " New device: " << device_path;
           // Add device property monitor before we request the
           // full property list, to ensure that we won't miss any
           // property changes.
           network_device_observers_[device_path] =
               new NetworkDeviceObserverList(this, device_path);

[stevenjb, 2011/05/12 22:33:06]

I think that only adding the observer when creating the NetworkDevice (in
ParseNetworkDevice) is sufficient and would be better (i.e. remove this line),
but I don't think this will cause any problems and should be fine for R12.

[Eric Shienbrood, 2011/05/12 22:41:18]

Since we're requesting device info immediately below, I'm concerned that there
would be a window during which we could miss property changes. It seems like the
only safe way to do things is to make sure we're monitoring properties *before*
we request device info. Do you disagree?

I'll hold off committing until I hear back from you.

On 2011/05/12 22:33:06, Steven Bennetts wrote:

[stevenjb, 2011/05/12 23:00:24]

Well, since the device doesn't exist yet (i.e. it hasn't been allocated), then
any property changes received wouldn't do anything anyway, so if that is really
happening we would need to keep track of property changes for devices that don't
exist yet... But it seems like when we receive a new device, it's properties
ought to be up to date.

What we might want to do however is call NotifyNetworkDeviceChanged(device) in
ParseNetworkDevice() to notify any UI monitoring devices but not the manager.

[Eric Shienbrood, 2011/05/13 21:48:48]

If they're not monitoring the manager, then they won't have found out when the
device was added, and thus wouldn't have set up an observer for the device. If
they are monitoring the manager, then calling NotifyNetworkDeviceChanged is
almost certainly redundant.
On 2011/05/12 23:00:24, Steven Bennetts wrote:

[Eric Shienbrood, 2011/05/13 21:48:48]

I still think there's a theoretical window of vulnerability, but in practice the
order of the signals sent out from flimflam is preserved, and there's only one
thread handling all the signals, so I think it's safe to ignore this. In other
words, I'm taking your suggestion.

-
+        } else {
+          VLOG(2) << " Adding existing device: " << device_path;
+          device_map_[device_path] = found->second;
+          old_device_map.erase(found);
         }
         RequestNetworkDeviceInfo(
             device_path.c_str(), &NetworkDeviceUpdate, this);
       }
     }
     // Delete any old devices that no longer exist.
     for (NetworkDeviceMap::iterator iter = old_device_map.begin();
          iter != old_device_map.end(); ++iter) {
       DeleteDeviceFromDeviceObserversMap(iter->first);
       // Delete device.
@@ skipping 28 matching lines @@
   void ParseNetworkDevice(const std::string& device_path,
                           const DictionaryValue* info) {
     NetworkDeviceMap::iterator found = device_map_.find(device_path);
     NetworkDevice* device;
     if (found != device_map_.end()) {
       device = found->second;
     } else {
       device = new NetworkDevice(device_path);
       VLOG(2) << " Adding device: " << device_path;
       device_map_[device_path] = device;
+      if (network_device_observers_.find(device_path) ==
+          network_device_observers_.end()) {
+        network_device_observers_[device_path] =
+            new NetworkDeviceObserverList(this, device_path);
+      }
     }
     device->ParseInfo(info);
     VLOG(1) << "ParseNetworkDevice:" << device->name();
     NotifyNetworkManagerChanged(false);  // Not forced.

[stevenjb, 2011/05/12 23:00:24]

NotifyNetworkDeviceChanged(device); ?

   }
 
   ////////////////////////////////////////////////////////////////////////////
 
   void EnableNetworkDeviceType(ConnectionType device, bool enable) {
     if (!EnsureCrosLoaded())
       return;
 
     // If network device is already enabled/disabled, then don't do anything.
     if (enable && (enabled_devices_ & (1 << device))) {
@@ skipping 703 matching lines @@
     return new NetworkLibraryStubImpl();
   else
     return new NetworkLibraryImpl();
 }
 
 }  // namespace chromeos
 
 // Allows InvokeLater without adding refcounting. This class is a Singleton and
 // won't be deleted until it's last InvokeLater is run.
 DISABLE_RUNNABLE_METHOD_REFCOUNT(chromeos::NetworkLibraryImpl);