Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(565)

Unified Diff: rlz/chromeos/lib/rlz_value_store_chromeos.cc

Issue 11308196: [cros] RlzValueStore made protected by a cross-process lock and not persisted over browser lifetime… (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: Comment Created 8 years, 1 month ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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 CalledOnValidThr
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 y
+ 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 happe
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

Powered by Google App Engine
This is Rietveld 408576698