updater: Refactor prefs to avoid exposing pref names.

Bug: 1092503
Change-Id: Ic2ed8e9aa94380dc5fe0b1624f13866913d89ebe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2307413
Auto-Submit: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789998}

chrome/updater/app/app_server.cc

@@ -25,25 +25,23 @@ void AppServer::Initialize() {
 }
 
 base::OnceClosure AppServer::ModeCheck() {
-  std::unique_ptr<UpdaterPrefs> local_prefs = CreateLocalPrefs();
-  if (!local_prefs->GetPrefService()->GetBoolean(kPrefQualified))
+  std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();
+  if (!local_prefs->GetQualified())
     return base::BindOnce(&AppServer::Qualify, this, std::move(local_prefs));
 
-  std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
+  std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
   if (!global_prefs) {
     return base::BindOnce(&AppServer::Shutdown, this,
                           kErrorFailedToLockPrefsMutex);
   }
 
   base::Version this_version(UPDATER_VERSION_STRING);
-  base::Version active_version(
-      global_prefs->GetPrefService()->GetString(kPrefActiveVersion));
+  base::Version active_version(global_prefs->GetActiveVersion());
   if (this_version < active_version)
     return base::BindOnce(&AppServer::UninstallSelf, this);
 
-  if (this_version > active_version ||
-      global_prefs->GetPrefService()->GetBoolean(kPrefSwapping)) {
-    if (!SwapVersions(global_prefs->GetPrefService()))
+  if (this_version > active_version || global_prefs->GetSwapping()) {
+    if (!SwapVersions(global_prefs.get()))
       return base::BindOnce(&AppServer::Shutdown, this, kErrorFailedToSwap);
   }
 
@@ -60,22 +58,22 @@ void AppServer::FirstTaskRun() {
   std::move(first_task_).Run();
 }
 
-void AppServer::Qualify(std::unique_ptr<UpdaterPrefs> local_prefs) {
+void AppServer::Qualify(std::unique_ptr<LocalPrefs> local_prefs) {
   // For now, assume qualification succeeds.
-  local_prefs->GetPrefService()->SetBoolean(kPrefQualified, true);
+  local_prefs->SetQualified(true);
   PrefsCommitPendingWrites(local_prefs->GetPrefService());
   Shutdown(kErrorOk);
 }
 
-bool AppServer::SwapVersions(PrefService* global_prefs) {
-  global_prefs->SetBoolean(kPrefSwapping, true);
-  PrefsCommitPendingWrites(global_prefs);
+bool AppServer::SwapVersions(GlobalPrefs* global_prefs) {
+  global_prefs->SetSwapping(true);
+  PrefsCommitPendingWrites(global_prefs->GetPrefService());
   bool result = SwapRPCInterfaces();
   if (!result)
     return false;
-  global_prefs->SetString(kPrefActiveVersion, UPDATER_VERSION_STRING);
-  global_prefs->SetBoolean(kPrefSwapping, false);
-  PrefsCommitPendingWrites(global_prefs);
+  global_prefs->SetActiveVersion(UPDATER_VERSION_STRING);
+  global_prefs->SetSwapping(false);
+  PrefsCommitPendingWrites(global_prefs->GetPrefService());
   return true;
 }
 

chrome/updater/app/app_server.h

@@ -11,12 +11,11 @@
 #include "base/memory/scoped_refptr.h"
 #include "chrome/updater/app/app.h"
 
-class PrefService;
-
 namespace updater {
 
 class Configurator;
-class UpdaterPrefs;
+class GlobalPrefs;
+class LocalPrefs;
 
 // AppServer runs as the updater server process. Multiple servers of different
 // application versions can be run side-by-side. Each such server is called a
@@ -60,8 +59,8 @@ class AppServer : public App {
   // the system is consistent with an incomplete swap, ModeCheck may have the
   // side effect of promoting this candidate to the active candidate.
   base::OnceClosure ModeCheck();
-  void Qualify(std::unique_ptr<UpdaterPrefs> local_prefs);
-  bool SwapVersions(PrefService* global_prefs);
+  void Qualify(std::unique_ptr<LocalPrefs> local_prefs);
+  bool SwapVersions(GlobalPrefs* global_prefs);
 
   base::OnceClosure first_task_;
 };

chrome/updater/app/app_server_unittest.cc

@@ -83,18 +83,18 @@ TEST_F(AppServerTestCase, SimpleQualify) {
   EXPECT_CALL(*app, SwapRPCInterfaces).Times(0);
   EXPECT_CALL(*app, UninstallSelf).Times(0);
   EXPECT_EQ(app->Run(), 0);
-  EXPECT_TRUE(CreateLocalPrefs()->GetPrefService()->GetBoolean(kPrefQualified));
+  EXPECT_TRUE(CreateLocalPrefs()->GetQualified());
 }
 
 TEST_F(AppServerTestCase, SelfUninstall) {
   {
     base::SingleThreadTaskExecutor main_task_executor(
         base::MessagePumpType::UI);
-    std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
-    global_prefs->GetPrefService()->SetString(kPrefActiveVersion, "9999999");
+    std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+    global_prefs->SetActiveVersion("9999999");
     PrefsCommitPendingWrites(global_prefs->GetPrefService());
-    std::unique_ptr<UpdaterPrefs> local_prefs = CreateLocalPrefs();
-    local_prefs->GetPrefService()->SetBoolean(kPrefQualified, true);
+    std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();
+    local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
   auto app = base::MakeRefCounted<AppServerTest>();
@@ -104,15 +104,15 @@ TEST_F(AppServerTestCase, SelfUninstall) {
   EXPECT_CALL(*app, SwapRPCInterfaces).Times(0);
   EXPECT_CALL(*app, UninstallSelf).Times(1);
   EXPECT_EQ(app->Run(), 0);
-  EXPECT_TRUE(CreateLocalPrefs()->GetPrefService()->GetBoolean(kPrefQualified));
+  EXPECT_TRUE(CreateLocalPrefs()->GetQualified());
 }
 
 TEST_F(AppServerTestCase, SelfPromote) {
   {
     base::SingleThreadTaskExecutor main_task_executor(
         base::MessagePumpType::UI);
-    std::unique_ptr<UpdaterPrefs> local_prefs = CreateLocalPrefs();
-    local_prefs->GetPrefService()->SetBoolean(kPrefQualified, true);
+    std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();
+    local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
   auto app = base::MakeRefCounted<AppServerTest>();
@@ -122,18 +122,17 @@ TEST_F(AppServerTestCase, SelfPromote) {
   EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(true));
   EXPECT_CALL(*app, UninstallSelf).Times(0);
   EXPECT_EQ(app->Run(), 0);
-  std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
-  EXPECT_FALSE(global_prefs->GetPrefService()->GetBoolean(kPrefSwapping));
-  EXPECT_EQ(global_prefs->GetPrefService()->GetString(kPrefActiveVersion),
-            UPDATER_VERSION_STRING);
+  std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+  EXPECT_FALSE(global_prefs->GetSwapping());
+  EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
 }
 
 TEST_F(AppServerTestCase, SelfPromoteFails) {
   {
     base::SingleThreadTaskExecutor main_task_executor(
         base::MessagePumpType::UI);
-    std::unique_ptr<UpdaterPrefs> local_prefs = CreateLocalPrefs();
-    local_prefs->GetPrefService()->SetBoolean(kPrefQualified, true);
+    std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();
+    local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
   auto app = base::MakeRefCounted<AppServerTest>();
@@ -143,21 +142,20 @@ TEST_F(AppServerTestCase, SelfPromoteFails) {
   EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(false));
   EXPECT_CALL(*app, UninstallSelf).Times(0);
   EXPECT_EQ(app->Run(), 2);
-  std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
-  EXPECT_TRUE(global_prefs->GetPrefService()->GetBoolean(kPrefSwapping));
-  EXPECT_EQ(global_prefs->GetPrefService()->GetString(kPrefActiveVersion), "0");
+  std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+  EXPECT_TRUE(global_prefs->GetSwapping());
+  EXPECT_EQ(global_prefs->GetActiveVersion(), "0");
 }
 
 TEST_F(AppServerTestCase, ActiveDutyAlready) {
   {
     base::SingleThreadTaskExecutor main_task_executor(
         base::MessagePumpType::UI);
-    std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
-    global_prefs->GetPrefService()->SetString(kPrefActiveVersion,
-                                              UPDATER_VERSION_STRING);
+    std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+    global_prefs->SetActiveVersion(UPDATER_VERSION_STRING);
     PrefsCommitPendingWrites(global_prefs->GetPrefService());
-    std::unique_ptr<UpdaterPrefs> local_prefs = CreateLocalPrefs();
-    local_prefs->GetPrefService()->SetBoolean(kPrefQualified, true);
+    std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();
+    local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
   auto app = base::MakeRefCounted<AppServerTest>();
@@ -167,23 +165,21 @@ TEST_F(AppServerTestCase, ActiveDutyAlready) {
   EXPECT_CALL(*app, SwapRPCInterfaces).Times(0);
   EXPECT_CALL(*app, UninstallSelf).Times(0);
   EXPECT_EQ(app->Run(), 0);
-  std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
-  EXPECT_FALSE(global_prefs->GetPrefService()->GetBoolean(kPrefSwapping));
-  EXPECT_EQ(global_prefs->GetPrefService()->GetString(kPrefActiveVersion),
-            UPDATER_VERSION_STRING);
+  std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+  EXPECT_FALSE(global_prefs->GetSwapping());
+  EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
 }
 
 TEST_F(AppServerTestCase, StateDirty) {
   {
     base::SingleThreadTaskExecutor main_task_executor(
         base::MessagePumpType::UI);
-    std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
-    global_prefs->GetPrefService()->SetString(kPrefActiveVersion,
-                                              UPDATER_VERSION_STRING);
-    global_prefs->GetPrefService()->SetBoolean(kPrefSwapping, true);
+    std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+    global_prefs->SetActiveVersion(UPDATER_VERSION_STRING);
+    global_prefs->SetSwapping(true);
     PrefsCommitPendingWrites(global_prefs->GetPrefService());
-    std::unique_ptr<UpdaterPrefs> local_prefs = CreateLocalPrefs();
-    local_prefs->GetPrefService()->SetBoolean(kPrefQualified, true);
+    std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();
+    local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
   auto app = base::MakeRefCounted<AppServerTest>();
@@ -194,23 +190,21 @@ TEST_F(AppServerTestCase, StateDirty) {
   EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(true));
   EXPECT_CALL(*app, UninstallSelf).Times(0);
   EXPECT_EQ(app->Run(), 0);
-  std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
-  EXPECT_FALSE(global_prefs->GetPrefService()->GetBoolean(kPrefSwapping));
-  EXPECT_EQ(global_prefs->GetPrefService()->GetString(kPrefActiveVersion),
-            UPDATER_VERSION_STRING);
+  std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+  EXPECT_FALSE(global_prefs->GetSwapping());
+  EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
 }
 
 TEST_F(AppServerTestCase, StateDirtySwapFails) {
   {
     base::SingleThreadTaskExecutor main_task_executor(
         base::MessagePumpType::UI);
-    std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
-    global_prefs->GetPrefService()->SetString(kPrefActiveVersion,
-                                              UPDATER_VERSION_STRING);
-    global_prefs->GetPrefService()->SetBoolean(kPrefSwapping, true);
+    std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+    global_prefs->SetActiveVersion(UPDATER_VERSION_STRING);
+    global_prefs->SetSwapping(true);
     PrefsCommitPendingWrites(global_prefs->GetPrefService());
-    std::unique_ptr<UpdaterPrefs> local_prefs = CreateLocalPrefs();
-    local_prefs->GetPrefService()->SetBoolean(kPrefQualified, true);
+    std::unique_ptr<LocalPrefs> local_prefs = CreateLocalPrefs();
+    local_prefs->SetQualified(true);
     PrefsCommitPendingWrites(local_prefs->GetPrefService());
   }
   auto app = base::MakeRefCounted<AppServerTest>();
@@ -220,10 +214,9 @@ TEST_F(AppServerTestCase, StateDirtySwapFails) {
   EXPECT_CALL(*app, SwapRPCInterfaces).WillOnce(Return(false));
   EXPECT_CALL(*app, UninstallSelf).Times(0);
   EXPECT_EQ(app->Run(), 2);
-  std::unique_ptr<UpdaterPrefs> global_prefs = CreateGlobalPrefs();
-  EXPECT_TRUE(global_prefs->GetPrefService()->GetBoolean(kPrefSwapping));
-  EXPECT_EQ(global_prefs->GetPrefService()->GetString(kPrefActiveVersion),
-            UPDATER_VERSION_STRING);
+  std::unique_ptr<GlobalPrefs> global_prefs = CreateGlobalPrefs();
+  EXPECT_TRUE(global_prefs->GetSwapping());
+  EXPECT_EQ(global_prefs->GetActiveVersion(), UPDATER_VERSION_STRING);
 }
 
 }  // namespace updater

chrome/updater/prefs.cc

@@ -21,21 +21,49 @@
 
 namespace updater {
 
+namespace {
+
 const char kPrefQualified[] = "qualified";
 const char kPrefSwapping[] = "swapping";
 const char kPrefActiveVersion[] = "active_version";
 
-UpdaterPrefs::UpdaterPrefs(std::unique_ptr<ScopedPrefsLock> lock,
-                           std::unique_ptr<PrefService> prefs)
+}  // namespace
+
+UpdaterPrefsImpl::UpdaterPrefsImpl(std::unique_ptr<ScopedPrefsLock> lock,
+                                   std::unique_ptr<PrefService> prefs)
     : lock_(std::move(lock)), prefs_(std::move(prefs)) {}
 
-UpdaterPrefs::~UpdaterPrefs() = default;
+UpdaterPrefsImpl::~UpdaterPrefsImpl() = default;
 
-PrefService* UpdaterPrefs::GetPrefService() const {
+PrefService* UpdaterPrefsImpl::GetPrefService() const {
   return prefs_.get();
 }
 
-std::unique_ptr<UpdaterPrefs> CreateGlobalPrefs() {
+bool UpdaterPrefsImpl::GetQualified() const {
+  return prefs_->GetBoolean(kPrefQualified);
+}
+
+void UpdaterPrefsImpl::SetQualified(bool value) {
+  prefs_->SetBoolean(kPrefQualified, value);
+}
+
+std::string UpdaterPrefsImpl::GetActiveVersion() const {
+  return prefs_->GetString(kPrefActiveVersion);
+}
+
+void UpdaterPrefsImpl::SetActiveVersion(std::string value) {
+  prefs_->SetString(kPrefActiveVersion, value);
+}
+
+bool UpdaterPrefsImpl::GetSwapping() const {
+  return prefs_->GetBoolean(kPrefSwapping);
+}
+
+void UpdaterPrefsImpl::SetSwapping(bool value) {
+  prefs_->SetBoolean(kPrefSwapping, value);
+}
+
+std::unique_ptr<GlobalPrefs> CreateGlobalPrefs() {
   std::unique_ptr<ScopedPrefsLock> lock =
       AcquireGlobalPrefsLock(base::TimeDelta::FromMinutes(2));
   if (!lock)
@@ -54,11 +82,11 @@ std::unique_ptr<UpdaterPrefs> CreateGlobalPrefs() {
   pref_registry->RegisterBooleanPref(kPrefSwapping, false);
   pref_registry->RegisterStringPref(kPrefActiveVersion, "0");
 
-  return std::make_unique<UpdaterPrefs>(
+  return std::make_unique<UpdaterPrefsImpl>(
       std::move(lock), pref_service_factory.Create(pref_registry));
 }
 
-std::unique_ptr<UpdaterPrefs> CreateLocalPrefs() {
+std::unique_ptr<LocalPrefs> CreateLocalPrefs() {
   base::FilePath local_prefs_dir;
   if (!GetVersionedDirectory(&local_prefs_dir))
     return nullptr;
@@ -71,7 +99,7 @@ std::unique_ptr<UpdaterPrefs> CreateLocalPrefs() {
   update_client::RegisterPrefs(pref_registry.get());
   pref_registry->RegisterBooleanPref(kPrefQualified, false);
 
-  return std::make_unique<UpdaterPrefs>(
+  return std::make_unique<UpdaterPrefsImpl>(
       nullptr, pref_service_factory.Create(pref_registry));
 }
 

chrome/updater/prefs.h

@@ -11,34 +11,43 @@ class PrefService;
 
 namespace updater {
 
-class ScopedPrefsLock;
-
-extern const char kPrefQualified[];
-extern const char kPrefSwapping[];
-extern const char kPrefActiveVersion[];
-
 class UpdaterPrefs {
  public:
-  UpdaterPrefs(std::unique_ptr<ScopedPrefsLock> lock,
-               std::unique_ptr<PrefService> prefs);
+  UpdaterPrefs() = default;
   UpdaterPrefs(const UpdaterPrefs&) = delete;
   UpdaterPrefs& operator=(const UpdaterPrefs&) = delete;
-  ~UpdaterPrefs();
+  virtual ~UpdaterPrefs() = default;
 
-  PrefService* GetPrefService() const;
+  virtual PrefService* GetPrefService() const = 0;
+};
+
+class LocalPrefs : public UpdaterPrefs {
+ public:
+  LocalPrefs() = default;
+  ~LocalPrefs() override = default;
+
+  virtual bool GetQualified() const = 0;
+  virtual void SetQualified(bool value) = 0;
+};
+
+class GlobalPrefs : public UpdaterPrefs {
+ public:
+  GlobalPrefs() = default;
+  ~GlobalPrefs() override = default;
 
- private:
-  std::unique_ptr<ScopedPrefsLock> lock_;
-  std::unique_ptr<PrefService> prefs_;
+  virtual std::string GetActiveVersion() const = 0;
+  virtual void SetActiveVersion(std::string value) = 0;
+  virtual bool GetSwapping() const = 0;
+  virtual void SetSwapping(bool value) = 0;
 };
 
 // Open the global prefs. These prefs are protected by a mutex, and shared by
 // all updaters on the system. Returns nullptr if the mutex cannot be acquired.
-std::unique_ptr<UpdaterPrefs> CreateGlobalPrefs();
+std::unique_ptr<GlobalPrefs> CreateGlobalPrefs();
 
 // Open the version-specific prefs. These prefs are not protected by any mutex
 // and not shared with other versions of the updater.
-std::unique_ptr<UpdaterPrefs> CreateLocalPrefs();
+std::unique_ptr<LocalPrefs> CreateLocalPrefs();
 
 // Commits prefs changes to storage. This function should only be called
 // when the changes must be written immediately, for instance, during program

chrome/updater/prefs_impl.h

@@ -7,6 +7,8 @@
 
 #include <memory>
 
+#include "prefs.h"
+
 namespace base {
 class TimeDelta;
 }  // namespace base
@@ -29,6 +31,30 @@ class ScopedPrefsLock {
   std::unique_ptr<ScopedPrefsLockImpl> impl_;
 };
 
+class UpdaterPrefsImpl : public LocalPrefs, public GlobalPrefs {
+ public:
+  UpdaterPrefsImpl(std::unique_ptr<ScopedPrefsLock> lock,
+                   std::unique_ptr<PrefService> prefs);
+  ~UpdaterPrefsImpl() override;
+
+  // Overrides for UpdaterPrefs.
+  PrefService* GetPrefService() const override;
+
+  // Overrides for LocalPrefs
+  bool GetQualified() const override;
+  void SetQualified(bool value) override;
+
+  // Overrides for GlobalPrfs
+  std::string GetActiveVersion() const override;
+  void SetActiveVersion(std::string value) override;
+  bool GetSwapping() const override;
+  void SetSwapping(bool value) override;
+
+ private:
+  std::unique_ptr<ScopedPrefsLock> lock_;
+  std::unique_ptr<PrefService> prefs_;
+};
+
 // Returns a ScopedPrefsLock, or nullptr if the lock could not be acquired
 // within the timeout. While the ScopedPrefsLock exists, no other process on
 // the machine may access global prefs.