Create a list of policy changes before notifying observers

chrome/browser/policy/policy_service_impl.cc

 // Copyright (c) 2012 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/policy/policy_service_impl.h"
 
 #include <algorithm>
 
+#include "base/bind.h"
+#include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "chrome/browser/policy/policy_map.h"
 
 namespace policy {
 
 typedef PolicyServiceImpl::Providers::const_iterator Iterator;
 
-PolicyServiceImpl::PolicyServiceImpl(const Providers& providers) {
+PolicyServiceImpl::PolicyServiceImpl(const Providers& providers)
+    : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
   initialization_complete_ = true;
   providers_ = providers;
   for (Iterator it = providers.begin(); it != providers.end(); ++it) {
     ConfigurationPolicyProvider* provider = *it;
     provider->AddObserver(this);
     initialization_complete_ &= provider->IsInitializationComplete();
   }
   // There are no observers yet, but calls to GetPolicies() should already get
   // the processed policy values.
   MergeAndTriggerUpdates();
@@ skipping 57 matching lines @@
 void PolicyServiceImpl::OnUpdatePolicy(ConfigurationPolicyProvider* provider) {
   DCHECK_EQ(1, std::count(providers_.begin(), providers_.end(), provider));
   refresh_pending_.erase(provider);
   MergeAndTriggerUpdates();
 }
 
 void PolicyServiceImpl::NotifyNamespaceUpdated(
     const PolicyBundle::PolicyNamespace& ns,
     const PolicyMap& previous,
     const PolicyMap& current) {
-  ObserverMap::iterator iterator = observers_.find(ns.first);
+  // If running a unit test that hasn't setup a MessageLoop, don't send any
+  // notifications.
+  if (!MessageLoop::current())
+    return;
+
+  // Don't queue up a task if we have no observers - that way Observers added
+  // later don't get notified of changes that happened during construction time.
+  if (observers_.find(ns.first) == observers_.end())
+    return;
+
+  // Notify Observers via a queued task, so Observers can't trigger a re-entrant
+  // call to MergeAndTriggerUpdates() by modifying policy.
+  scoped_ptr<PolicyChangeInfo> changes(

[Joao da Silva, 2013/01/07 09:23:52]

#include "base/memory/scoped_ptr.h"

[Andrew T Wilson (Slow), 2013/01/07 14:11:32]

Done.

+      new PolicyChangeInfo(ns, previous, current));
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&PolicyServiceImpl::NotifyNamespaceUpdatedTask,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Passed(&changes)));
+}
+
+void PolicyServiceImpl::NotifyNamespaceUpdatedTask(
+    scoped_ptr<PolicyChangeInfo> changes) {
+  ObserverMap::iterator iterator = observers_.find(
+      changes->policy_namespace_.first);
   if (iterator != observers_.end()) {
     FOR_EACH_OBSERVER(
         PolicyService::Observer,
         *iterator->second,
-        OnPolicyUpdated(ns.first, ns.second, previous, current));
+        OnPolicyUpdated(changes->policy_namespace_.first,
+                        changes->policy_namespace_.second,
+                        changes->previous_,
+                        changes->current_));
   }
 }
 
+PolicyServiceImpl::PolicyChangeInfo::PolicyChangeInfo(

[Joao da Silva, 2013/01/07 09:23:52]

Place the definitions for PolicyChangeInfo before the definitions for
PolicyServiceImpl.

[Andrew T Wilson (Slow), 2013/01/07 14:11:32]

Done.

+    const PolicyBundle::PolicyNamespace& policy_namespace,
+    const PolicyMap& previous, const PolicyMap& current)
+    : policy_namespace_(policy_namespace) {
+  previous_.CopyFrom(previous);
+  current_.CopyFrom(current);
+}
+
+PolicyServiceImpl::PolicyChangeInfo::~PolicyChangeInfo() {
+}
+
 void PolicyServiceImpl::MergeAndTriggerUpdates() {
   // Merge from each provider in their order of priority.
   PolicyBundle bundle;
   for (Iterator it = providers_.begin(); it != providers_.end(); ++it)
     bundle.MergeFrom((*it)->policies());
 
   // Swap first, so that observers that call GetPolicies() see the current
   // values.
   policy_bundle_.Swap(&bundle);
 
@@ skipping 60 matching lines @@
   if (refresh_pending_.empty() && !refresh_callbacks_.empty()) {
     std::vector<base::Closure> callbacks;
     callbacks.swap(refresh_callbacks_);
     std::vector<base::Closure>::iterator it;
     for (it = callbacks.begin(); it != callbacks.end(); ++it)
       it->Run();
   }
 }
 
 }  // namespace policy