update_client: Adjust how Prefs data is stored to allow for ids containing "."s.

Bug: 1092503
Change-Id: Ia4fffa66a320904b2ac6985d82416cbba5483a19
Fixed: 1092503
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2316668
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792058}

components/update_client/persisted_data.cc

@@ -10,7 +10,6 @@
 #include "base/guid.h"
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
-#include "base/threading/thread_checker.h"
 #include "base/values.h"
 #include "base/version.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -28,44 +27,43 @@ PersistedData::PersistedData(PrefService* pref_service,
       activity_data_service_(activity_data_service) {}
 
 PersistedData::~PersistedData() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }
 
-int PersistedData::GetInt(const std::string& id,
-                          const std::string& key,
-                          int fallback) const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  // We assume ids do not contain '.' characters.
-  DCHECK_EQ(std::string::npos, id.find('.'));
+const base::Value* PersistedData::GetAppKey(const std::string& id) const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!pref_service_)
-    return fallback;
+    return nullptr;
   const base::DictionaryValue* dict =
       pref_service_->GetDictionary(kPersistedDataPreference);
   if (!dict)
+    return nullptr;
+  const base::Value* apps = dict->FindDictKey("apps");
+  if (!apps)
+    return nullptr;
+  return apps->FindDictKey(id);
+}
+
+int PersistedData::GetInt(const std::string& id,
+                          const std::string& key,
+                          int fallback) const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  const base::Value* app_key = GetAppKey(id);
+  if (!app_key)
     return fallback;
-  int result = 0;
-  return dict->GetInteger(
-             base::StringPrintf("apps.%s.%s", id.c_str(), key.c_str()), &result)
-             ? result
-             : fallback;
+  return app_key->FindIntKey(key).value_or(fallback);
 }
 
 std::string PersistedData::GetString(const std::string& id,
                                      const std::string& key) const {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  // We assume ids do not contain '.' characters.
-  DCHECK_EQ(std::string::npos, id.find('.'));
-  if (!pref_service_)
-    return std::string();
-  const base::DictionaryValue* dict =
-      pref_service_->GetDictionary(kPersistedDataPreference);
-  if (!dict)
-    return std::string();
-  std::string result;
-  return dict->GetString(
-             base::StringPrintf("apps.%s.%s", id.c_str(), key.c_str()), &result)
-             ? result
-             : std::string();
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  const base::Value* app_key = GetAppKey(id);
+  if (!app_key)
+    return {};
+  const std::string* value = app_key->FindStringKey(key);
+  if (!value)
+    return {};
+  return *value;
 }
 
 int PersistedData::GetDateLastRollCall(const std::string& id) const {
@@ -93,33 +91,41 @@ std::string PersistedData::GetCohortHint(const std::string& id) const {
   return GetString(id, "cohorthint");
 }
 
+base::Value* PersistedData::GetOrCreateAppKey(const std::string& id,
+                                              base::Value* root) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  base::Value* apps = root->FindDictKey("apps");
+  if (!apps)
+    apps = root->SetKey("apps", base::Value(base::Value::Type::DICTIONARY));
+  base::Value* app = apps->FindDictKey(id);
+  if (!app)
+    app = apps->SetKey(id, base::Value(base::Value::Type::DICTIONARY));
+  return app;
+}
+
 void PersistedData::SetDateLastRollCall(const std::vector<std::string>& ids,
                                         int datenum) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!pref_service_ || datenum < 0)
     return;
   DictionaryPrefUpdate update(pref_service_, kPersistedDataPreference);
   for (const auto& id : ids) {
-    // We assume ids do not contain '.' characters.
-    DCHECK_EQ(std::string::npos, id.find('.'));
-    update->SetInteger(base::StringPrintf("apps.%s.dlrc", id.c_str()), datenum);
-    update->SetString(base::StringPrintf("apps.%s.pf", id.c_str()),
-                      base::GenerateGUID());
+    base::Value* app_key = GetOrCreateAppKey(id, update.Get());
+    app_key->SetIntKey("dlrc", datenum);
+    app_key->SetStringKey("pf", base::GenerateGUID());
   }
 }
 
 void PersistedData::SetDateLastActive(const std::vector<std::string>& ids,
                                       int datenum) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!pref_service_ || datenum < 0)
     return;
   DictionaryPrefUpdate update(pref_service_, kPersistedDataPreference);
   for (const auto& id : ids) {
     if (GetActiveBit(id)) {
-      // We assume ids do not contain '.' characters.
-      DCHECK_EQ(std::string::npos, id.find('.'));
-      update->SetInteger(base::StringPrintf("apps.%s.dla", id.c_str()),
-                         datenum);
+      base::Value* app_key = GetOrCreateAppKey(id, update.Get());
+      app_key->SetIntKey("dla", datenum);
       activity_data_service_->ClearActiveBit(id);
     }
   }
@@ -128,12 +134,11 @@ void PersistedData::SetDateLastActive(const std::vector<std::string>& ids,
 void PersistedData::SetString(const std::string& id,
                               const std::string& key,
                               const std::string& value) {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (!pref_service_)
     return;
   DictionaryPrefUpdate update(pref_service_, kPersistedDataPreference);
-  update->SetString(base::StringPrintf("apps.%s.%s", id.c_str(), key.c_str()),
-                    value);
+  GetOrCreateAppKey(id, update.Get())->SetStringKey(key, value);
 }
 
 void PersistedData::SetCohort(const std::string& id,
@@ -156,14 +161,14 @@ bool PersistedData::GetActiveBit(const std::string& id) const {
 }
 
 int PersistedData::GetDaysSinceLastRollCall(const std::string& id) const {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return activity_data_service_
              ? activity_data_service_->GetDaysSinceLastRollCall(id)
              : kDaysUnknown;
 }
 
 int PersistedData::GetDaysSinceLastActive(const std::string& id) const {
-  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   return activity_data_service_
              ? activity_data_service_->GetDaysSinceLastActive(id)
              : kDaysUnknown;

components/update_client/persisted_data.h

@@ -9,7 +9,7 @@
 #include <vector>
 
 #include "base/macros.h"
-#include "base/threading/thread_checker.h"
+#include "base/sequence_checker.h"
 #include "base/values.h"
 
 class PrefRegistrySimple;
@@ -113,13 +113,23 @@ class PersistedData {
   void SetFingerprint(const std::string& id, const std::string& fingerprint);
 
  private:
+  // Returns nullptr if the app key does not exist.
+  const base::Value* GetAppKey(const std::string& id) const;
+
+  // Returns an existing or newly created app key under a root pref.
+  base::Value* GetOrCreateAppKey(const std::string& id, base::Value* root);
+
+  // Returns fallback if the key does not exist.
   int GetInt(const std::string& id, const std::string& key, int fallback) const;
+
+  // Returns the empty string if the key does not exist.
   std::string GetString(const std::string& id, const std::string& key) const;
+
   void SetString(const std::string& id,
                  const std::string& key,
                  const std::string& value);
 
-  base::ThreadChecker thread_checker_;
+  SEQUENCE_CHECKER(sequence_checker_);
   PrefService* pref_service_;
   ActivityDataService* activity_data_service_;
 

components/update_client/persisted_data_unittest.cc

@@ -23,12 +23,16 @@ TEST(PersistedDataTest, Simple) {
   EXPECT_EQ(-2, metadata->GetDateLastActive("someappid"));
   EXPECT_EQ(-2, metadata->GetDaysSinceLastRollCall("someappid"));
   EXPECT_EQ(-2, metadata->GetDaysSinceLastActive("someappid"));
+  EXPECT_EQ(-2, metadata->GetDaysSinceLastActive("someappid.withdot"));
   std::vector<std::string> items;
   items.push_back("someappid");
+  items.push_back("someappid.withdot");
   metadata->SetDateLastRollCall(items, 3383);
   metadata->SetDateLastActive(items, 3383);
   EXPECT_EQ(3383, metadata->GetDateLastRollCall("someappid"));
+  EXPECT_EQ(3383, metadata->GetDateLastRollCall("someappid.withdot"));
   EXPECT_EQ(-2, metadata->GetDateLastActive("someappid"));
+  EXPECT_EQ(-2, metadata->GetDateLastActive("someappid.withdot"));
   EXPECT_EQ(-2, metadata->GetDaysSinceLastRollCall("someappid"));
   EXPECT_EQ(-2, metadata->GetDaysSinceLastActive("someappid"));
   EXPECT_EQ(-2, metadata->GetDateLastRollCall("someotherappid"));