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/memory/scoped_vector.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) {
   initialization_complete_ = true;
   providers_ = providers;
@@ skipping 74 matching lines @@
     const PolicyMap& current) {
   ObserverMap::iterator iterator = observers_.find(ns.first);
   if (iterator != observers_.end()) {
     FOR_EACH_OBSERVER(
         PolicyService::Observer,
         *iterator->second,
         OnPolicyUpdated(ns.first, ns.second, previous, current));
   }
 }
 
+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;
   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);
 
-  // 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();
   PolicyBundle::const_iterator end_old = bundle.end();
   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;
     }
   }
 
   // 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();
 }
 
 void PolicyServiceImpl::CheckInitializationComplete() {
   // Check if all providers became initialized just now, if they weren't before.
   if (!initialization_complete_) {
     initialization_complete_ = true;
     for (Iterator it = providers_.begin(); it != providers_.end(); ++it) {
@@ skipping 18 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