Defer NetworkConfigurationHandler::CreateShillConfiguration callback

chromeos/network/network_configuration_handler.cc

Index: chromeos/network/network_configuration_handler.cc
diff --git a/chromeos/network/network_configuration_handler.cc b/chromeos/network/network_configuration_handler.cc
index 03bf1fbe0b4f9e8ee0fd5c4ee91c008654b897d2..a890a25d50889d53a04b540ba0dad8a5d0eca75c 100644
--- a/chromeos/network/network_configuration_handler.cc
+++ b/chromeos/network/network_configuration_handler.cc
@@ -79,8 +79,7 @@ void LogConfigProperties(const std::string& desc,
 // Service and delete the service from each profile. Triggers either
 // |callback| on success or |error_callback| on failure, and calls
 // |handler|->ProfileEntryDeleterCompleted() on completion to delete itself.
-class NetworkConfigurationHandler::ProfileEntryDeleter
-    : public base::SupportsWeakPtr<ProfileEntryDeleter> {
+class NetworkConfigurationHandler::ProfileEntryDeleter {
  public:
   ProfileEntryDeleter(NetworkConfigurationHandler* handler,
                       const std::string& service_path,
@@ -93,7 +92,8 @@ class NetworkConfigurationHandler::ProfileEntryDeleter
         guid_(guid),
         source_(source),
         callback_(callback),
-        error_callback_(error_callback) {}
+        error_callback_(error_callback),
+        weak_ptr_factory_(this) {}
 
   void Run() {
     DBusThreadManager::Get()
@@ -101,7 +101,7 @@ class NetworkConfigurationHandler::ProfileEntryDeleter
         ->GetLoadableProfileEntries(
             dbus::ObjectPath(service_path_),
             base::Bind(&ProfileEntryDeleter::GetProfileEntriesToDeleteCallback,
-                       AsWeakPtr()));
+                       weak_ptr_factory_.GetWeakPtr()));
   }
 
  private:
@@ -138,9 +138,9 @@ class NetworkConfigurationHandler::ProfileEntryDeleter
       DBusThreadManager::Get()->GetShillProfileClient()->DeleteEntry(
           dbus::ObjectPath(profile_path), entry_path,
           base::Bind(&ProfileEntryDeleter::ProfileEntryDeletedCallback,
-                     AsWeakPtr(), profile_path, entry_path),
-          base::Bind(&ProfileEntryDeleter::ShillErrorCallback, AsWeakPtr(),
-                     profile_path, entry_path));
+                     weak_ptr_factory_.GetWeakPtr(), profile_path, entry_path),
+          base::Bind(&ProfileEntryDeleter::ShillErrorCallback,
+                     weak_ptr_factory_.GetWeakPtr(), profile_path, entry_path));
     }
   }
 
@@ -183,6 +183,8 @@ class NetworkConfigurationHandler::ProfileEntryDeleter
   // Map of pending profile entry deletions, indexed by profile path.
   std::map<std::string, std::string> profile_delete_entries_;
 
+  base::WeakPtrFactory<ProfileEntryDeleter> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ProfileEntryDeleter);
 };
 
@@ -206,7 +208,8 @@ void NetworkConfigurationHandler::GetShillProperties(
   DBusThreadManager::Get()->GetShillServiceClient()->GetProperties(
       dbus::ObjectPath(service_path),
       base::Bind(&NetworkConfigurationHandler::GetPropertiesCallback,
-                 AsWeakPtr(), callback, error_callback, service_path));
+                 weak_ptr_factory_.GetWeakPtr(), callback, error_callback,
+                 service_path));
 }
 
 void NetworkConfigurationHandler::SetShillProperties(
@@ -243,10 +246,10 @@ void NetworkConfigurationHandler::SetShillProperties(
   DBusThreadManager::Get()->GetShillServiceClient()->SetProperties(
       dbus::ObjectPath(service_path), *properties_to_set,
       base::Bind(&NetworkConfigurationHandler::SetPropertiesSuccessCallback,
-                 AsWeakPtr(), service_path, base::Passed(&properties_copy),
-                 source, callback),
+                 weak_ptr_factory_.GetWeakPtr(), service_path,
+                 base::Passed(&properties_copy), source, callback),
       base::Bind(&NetworkConfigurationHandler::SetPropertiesErrorCallback,
-                 AsWeakPtr(), service_path, error_callback));
+                 weak_ptr_factory_.GetWeakPtr(), service_path, error_callback));
 
   // If we set the StaticIPConfig property, request an IP config refresh
   // after calling SetProperties.
@@ -272,9 +275,9 @@ void NetworkConfigurationHandler::ClearShillProperties(
   DBusThreadManager::Get()->GetShillServiceClient()->ClearProperties(
       dbus::ObjectPath(service_path), names,
       base::Bind(&NetworkConfigurationHandler::ClearPropertiesSuccessCallback,
-                 AsWeakPtr(), service_path, names, callback),
+                 weak_ptr_factory_.GetWeakPtr(), service_path, names, callback),
       base::Bind(&NetworkConfigurationHandler::ClearPropertiesErrorCallback,
-                 AsWeakPtr(), service_path, error_callback));
+                 weak_ptr_factory_.GetWeakPtr(), service_path, error_callback));
 }
 
 void NetworkConfigurationHandler::CreateShillConfiguration(
@@ -322,8 +325,8 @@ void NetworkConfigurationHandler::CreateShillConfiguration(
       properties_to_set->DeepCopy());
   manager->ConfigureServiceForProfile(
       dbus::ObjectPath(profile_path), *properties_to_set,
-      base::Bind(&NetworkConfigurationHandler::RunCreateNetworkCallback,
-                 AsWeakPtr(), profile_path, source,
+      base::Bind(&NetworkConfigurationHandler::ConfigurationCompleted,
+                 weak_ptr_factory_.GetWeakPtr(), profile_path, source,
                  base::Passed(&properties_copy), callback),
       base::Bind(&network_handler::ShillErrorCallbackFunction,
                  "Config.CreateConfiguration Failed", "", error_callback));
@@ -367,16 +370,35 @@ void NetworkConfigurationHandler::SetNetworkProfile(
       dbus::ObjectPath(service_path), shill::kProfileProperty,
       profile_path_value,
       base::Bind(&NetworkConfigurationHandler::SetNetworkProfileCompleted,
-                 AsWeakPtr(), service_path, profile_path, source, callback),
+                 weak_ptr_factory_.GetWeakPtr(), service_path, profile_path,
+                 source, callback),
       base::Bind(&SetNetworkProfileErrorCallback, service_path, profile_path,
                  error_callback));
 }
 
+// NetworkStateHandlerObserver methods
+void NetworkConfigurationHandler::NetworkListChanged() {
+  for (auto iter = configure_callbacks_.begin();
+       iter != configure_callbacks_.end();) {
+    const std::string& service_path = iter->first;
+    const NetworkState* state =
+        network_state_handler_->GetNetworkStateFromServicePath(service_path,
+                                                               true);
+    if (!state) {
+      NET_LOG(ERROR) << "Configured network not in list: " << service_path;
+      ++iter;
+      continue;
+    }
+    network_handler::ServiceResultCallback& callback = iter->second;
+    callback.Run(service_path, state->guid());
+    iter = configure_callbacks_.erase(iter);
+  }
+}
+
 // NetworkConfigurationHandler Private methods
 
 NetworkConfigurationHandler::NetworkConfigurationHandler()
-    : network_state_handler_(NULL) {
-}
+    : network_state_handler_(nullptr), weak_ptr_factory_(this) {}
 
 NetworkConfigurationHandler::~NetworkConfigurationHandler() {
 }
@@ -386,30 +408,34 @@ void NetworkConfigurationHandler::Init(
     NetworkDeviceHandler* network_device_handler) {
   network_state_handler_ = network_state_handler;
   network_device_handler_ = network_device_handler;
+
+  network_state_handler_->AddObserver(this, FROM_HERE);

[tbarzic, 2017/02/10 19:20:04]

the observer is never removed - and NetworkStateHandler observer list has
|check_empty| set.

[stevenjb, 2017/02/10 21:04:17]

Doh. Good catch, thanks. Done.

 }
 
-void NetworkConfigurationHandler::RunCreateNetworkCallback(
+void NetworkConfigurationHandler::ConfigurationCompleted(
     const std::string& profile_path,
     NetworkConfigurationObserver::Source source,
     std::unique_ptr<base::DictionaryValue> configure_properties,
     const network_handler::ServiceResultCallback& callback,
     const dbus::ObjectPath& service_path) {
-  if (!callback.is_null()) {
-    std::string guid;
-    configure_properties->GetStringWithoutPathExpansion(
-        ::onc::network_config::kGUID, &guid);
-    DCHECK(!guid.empty());
-    callback.Run(service_path.value(), guid);
-  }
+  // Request an update to the NetworkStateHandler cache.
+  network_state_handler_->RequestUpdateForNetwork(service_path.value());

[tbarzic, 2017/02/10 19:20:04]

this is to ensure NetworkListChanged is emitted (and only after network state
properties were updated), right? I'd add a comment about that.

[stevenjb, 2017/02/10 21:04:17]

Kind of. Clarified the comment.

+
+  // Notify observers immediately. (Note: Currently this is primarily used
+  // by tests).
   for (auto& observer : observers_) {
     observer.OnConfigurationCreated(service_path.value(), profile_path,
                                     *configure_properties, source);
   }
-  // This may also get called when CreateConfiguration is used to update an
-  // existing configuration, so request a service update just in case.
-  // TODO(pneubeck): Separate 'Create' and 'Update' calls and only trigger
-  // this on an update.
-  network_state_handler_->RequestUpdateForNetwork(service_path.value());
+
+  if (callback.is_null())
+    return;
+
+  // |configure_callbacks_| will get triggered when NetworkStateHandler
+  // notifies this that a state list update has occured. |service_path|
+  // should be unique so can not already exist in |configure_callbacks_}.

[tbarzic, 2017/02/10 19:20:04]

Service path is unique per GUID, right?

Could we end up in situation where there are two configuration requests for a
network at the same time?
e.g. if CreateShillConfiguration is called for the second time before the first
call's callback is run? 

+nit:
s/can not/it cannot/

[stevenjb, 2017/02/10 21:04:17]

While it may be theoretically possible for two updates to the same configuration
to occur, it's an extremely unlikely edge case, and we probably do not want the
first callback to occur in that case, but should not DCHECK.

Done/clarified and removed the DCHECK.

+  DCHECK(configure_callbacks_.count(service_path.value()) == 0);
+  configure_callbacks_[service_path.value()] = callback;
 }
 
 void NetworkConfigurationHandler::ProfileEntryDeleterCompleted(