[cros] RlzValueStore made protected by a cross-process lock and not persisted over browser lifetime (like on Mac).

*) Moved RecursiveCrossProcessLock out of .mm file to a common _posix file.
*) Added static method to ImportantFileWriter that does blocking write on the current thread.
*) Dedicated RLZ thread gone, replaced back with shutdown-blocking worker pool.


BUG=157348,62328


Review URL: https://chromiumcodereview.appspot.com/11308196

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@170179 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 172 insertions, 136 deletions

diff --git a/rlz/chromeos/lib/rlz_value_store_chromeos.cc b/rlz/chromeos/lib/rlz_value_store_chromeos.cc
index ddf73e7..bb9c431 100644
--- a/rlz/chromeos/lib/rlz_value_store_chromeos.cc
+++ b/rlz/chromeos/lib/rlz_value_store_chromeos.cc
@@ -4,16 +4,16 @@
 
 #include "rlz/chromeos/lib/rlz_value_store_chromeos.h"
 
-#include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/files/important_file_writer.h"
+#include "base/json/json_file_value_serializer.h"
+#include "base/json/json_string_value_serializer.h"
 #include "base/logging.h"
-#include "base/memory/singleton.h"
-#include "base/prefs/json_pref_store.h"
 #include "base/sequenced_task_runner.h"
 #include "base/string_number_conversions.h"
 #include "base/values.h"
 #include "rlz/lib/lib_values.h"
-#include "rlz/lib/recursive_lock.h"
+#include "rlz/lib/recursive_cross_process_lock_posix.h"
 #include "rlz/lib/rlz_lib.h"
 
 namespace rlz_lib {
@@ -36,6 +36,10 @@ const char kNoSupplementaryBrand[] = "_";
 // RLZ store filename.
 const FilePath::CharType kRLZDataFileName[] = FILE_PATH_LITERAL("RLZ Data");
 
+// RLZ store lock filename
+const FilePath::CharType kRLZLockFileName[] =
+    FILE_PATH_LITERAL("RLZ Data.lock");
+
 // RLZ store path for testing.
 FilePath g_testing_rlz_store_path_;
 
@@ -46,6 +50,13 @@ FilePath GetRlzStorePath() {
       g_testing_rlz_store_path_.Append(kRLZDataFileName);
 }
 
+// Returns file path of the RLZ storage lock file.
+FilePath GetRlzStoreLockPath() {
+  return g_testing_rlz_store_path_.empty() ?
+      file_util::GetHomeDir().Append(kRLZLockFileName) :
+      g_testing_rlz_store_path_.Append(kRLZLockFileName);
+}
+
 // Returns the dictionary key for storing access point-related prefs.
 std::string GetKeyName(std::string key, AccessPoint access_point) {
   std::string brand = SupplementaryBranding::GetBrand();
@@ -64,89 +75,58 @@ std::string GetKeyName(std::string key, Product product) {
 
 }  // namespace
 
-// static
-base::SequencedTaskRunner* RlzValueStoreChromeOS::io_task_runner_ = NULL;
-
-// static
-bool RlzValueStoreChromeOS::created_;
-
-// static
-RlzValueStoreChromeOS* RlzValueStoreChromeOS::GetInstance() {
-  return Singleton<RlzValueStoreChromeOS>::get();
-}
-
-// static
-void RlzValueStoreChromeOS::SetIOTaskRunner(
-    base::SequencedTaskRunner* io_task_runner) {
-  io_task_runner_ = io_task_runner;
-  // Make sure |io_task_runner_| lives until constructor is called.
-  io_task_runner_->AddRef();
-}
-
-// static
-void RlzValueStoreChromeOS::ResetForTesting() {
-  // Make sure we don't create an instance if it didn't exist.
-  if (created_)
-    GetInstance()->ReadPrefs();
-}
-
-// static
-void RlzValueStoreChromeOS::Cleanup() {
-  if (created_)
-    GetInstance()->rlz_store_ = NULL;
-}
-
-RlzValueStoreChromeOS::RlzValueStoreChromeOS() {
-  ReadPrefs();
-  created_ = true;
+RlzValueStoreChromeOS::RlzValueStoreChromeOS(const FilePath& store_path)
+    : rlz_store_(new base::DictionaryValue),
+      store_path_(store_path),
+      read_only_(true) {
+  ReadStore();
 }
 
 RlzValueStoreChromeOS::~RlzValueStoreChromeOS() {
+  WriteStore();
 }
 
 bool RlzValueStoreChromeOS::HasAccess(AccessType type) {
-  return type == kReadAccess || !rlz_store_->ReadOnly();
+  DCHECK(CalledOnValidThread());
+  return type == kReadAccess || !read_only_;
 }
 
 bool RlzValueStoreChromeOS::WritePingTime(Product product, int64 time) {
-  std::string value = base::Int64ToString(time);
-  rlz_store_->SetValue(GetKeyName(kPingTimeKey, product),
-                       base::Value::CreateStringValue(value));
+  DCHECK(CalledOnValidThread());
+  rlz_store_->SetString(GetKeyName(kPingTimeKey, product),
+                        base::Int64ToString(time));
   return true;
 }
 
 bool RlzValueStoreChromeOS::ReadPingTime(Product product, int64* time) {
-  const base::Value* value = NULL;
-  rlz_store_->GetValue(GetKeyName(kPingTimeKey, product), &value);
-  std::string s_value;
-  return value && value->GetAsString(&s_value) &&
-      base::StringToInt64(s_value, time);
+  DCHECK(CalledOnValidThread());
+  std::string ping_time;
+  return rlz_store_->GetString(GetKeyName(kPingTimeKey, product), &ping_time) &&
+      base::StringToInt64(ping_time, time);
 }
 
 bool RlzValueStoreChromeOS::ClearPingTime(Product product) {
-  rlz_store_->RemoveValue(GetKeyName(kPingTimeKey, product));
+  DCHECK(CalledOnValidThread());
+  rlz_store_->Remove(GetKeyName(kPingTimeKey, product), NULL);
   return true;
 }
 
 bool RlzValueStoreChromeOS::WriteAccessPointRlz(AccessPoint access_point,
                                                 const char* new_rlz) {
-  rlz_store_->SetValue(
-      GetKeyName(kAccessPointKey, access_point),
-      base::Value::CreateStringValue(new_rlz));
+  DCHECK(CalledOnValidThread());
+  rlz_store_->SetString(
+      GetKeyName(kAccessPointKey, access_point), new_rlz);
   return true;
 }
 
 bool RlzValueStoreChromeOS::ReadAccessPointRlz(AccessPoint access_point,
                                                char* rlz,
                                                size_t rlz_size) {
-  const base::Value* value = NULL;
-  rlz_store_->GetValue(
-      GetKeyName(kAccessPointKey, access_point), &value);
-  std::string s_value;
-  if (value)
-    value->GetAsString(&s_value);
-  if (s_value.size() < rlz_size) {
-    strncpy(rlz, s_value.c_str(), rlz_size);
+  DCHECK(CalledOnValidThread());
+  std::string rlz_value;
+  rlz_store_->GetString(GetKeyName(kAccessPointKey, access_point), &rlz_value);
+  if (rlz_value.size() < rlz_size) {
+    strncpy(rlz, rlz_value.c_str(), rlz_size);
     return true;
   }
   if (rlz_size > 0)
@@ -155,13 +135,14 @@ bool RlzValueStoreChromeOS::ReadAccessPointRlz(AccessPoint access_point,
 }
 
 bool RlzValueStoreChromeOS::ClearAccessPointRlz(AccessPoint access_point) {
-  rlz_store_->RemoveValue(
-      GetKeyName(kAccessPointKey, access_point));
+  DCHECK(CalledOnValidThread());
+  rlz_store_->Remove(GetKeyName(kAccessPointKey, access_point), NULL);
   return true;
 }
 
 bool RlzValueStoreChromeOS::AddProductEvent(Product product,
                                             const char* event_rlz) {
+  DCHECK(CalledOnValidThread());
   return AddValueToList(GetKeyName(kProductEventKey, product),
                         base::Value::CreateStringValue(event_rlz));
 }
@@ -169,8 +150,9 @@ bool RlzValueStoreChromeOS::AddProductEvent(Product product,
 bool RlzValueStoreChromeOS::ReadProductEvents(
     Product product,
     std::vector<std::string>* events) {
-  base::ListValue* events_list = GetList(GetKeyName(kProductEventKey, product));
-  if (!events_list)
+  DCHECK(CalledOnValidThread());
+  base::ListValue* events_list = NULL; ;
+  if (!rlz_store_->GetList(GetKeyName(kProductEventKey, product), &events_list))
     return false;
   events->clear();
   for (size_t i = 0; i < events_list->GetSize(); ++i) {
@@ -183,99 +165,162 @@ bool RlzValueStoreChromeOS::ReadProductEvents(
 
 bool RlzValueStoreChromeOS::ClearProductEvent(Product product,
                                               const char* event_rlz) {
+  DCHECK(CalledOnValidThread());
   base::StringValue event_value(event_rlz);
   return RemoveValueFromList(GetKeyName(kProductEventKey, product),
                              event_value);
 }
 
 bool RlzValueStoreChromeOS::ClearAllProductEvents(Product product) {
-  rlz_store_->RemoveValue(GetKeyName(kProductEventKey, product));
+  DCHECK(CalledOnValidThread());
+  rlz_store_->Remove(GetKeyName(kProductEventKey, product), NULL);
   return true;
 }
 
 bool RlzValueStoreChromeOS::AddStatefulEvent(Product product,
                                              const char* event_rlz) {
+  DCHECK(CalledOnValidThread());
   return AddValueToList(GetKeyName(kStatefulEventKey, product),
                         base::Value::CreateStringValue(event_rlz));
 }
 
 bool RlzValueStoreChromeOS::IsStatefulEvent(Product product,
                                             const char* event_rlz) {
-  base::ListValue* events_list =
-      GetList(GetKeyName(kStatefulEventKey, product));
+  DCHECK(CalledOnValidThread());
   base::StringValue event_value(event_rlz);
-  return events_list && events_list->Find(event_value) != events_list->end();
+  base::ListValue* events_list = NULL;
+  return rlz_store_->GetList(GetKeyName(kStatefulEventKey, product),
+                             &events_list) &&
+      events_list->Find(event_value) != events_list->end();
 }
 
 bool RlzValueStoreChromeOS::ClearAllStatefulEvents(Product product) {
-  rlz_store_->RemoveValue(GetKeyName(kStatefulEventKey, product));
+  DCHECK(CalledOnValidThread());
+  rlz_store_->Remove(GetKeyName(kStatefulEventKey, product), NULL);
   return true;
 }
 
 void RlzValueStoreChromeOS::CollectGarbage() {
+  DCHECK(CalledOnValidThread());
   NOTIMPLEMENTED();
 }
 
-void RlzValueStoreChromeOS::ReadPrefs() {
-  DCHECK(io_task_runner_)
-      << "Calling GetInstance or ResetForTesting before SetIOTaskRunner?";
-  rlz_store_ = new JsonPrefStore(GetRlzStorePath(), io_task_runner_);
-  rlz_store_->ReadPrefs();
-  switch (rlz_store_->GetReadError()) {
-    case PersistentPrefStore::PREF_READ_ERROR_NONE:
-    case PersistentPrefStore::PREF_READ_ERROR_NO_FILE:
+void RlzValueStoreChromeOS::ReadStore() {
+  int error_code = 0;
+  std::string error_msg;
+  JSONFileValueSerializer serializer(store_path_);
+  scoped_ptr<base::Value> value(
+      serializer.Deserialize(&error_code, &error_msg));
+  switch (error_code) {
+    case JSONFileValueSerializer::JSON_NO_SUCH_FILE:
+      read_only_ = false;
+      break;
+    case JSONFileValueSerializer::JSON_NO_ERROR:
+      read_only_ = false;
+      rlz_store_.reset(static_cast<base::DictionaryValue*>(value.release()));
       break;
     default:
-      LOG(ERROR) << "Error read RLZ store: " << rlz_store_->GetReadError();
+      LOG(ERROR) << "Error reading RLZ store: " << error_msg;
   }
-  // Restore refcount modified by SetIOTaskRunner().
-  io_task_runner_->Release();
-  io_task_runner_ = NULL;
 }
 
-base::ListValue* RlzValueStoreChromeOS::GetList(std::string list_name) {
-  base::Value* list_value = NULL;
-  rlz_store_->GetMutableValue(list_name, &list_value);
-  base::ListValue* list = NULL;
-  if (!list_value || !list_value->GetAsList(&list))
-    return NULL;
-  return list;
+void RlzValueStoreChromeOS::WriteStore() {
+  std::string json_data;
+  JSONStringValueSerializer serializer(&json_data);
+  serializer.set_pretty_print(true);
+  scoped_ptr<DictionaryValue> copy(rlz_store_->DeepCopyWithoutEmptyChildren());
+  if (!serializer.Serialize(*copy.get())) {
+    LOG(ERROR) << "Failed to serialize RLZ data";
+    NOTREACHED();
+    return;
+  }
+  if (!base::ImportantFileWriter::WriteFileAtomically(store_path_, json_data))
+    LOG(ERROR) << "Error writing RLZ store";
 }
 
 bool RlzValueStoreChromeOS::AddValueToList(std::string list_name,
                                            base::Value* value) {
-  base::ListValue* list = GetList(list_name);
-  if (!list) {
-    list = new base::ListValue;
-    rlz_store_->SetValue(list_name, list);
+  base::ListValue* list_value = NULL;
+  if (!rlz_store_->GetList(list_name, &list_value)) {
+    list_value = new base::ListValue;
+    rlz_store_->Set(list_name, list_value);
   }
-  if (list->AppendIfNotPresent(value))
-    rlz_store_->ReportValueChanged(list_name);
+  list_value->AppendIfNotPresent(value);
   return true;
 }
 
 bool RlzValueStoreChromeOS::RemoveValueFromList(std::string list_name,
                                                 const base::Value& value) {
-  base::ListValue* list = GetList(list_name);
-  if (!list)
+  base::ListValue* list_value = NULL;
+  if (!rlz_store_->GetList(list_name, &list_value))
     return false;
-  rlz_store_->MarkNeedsEmptyValue(list_name);
   size_t index;
-  if (list->Remove(value, &index))
-    rlz_store_->ReportValueChanged(list_name);
+  list_value->Remove(value, &index);
   return true;
 }
 
+namespace {
 
-ScopedRlzValueStoreLock::ScopedRlzValueStoreLock()
-    : store_(RlzValueStoreChromeOS::GetInstance()) {
+// RlzValueStoreChromeOS keeps its data in memory and only writes it to disk
+// when ScopedRlzValueStoreLock goes out of scope. Hence, if several
+// ScopedRlzValueStoreLocks are nested, they all need to use the same store
+// object.
+
+RecursiveCrossProcessLock g_recursive_lock =
+    RECURSIVE_CROSS_PROCESS_LOCK_INITIALIZER;
+
+// This counts the nesting depth of |ScopedRlzValueStoreLock|.
+int g_lock_depth = 0;
+
+// This is the shared store object. Non-|NULL| only when |g_lock_depth > 0|.
+RlzValueStoreChromeOS* g_store = NULL;
+
+}  // namespace
+
+ScopedRlzValueStoreLock::ScopedRlzValueStoreLock() {
+  bool got_cross_process_lock =
+      g_recursive_lock.TryGetCrossProcessLock(GetRlzStoreLockPath());
+  // At this point, we hold the in-process lock, no matter the value of
+  // |got_cross_process_lock|.
+
+  ++g_lock_depth;
+  if (!got_cross_process_lock) {
+    // Acquiring cross-process lock failed, so simply return here.
+    // In-process lock will be released in dtor.
+    DCHECK(!g_store);
+    return;
+  }
+
+  if (g_lock_depth > 1) {
+    // Reuse the already existing store object.
+    DCHECK(g_store);
+    store_.reset(g_store);
+    return;
+  }
+
+  // This is the topmost lock, create a new store object.
+  DCHECK(!g_store);
+  g_store = new RlzValueStoreChromeOS(GetRlzStorePath());
+  store_.reset(g_store);
 }
 
 ScopedRlzValueStoreLock::~ScopedRlzValueStoreLock() {
+  --g_lock_depth;
+  DCHECK(g_lock_depth >= 0);
+
+  if (g_lock_depth > 0) {
+    // Other locks are still using store_, so don't free it yet.
+    ignore_result(store_.release());
+    return;
+  }
+
+  g_store = NULL;
+
+  g_recursive_lock.ReleaseLock();
 }
 
 RlzValueStore* ScopedRlzValueStoreLock::GetStore() {
-  return store_;
+  return store_.get();
 }
 
 namespace testing {
@@ -284,6 +329,10 @@ void SetRlzStoreDirectory(const FilePath& directory) {
   g_testing_rlz_store_path_ = directory;
 }
 
+std::string RlzStoreFilenameStr() {
+  return GetRlzStorePath().value();
+}
+
 }  // namespace testing
 
 }  // namespace rlz_lib
diff --git a/rlz/chromeos/lib/rlz_value_store_chromeos.h b/rlz/chromeos/lib/rlz_value_store_chromeos.h
index c9456a8..4760418 100644
--- a/rlz/chromeos/lib/rlz_value_store_chromeos.h
+++ b/rlz/chromeos/lib/rlz_value_store_chromeos.h
@@ -5,8 +5,9 @@
 #ifndef RLZ_CHROMEOS_LIB_RLZ_VALUE_STORE_CHROMEOS_H_
 #define RLZ_CHROMEOS_LIB_RLZ_VALUE_STORE_CHROMEOS_H_
 
-#include "base/prefs/persistent_pref_store.h"
+#include "base/file_path.h"
 #include "base/threading/non_thread_safe.h"
+#include "base/values.h"
 #include "rlz/lib/rlz_value_store.h"
 
 namespace base {
@@ -15,28 +16,19 @@ class SequencedTaskRunner;
 class Value;
 }
 
-template <typename T> struct DefaultSingletonTraits;
-
 namespace rlz_lib {
 
-// An implementation of RlzValueStore for ChromeOS. Unlike Mac and Win
-// counterparts, it's non thread-safe and should only be accessed on a single
-// Thread instance that has a MessageLoop.
-class RlzValueStoreChromeOS : public RlzValueStore {
+// An implementation of RlzValueStore for ChromeOS.
+class RlzValueStoreChromeOS : public RlzValueStore,
+                              public base::NonThreadSafe {
  public:
-  static RlzValueStoreChromeOS* GetInstance();
-
-  // Sets the MessageLoopProxy that underlying PersistentPrefStore will post I/O
-  // tasks to. Must be called before the first GetInstance() call.
-  static void SetIOTaskRunner(base::SequencedTaskRunner* io_task_runner);
-
-  // Must be invoked during shutdown to commit pending I/O.
-  static void Cleanup();
+  // // Sets the task runner that will be used by ImportantFileWriter for write
+  // // operations.
+  // static void SetFileTaskRunner(base::SequencedTaskRunner* file_task_runner);
 
-  // Resets the store to its initial state. Should only be used for testing.
-  // Same restrictions as for calling GetInstance() for the first time apply,
-  // i.e. must call SetIOTaskRunner first.
-  static void ResetForTesting();
+  // Creates new instance and synchronously reads data from file.
+  RlzValueStoreChromeOS(const FilePath& store_path);
+  virtual ~RlzValueStoreChromeOS();
 
   // RlzValueStore overrides:
   virtual bool HasAccess(AccessType type) OVERRIDE;
@@ -68,28 +60,23 @@ class RlzValueStoreChromeOS : public RlzValueStore {
   virtual void CollectGarbage() OVERRIDE;
 
  private:
-  friend struct DefaultSingletonTraits<RlzValueStoreChromeOS>;
-
-  // Used by JsonPrefStore for write operations.
-  static base::SequencedTaskRunner* io_task_runner_;
+  // Reads RLZ store from file.
+  void ReadStore();
 
-  static bool created_;
+  // Writes RLZ store back to file.
+  void WriteStore();
 
-  RlzValueStoreChromeOS();
-  virtual ~RlzValueStoreChromeOS();
-
-  // Initializes RLZ store.
-  void ReadPrefs();
-
-  // Retrieves list at path |list_name| from JSON store.
-  base::ListValue* GetList(std::string list_name);
   // Adds |value| to list at |list_name| path in JSON store.
   bool AddValueToList(std::string list_name, base::Value* value);
   // Removes |value| from list at |list_name| path in JSON store.
   bool RemoveValueFromList(std::string list_name, const base::Value& value);
 
-  // Store with RLZ data.
-  scoped_refptr<PersistentPrefStore> rlz_store_;
+  // In-memory store with RLZ data.
+  scoped_ptr<base::DictionaryValue> rlz_store_;
+
+  FilePath store_path_;
+
+  bool read_only_;
 
   DISALLOW_COPY_AND_ASSIGN(RlzValueStoreChromeOS);
 };