ManagedNetworkConfigurationHandler: Expose status about policy application.

Before, it was hard or impossible to track whether a policy application is currently running.
Now a notification (PoliciesApplied) and the current status (AnyPolicyApplicationRunning) are exposed.

BUG=425049

Review URL: https://codereview.chromium.org/661803005

Cr-Commit-Position: refs/heads/master@{#301817}

11 files changed, 372 insertions, 122 deletions

diff --git a/chromeos/network/client_cert_resolver.cc b/chromeos/network/client_cert_resolver.cc
index 97a0cb0..cd84217 100644
--- a/chromeos/network/client_cert_resolver.cc
+++ b/chromeos/network/client_cert_resolver.cc
@@ -345,8 +345,9 @@ void ClientCertResolver::OnCertificatesLoaded(
   ResolveNetworks(networks);
 }
 
-void ClientCertResolver::PolicyApplied(const std::string& service_path) {
-  VLOG(2) << "PolicyApplied " << service_path;
+void ClientCertResolver::PolicyAppliedToNetwork(
+    const std::string& service_path) {
+  VLOG(2) << "PolicyAppliedToNetwork " << service_path;
   if (!ClientCertificatesLoaded())
     return;
   // Compare this network with all certificates.
diff --git a/chromeos/network/client_cert_resolver.h b/chromeos/network/client_cert_resolver.h
index fc22d9b..9731693 100644
--- a/chromeos/network/client_cert_resolver.h
+++ b/chromeos/network/client_cert_resolver.h
@@ -66,7 +66,7 @@ class CHROMEOS_EXPORT ClientCertResolver : public NetworkStateHandlerObserver,
                                     bool initial_load) override;
 
   // NetworkPolicyObserver overrides
-  virtual void PolicyApplied(const std::string& service_path) override;
+  virtual void PolicyAppliedToNetwork(const std::string& service_path) override;
 
   // Check which networks of |networks| are configured with a client certificate
   // pattern. Search for certificates, on the worker thread, and configure the
diff --git a/chromeos/network/managed_network_configuration_handler.h b/chromeos/network/managed_network_configuration_handler.h
index 7ed8c17..4fb0eff 100644
--- a/chromeos/network/managed_network_configuration_handler.h
+++ b/chromeos/network/managed_network_configuration_handler.h
@@ -1,4 +1,4 @@
- // Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Copyright (c) 2013 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.
 
@@ -113,6 +113,10 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandler {
       const base::ListValue& network_configs_onc,
       const base::DictionaryValue& global_network_config) = 0;
 
+  // Returns true if any policy application is currently running or pending.
+  // NetworkPolicyObservers are notified about applications finishing.
+  virtual bool IsAnyPolicyApplicationRunning() const = 0;
+
   // Returns the user policy for user |userhash| or device policy, which has
   // |guid|. If |userhash| is empty, only looks for a device policy. If such
   // doesn't exist, returns NULL. Sets |onc_source| accordingly.
diff --git a/chromeos/network/managed_network_configuration_handler_impl.cc b/chromeos/network/managed_network_configuration_handler_impl.cc
index 44f3da7..b47f2dd 100644
--- a/chromeos/network/managed_network_configuration_handler_impl.cc
+++ b/chromeos/network/managed_network_configuration_handler_impl.cc
@@ -11,8 +11,8 @@
 #include "base/guid.h"
 #include "base/location.h"
 #include "base/logging.h"
-#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "base/values.h"
 #include "chromeos/dbus/shill_manager_client.h"
@@ -403,24 +403,53 @@ void ManagedNetworkConfigurationHandlerImpl::SetPolicy(
   }
 
   STLDeleteValues(&old_per_network_config);
+  ApplyOrQueuePolicies(userhash, &modified_policies);
+  FOR_EACH_OBSERVER(NetworkPolicyObserver, observers_, PolicyChanged(userhash));
+}
+
+bool ManagedNetworkConfigurationHandlerImpl::IsAnyPolicyApplicationRunning()
+    const {
+  return !policy_applicators_.empty() || !queued_modified_policies_.empty();
+}
+
+bool ManagedNetworkConfigurationHandlerImpl::ApplyOrQueuePolicies(
+    const std::string& userhash,
+    std::set<std::string>* modified_policies) {
+  DCHECK(modified_policies);
 
   const NetworkProfile* profile =
       network_profile_handler_->GetProfileForUserhash(userhash);
-  if (profile) {
-    scoped_refptr<PolicyApplicator> applicator =
-        new PolicyApplicator(weak_ptr_factory_.GetWeakPtr(),
-                             *profile,
-                             policies->per_network_config,
-                             policies->global_network_config,
-                             &modified_policies);
-    applicator->Run();
-  } else {
+  if (!profile) {
     VLOG(1) << "The relevant Shill profile isn't initialized yet, postponing "
             << "policy application.";
-    // See OnProfileAdded.
+    // OnProfileAdded will apply all policies for this userhash.
+    return false;
   }
 
-  FOR_EACH_OBSERVER(NetworkPolicyObserver, observers_, PolicyChanged(userhash));
+  if (ContainsKey(policy_applicators_, userhash)) {
+    // A previous policy application is still running. Queue the modified
+    // policies.
+    // Note, even if |modified_policies| is empty, this means that a policy
+    // application will be queued.
+    queued_modified_policies_[userhash].insert(modified_policies->begin(),
+                                               modified_policies->end());
+    VLOG(1) << "Previous PolicyApplicator still running. Postponing policy "
+               "application.";
+    return false;
+  }
+
+  const Policies* policies = policies_by_user_[userhash].get();
+  DCHECK(policies);
+
+  PolicyApplicator* applicator =
+      new PolicyApplicator(*profile,
+                           policies->per_network_config,
+                           policies->global_network_config,
+                           this,
+                           modified_policies);
+  policy_applicators_[userhash] = make_linked_ptr(applicator);
+  applicator->Run();
+  return true;
 }
 
 void ManagedNetworkConfigurationHandlerImpl::OnProfileAdded(
@@ -442,13 +471,9 @@ void ManagedNetworkConfigurationHandlerImpl::OnProfileAdded(
     policy_guids.insert(it->first);
   }
 
-  scoped_refptr<PolicyApplicator> applicator =
-      new PolicyApplicator(weak_ptr_factory_.GetWeakPtr(),
-                           profile,
-                           policies->per_network_config,
-                           policies->global_network_config,
-                           &policy_guids);
-  applicator->Run();
+  const bool started_policy_application =
+      ApplyOrQueuePolicies(profile.userhash, &policy_guids);
+  DCHECK(started_policy_application);
 }
 
 void ManagedNetworkConfigurationHandlerImpl::OnProfileRemoved(
@@ -503,7 +528,25 @@ void ManagedNetworkConfigurationHandlerImpl::
       base::Bind(&LogErrorWithDict, FROM_HERE));
 }
 
-void ManagedNetworkConfigurationHandlerImpl::OnPoliciesApplied() {
+void ManagedNetworkConfigurationHandlerImpl::OnPoliciesApplied(
+    const NetworkProfile& profile) {
+  const std::string& userhash = profile.userhash;
+  VLOG(1) << "Policy application for user '" << userhash << "' finished.";
+
+  base::MessageLoop::current()->DeleteSoon(
+      FROM_HERE, policy_applicators_[userhash].release());
+  policy_applicators_.erase(userhash);
+
+  if (ContainsKey(queued_modified_policies_, userhash)) {
+    std::set<std::string> modified_policies;
+    queued_modified_policies_[userhash].swap(modified_policies);
+    // Remove |userhash| from the queue.
+    queued_modified_policies_.erase(userhash);
+    ApplyOrQueuePolicies(userhash, &modified_policies);
+  } else {
+    FOR_EACH_OBSERVER(
+        NetworkPolicyObserver, observers_, PoliciesApplied(userhash));
+  }
 }
 
 const base::DictionaryValue*
@@ -587,7 +630,9 @@ ManagedNetworkConfigurationHandlerImpl::ManagedNetworkConfigurationHandlerImpl()
       network_profile_handler_(NULL),
       network_configuration_handler_(NULL),
       network_device_handler_(NULL),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  CHECK(base::MessageLoop::current());
+}
 
 ManagedNetworkConfigurationHandlerImpl::
     ~ManagedNetworkConfigurationHandlerImpl() {
@@ -611,7 +656,7 @@ void ManagedNetworkConfigurationHandlerImpl::OnPolicyAppliedToNetwork(
   if (service_path.empty())
     return;
   FOR_EACH_OBSERVER(
-      NetworkPolicyObserver, observers_, PolicyApplied(service_path));
+      NetworkPolicyObserver, observers_, PolicyAppliedToNetwork(service_path));
 }
 
 // Get{Managed}Properties helpers
diff --git a/chromeos/network/managed_network_configuration_handler_impl.h b/chromeos/network/managed_network_configuration_handler_impl.h
index aaa8635..8c56fb8 100644
--- a/chromeos/network/managed_network_configuration_handler_impl.h
+++ b/chromeos/network/managed_network_configuration_handler_impl.h
@@ -6,6 +6,7 @@
 #define CHROMEOS_NETWORK_MANAGED_NETWORK_CONFIGURATION_HANDLER_IMPL_H_
 
 #include <map>
+#include <set>
 #include <string>
 
 #include "base/basictypes.h"
@@ -34,71 +35,72 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandlerImpl
       public NetworkProfileObserver,
       public PolicyApplicator::ConfigurationHandler {
  public:
-  virtual ~ManagedNetworkConfigurationHandlerImpl();
+  ~ManagedNetworkConfigurationHandlerImpl() override;
 
   // ManagedNetworkConfigurationHandler overrides
-  virtual void AddObserver(NetworkPolicyObserver* observer) override;
-  virtual void RemoveObserver(NetworkPolicyObserver* observer) override;
+  void AddObserver(NetworkPolicyObserver* observer) override;
+  void RemoveObserver(NetworkPolicyObserver* observer) override;
 
-  virtual void GetProperties(
+  void GetProperties(
       const std::string& service_path,
       const network_handler::DictionaryResultCallback& callback,
       const network_handler::ErrorCallback& error_callback) override;
 
-  virtual void GetManagedProperties(
+  void GetManagedProperties(
       const std::string& userhash,
       const std::string& service_path,
       const network_handler::DictionaryResultCallback& callback,
       const network_handler::ErrorCallback& error_callback) override;
 
-  virtual void SetProperties(
+  void SetProperties(
       const std::string& service_path,
       const base::DictionaryValue& user_settings,
       const base::Closure& callback,
       const network_handler::ErrorCallback& error_callback) const override;
 
-  virtual void CreateConfiguration(
+  void CreateConfiguration(
       const std::string& userhash,
       const base::DictionaryValue& properties,
       const network_handler::StringResultCallback& callback,
       const network_handler::ErrorCallback& error_callback) const override;
 
-  virtual void RemoveConfiguration(
+  void RemoveConfiguration(
       const std::string& service_path,
       const base::Closure& callback,
       const network_handler::ErrorCallback& error_callback) const override;
 
-  virtual void SetPolicy(
-      onc::ONCSource onc_source,
-      const std::string& userhash,
-      const base::ListValue& network_configs_onc,
-      const base::DictionaryValue& global_network_config) override;
+  void SetPolicy(onc::ONCSource onc_source,
+                 const std::string& userhash,
+                 const base::ListValue& network_configs_onc,
+                 const base::DictionaryValue& global_network_config) override;
+
+  bool IsAnyPolicyApplicationRunning() const override;
 
-  virtual const base::DictionaryValue* FindPolicyByGUID(
+  const base::DictionaryValue* FindPolicyByGUID(
       const std::string userhash,
       const std::string& guid,
       onc::ONCSource* onc_source) const override;
 
-  virtual const base::DictionaryValue* GetGlobalConfigFromPolicy(
+  const base::DictionaryValue* GetGlobalConfigFromPolicy(
       const std::string userhash) const override;
 
-  virtual const base::DictionaryValue* FindPolicyByGuidAndProfile(
+  const base::DictionaryValue* FindPolicyByGuidAndProfile(
       const std::string& guid,
       const std::string& profile_path) const override;
 
   // NetworkProfileObserver overrides
-  virtual void OnProfileAdded(const NetworkProfile& profile) override;
-  virtual void OnProfileRemoved(const NetworkProfile& profile) override;
+  void OnProfileAdded(const NetworkProfile& profile) override;
+  void OnProfileRemoved(const NetworkProfile& profile) override;
 
   // PolicyApplicator::ConfigurationHandler overrides
-  virtual void CreateConfigurationFromPolicy(
+  void CreateConfigurationFromPolicy(
       const base::DictionaryValue& shill_properties) override;
 
-  virtual void UpdateExistingConfigurationWithPropertiesFromPolicy(
+  void UpdateExistingConfigurationWithPropertiesFromPolicy(
       const base::DictionaryValue& existing_properties,
       const base::DictionaryValue& new_properties) override;
 
-  virtual void OnPoliciesApplied() override;
+  void OnPoliciesApplied(const NetworkProfile& profile) override;
 
  private:
   friend class ClientCertResolverTest;
@@ -107,10 +109,14 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandlerImpl
   friend class NetworkHandler;
 
   struct Policies;
-  typedef std::map<std::string, linked_ptr<Policies> > UserToPoliciesMap;
   typedef base::Callback<void(const std::string& service_path,
                               scoped_ptr<base::DictionaryValue> properties)>
       GetDevicePropertiesCallback;
+  typedef std::map<std::string, linked_ptr<Policies> > UserToPoliciesMap;
+  typedef std::map<std::string, linked_ptr<PolicyApplicator>>
+      UserToPolicyApplicatorMap;
+  typedef std::map<std::string, std::set<std::string>>
+      UserToModifiedPoliciesMap;
 
   ManagedNetworkConfigurationHandlerImpl();
 
@@ -169,6 +175,13 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandlerImpl
       const std::string& error_name,
       scoped_ptr<base::DictionaryValue> error_data);
 
+  // Applies policies for |userhash|. |modified_policies| must be not null and
+  // contain the GUIDs of the network configurations that changed since the last
+  // policy application. Returns true if policy application was started and
+  // false if it was queued or delayed.
+  bool ApplyOrQueuePolicies(const std::string& userhash,
+                            std::set<std::string>* modified_policies);
+
   // If present, the empty string maps to the device policy.
   UserToPoliciesMap policies_by_user_;
 
@@ -178,6 +191,16 @@ class CHROMEOS_EXPORT ManagedNetworkConfigurationHandlerImpl
   NetworkConfigurationHandler* network_configuration_handler_;
   NetworkDeviceHandler* network_device_handler_;
 
+  // Owns the currently running PolicyApplicators.
+  UserToPolicyApplicatorMap policy_applicators_;
+
+  // Per userhash (or empty string for device policy), contains the GUIDs of the
+  // policies that were modified.
+  // If this map contains a userhash as key, it means that a policy application
+  // for this userhash is pending even if no policies were modified and the
+  // associated set of GUIDs is empty.
+  UserToModifiedPoliciesMap queued_modified_policies_;
+
   ObserverList<NetworkPolicyObserver> observers_;
 
   // For Shill client callbacks
diff --git a/chromeos/network/managed_network_configuration_handler_unittest.cc b/chromeos/network/managed_network_configuration_handler_unittest.cc
index b72fb86..94f9098 100644
--- a/chromeos/network/managed_network_configuration_handler_unittest.cc
+++ b/chromeos/network/managed_network_configuration_handler_unittest.cc
@@ -6,6 +6,7 @@
 #include <sstream>
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/location.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
@@ -18,6 +19,7 @@
 #include "chromeos/dbus/shill_client_helper.h"
 #include "chromeos/network/managed_network_configuration_handler_impl.h"
 #include "chromeos/network/network_configuration_handler.h"
+#include "chromeos/network/network_policy_observer.h"
 #include "chromeos/network/network_profile_handler.h"
 #include "chromeos/network/onc/onc_test_utils.h"
 #include "chromeos/network/onc/onc_utils.h"
@@ -48,6 +50,12 @@ std::string ValueToString(const base::Value* value) {
   return str.str();
 }
 
+void DereferenceAndCall(
+    base::Callback<void(const base::DictionaryValue& result)> callback,
+    const base::DictionaryValue* value) {
+  callback.Run(*value);
+}
+
 const char kUser1[] = "user1";
 const char kUser1ProfilePath[] = "/profile/user1/shill";
 
@@ -108,7 +116,10 @@ class ShillProfileTestClient {
     const std::string& userhash = profile_to_user_[profile_path.value()];
     result->SetStringWithoutPathExpansion(shill::kUserHashProperty, userhash);
 
-    callback.Run(*result);
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(base::Bind(&DereferenceAndCall, callback),
+                   base::Owned(result.release())));
   }
 
   void GetEntry(const dbus::ObjectPath& profile_path,
@@ -123,7 +134,10 @@ class ShillProfileTestClient {
     base::DictionaryValue* entry = NULL;
     entries->GetDictionaryWithoutPathExpansion(entry_path, &entry);
     ASSERT_TRUE(entry);
-    callback.Run(*entry);
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(base::Bind(&DereferenceAndCall, callback),
+                   base::Owned(entry->DeepCopy())));
   }
 
  protected:
@@ -167,6 +181,26 @@ class TestNetworkProfileHandler : public NetworkProfileHandler {
   DISALLOW_COPY_AND_ASSIGN(TestNetworkProfileHandler);
 };
 
+class TestNetworkPolicyObserver : public NetworkPolicyObserver {
+ public:
+  TestNetworkPolicyObserver() : policies_applied_count_(0) {}
+
+  void PoliciesApplied(const std::string& userhash) override {
+    policies_applied_count_++;
+  };
+
+  int GetPoliciesAppliedCountAndReset() {
+    int count = policies_applied_count_;
+    policies_applied_count_ = 0;
+    return count;
+  }
+
+ private:
+  int policies_applied_count_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestNetworkPolicyObserver);
+};
+
 }  // namespace
 
 class ManagedNetworkConfigurationHandlerTest : public testing::Test {
@@ -180,7 +214,7 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
   virtual ~ManagedNetworkConfigurationHandlerTest() {
   }
 
-  virtual void SetUp() override {
+  void SetUp() override {
     scoped_ptr<DBusThreadManagerSetter> dbus_setter =
         DBusThreadManager::GetSetterForTesting();
     mock_manager_client_ = new StrictMock<MockShillManagerClient>();
@@ -218,11 +252,14 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
         network_profile_handler_.get(),
         network_configuration_handler_.get(),
         NULL /* no DeviceHandler */);
+    managed_network_configuration_handler_->AddObserver(&policy_observer_);
 
     message_loop_.RunUntilIdle();
   }
 
-  virtual void TearDown() override {
+  void TearDown() override {
+    if (managed_network_configuration_handler_)
+      managed_network_configuration_handler_->RemoveObserver(&policy_observer_);
     managed_network_configuration_handler_.reset();
     network_configuration_handler_.reset();
     network_profile_handler_.reset();
@@ -321,6 +358,7 @@ class ManagedNetworkConfigurationHandlerTest : public testing::Test {
   MockShillServiceClient* mock_service_client_;
   ShillProfileTestClient profiles_stub_;
   ShillServiceTestClient services_stub_;
+  TestNetworkPolicyObserver policy_observer_;
   scoped_ptr<TestNetworkProfileHandler> network_profile_handler_;
   scoped_ptr<NetworkConfigurationHandler> network_configuration_handler_;
   scoped_ptr<ManagedNetworkConfigurationHandlerImpl>
@@ -428,6 +466,7 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyIgnoreUnmodified) {
 
   SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc");
   message_loop_.RunUntilIdle();
+  EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset());
   VerifyAndClearExpectations();
 
   SetUpEntry("policy/shill_policy_on_unmanaged_wifi1.json",
@@ -441,7 +480,82 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyIgnoreUnmodified) {
       GetEntry(dbus::ObjectPath(kUser1ProfilePath), "some_entry_path", _, _));
 
   SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc");
+
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset());
+}
+
+TEST_F(ManagedNetworkConfigurationHandlerTest, PolicyApplicationRunning) {
+  InitializeStandardProfiles();
+  EXPECT_CALL(*mock_profile_client_, GetProperties(_, _, _)).Times(AnyNumber());
+  EXPECT_CALL(*mock_manager_client_, ConfigureServiceForProfile(_, _, _, _))
+      .Times(AnyNumber());
+  EXPECT_CALL(*mock_profile_client_, GetEntry(_, _, _, _)).Times(AnyNumber());
+
+  EXPECT_FALSE(managed_handler()->IsAnyPolicyApplicationRunning());
+
+  SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc");
+  managed_handler()->SetPolicy(
+      ::onc::ONC_SOURCE_DEVICE_POLICY,
+      std::string(),             // no userhash
+      base::ListValue(),         // no device network policy
+      base::DictionaryValue());  // no device global config
+
+  EXPECT_TRUE(managed_handler()->IsAnyPolicyApplicationRunning());
+  message_loop_.RunUntilIdle();
+  EXPECT_FALSE(managed_handler()->IsAnyPolicyApplicationRunning());
+
+  SetUpEntry("policy/shill_policy_on_unmanaged_wifi1.json",
+             kUser1ProfilePath,
+             "some_entry_path");
+
+  SetPolicy(
+      ::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1_update.onc");
+  EXPECT_TRUE(managed_handler()->IsAnyPolicyApplicationRunning());
+  message_loop_.RunUntilIdle();
+  EXPECT_FALSE(managed_handler()->IsAnyPolicyApplicationRunning());
+}
+
+TEST_F(ManagedNetworkConfigurationHandlerTest, UpdatePolicyAfterFinished) {
+  InitializeStandardProfiles();
+  EXPECT_CALL(*mock_profile_client_, GetProperties(_, _, _));
+  EXPECT_CALL(*mock_manager_client_, ConfigureServiceForProfile(_, _, _, _));
+
+  SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc");
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset());
+  VerifyAndClearExpectations();
+
+  SetUpEntry("policy/shill_policy_on_unmanaged_wifi1.json",
+             kUser1ProfilePath,
+             "some_entry_path");
+
+  EXPECT_CALL(*mock_profile_client_, GetProperties(_, _, _));
+  EXPECT_CALL(
+      *mock_profile_client_,
+      GetEntry(dbus::ObjectPath(kUser1ProfilePath), "some_entry_path", _, _));
+  EXPECT_CALL(*mock_manager_client_, ConfigureServiceForProfile(_, _, _, _));
+
+  SetPolicy(
+      ::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1_update.onc");
+  message_loop_.RunUntilIdle();
+  EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset());
+}
+
+TEST_F(ManagedNetworkConfigurationHandlerTest, UpdatePolicyBeforeFinished) {
+  InitializeStandardProfiles();
+  EXPECT_CALL(*mock_profile_client_, GetProperties(_, _, _)).Times(2);
+  EXPECT_CALL(*mock_manager_client_, ConfigureServiceForProfile(_, _, _, _))
+      .Times(2);
+
+  SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc");
+  // Usually the first call will cause a profile entry to be created, which we
+  // don't fake here.
+  SetPolicy(
+      ::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1_update.onc");
+
   message_loop_.RunUntilIdle();
+  EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset());
 }
 
 TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyManageUnmanaged) {
@@ -690,6 +804,7 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, SetEmptyPolicyIgnoreUnmanaged) {
 
   SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "");
   message_loop_.RunUntilIdle();
+  EXPECT_EQ(1, policy_observer_.GetPoliciesAppliedCountAndReset());
 }
 
 TEST_F(ManagedNetworkConfigurationHandlerTest, SetPolicyIgnoreUnmanaged) {
@@ -809,7 +924,7 @@ TEST_F(ManagedNetworkConfigurationHandlerTest, LateProfileLoading) {
 class ManagedNetworkConfigurationHandlerShutdownTest
     : public ManagedNetworkConfigurationHandlerTest {
  public:
-  virtual void SetUp() override {
+  void SetUp() override {
     ManagedNetworkConfigurationHandlerTest::SetUp();
     ON_CALL(*mock_profile_client_, GetProperties(_, _, _)).WillByDefault(
         Invoke(&ManagedNetworkConfigurationHandlerShutdownTest::GetProperties));
@@ -840,6 +955,7 @@ TEST_F(ManagedNetworkConfigurationHandlerShutdownTest,
               GetProperties(dbus::ObjectPath(kUser1ProfilePath), _, _));
 
   SetPolicy(::onc::ONC_SOURCE_USER_POLICY, kUser1, "policy/policy_wifi1.onc");
+  managed_network_configuration_handler_->RemoveObserver(&policy_observer_);
   managed_network_configuration_handler_.reset();
   message_loop_.RunUntilIdle();
 }
diff --git a/chromeos/network/mock_managed_network_configuration_handler.h b/chromeos/network/mock_managed_network_configuration_handler.h
index bc8954a..11d5342 100644
--- a/chromeos/network/mock_managed_network_configuration_handler.h
+++ b/chromeos/network/mock_managed_network_configuration_handler.h
@@ -5,6 +5,8 @@
 #ifndef CHROMEOS_NETWORK_MOCK_MANAGED_NETWORK_CONFIGURATION_HANDLER_H_
 #define CHROMEOS_NETWORK_MOCK_MANAGED_NETWORK_CONFIGURATION_HANDLER_H_
 
+#include <string>
+
 #include "base/basictypes.h"
 #include "base/values.h"
 #include "chromeos/chromeos_export.h"
@@ -53,6 +55,7 @@ class CHROMEOS_EXPORT MockManagedNetworkConfigurationHandler
                     const std::string& userhash,
                     const base::ListValue& network_configs_onc,
                     const base::DictionaryValue& global_network_config));
+  MOCK_CONST_METHOD0(IsAnyPolicyApplicationRunning, bool());
   MOCK_CONST_METHOD3(FindPolicyByGUID,
                      const base::DictionaryValue*(
                          const std::string userhash,
diff --git a/chromeos/network/network_policy_observer.h b/chromeos/network/network_policy_observer.h
index defbb26..c7af61e 100644
--- a/chromeos/network/network_policy_observer.h
+++ b/chromeos/network/network_policy_observer.h
@@ -16,13 +16,17 @@ class NetworkPolicyObserver {
   // Called when the policy for |userhash| was set (also when it was updated).
   // Note that the policy might not have been applied yet at that time.
   // An empty |userhash| designates the device policy.
-  virtual void PolicyChanged(const std::string& userhash) {};
+  virtual void PolicyChanged(const std::string& userhash) {}
 
-  // Called every time a network is create or updated because of a policy.
-  virtual void PolicyApplied(const std::string& service_path) {};
+  // Called every time a policy application for |userhash| finished. This is
+  // only called once no more policies are pending for |userhash|.
+  virtual void PoliciesApplied(const std::string& userhash) {}
+
+  // Called every time a network is created or updated because of a policy.
+  virtual void PolicyAppliedToNetwork(const std::string& service_path) {}
 
  protected:
-  virtual ~NetworkPolicyObserver() {};
+  virtual ~NetworkPolicyObserver() {}
 
  private:
   DISALLOW_ASSIGN(NetworkPolicyObserver);
diff --git a/chromeos/network/policy_applicator.cc b/chromeos/network/policy_applicator.cc
index 2c1e720..334c106 100644
--- a/chromeos/network/policy_applicator.cc
+++ b/chromeos/network/policy_applicator.cc
@@ -46,12 +46,12 @@ const base::DictionaryValue* GetByGUID(
 }  // namespace
 
 PolicyApplicator::PolicyApplicator(
-    base::WeakPtr<ConfigurationHandler> handler,
     const NetworkProfile& profile,
     const GuidToPolicyMap& all_policies,
     const base::DictionaryValue& global_network_config,
+    ConfigurationHandler* handler,
     std::set<std::string>* modified_policies)
-    : handler_(handler), profile_(profile) {
+    : handler_(handler), profile_(profile), weak_ptr_factory_(this) {
   global_network_config_.MergeDictionary(&global_network_config);
   remaining_policies_.swap(*modified_policies);
   for (GuidToPolicyMap::const_iterator it = all_policies.begin();
@@ -60,21 +60,30 @@ PolicyApplicator::PolicyApplicator(
   }
 }
 
+PolicyApplicator::~PolicyApplicator() {
+  STLDeleteValues(&all_policies_);
+  VLOG(1) << "Destroying PolicyApplicator for " << profile_.userhash;
+}
+
 void PolicyApplicator::Run() {
   DBusThreadManager::Get()->GetShillProfileClient()->GetProperties(
       dbus::ObjectPath(profile_.path),
-      base::Bind(&PolicyApplicator::GetProfilePropertiesCallback, this),
-      base::Bind(&LogErrorMessage, FROM_HERE));
+      base::Bind(&PolicyApplicator::GetProfilePropertiesCallback,
+                 weak_ptr_factory_.GetWeakPtr()),
+      base::Bind(&PolicyApplicator::GetProfilePropertiesError,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
-void PolicyApplicator::GetProfilePropertiesCallback(
-    const base::DictionaryValue& profile_properties) {
-  if (!handler_) {
-    LOG(WARNING) << "Handler destructed during policy application to profile "
-                 << profile_.ToDebugString();
-    return;
+void PolicyApplicator::ProfileEntryFinished(const std::string& entry) {
+  pending_get_entry_calls_.erase(entry);
+  if (pending_get_entry_calls_.empty()) {
+    ApplyRemainingPolicies();
+    NotifyConfigurationHandlerAndFinish();
   }
+}
 
+void PolicyApplicator::GetProfilePropertiesCallback(
+    const base::DictionaryValue& profile_properties) {
   VLOG(2) << "Received properties for profile " << profile_.ToDebugString();
   const base::ListValue* entries = NULL;
   if (!profile_properties.GetListWithoutPathExpansion(
@@ -82,6 +91,7 @@ void PolicyApplicator::GetProfilePropertiesCallback(
     LOG(ERROR) << "Profile " << profile_.ToDebugString()
                << " doesn't contain the property "
                << shill::kEntriesProperty;
+    NotifyConfigurationHandlerAndFinish();
     return;
   }
 
@@ -90,23 +100,34 @@ void PolicyApplicator::GetProfilePropertiesCallback(
     std::string entry;
     (*it)->GetAsString(&entry);
 
+    pending_get_entry_calls_.insert(entry);
     DBusThreadManager::Get()->GetShillProfileClient()->GetEntry(
         dbus::ObjectPath(profile_.path),
         entry,
-        base::Bind(&PolicyApplicator::GetEntryCallback, this, entry),
-        base::Bind(&LogErrorMessage, FROM_HERE));
+        base::Bind(&PolicyApplicator::GetEntryCallback,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   entry),
+        base::Bind(&PolicyApplicator::GetEntryError,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   entry));
+  }
+  if (pending_get_entry_calls_.empty()) {
+    ApplyRemainingPolicies();
+    NotifyConfigurationHandlerAndFinish();
   }
 }
 
+void PolicyApplicator::GetProfilePropertiesError(
+    const std::string& error_name,
+    const std::string& error_message) {
+  LOG(ERROR) << "Could not retrieve properties of profile " << profile_.path
+             << ": " << error_message;
+  NotifyConfigurationHandlerAndFinish();
+}
+
 void PolicyApplicator::GetEntryCallback(
     const std::string& entry,
     const base::DictionaryValue& entry_properties) {
-  if (!handler_) {
-    LOG(WARNING) << "Handler destructed during policy application to profile "
-                 << profile_.ToDebugString();
-    return;
-  }
-
   VLOG(2) << "Received properties for entry " << entry << " of profile "
           << profile_.ToDebugString();
 
@@ -235,6 +256,16 @@ void PolicyApplicator::GetEntryCallback(
           entry_properties, shill_properties_to_update);
     }
   }
+
+  ProfileEntryFinished(entry);
+}
+
+void PolicyApplicator::GetEntryError(const std::string& entry,
+                                     const std::string& error_name,
+                                     const std::string& error_message) {
+  LOG(ERROR) << "Could not retrieve entry " << entry << " of profile "
+             << profile_.path << ": " << error_message;
+  ProfileEntryFinished(entry);
 }
 
 void PolicyApplicator::DeleteEntry(const std::string& entry) {
@@ -271,21 +302,8 @@ void PolicyApplicator::WriteNewShillConfiguration(
     handler_->CreateConfigurationFromPolicy(shill_dictionary);
 }
 
-PolicyApplicator::~PolicyApplicator() {
-  ApplyRemainingPolicies();
-  STLDeleteValues(&all_policies_);
-  // Notify the handler about all policies being applied, so that the network
-  // lists can be updated.
-  if (handler_)
-    handler_->OnPoliciesApplied();
-}
-
 void PolicyApplicator::ApplyRemainingPolicies() {
-  if (!handler_) {
-    LOG(WARNING) << "Handler destructed during policy application to profile "
-                 << profile_.ToDebugString();
-    return;
-  }
+  DCHECK(pending_get_entry_calls_.empty());
 
   // Write all queued configurations now.
   for (ScopedVector<base::DictionaryValue>::const_iterator it =
@@ -294,12 +312,12 @@ void PolicyApplicator::ApplyRemainingPolicies() {
        ++it) {
     handler_->CreateConfigurationFromPolicy(**it);
   }
+  new_shill_configurations_.clear();
 
-  if (remaining_policies_.empty())
-    return;
+  VLOG_IF(2, !remaining_policies_.empty())
+      << "Create new managed network configurations in profile"
+      << profile_.ToDebugString() << ".";
 
-  VLOG(2) << "Create new managed network configurations in profile"
-          << profile_.ToDebugString() << ".";
   // All profile entries were compared to policies. |remaining_policies_|
   // contains all modified policies that didn't match any entry. For these
   // remaining policies, new configurations have to be created.
@@ -320,6 +338,12 @@ void PolicyApplicator::ApplyRemainingPolicies() {
     WriteNewShillConfiguration(
         *shill_dictionary, *network_policy, false /* write now */);
   }
+  remaining_policies_.clear();
+}
+
+void PolicyApplicator::NotifyConfigurationHandlerAndFinish() {
+  weak_ptr_factory_.InvalidateWeakPtrs();
+  handler_->OnPoliciesApplied(profile_);
 }
 
 }  // namespace chromeos
diff --git a/chromeos/network/policy_applicator.h b/chromeos/network/policy_applicator.h
index b9401a4..bca3b06 100644
--- a/chromeos/network/policy_applicator.h
+++ b/chromeos/network/policy_applicator.h
@@ -9,7 +9,6 @@
 #include <set>
 #include <string>
 
-#include "base/memory/ref_counted.h"
 #include "base/memory/scoped_vector.h"
 #include "base/memory/weak_ptr.h"
 #include "base/values.h"
@@ -22,54 +21,65 @@ namespace chromeos {
 // entries in parallel (GetProfilePropertiesCallback), compares each entry with
 // the current policies (GetEntryCallback) and adds all missing policies
 // (~PolicyApplicator).
-class PolicyApplicator : public base::RefCounted<PolicyApplicator> {
+class PolicyApplicator {
  public:
   class ConfigurationHandler {
-    public:
-     virtual ~ConfigurationHandler() {}
-     // Write the new configuration with the properties |shill_properties| to
-     // Shill. This configuration comes from a policy. Any conflicting or
-     // existing configuration for the same network will have been removed
-     // before.
-     virtual void CreateConfigurationFromPolicy(
-         const base::DictionaryValue& shill_properties) = 0;
-
-     virtual void UpdateExistingConfigurationWithPropertiesFromPolicy(
-         const base::DictionaryValue& existing_properties,
-         const base::DictionaryValue& new_properties) = 0;
-
-     // Called after all policies were applied. At this point, the list of
-     // networks should be updated.
-     virtual void OnPoliciesApplied() = 0;
-
-    private:
-     DISALLOW_ASSIGN(ConfigurationHandler);
+   public:
+    virtual ~ConfigurationHandler() {}
+    // Write the new configuration with the properties |shill_properties| to
+    // Shill. This configuration comes from a policy. Any conflicting or
+    // existing configuration for the same network will have been removed
+    // before.
+    virtual void CreateConfigurationFromPolicy(
+        const base::DictionaryValue& shill_properties) = 0;
+
+    virtual void UpdateExistingConfigurationWithPropertiesFromPolicy(
+        const base::DictionaryValue& existing_properties,
+        const base::DictionaryValue& new_properties) = 0;
+
+    // Called after all policies for |profile| were applied. At this point, the
+    // list of networks should be updated.
+    virtual void OnPoliciesApplied(const NetworkProfile& profile) = 0;
+
+   private:
+    DISALLOW_ASSIGN(ConfigurationHandler);
   };
 
   typedef std::map<std::string, const base::DictionaryValue*> GuidToPolicyMap;
 
+  // |handler| must outlive this object.
   // |modified_policies| must not be NULL and will be empty afterwards.
-  PolicyApplicator(base::WeakPtr<ConfigurationHandler> handler,
-                   const NetworkProfile& profile,
+  PolicyApplicator(const NetworkProfile& profile,
                    const GuidToPolicyMap& all_policies,
                    const base::DictionaryValue& global_network_config,
+                   ConfigurationHandler* handler,
                    std::set<std::string>* modified_policies);
 
+  ~PolicyApplicator();
+
   void Run();
 
  private:
-  friend class base::RefCounted<PolicyApplicator>;
+  // Removes |entry| from the list of pending profile entries.
+  // If all entries were processed, applies the remaining policies and notifies
+  // |handler_|.
+  void ProfileEntryFinished(const std::string& entry);
 
   // Called with the properties of the profile |profile_|. Requests the
   // properties of each entry, which are processed by GetEntryCallback.
   void GetProfilePropertiesCallback(
       const base::DictionaryValue& profile_properties);
+  void GetProfilePropertiesError(const std::string& error_name,
+                                 const std::string& error_message);
 
   // Called with the properties of the profile entry |entry|. Checks whether the
   // entry was previously managed, whether a current policy applies and then
   // either updates, deletes or not touches the entry.
   void GetEntryCallback(const std::string& entry,
                         const base::DictionaryValue& entry_properties);
+  void GetEntryError(const std::string& entry,
+                     const std::string& error_name,
+                     const std::string& error_message);
 
   // Sends Shill the command to delete profile entry |entry| from |profile_|.
   void DeleteEntry(const std::string& entry);
@@ -80,20 +90,23 @@ class PolicyApplicator : public base::RefCounted<PolicyApplicator> {
                                   const base::DictionaryValue& policy,
                                   bool write_later);
 
-  // Called once all Profile entries are processed. Calls
-  // ApplyRemainingPolicies.
-  virtual ~PolicyApplicator();
-
   // Creates new entries for all remaining policies, i.e. for which no matching
   // Profile entry was found.
+  // This should only be called if all profile entries were processed.
   void ApplyRemainingPolicies();
 
+  // Called after all policies are applied or an error occurred. Notifies
+  // |handler_|.
+  void NotifyConfigurationHandlerAndFinish();
+
   std::set<std::string> remaining_policies_;
-  base::WeakPtr<ConfigurationHandler> handler_;
+  std::set<std::string> pending_get_entry_calls_;
+  ConfigurationHandler* handler_;
   NetworkProfile profile_;
   GuidToPolicyMap all_policies_;
   base::DictionaryValue global_network_config_;
   ScopedVector<base::DictionaryValue> new_shill_configurations_;
+  base::WeakPtrFactory<PolicyApplicator> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PolicyApplicator);
 };
diff --git a/chromeos/test/data/network/policy/policy_wifi1_update.onc b/chromeos/test/data/network/policy/policy_wifi1_update.onc
new file mode 100644
index 0000000..a1360fb
--- /dev/null
+++ b/chromeos/test/data/network/policy/policy_wifi1_update.onc
@@ -0,0 +1,17 @@
+{
+  "NetworkConfigurations": [
+    {
+      "GUID": "policy_wifi1",
+      "Type": "WiFi",
+      "Name": "Managed wifi1",
+      "WiFi": {
+        "Passphrase": "another passphrase",
+        "Recommended": [ "AutoConnect", "Passphrase" ],
+        "SSID": "wifi1",
+        "Security": "WPA-PSK"
+      }
+    }
+  ],
+  "Type": "UnencryptedConfiguration"
+}
+