Fix a race condition in preference metric reporting.

base/prefs/json_pref_store.cc

Index: base/prefs/json_pref_store.cc
diff --git a/base/prefs/json_pref_store.cc b/base/prefs/json_pref_store.cc
index e230407043aea5ebc29056618ece54571e274b9c..9337df6ac2b53d40816f1b46f147158b4963933c 100644
--- a/base/prefs/json_pref_store.cc
+++ b/base/prefs/json_pref_store.cc
@@ -177,6 +177,10 @@ void JsonPrefStore::AddObserver(PrefStore::Observer* observer) {
 
 void JsonPrefStore::RemoveObserver(PrefStore::Observer* observer) {
   observers_.RemoveObserver(observer);
+  ScopedVector<PrefStore::Observer>::iterator ownership =
+      std::find(owned_observers_.begin(), owned_observers_.end(), observer);

[gab, 2013/11/27 23:43:27]

When would you want to remove an owned observer? Isn't it the whole point that
no one has a pointer to that observer anyways..? Maybe I'll find out as I keep
reading this CL...

Ah I see.. it's because you need to call it in ~JsonPrefStore... see my comment
there.

+  if (ownership != owned_observers_.end())
+    owned_observers_.erase(ownership);
 }
 
 bool JsonPrefStore::HasObservers() const {
@@ -187,6 +191,11 @@ bool JsonPrefStore::IsInitializationComplete() const {
   return initialized_;
 }
 
+void JsonPrefStore::AddOwnedObserver(scoped_ptr<PrefStore::Observer> observer) {
+  observers_.AddObserver(observer.get());
+  owned_observers_.push_back(observer.release());
+}
+
 bool JsonPrefStore::GetMutableValue(const std::string& key,
                                     base::Value** result) {
   return prefs_->Get(key, result);
@@ -321,6 +330,12 @@ void JsonPrefStore::OnFileRead(base::Value* value_owned,
 
 JsonPrefStore::~JsonPrefStore() {
   CommitPendingWrite();
+  for (ScopedVector<PrefStore::Observer>::iterator it =
+           owned_observers_.begin();
+       it != owned_observers_.end();
+       ++it) {
+    RemoveObserver(*it);

[gab, 2013/11/27 23:43:27]

This is operating on owned_observers_ while iterating over it, in particular
this will make |it| invalid and ++it is undefined after this operation I
believe.

How about keeping a simple RemoveObserver method that only affects |observers_|
and then either explicitly calling owned_observers_.clear() after this loop or
letting the ScopedVector do its work as the object is destroyed.

In fact, do we even need to remove the observers at all? They shouldn't be used
after this object is destroyed. The implicit end of this destructor taking care
of this object's members will actually clear out the ScopedVector et al.

[robertshield, 2013/11/28 02:54:26]

doesn't this call owned_observers_.erase() invalidating the iterator?  It could
call observers_.RemoveObserver(*it); directly, then call owned_observers.clear()
after the loop instead.

[erikwright (departed), 2013/11/28 17:48:07]

You're both right, it's a mistake that this affects owned_observers_ while
iterating over it.

I don't care about affecting owned_observers_ here but I do want to affect it if
a client calls RemoveObserver during the lifetime of the object.

+  }
 }
 
 bool JsonPrefStore::SerializeData(std::string* output) {