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..877f2be88a25cb6dce4ab00b56ee7d67ea62d964 100644
--- a/base/prefs/json_pref_store.cc
+++ b/base/prefs/json_pref_store.cc
@@ -13,6 +13,7 @@
 #include "base/json/json_string_value_serializer.h"
 #include "base/memory/ref_counted.h"
 #include "base/message_loop/message_loop_proxy.h"
+#include "base/prefs/pref_filter.h"
 #include "base/sequenced_task_runner.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/values.h"
@@ -151,12 +152,14 @@ scoped_refptr<base::SequencedTaskRunner> JsonPrefStore::GetTaskRunnerForFile(
 }
 
 JsonPrefStore::JsonPrefStore(const base::FilePath& filename,
-                             base::SequencedTaskRunner* sequenced_task_runner)
+                             base::SequencedTaskRunner* sequenced_task_runner,
+                             scoped_ptr<PrefFilter> pref_filter)
     : path_(filename),
       sequenced_task_runner_(sequenced_task_runner),
       prefs_(new base::DictionaryValue()),
       read_only_(false),
       writer_(filename, sequenced_task_runner),
+      pref_filter_(pref_filter.Pass()),
       initialized_(false),
       read_error_(PREF_READ_ERROR_OTHER) {}
 
@@ -268,7 +271,14 @@ void JsonPrefStore::CommitPendingWrite() {
 }
 
 void JsonPrefStore::ReportValueChanged(const std::string& key) {
-  FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
+  // We don't notify observers about changes during initialization (i.e., due to

[gab, 2013/12/06 17:23:38]

Remove "We"

[erikwright (departed), 2013/12/09 17:59:40]

Done.

+  // filters).
+  if (initialized_) {
+    if (pref_filter_.get())
+      pref_filter_->FilterUpdate(this, key);

[gab, 2013/12/06 17:23:38]

To continue my point about naming; to me here FilterUpdate feels like a method
which would return a boolean and basically tell you whether to continue with the
update or not.

[erikwright (departed), 2013/12/09 17:59:40]

I thought about that.

(1) It's not necessary now.
(2) It's clear what the right behaviour would be. If someone uses a PrefValue to
change a preference, and we refuse it, do we still need to fire an Update event
to let them know that the value is not what they expect it is? Etc.

Anyway, as previously mentioned, I'm indifferent to the name but rather than
change it twice I'll take Bernhard's input.

+    FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
+  }
+
   if (!read_only_)
     writer_.ScheduleWrite(this);
 }
@@ -286,8 +296,6 @@ void JsonPrefStore::OnFileRead(base::Value* value_owned,
     return;
   }
 
-  initialized_ = true;
-
   switch (error) {
     case PREF_READ_ERROR_ACCESS_DENIED:
     case PREF_READ_ERROR_FILE_OTHER:
@@ -299,6 +307,8 @@ void JsonPrefStore::OnFileRead(base::Value* value_owned,
     case PREF_READ_ERROR_NONE:
       DCHECK(value.get());
       prefs_.reset(static_cast<base::DictionaryValue*>(value.release()));
+      if (pref_filter_.get())
+        pref_filter_->FilterOnLoad(this);
       break;
     case PREF_READ_ERROR_NO_FILE:
       // If the file just doesn't exist, maybe this is first run.  In any case
@@ -311,6 +321,8 @@ void JsonPrefStore::OnFileRead(base::Value* value_owned,
       NOTREACHED() << "Unknown error: " << error;
   }
 
+  initialized_ = true;
+
   if (error_delegate_.get() && error != PREF_READ_ERROR_NONE)
     error_delegate_->OnError(error);