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
+  // filters).
+  if (initialized_) {
+    if (pref_filter_.get())
+      pref_filter_->FilterUpdate(this, key);
+    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,

[gab, 2013/12/04 22:22:30]

What about in this case? Should we not do/report something if the profile-dir
was deleted altogether?

[erikwright (departed), 2013/12/05 23:48:00]

If the store is missing but the directory is there, a new store is created. But
this case is when the entire profile has been deleted. I don't think it's the
job of this class to react to that situation. In fact, I'd assume that someone
should fail in that case before we even get here.

                       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()));
 }