Fix a race condition in preference metric reporting.

base/prefs/json_pref_store.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 "base/prefs/json_pref_store.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 159 matching lines @@
     *result = tmp;
   return true;
 }
 
 void JsonPrefStore::AddObserver(PrefStore::Observer* observer) {
   observers_.AddObserver(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 {
   return observers_.might_have_observers();
 }
 
 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);
 }
 
 void JsonPrefStore::SetValue(const std::string& key, base::Value* value) {
   DCHECK(value);
   scoped_ptr<base::Value> new_value(value);
   base::Value* old_value = NULL;
   prefs_->Get(key, &old_value);
@@ skipping 114 matching lines @@
   if (error_delegate_.get() && error != PREF_READ_ERROR_NONE)
     error_delegate_->OnError(error);
 
   FOR_EACH_OBSERVER(PrefStore::Observer,
                     observers_,
                     OnInitializationCompleted(true));
 }
 
 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) {
   // TODO(tc): Do we want to prune webkit preferences that match the default
   // value?
   JSONStringValueSerializer serializer(output);
   serializer.set_pretty_print(true);
   scoped_ptr<base::DictionaryValue> copy(
       prefs_->DeepCopyWithoutEmptyChildren());
 
@@ skipping 15 matching lines @@
         copy->Set(key, new base::ListValue);
     } else if (value->IsType(base::Value::TYPE_DICTIONARY)) {
       const base::DictionaryValue* dict = NULL;
       if (value->GetAsDictionary(&dict) && dict->empty())
         copy->Set(key, new base::DictionaryValue);
     }
   }
 
   return serializer.Serialize(*(copy.get()));
 }