[Sheriff] Revert "Register the Updater and Chrome app ids with the //chrome/updater."

This reverts commit afcc5be9.

Reason for revert: Appears to have closed the tree by breaking "compile":
see https://ci.chromium.org/p/chromium/builders/ci/Win%20Builder/89067?:
FAILED: obj/chrome/updater/base/installer.obj 
../../chrome/updater/installer.cc(52,10): error: use of undeclared identifier 'sequence_checker_'
  DCHECK(sequence_checker_.CalledOnValidSequence());
         ^
../../chrome/updater/installer.cc(56,10): error: use of undeclared identifier 'sequence_checker_'
  DCHECK(sequence_checker_.CalledOnValidSequence());
         ^

Original change's description:
> 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}

TBR=sorin@chromium.org,waffles@chromium.org

Change-Id: I0e270d1c6b4fc855f0c4686ee6de0551dbfcbcd1
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1020285
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2078844Reviewed-by: Katie Dektar <katie@chromium.org>
Commit-Queue: Katie Dektar <katie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#745130}

chrome/updater/BUILD.gn

@@ -42,8 +42,6 @@ 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",
@@ -130,20 +128,17 @@ if (is_win || is_mac) {
     testonly = true
 
     sources = [
-      "persisted_data_unittest.cc",
       "run_all_unittests.cc",
       "updater_unittest.cc",
     ]
 
     deps = [
-      ":base",
+      ":lib",
       ":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,6 +10,7 @@
 #include <vector>
 
 #include "base/containers/flat_map.h"
+#include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "components/update_client/configurator.h"
 
@@ -31,8 +32,6 @@ 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;
@@ -71,6 +70,7 @@ 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,7 +9,6 @@ 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,8 +15,6 @@ extern const char kUpdaterAppId[];
 // Chrome's app ID.
 extern const char kChromeAppId[];
 
-extern const char kNullVersion[];
-
 // Command line switches.
 //
 

chrome/updater/installer.cc

@@ -5,7 +5,6 @@
 #include "chrome/updater/installer.h"
 
 #include <utility>
-#include <vector>
 
 #include "base/callback.h"
 #include "base/files/file_enumerator.h"
@@ -25,6 +24,9 @@ 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(),
@@ -44,26 +46,15 @@ base::FilePath GetAppInstallDir(const std::string& app_id) {
 
 }  // namespace
 
-Installer::Installer(const std::string& app_id,
-                     scoped_refptr<PersistedData> persisted_data)
-    : app_id_(app_id), persisted_data_(persisted_data) {}
-
-Installer::~Installer() {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
-}
-
-update_client::CrxComponent Installer::MakeCrxComponent() {
-  DCHECK(sequence_checker_.CalledOnValidSequence());
+Installer::InstallInfo::InstallInfo() : version(kNullVersion) {}
+Installer::InstallInfo::~InstallInfo() = default;
 
-  VLOG(1) << __func__ << " for " << app_id_;
+Installer::Installer(const std::string& app_id)
+    : app_id_(app_id), install_info_(std::make_unique<InstallInfo>()) {}
 
-  // |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_);
+Installer::~Installer() = default;
 
+update_client::CrxComponent Installer::MakeCrxComponent() {
   update_client::CrxComponent component;
   component.installer = scoped_refptr<Installer>(this);
   component.action_handler = MakeActionHandler();
@@ -72,40 +63,76 @@ update_client::CrxComponent Installer::MakeCrxComponent() {
       crx_file::VerifierFormat::CRX3_WITH_PUBLISHER_PROOF;
   component.app_id = app_id_;
   component.name = app_id_;
-  component.version = pv_;
-  component.fingerprint = fingerprint_;
+  component.version = install_info_->version;
+  component.fingerprint = install_info_->fingerprint;
   return component;
 }
 
-void Installer::DeleteOlderInstallPaths() {
-  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
-                                                base::BlockingType::WILL_BLOCK);
+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_;
 
   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_dir(path.BaseName().MaybeAsASCII());
+    const base::Version version(path.BaseName().MaybeAsASCII());
+
+    // Ignore folders that don't have valid version names.
+    if (!version.IsValid())
+      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);
+    // 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;
     }
+
+    // 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);
@@ -114,12 +141,13 @@ Installer::Result Installer::InstallHelper(
   local_manifest->GetStringASCII("version", &version_ascii);
   const base::Version manifest_version(version_ascii);
 
-  VLOG(1) << "Installed version=" << pv_
+  VLOG(1) << "Installed version=" << install_info_->version.GetString()
           << ", installing version=" << manifest_version.GetString();
+
   if (!manifest_version.IsValid())
     return Result(update_client::InstallError::INVALID_VERSION);
 
-  if (pv_.CompareTo(manifest_version) > 0)
+  if (install_info_->version.CompareTo(manifest_version) > 0)
     return Result(update_client::InstallError::VERSION_NOT_UPGRADED);
 
   const base::FilePath app_install_dir = GetAppInstallDir(app_id_);
@@ -150,26 +178,25 @@ 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);
-
-  // Assume the install params are ASCII for now.
-  const auto application_installer =
-      versioned_install_dir.AppendASCII(install_params->run);
-  if (!base::PathExists(application_installer))
+  base::FilePath application_installer;
+  if (!GetInstalledFile(install_params->run, &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);
 }
@@ -181,7 +208,6 @@ 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);
@@ -204,16 +230,10 @@ void Installer::Install(const base::FilePath& unpack_path,
 
 bool Installer::GetInstalledFile(const std::string& file,
                                  base::FilePath* installed_file) {
-  base::ScopedBlockingCall scoped_blocking_call(FROM_HERE,
-                                                base::BlockingType::WILL_BLOCK);
-  if (pv_ == base::Version(kNullVersion))
+  if (install_info_->version == base::Version(kNullVersion))
     return false;  // No component has been installed yet.
 
-  const auto install_dir = GetCurrentInstallDir();
-  if (install_dir.empty())
-    return false;
-
-  *installed_file = install_dir.AppendASCII(file);
+  *installed_file = install_info_->install_dir.AppendASCII(file);
   return true;
 }
 
@@ -221,12 +241,6 @@ 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,51 +5,50 @@
 #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:
-  Installer(const std::string& app_id,
-            scoped_refptr<PersistedData> persisted_data);
-  Installer(const Installer&) = delete;
-  Installer& operator=(const Installer&) = delete;
+  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);
 
   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. 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|.
+  // state of the app.
   update_client::CrxComponent MakeCrxComponent();
 
  private:
@@ -84,20 +83,10 @@ 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_;
-  scoped_refptr<PersistedData> persisted_data_;
+  std::unique_ptr<InstallInfo> install_info_;
 
-  // These members are not updated when the installer succeeds.
-  base::Version pv_;
-  std::string fingerprint_;
+  DISALLOW_COPY_AND_ASSIGN(Installer);
 };
 
 }  // 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,10 +4,6 @@
 
 #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"
@@ -18,7 +14,6 @@
 #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"
@@ -30,8 +25,6 @@ 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)) {}
 
@@ -50,23 +43,32 @@ void UpdateServiceInProcess::UpdateAll(
     base::OnceCallback<void(update_client::Error)> callback) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  const auto app_ids = persisted_data_->GetAppIds();
-  DCHECK(base::Contains(app_ids, kUpdaterAppId));
+  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());
+  }
 
   update_client_->Update(
       app_ids,
       base::BindOnce(
-          [](scoped_refptr<PersistedData> persisted_data,
+          [](const std::vector<base::Optional<update_client::CrxComponent>>&
+                 components,
              const std::vector<std::string>& ids) {
-            std::vector<base::Optional<update_client::CrxComponent>> components;
-            for (const auto& id : ids) {
-              components.push_back(
-                  base::MakeRefCounted<Installer>(id, persisted_data)
-                      ->MakeCrxComponent());
-            }
+            DCHECK_EQ(components.size(), ids.size());
             return components;
           },
-          persisted_data_),
+          components),
       false, std::move(callback));
 }
 

chrome/updater/update_service_in_process.h

@@ -5,9 +5,6 @@
 #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"
@@ -24,7 +21,6 @@ class UpdateClient;
 }  // namespace update_client
 
 namespace updater {
-class PersistedData;
 struct RegistrationRequest;
 struct RegistrationResponse;
 
@@ -54,7 +50,6 @@ 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,7 +139,6 @@ 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,13 +29,10 @@
 #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"
@@ -121,51 +118,56 @@ 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();
-    ParamOnWaitingToInstall(const ParamOnWaitingToInstall&) = delete;
-    ParamOnWaitingToInstall& operator=(const ParamOnWaitingToInstall&) = delete;
-
     base::string16 app_id;
     base::string16 app_name;
+
+   private:
+    ParamOnWaitingToInstall(const ParamOnWaitingToInstall&) = delete;
+    ParamOnWaitingToInstall& operator=(const ParamOnWaitingToInstall&) = delete;
   };
 
   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_);
@@ -387,11 +389,8 @@ class InstallAppController : public ui::ProgressWndEvents,
                              public update_client::UpdateClient::Observer,
                              public WTL::CMessageFilter {
  public:
-  explicit InstallAppController(
-      scoped_refptr<update_client::Configurator> configurator);
+  InstallAppController();
   ~InstallAppController() override;
-  InstallAppController(const InstallAppController&) = delete;
-  InstallAppController& operator=(const InstallAppController&) = delete;
 
   int InstallApp(const std::string& app_id);
 
@@ -424,7 +423,7 @@ class InstallAppController : public ui::ProgressWndEvents,
   void RunUI();
 
   // These functions are called on the main updater thread.
-  void DoInstallApp();
+  void DoInstallApp(update_client::CrxComponent component);
   void InstallComplete();
   void HandleInstallResult(Events event,
                            const update_client::CrxUpdateItem& update_item);
@@ -453,7 +452,6 @@ 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_;
 
@@ -466,44 +464,68 @@ 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(
-    scoped_refptr<update_client::Configurator> configurator)
+InstallAppController::InstallAppController()
     : 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_(configurator),
-      persisted_data_(
-          base::MakeRefCounted<PersistedData>(config_->GetPrefService())) {}
+      config_(base::MakeRefCounted<Configurator>()) {}
 
 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;
 
-  ui_task_runner_->PostTaskAndReply(
+  // 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(
                 FROM_HERE,
                 base::BindOnce(&InstallAppController::InitializeUI,
-                     base::Unretained(this)),
+                               base::Unretained(controller)),
                 base::BindOnce(&InstallAppController::DoInstallApp,
-                     base::Unretained(this)));
+                               base::Unretained(controller),
+                               std::move(component)));
+          },
+          this));
   runloop_.Run();
+  FlushPrefs();
   return 0;
 }
 
-void InstallAppController::DoInstallApp() {
+void InstallAppController::DoInstallApp(update_client::CrxComponent component) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
   // At this point, the UI has been initialized, which means the UI can be
@@ -523,14 +545,13 @@ void InstallAppController::DoInstallApp() {
   update_client_->Install(
       app_id_,
       base::BindOnce(
-          [](scoped_refptr<PersistedData> persisted_data,
+          [](const update_client::CrxComponent& component,
              const std::vector<std::string>& ids)
               -> std::vector<base::Optional<update_client::CrxComponent>> {
             DCHECK_EQ(1u, ids.size());
-            return {base::MakeRefCounted<Installer>(ids[0], persisted_data)
-                        ->MakeCrxComponent()};
+            return {component};
           },
-          persisted_data_),
+          std::move(component)),
       base::BindOnce(
           [](InstallAppController* install_app_controller,
              update_client::Error error) {
@@ -712,6 +733,8 @@ DWORD InstallAppController::GetUIThreadID() const {
 }  // namespace
 
 int SetupUpdater() {
+  base::ScopedDisallowBlocking no_blocking_allowed;
+
   DCHECK(base::ThreadTaskRunnerHandle::IsSet());
 
   ui::SplashScreen splash_screen(kAppNameChrome);
@@ -735,28 +758,21 @@ 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 != 0)
+  if (result)
     return result;
 
-  // 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);
+  return InstallAppController().InstallApp(app_id);
 }
 
 }  // namespace updater

components/update_client/persisted_data.h

@@ -103,8 +103,7 @@ 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|. Returns an empty
-  // version, if the version is not found.
+  // These functions access |pv| data for the specified |id|.
   base::Version GetProductVersion(const std::string& id) const;
   void SetProductVersion(const std::string& id, const base::Version& pv);