Really fix chromium-os:14180 this time.

chrome/browser/chromeos/cros/network_library.cc

Index: chrome/browser/chromeos/cros/network_library.cc
diff --git a/chrome/browser/chromeos/cros/network_library.cc b/chrome/browser/chromeos/cros/network_library.cc
index 4cef679b9b0a7499dbbc7bace86383b48d18b3c8..f20cdbdd42f655514a1ae99a706320c2f8548f62 100644
--- a/chrome/browser/chromeos/cros/network_library.cc
+++ b/chrome/browser/chromeos/cros/network_library.cc
@@ -3665,16 +3665,17 @@ class NetworkLibraryImpl : public NetworkLibrary  {
       (*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);
@@ -3723,6 +3724,11 @@ class NetworkLibraryImpl : public NetworkLibrary  {
       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();