Create a list of policy changes before notifying observers

chrome/browser/policy/policy_service_impl.cc

Index: chrome/browser/policy/policy_service_impl.cc
diff --git a/chrome/browser/policy/policy_service_impl.cc b/chrome/browser/policy/policy_service_impl.cc
index 2d0a2435e1f6354ba9841cb73b3ed4b58167813b..4b9c335ea7c80a417faed0b46ac9a2f32185497b 100644
--- a/chrome/browser/policy/policy_service_impl.cc
+++ b/chrome/browser/policy/policy_service_impl.cc
@@ -6,6 +6,8 @@
 
 #include <algorithm>
 
+#include "base/bind.h"
+#include "base/message_loop.h"
 #include "base/stl_util.h"
 #include "chrome/browser/policy/policy_map.h"
 
@@ -13,7 +15,8 @@ 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) {
@@ -91,15 +94,53 @@ 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;