Register the Updater and Chrome app ids with the //chrome/updater.

This change uses prefs to store and retrieve app ids for the apps
registered for updates.

Bug: 1020285
Change-Id: I7dea36049e6e09ff7039b61f79acb268e94baddb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2053584
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745125}

chrome/updater/BUILD.gn

@@ -42,6 +42,8 @@ if (is_win || is_mac) {
       "installer_win.cc",
       "patcher.cc",
       "patcher.h",
+      "persisted_data.cc",
+      "persisted_data.h",
       "prefs.cc",
       "prefs.h",
       "registration_data.cc",
@@ -128,17 +130,20 @@ if (is_win || is_mac) {
     testonly = true
 
     sources = [
+      "persisted_data_unittest.cc",
       "run_all_unittests.cc",
       "updater_unittest.cc",
     ]
 
     deps = [
-      ":lib",
+      ":base",
       ":updater",
       ":version_header",
       "//base",
       "//base/test:test_support",
       "//chrome/common:constants",
+      "//components/prefs:test_support",
+      "//components/update_client",
       "//testing/gtest",
     ]
 

chrome/updater/configurator.h

@@ -10,7 +10,6 @@
 #include <vector>
 
 #include "base/containers/flat_map.h"
-#include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "components/update_client/configurator.h"
 
@@ -32,6 +31,8 @@ namespace updater {
 class Configurator : public update_client::Configurator {
  public:
   Configurator();
+  Configurator(const Configurator&) = delete;
+  Configurator& operator=(const Configurator&) = delete;
 
   // Configurator for update_client::Configurator.
   int InitialDelay() const override;
@@ -70,7 +71,6 @@ class Configurator : public update_client::Configurator {
   scoped_refptr<update_client::NetworkFetcherFactory> network_fetcher_factory_;
   scoped_refptr<update_client::UnzipperFactory> unzip_factory_;
   scoped_refptr<update_client::PatcherFactory> patch_factory_;
-  DISALLOW_COPY_AND_ASSIGN(Configurator);
 };
 
 }  // namespace updater

chrome/updater/constants.cc

@@ -9,6 +9,7 @@ namespace updater {
 // App ids.
 const char kUpdaterAppId[] = "{44FC7FE2-65CE-487C-93F4-EDEE46EEAAAB}";
 const char kChromeAppId[] = "{8A69D345-D564-463C-AFF1-A69D9E530F96}";
+const char kNullVersion[] = "0.0.0.0";
 
 // Command line arguments.
 const char kServerSwitch[] = "server";

chrome/updater/constants.h

@@ -15,6 +15,8 @@ extern const char kUpdaterAppId[];
 // Chrome's app ID.
 extern const char kChromeAppId[];
 
+extern const char kNullVersion[];
+
 // Command line switches.
 //
 

chrome/updater/installer.cc

@@ -5,6 +5,7 @@
 #include "chrome/updater/installer.h"
 
 #include <utility>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/files/file_enumerator.h"
@@ -24,9 +25,6 @@ namespace updater {
 
 namespace {
 
-// Version "0" corresponds to no installed version.
-const char kNullVersion[] = "0.0.0.0";
-
 // This task joins a process, hence .WithBaseSyncPrimitives().
 static constexpr base::TaskTraits kTaskTraitsBlockWithSyncPrimitives = {
     base::ThreadPool(), base::MayBlock(), base::WithBaseSyncPrimitives(),
@@ -46,15 +44,26 @@ base::FilePath GetAppInstallDir(const std::string& app_id) {
 
 }  // namespace
 
-Installer::InstallInfo::InstallInfo() : version(kNullVersion) {}
-Installer::InstallInfo::~InstallInfo() = default;
-
-Installer::Installer(const std::string& app_id)
-    : app_id_(app_id), install_info_(std::make_unique<InstallInfo>()) {}
+Installer::Installer(const std::string& app_id,
+                     scoped_refptr<PersistedData> persisted_data)
+    : app_id_(app_id), persisted_data_(persisted_data) {}
 
-Installer::~Installer() = default;
+Installer::~Installer() {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+}
 
 update_client::CrxComponent Installer::MakeCrxComponent() {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+
+  VLOG(1) << __func__ << " for " << app_id_;
+
+  // |pv| is the version of the registered app, persisted in prefs, and used
+  // in the update checks and pings.
+  const auto pv = persisted_data_->GetProductVersion(app_id_);
+  if (pv.IsValid())
+    pv_ = pv;
+  fingerprint_ = persisted_data_->GetFingerprint(app_id_);
+
   update_client::CrxComponent component;
   component.installer = scoped_refptr<Installer>(this);
   component.action_handler = MakeActionHandler();
@@ -63,76 +72,40 @@ update_client::CrxComponent Installer::MakeCrxComponent() {
       crx_file::VerifierFormat::CRX3_WITH_PUBLISHER_PROOF;
   component.app_id = app_id_;
   component.name = app_id_;
-  component.version = install_info_->version;
-  component.fingerprint = install_info_->fingerprint;
+  component.version = pv_;
+  component.fingerprint = fingerprint_;
   return component;
 }
 
-std::vector<std::string> Installer::FindAppIds() {
-  base::FilePath app_install_dir;
-  if (!GetProductDirectory(&app_install_dir))
-    return {};
-  app_install_dir = app_install_dir.AppendASCII(kAppsDir);
-  std::vector<std::string> app_ids;
-  base::FileEnumerator file_enumerator(app_install_dir, false,
-                                       base::FileEnumerator::DIRECTORIES);
-  for (auto path = file_enumerator.Next(); !path.value().empty();
-       path = file_enumerator.Next()) {
-    app_ids.push_back(path.BaseName().MaybeAsASCII());
-  }
-  return app_ids;
-}
-
-void Installer::FindInstallOfApp() {
-  VLOG(1) << __func__ << " for " << app_id_;
+void Installer::DeleteOlderInstallPaths() {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::WILL_BLOCK);
 
   const base::FilePath app_install_dir = GetAppInstallDir(app_id_);
   if (app_install_dir.empty() || !base::PathExists(app_install_dir)) {
-    install_info_ = std::make_unique<InstallInfo>();
     return;
   }
 
-  base::Version latest_version(kNullVersion);
-  base::FilePath latest_path;
-  std::vector<base::FilePath> older_paths;
   base::FileEnumerator file_enumerator(app_install_dir, false,
                                        base::FileEnumerator::DIRECTORIES);
   for (auto path = file_enumerator.Next(); !path.value().empty();
        path = file_enumerator.Next()) {
-    const base::Version version(path.BaseName().MaybeAsASCII());
-
-    // Ignore folders that don't have valid version names.
-    if (!version.IsValid())
-      continue;
+    const base::Version version_dir(path.BaseName().MaybeAsASCII());
 
-    // The |version| not newer than the latest found version is marked for
-    // removal. |kNullVersion| is also removed.
-    if (version.CompareTo(latest_version) <= 0) {
-      older_paths.push_back(path);
-      continue;
+    // Mark for deletion any valid versioned directory except the directory
+    // for the currently registered app.
+    if (version_dir.IsValid() && version_dir.CompareTo(pv_)) {
+      base::DeleteFileRecursively(path);
     }
-
-    // New valid |version| folder found.
-    if (!latest_path.empty())
-      older_paths.push_back(latest_path);
-
-    latest_version = version;
-    latest_path = path;
   }
-
-  install_info_->version = latest_version;
-  install_info_->install_dir = latest_path;
-  install_info_->manifest = update_client::ReadManifest(latest_path);
-  base::ReadFileToString(latest_path.AppendASCII("manifest.fingerprint"),
-                         &install_info_->fingerprint);
-
-  for (const auto& older_path : older_paths)
-    base::DeleteFileRecursively(older_path);
 }
 
 Installer::Result Installer::InstallHelper(
     const base::FilePath& unpack_path,
     std::unique_ptr<InstallParams> install_params) {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::WILL_BLOCK);
+
   auto local_manifest = update_client::ReadManifest(unpack_path);
   if (!local_manifest)
     return Result(update_client::InstallError::BAD_MANIFEST);
@@ -141,13 +114,12 @@ Installer::Result Installer::InstallHelper(
   local_manifest->GetStringASCII("version", &version_ascii);
   const base::Version manifest_version(version_ascii);
 
-  VLOG(1) << "Installed version=" << install_info_->version.GetString()
+  VLOG(1) << "Installed version=" << pv_
           << ", installing version=" << manifest_version.GetString();
-
   if (!manifest_version.IsValid())
     return Result(update_client::InstallError::INVALID_VERSION);
 
-  if (install_info_->version.CompareTo(manifest_version) > 0)
+  if (pv_.CompareTo(manifest_version) > 0)
     return Result(update_client::InstallError::VERSION_NOT_UPGRADED);
 
   const base::FilePath app_install_dir = GetAppInstallDir(app_id_);
@@ -178,25 +150,26 @@ Installer::Result Installer::InstallHelper(
   DCHECK(!base::PathExists(unpack_path));
   DCHECK(base::PathExists(versioned_install_dir));
 
-  install_info_->manifest = std::move(local_manifest);
-  install_info_->version = manifest_version;
-  install_info_->install_dir = versioned_install_dir;
-  base::ReadFileToString(
-      versioned_install_dir.AppendASCII("manifest.fingerprint"),
-      &install_info_->fingerprint);
-
   // Resolve the path to an installer file, which is included in the CRX, and
   // specified by the |run| attribute in the manifest object of an update
   // response.
   if (!install_params || install_params->run.empty())
     return Result(kErrorMissingInstallParams);
-  base::FilePath application_installer;
-  if (!GetInstalledFile(install_params->run, &application_installer))
+
+  // Assume the install params are ASCII for now.
+  const auto application_installer =
+      versioned_install_dir.AppendASCII(install_params->run);
+  if (!base::PathExists(application_installer))
     return Result(kErrorMissingRunableFile);
 
   // TODO(sorin): the installer API needs to be handled here. crbug.com/1014630.
   const int exit_code =
       RunApplicationInstaller(application_installer, install_params->arguments);
+
+  // Upon success, when the control flow returns back to the |update_client|,
+  // the prefs are updated asynchronously with the new |pv| and |fingerprint|.
+  // The task sequencing guarantees that the prefs will be updated by the
+  // time another CrxDataCallback is invoked, which needs updated values.
   return exit_code == 0 ? Result(update_client::InstallError::NONE)
                         : Result(kErrorApplicationInstallerFailed, exit_code);
 }
@@ -208,6 +181,7 @@ void Installer::InstallWithSyncPrimitives(
     Callback callback) {
   base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
                                                 base::BlockingType::WILL_BLOCK);
+  DeleteOlderInstallPaths();
   const auto result = InstallHelper(unpack_path, std::move(install_params));
   base::DeleteFileRecursively(unpack_path);
   std::move(callback).Run(result);
@@ -230,10 +204,16 @@ void Installer::Install(const base::FilePath& unpack_path,
 
 bool Installer::GetInstalledFile(const std::string& file,
                                  base::FilePath* installed_file) {
-  if (install_info_->version == base::Version(kNullVersion))
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::WILL_BLOCK);
+  if (pv_ == base::Version(kNullVersion))
     return false;  // No component has been installed yet.
 
-  *installed_file = install_info_->install_dir.AppendASCII(file);
+  const auto install_dir = GetCurrentInstallDir();
+  if (install_dir.empty())
+    return false;
+
+  *installed_file = install_dir.AppendASCII(file);
   return true;
 }
 
@@ -241,6 +221,12 @@ bool Installer::Uninstall() {
   return false;
 }
 
+base::FilePath Installer::GetCurrentInstallDir() const {
+  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
+                                                base::BlockingType::WILL_BLOCK);
+  return GetAppInstallDir(app_id_).AppendASCII(pv_.GetString());
+}
+
 #if !defined(OS_WIN)
 int Installer::RunApplicationInstaller(const base::FilePath& app_installer,
                                        const std::string& arguments) {

chrome/updater/installer.h

@@ -5,50 +5,51 @@
 #ifndef CHROME_UPDATER_INSTALLER_H_
 #define CHROME_UPDATER_INSTALLER_H_
 
-#include <stdint.h>
-
 #include <memory>
 #include <string>
-#include <vector>
 
 #include "base/callback_forward.h"
 #include "base/files/file_path.h"
-#include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/sequence_checker.h"
 #include "base/values.h"
 #include "base/version.h"
+#include "chrome/updater/persisted_data.h"
 #include "components/update_client/update_client.h"
 
 namespace updater {
 
+// Manages the install of one application. Some of the functions of this
+// class are blocking and can't be invoked on the main sequence.
+//
+// If the application installer completes with success, then the following
+// post conditions are true: |update_client| updates persisted data in prefs,
+// the CRX is installed in a versioned directory in apps/app_id/version,
+// the application is considered to be registered for updates, and the
+// application installed version matches the version recorded in prefs.
+//
+// If installing the CRX fails, or the installer fails, then prefs is not
+// going to be updated. There will be some files left on the file system, which
+// are going to be cleaned up next time the installer runs.
+//
+// Install directories not matching the |pv| version are lazy-deleted.
 class Installer final : public update_client::CrxInstaller {
  public:
-  struct InstallInfo {
-    InstallInfo();
-    ~InstallInfo();
-
-    base::FilePath install_dir;
-    base::Version version;
-    std::string fingerprint;
-    std::unique_ptr<base::DictionaryValue> manifest;
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(InstallInfo);
-  };
-
-  explicit Installer(const std::string& app_id);
+  Installer(const std::string& app_id,
+            scoped_refptr<PersistedData> persisted_data);
+  Installer(const Installer&) = delete;
+  Installer& operator=(const Installer&) = delete;
 
   const std::string app_id() const { return app_id_; }
 
-  // Returns the app ids that are managed by the CRX installer.
-  static std::vector<std::string> FindAppIds();
-
-  // Finds the highest version install of the app, and updates the install
-  // info for this installer instance.
-  void FindInstallOfApp();
-
   // Returns a CrxComponent instance that describes the current install
-  // state of the app.
+  // state of the app. Updates the values of |pv_| and the |fingerprint_| with
+  // the persisted values in prefs.
+  //
+  // Callers should only invoke this function when handling a CrxDataCallback
+  // callback from update_client::Install or from update_client::Update. This
+  // ensure that prefs has been updated with the most recent values, including
+  // |pv| and |fingerprint|.
   update_client::CrxComponent MakeCrxComponent();
 
  private:
@@ -83,10 +84,20 @@ class Installer final : public update_client::CrxInstaller {
   int RunApplicationInstaller(const base::FilePath& app_installer,
                               const std::string& arguments);
 
+  // Deletes recursively the install paths not matching the |pv_| version.
+  void DeleteOlderInstallPaths();
+
+  // Returns an install directory matching the |pv_| version.
+  base::FilePath GetCurrentInstallDir() const;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
   const std::string app_id_;
-  std::unique_ptr<InstallInfo> install_info_;
+  scoped_refptr<PersistedData> persisted_data_;
 
-  DISALLOW_COPY_AND_ASSIGN(Installer);
+  // These members are not updated when the installer succeeds.
+  base::Version pv_;
+  std::string fingerprint_;
 };
 
 }  // namespace updater

chrome/updater/persisted_data.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/updater/persisted_data.h"
+
+#include "base/logging.h"
+#include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/values.h"
+#include "base/version.h"
+#include "components/prefs/pref_service.h"
+#include "components/prefs/scoped_user_pref_update.h"
+
+namespace {
+
+// Uses the same pref as the update_client code.
+constexpr char kPersistedDataPreference[] = "updateclientdata";
+
+constexpr char kPV[] = "pv";
+constexpr char kFP[] = "fp";
+
+}  // namespace
+
+namespace updater {
+
+PersistedData::PersistedData(PrefService* pref_service)
+    : pref_service_(pref_service) {
+  DCHECK(pref_service_);
+  DCHECK(pref_service_->FindPreference(kPersistedDataPreference));
+}
+
+PersistedData::~PersistedData() {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+}
+
+base::Version PersistedData::GetProductVersion(const std::string& id) const {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  return base::Version(GetString(id, kPV));
+}
+
+void PersistedData::SetProductVersion(const std::string& id,
+                                      const base::Version& pv) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK(pv.IsValid());
+  SetString(id, kPV, pv.GetString());
+}
+
+std::string PersistedData::GetFingerprint(const std::string& id) const {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  return GetString(id, kFP);
+}
+
+void PersistedData::SetFingerprint(const std::string& id,
+                                   const std::string& fingerprint) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  SetString(id, kFP, fingerprint);
+}
+
+std::vector<std::string> PersistedData::GetAppIds() const {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+
+  // The prefs is a dictionary of dictionaries, where each inner dictionary
+  // corresponds to an app:
+  // {"updateclientdata":{"apps":{"{44FC7FE2-65CE-487C-93F4-EDEE46EEAAAB}":{...
+  const auto* pref = pref_service_->GetDictionary(kPersistedDataPreference);
+  if (!pref)
+    return {};
+  const auto* apps = pref->FindKey("apps");
+  if (!apps || !apps->is_dict())
+    return {};
+  std::vector<std::string> app_ids;
+  for (const auto& kv : apps->DictItems()) {
+    const auto& app_id = kv.first;
+    const auto pv = GetProductVersion(app_id);
+    if (pv.IsValid())
+      app_ids.push_back(app_id);
+  }
+  return app_ids;
+}
+
+std::string PersistedData::GetString(const std::string& id,
+                                     const std::string& key) const {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK(!base::Contains(id, '.'));  // Assume the id does not contain '.'.
+
+  const base::DictionaryValue* dict =
+      pref_service_->GetDictionary(kPersistedDataPreference);
+  if (!dict)
+    return {};
+  std::string result;
+  return dict->GetString(
+             base::StringPrintf("apps.%s.%s", id.c_str(), key.c_str()), &result)
+             ? result
+             : std::string();
+}
+
+void PersistedData::SetString(const std::string& id,
+                              const std::string& key,
+                              const std::string& value) {
+  DCHECK(sequence_checker_.CalledOnValidSequence());
+  DCHECK(!base::Contains(id, '.'));  // Assume the id does not contain '.'.
+
+  DictionaryPrefUpdate update(pref_service_, kPersistedDataPreference);
+  update->SetString(base::StringPrintf("apps.%s.%s", id.c_str(), key.c_str()),
+                    value);
+}
+
+}  // namespace updater

chrome/updater/persisted_data.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_UPDATER_PERSISTED_DATA_H_
+#define CHROME_UPDATER_PERSISTED_DATA_H_
+
+#include <string>
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "base/sequence_checker.h"
+
+class PrefService;
+
+namespace base {
+class Version;
+}
+
+namespace updater {
+
+// PersistedData uses the PrefService to persist updater data that outlives
+// the updater processes.
+//
+// This class has sequence affinity.
+//
+// A mechanism to remove apps or app versions from prefs is needed.
+// TODO(sorin): crbug.com/1056450
+class PersistedData : public base::RefCounted<PersistedData> {
+ public:
+  // Constructs a provider using the specified |pref_service|.
+  // The associated preferences are assumed to already be registered.
+  // The |pref_service| must outlive the instance of this class.
+  explicit PersistedData(PrefService* pref_service);
+  PersistedData(const PersistedData&) = delete;
+  PersistedData& operator=(const PersistedData&) = delete;
+
+  // These functions access |pv| data for the specified |id|. Returns an empty
+  // version, if the version is not found.
+  base::Version GetProductVersion(const std::string& id) const;
+  void SetProductVersion(const std::string& id, const base::Version& pv);
+
+  // These functions access |fingerprint| data for the specified |id|.
+  std::string GetFingerprint(const std::string& id) const;
+  void SetFingerprint(const std::string& id, const std::string& fp);
+
+  // Returns the app ids of the applications registered in prefs, if the
+  // application has a valid version.
+  std::vector<std::string> GetAppIds() const;
+
+ private:
+  friend class base::RefCounted<PersistedData>;
+  ~PersistedData();
+
+  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);
+
+  SEQUENCE_CHECKER(sequence_checker_);
+
+  PrefService* pref_service_ = nullptr;  // Not owned by this class.
+};
+
+}  // namespace updater
+
+#endif  // CHROME_UPDATER_PERSISTED_DATA_H_

chrome/updater/persisted_data_unittest.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+#include <string>
+
+#include "base/memory/scoped_refptr.h"
+#include "base/stl_util.h"
+#include "base/version.h"
+#include "chrome/updater/persisted_data.h"
+#include "components/prefs/testing_pref_service.h"
+#include "components/update_client/update_client.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace updater {
+
+TEST(PersistedDataTest, Simple) {
+  auto pref = std::make_unique<TestingPrefServiceSimple>();
+  update_client::RegisterPrefs(pref->registry());
+  auto metadata = base::MakeRefCounted<PersistedData>(pref.get());
+
+  EXPECT_FALSE(metadata->GetProductVersion("someappid").IsValid());
+  EXPECT_TRUE(metadata->GetFingerprint("someappid").empty());
+  EXPECT_TRUE(metadata->GetAppIds().empty());
+
+  metadata->SetProductVersion("someappid", base::Version("1.0"));
+  EXPECT_STREQ("1.0",
+               metadata->GetProductVersion("someappid").GetString().c_str());
+
+  metadata->SetFingerprint("someappid", "fp1");
+  EXPECT_STREQ("fp1", metadata->GetFingerprint("someappid").c_str());
+
+  // Store some more apps in prefs, in addition to "someappid". Expect only
+  // the app ids for apps with valid versions to be returned.
+  metadata->SetProductVersion("appid1", base::Version("2.0"));
+  metadata->SetFingerprint("appid2-nopv", "somefp");
+  EXPECT_FALSE(metadata->GetProductVersion("appid2-nopv").IsValid());
+  const auto app_ids = metadata->GetAppIds();
+  EXPECT_EQ(2u, app_ids.size());
+  EXPECT_TRUE(base::Contains(app_ids, "someappid"));
+  EXPECT_TRUE(base::Contains(app_ids, "appid1"));
+  EXPECT_FALSE(base::Contains(app_ids, "appid2-nopv"));  // No valid pv.
+}
+
+TEST(PersistedDataTest, SharedPref) {
+  auto pref = std::make_unique<TestingPrefServiceSimple>();
+  update_client::RegisterPrefs(pref->registry());
+  auto metadata = base::MakeRefCounted<PersistedData>(pref.get());
+
+  metadata->SetProductVersion("someappid", base::Version("1.0"));
+  EXPECT_STREQ("1.0",
+               metadata->GetProductVersion("someappid").GetString().c_str());
+
+  // Now, create a new PersistedData reading from the same path, verify
+  // that it loads the value.
+  metadata = base::MakeRefCounted<PersistedData>(pref.get());
+  EXPECT_STREQ("1.0",
+               metadata->GetProductVersion("someappid").GetString().c_str());
+}
+
+}  // namespace updater

chrome/updater/update_service_in_process.cc

@@ -4,6 +4,10 @@
 
 #include "chrome/updater/update_service_in_process.h"
 
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "base/bind.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/run_loop.h"
@@ -14,6 +18,7 @@
 #include "chrome/updater/configurator.h"
 #include "chrome/updater/constants.h"
 #include "chrome/updater/installer.h"
+#include "chrome/updater/persisted_data.h"
 #include "chrome/updater/registration_data.h"
 #include "components/prefs/pref_service.h"
 #include "components/update_client/crx_update_item.h"
@@ -25,6 +30,8 @@ namespace updater {
 UpdateServiceInProcess::UpdateServiceInProcess(
     scoped_refptr<update_client::Configurator> config)
     : config_(config),
+      persisted_data_(
+          base::MakeRefCounted<PersistedData>(config_->GetPrefService())),
       main_task_runner_(base::SequencedTaskRunnerHandle::Get()),
       update_client_(update_client::UpdateClientFactory(config)) {}
 
@@ -43,32 +50,23 @@ void UpdateServiceInProcess::UpdateAll(
     base::OnceCallback<void(update_client::Error)> callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  auto app_ids = Installer::FindAppIds();
-
-  // Include the app id for the updater if it is not found. This could happen
-  // before the first update for the updater has been handled. This is a
-  // temporary workaround until the source of truth for the registered
-  // version is resolved.
-  if (!base::Contains(app_ids, kUpdaterAppId))
-    app_ids.push_back(kUpdaterAppId);
-
-  std::vector<base::Optional<update_client::CrxComponent>> components;
-  for (const auto& app_id : app_ids) {
-    auto installer = base::MakeRefCounted<Installer>(app_id);
-    installer->FindInstallOfApp();
-    components.push_back(installer->MakeCrxComponent());
-  }
+  const auto app_ids = persisted_data_->GetAppIds();
+  DCHECK(base::Contains(app_ids, kUpdaterAppId));
 
   update_client_->Update(
       app_ids,
       base::BindOnce(
-          [](const std::vector<base::Optional<update_client::CrxComponent>>&
-                 components,
+          [](scoped_refptr<PersistedData> persisted_data,
              const std::vector<std::string>& ids) {
-            DCHECK_EQ(components.size(), ids.size());
+            std::vector<base::Optional<update_client::CrxComponent>> components;
+            for (const auto& id : ids) {
+              components.push_back(
+                  base::MakeRefCounted<Installer>(id, persisted_data)
+                      ->MakeCrxComponent());
+            }
             return components;
           },
-          components),
+          persisted_data_),
       false, std::move(callback));
 }
 

chrome/updater/update_service_in_process.h

@@ -5,6 +5,9 @@
 #ifndef CHROME_UPDATER_UPDATE_SERVICE_IN_PROCESS_H_
 #define CHROME_UPDATER_UPDATE_SERVICE_IN_PROCESS_H_
 
+#include <string>
+#include <vector>
+
 #include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 #include "base/sequence_checker.h"
@@ -21,6 +24,7 @@ class UpdateClient;
 }  // namespace update_client
 
 namespace updater {
+class PersistedData;
 struct RegistrationRequest;
 struct RegistrationResponse;
 
@@ -50,6 +54,7 @@ class UpdateServiceInProcess : public UpdateService {
   SEQUENCE_CHECKER(sequence_checker_);
 
   scoped_refptr<update_client::Configurator> config_;
+  scoped_refptr<PersistedData> persisted_data_;
   scoped_refptr<base::SequencedTaskRunner> main_task_runner_;
   scoped_refptr<update_client::UpdateClient> update_client_;
 };

chrome/updater/win/BUILD.gn

@@ -139,6 +139,7 @@ source_set("install_app") {
     "//base:i18n",
     "//chrome/updater:base",
     "//chrome/updater:lib",
+    "//chrome/updater:version_header",
     "//chrome/updater/win/ui",
     "//components/prefs",
     "//components/update_client",

chrome/updater/win/install_app.cc

@@ -29,10 +29,13 @@
 #include "base/threading/thread_checker.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/version.h"
 #include "base/win/atl.h"
 #include "chrome/updater/configurator.h"
 #include "chrome/updater/constants.h"
 #include "chrome/updater/installer.h"
+#include "chrome/updater/persisted_data.h"
+#include "chrome/updater/updater_version.h"
 #include "chrome/updater/win/install_progress_observer.h"
 #include "chrome/updater/win/setup/setup.h"
 #include "chrome/updater/win/ui/progress_wnd.h"
@@ -118,56 +121,51 @@ class InstallProgressObserverIPC : public InstallProgressObserver {
 
   struct ParamOnUpdateAvailable {
     ParamOnUpdateAvailable();
+    ParamOnUpdateAvailable(const ParamOnUpdateAvailable&) = delete;
+    ParamOnUpdateAvailable& operator=(const ParamOnUpdateAvailable&) = delete;
+
     base::string16 app_id;
     base::string16 app_name;
     base::string16 version_string;
-
-   private:
-    ParamOnUpdateAvailable(const ParamOnUpdateAvailable&) = delete;
-    ParamOnUpdateAvailable& operator=(const ParamOnUpdateAvailable&) = delete;
   };
 
   struct ParamOnDownloading {
     ParamOnDownloading();
+    ParamOnDownloading(const ParamOnDownloading&) = delete;
+    ParamOnDownloading& operator=(const ParamOnDownloading&) = delete;
+
     base::string16 app_id;
     base::string16 app_name;
     int time_remaining_ms = 0;
     int pos = 0;
-
-   private:
-    ParamOnDownloading(const ParamOnDownloading&) = delete;
-    ParamOnDownloading& operator=(const ParamOnDownloading&) = delete;
   };
 
   struct ParamOnWaitingToInstall {
     ParamOnWaitingToInstall();
-    base::string16 app_id;
-    base::string16 app_name;
-
-   private:
     ParamOnWaitingToInstall(const ParamOnWaitingToInstall&) = delete;
     ParamOnWaitingToInstall& operator=(const ParamOnWaitingToInstall&) = delete;
+
+    base::string16 app_id;
+    base::string16 app_name;
   };
 
   struct ParamOnInstalling {
     ParamOnInstalling();
+    ParamOnInstalling(const ParamOnInstalling&) = delete;
+    ParamOnInstalling& operator=(const ParamOnInstalling&) = delete;
+
     base::string16 app_id;
     base::string16 app_name;
     int time_remaining_ms = 0;
     int pos = 0;
-
-   private:
-    ParamOnInstalling(const ParamOnInstalling&) = delete;
-    ParamOnInstalling& operator=(const ParamOnInstalling&) = delete;
   };
 
   struct ParamOnComplete {
     ParamOnComplete();
-    ObserverCompletionInfo observer_info;
-
-   private:
     ParamOnComplete(const ParamOnComplete&) = delete;
     ParamOnComplete& operator=(const ParamOnComplete&) = delete;
+
+    ObserverCompletionInfo observer_info;
   };
 
   THREAD_CHECKER(thread_checker_);
@@ -389,8 +387,11 @@ class InstallAppController : public ui::ProgressWndEvents,
                              public update_client::UpdateClient::Observer,
                              public WTL::CMessageFilter {
  public:
-  InstallAppController();
+  explicit InstallAppController(
+      scoped_refptr<update_client::Configurator> configurator);
   ~InstallAppController() override;
+  InstallAppController(const InstallAppController&) = delete;
+  InstallAppController& operator=(const InstallAppController&) = delete;
 
   int InstallApp(const std::string& app_id);
 
@@ -423,7 +424,7 @@ class InstallAppController : public ui::ProgressWndEvents,
   void RunUI();
 
   // These functions are called on the main updater thread.
-  void DoInstallApp(update_client::CrxComponent component);
+  void DoInstallApp();
   void InstallComplete();
   void HandleInstallResult(Events event,
                            const update_client::CrxUpdateItem& update_item);
@@ -452,6 +453,7 @@ class InstallAppController : public ui::ProgressWndEvents,
 
   // The |update_client| objects and dependencies.
   scoped_refptr<update_client::Configurator> config_;
+  scoped_refptr<PersistedData> persisted_data_;
   scoped_refptr<update_client::UpdateClient> update_client_;
   std::unique_ptr<Observer> observer_;
 
@@ -464,68 +466,44 @@ class InstallAppController : public ui::ProgressWndEvents,
   // The adapter for the inter-thread calls between the updater main thread
   // and the UI thread.
   std::unique_ptr<InstallProgressObserverIPC> install_progress_observer_ipc_;
-
-  InstallAppController(const InstallAppController&) = delete;
-  InstallAppController& operator=(const InstallAppController&) = delete;
 };
 
 // TODO(sorin): fix the hardcoding of the application name.
 // https:crbug.com/1014298
-InstallAppController::InstallAppController()
+InstallAppController::InstallAppController(
+    scoped_refptr<update_client::Configurator> configurator)
     : main_task_runner_(base::SequencedTaskRunnerHandle::Get()),
       ui_task_runner_(base::ThreadPool::CreateSingleThreadTaskRunner(
           {base::TaskPriority::USER_BLOCKING,
            base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
           base::SingleThreadTaskRunnerThreadMode::DEDICATED)),
       app_name_(kAppNameChrome),
-      config_(base::MakeRefCounted<Configurator>()) {}
+      config_(configurator),
+      persisted_data_(
+          base::MakeRefCounted<PersistedData>(config_->GetPrefService())) {}
 
 InstallAppController::~InstallAppController() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  FlushPrefs();
 }
 
 int InstallAppController::InstallApp(const std::string& app_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(base::ThreadTaskRunnerHandle::IsSet());
 
-  base::i18n::InitializeICU();
-  base::ScopedDisallowBlocking no_blocking_allowed_on_main_thread;
-
   app_id_ = app_id;
 
-  // Creates a CRX installer bound to this |app_id| on a blocking task runner,
-  // since such code requires accessing the file system. Once this task
-  // completes, the reply initializes the UI code on the UI thread, and then,
-  // it invokes |DoInstallApp| on the main updater thread.
-  base::ThreadPool::PostTaskAndReplyWithResult(
-      FROM_HERE,
-      {base::MayBlock(), base::TaskPriority::USER_BLOCKING,
-       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN},
-      base::BindOnce(
-          [](const std::string& app_id) {
-            auto installer = base::MakeRefCounted<Installer>(app_id);
-            installer->FindInstallOfApp();
-            return installer->MakeCrxComponent();
-          },
-          app_id_),
-      base::BindOnce(
-          [](InstallAppController* controller,
-             update_client::CrxComponent component) {
-            controller->ui_task_runner_->PostTaskAndReply(
+  ui_task_runner_->PostTaskAndReply(
       FROM_HERE,
       base::BindOnce(&InstallAppController::InitializeUI,
-                               base::Unretained(controller)),
+                     base::Unretained(this)),
       base::BindOnce(&InstallAppController::DoInstallApp,
-                               base::Unretained(controller),
-                               std::move(component)));
-          },
-          this));
+                     base::Unretained(this)));
   runloop_.Run();
-  FlushPrefs();
   return 0;
 }
 
-void InstallAppController::DoInstallApp(update_client::CrxComponent component) {
+void InstallAppController::DoInstallApp() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // At this point, the UI has been initialized, which means the UI can be
@@ -545,13 +523,14 @@ void InstallAppController::DoInstallApp(update_client::CrxComponent component) {
   update_client_->Install(
       app_id_,
       base::BindOnce(
-          [](const update_client::CrxComponent& component,
+          [](scoped_refptr<PersistedData> persisted_data,
              const std::vector<std::string>& ids)
               -> std::vector<base::Optional<update_client::CrxComponent>> {
             DCHECK_EQ(1u, ids.size());
-            return {component};
+            return {base::MakeRefCounted<Installer>(ids[0], persisted_data)
+                        ->MakeCrxComponent()};
           },
-          std::move(component)),
+          persisted_data_),
       base::BindOnce(
           [](InstallAppController* install_app_controller,
              update_client::Error error) {
@@ -733,8 +712,6 @@ DWORD InstallAppController::GetUIThreadID() const {
 }  // namespace
 
 int SetupUpdater() {
-  base::ScopedDisallowBlocking no_blocking_allowed;
-
   DCHECK(base::ThreadTaskRunnerHandle::IsSet());
 
   ui::SplashScreen splash_screen(kAppNameChrome);
@@ -758,21 +735,28 @@ int SetupUpdater() {
                 std::move(quit_closure)));
           },
           &splash_screen, runloop.QuitWhenIdleClosure(), &setup_result));
-
   runloop.Run();
-
   return setup_result;
 }
 
 int InstallApp(const std::string& app_id) {
+  base::i18n::InitializeICU();
+  auto config = base::MakeRefCounted<Configurator>();
+
   // Use MessagePumpType::UI to handle native window messages on the main
   // thread of the updater.
   base::SingleThreadTaskExecutor main_task_executor(base::MessagePumpType::UI);
+  base::ScopedDisallowBlocking no_blocking_allowed_on_main_thread;
+
   int result = SetupUpdater();
-  if (result)
+  if (result != 0)
     return result;
 
-  return InstallAppController().InstallApp(app_id);
+  // Register the updater app id for updates.
+  base::MakeRefCounted<PersistedData>(config->GetPrefService())
+      ->SetProductVersion(kUpdaterAppId, base::Version(UPDATER_VERSION_STRING));
+
+  return InstallAppController(config).InstallApp(app_id);
 }
 
 }  // namespace updater

components/update_client/persisted_data.h

@@ -103,7 +103,8 @@ class PersistedData {
   int GetDaysSinceLastRollCall(const std::string& id) const;
   int GetDaysSinceLastActive(const std::string& id) const;
 
-  // These functions access |pv| data for the specified |id|.
+  // These functions access |pv| data for the specified |id|. Returns an empty
+  // version, if the version is not found.
   base::Version GetProductVersion(const std::string& id) const;
   void SetProductVersion(const std::string& id, const base::Version& pv);