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..7589927be72a2cf1ada50294fd219c9c2c4c8cc8 100644
--- a/chrome/browser/policy/policy_service_impl.cc
+++ b/chrome/browser/policy/policy_service_impl.cc
@@ -6,6 +6,7 @@
 
 #include <algorithm>
 
+#include "base/memory/scoped_vector.h"
 #include "base/stl_util.h"
 #include "chrome/browser/policy/policy_map.h"
 
@@ -100,6 +101,26 @@ void PolicyServiceImpl::NotifyNamespaceUpdated(
   }
 }
 
+namespace {
+
+// Information we batch up for pending policy changes before sending updates to
+// observers.
+class PolicyChangeInfo {
+ public:
+  PolicyChangeInfo(const PolicyBundle::PolicyNamespace& policy_namespace,
+                   const PolicyMap& previous, const PolicyMap& current)
+      : policy_namespace_(policy_namespace) {
+    previous_.CopyFrom(previous);
+    current_.CopyFrom(current);
+  }
+
+  PolicyBundle::PolicyNamespace policy_namespace_;
+  PolicyMap previous_;
+  PolicyMap current_;
+};
+
+}  // namespace
+
 void PolicyServiceImpl::MergeAndTriggerUpdates() {
   // Merge from each provider in their order of priority.
   PolicyBundle bundle;
@@ -110,8 +131,11 @@ void PolicyServiceImpl::MergeAndTriggerUpdates() {
   // values.
   policy_bundle_.Swap(&bundle);
 
-  // Only notify observers of namespaces that have been modified.
+  // Walk the changes and create a list of updates - otherwise, activities
+  // taken by observers that modify the underlying policy store (like signing
+  // out and deleting policy) can cause crashes.
   const PolicyMap kEmpty;
+  ScopedVector<PolicyChangeInfo> changes;
   PolicyBundle::const_iterator it_new = policy_bundle_.begin();
   PolicyBundle::const_iterator end_new = policy_bundle_.end();
   PolicyBundle::const_iterator it_old = bundle.begin();
@@ -119,16 +143,20 @@ void PolicyServiceImpl::MergeAndTriggerUpdates() {
   while (it_new != end_new && it_old != end_old) {
     if (it_new->first < it_old->first) {
       // A new namespace is available.
-      NotifyNamespaceUpdated(it_new->first, kEmpty, *it_new->second);
+      changes.push_back(
+          new PolicyChangeInfo(it_new->first, kEmpty, *it_new->second));
       ++it_new;
     } else if (it_new->first > it_old->first) {
       // A previously available namespace is now gone.
-      NotifyNamespaceUpdated(it_old->first, *it_old->second, kEmpty);
+      changes.push_back(
+          new PolicyChangeInfo(it_old->first, *it_old->second, kEmpty));
       ++it_old;
     } else {
       if (!it_new->second->Equals(*it_old->second)) {
         // An existing namespace's policies have changed.
-        NotifyNamespaceUpdated(it_new->first, *it_old->second, *it_new->second);
+        changes.push_back(
+            new PolicyChangeInfo(it_new->first, *it_old->second,
+                                 *it_new->second));
       }
       ++it_new;
       ++it_old;
@@ -136,12 +164,23 @@ void PolicyServiceImpl::MergeAndTriggerUpdates() {
   }
 
   // Send updates for the remaining new namespaces, if any.
-  for (; it_new != end_new; ++it_new)
-    NotifyNamespaceUpdated(it_new->first, kEmpty, *it_new->second);
+  for (; it_new != end_new; ++it_new) {
+    changes.push_back(
+        new PolicyChangeInfo(it_new->first, kEmpty, *it_new->second));
+  }
 
   // Sends updates for the remaining removed namespaces, if any.
-  for (; it_old != end_old; ++it_old)
-    NotifyNamespaceUpdated(it_old->first, *it_old->second, kEmpty);
+  for (; it_old != end_old; ++it_old) {
+    changes.push_back(
+        new PolicyChangeInfo(it_old->first, *it_new->second, kEmpty));

[Joao da Silva, 2012/12/22 21:42:00]

The 2nd arg should be *it_old->second

[Andrew T Wilson (Slow), 2013/01/04 15:54:48]

Good catch :)

+  }
+
+  // Walk our pending list of changes and send updates.
+  for (ScopedVector<PolicyChangeInfo>::const_iterator iter = changes.begin();
+       iter != changes.end(); ++iter) {
+    NotifyNamespaceUpdated((*iter)->policy_namespace_, (*iter)->previous_,
+                           (*iter)->current_);
+  }
 
   CheckInitializationComplete();
   CheckRefreshComplete();