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"
 #include "base/file_util.h"
 #include "base/json/json_file_value_serializer.h"
 #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"
 
 namespace {
 
 // Some extensions we'll tack on to copies of the Preferences files.
 const base::FilePath::CharType* kBadExtension = FILE_PATH_LITERAL("bad");
 
 // Differentiates file loading between origin thread and passed
@@ skipping 118 matching lines @@
     const base::FilePath& filename,
     base::SequencedWorkerPool* worker_pool) {
   std::string token("json_pref_store-");
   token.append(filename.AsUTF8Unsafe());
   return worker_pool->GetSequencedTaskRunnerWithShutdownBehavior(
       worker_pool->GetNamedSequenceToken(token),
       base::SequencedWorkerPool::BLOCK_SHUTDOWN);
 }
 
 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) {}
 
 bool JsonPrefStore::GetValue(const std::string& key,
                              const base::Value** result) const {
   base::Value* tmp = NULL;
   if (!prefs_->Get(key, &tmp))
     return false;
 
   if (result)
@@ skipping 91 matching lines @@
       new FileThreadDeserializer(this, sequenced_task_runner_.get()));
   deserializer->Start(path_);
 }
 
 void JsonPrefStore::CommitPendingWrite() {
   if (writer_.HasPendingWrite() && !read_only_)
     writer_.DoScheduledWrite();
 }
 
 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);
 }
 
 void JsonPrefStore::OnFileRead(base::Value* value_owned,
                                PersistentPrefStore::PrefReadError error,
                                bool no_dir) {
   scoped_ptr<base::Value> value(value_owned);
   read_error_ = error;
 
   if (no_dir) {
     FOR_EACH_OBSERVER(PrefStore::Observer,
                       observers_,
                       OnInitializationCompleted(false));
     return;
   }
 
-  initialized_ = true;
-
   switch (error) {
     case PREF_READ_ERROR_ACCESS_DENIED:
     case PREF_READ_ERROR_FILE_OTHER:
     case PREF_READ_ERROR_FILE_LOCKED:
     case PREF_READ_ERROR_JSON_TYPE:
     case PREF_READ_ERROR_FILE_NOT_SPECIFIED:
       read_only_ = true;
       break;
     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
       // there's no harm in writing out default prefs in this case.
       break;
     case PREF_READ_ERROR_JSON_PARSE:
     case PREF_READ_ERROR_JSON_REPEAT:
       break;
     default:
       NOTREACHED() << "Unknown error: " << error;
   }
 
+  initialized_ = true;
+
   if (error_delegate_.get() && error != PREF_READ_ERROR_NONE)
     error_delegate_->OnError(error);
 
   FOR_EACH_OBSERVER(PrefStore::Observer,
                     observers_,
                     OnInitializationCompleted(true));
 }
 
 JsonPrefStore::~JsonPrefStore() {
   CommitPendingWrite();
@@ skipping 25 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()));
 }