[cros] RlzValueStore made protected by a cross-process lock and not persisted over browser lifetime…

rlz/chromeos/lib/rlz_value_store_chromeos.cc

Index: rlz/chromeos/lib/rlz_value_store_chromeos.cc
diff --git a/rlz/chromeos/lib/rlz_value_store_chromeos.cc b/rlz/chromeos/lib/rlz_value_store_chromeos.cc
index ddf73e7e1d10b51a01194f463bb367a2ce152185..c94d801e27a54584a2f0e48937ea6e6a4ded9c49 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,60 @@ 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) {
+  DCHECK(CalledOnValidThread());

[Roger Tawa OOO till Jul 10th, 2012/11/26 19:16:51]

why call CalledOnValidThread() in ctor?

[Ivan Korotkov, 2012/11/27 11:37:25]

Right, removed.

+  ReadStore();
 }
 
 RlzValueStoreChromeOS::~RlzValueStoreChromeOS() {
+  DCHECK(CalledOnValidThread());

[Roger Tawa OOO till Jul 10th, 2012/11/26 19:16:51]

the base class dtor already calls CalledOnValidThread().

[Ivan Korotkov, 2012/11/27 11:37:25]

Removed.

+  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));

[Roger Tawa OOO till Jul 10th, 2012/11/26 19:16:51]

one extra space.

[Ivan Korotkov, 2012/11/27 11:37:25]

Done.

   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 +137,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 +152,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 +167,163 @@ 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;

[Roger Tawa OOO till Jul 10th, 2012/11/26 19:16:51]

why set read-only to false here?

[Ivan Korotkov, 2012/11/27 11:37:25]

NO_SUCH_FILE means RLZ store hasn't been created yet and we're starting with an
empty one. But it should be writable, otherwise we never create one :)

+      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::WriteToDisk(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);
+  // rlz_store_->MarkNeedsEmptyValue(list_name);

[Roger Tawa OOO till Jul 10th, 2012/11/26 19:16:51]

remove line?

[Ivan Korotkov, 2012/11/27 11:37:25]

Done.

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

[Roger Tawa OOO till Jul 10th, 2012/11/26 19:16:51]

Maybe add DCHECK(g_lock_depth==1)?  Does not happen today, but if a recursive
ScopedRlzValueStoreLock were created when the first one failed to get the
process lock, seems like the mutex will not get release in the dtor.  A DCHECK
here could catch that.

[Ivan Korotkov, 2012/11/27 11:37:25]

See my comment in Lock

+    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 +332,10 @@ void SetRlzStoreDirectory(const FilePath& directory) {
   g_testing_rlz_store_path_ = directory;
 }
 
+std::string RlzStoreFilenameStr() {
+  return GetRlzStorePath().value();
+}
+
 }  // namespace testing
 
 }  // namespace rlz_lib