Refactor how committing prefs occurs in the updater.

Updating and committing prefs must happen on the same sequence while
the thread pool is still active.

This CL introduces an updater::App::Uninitialize virtual to provide a
call site for committing prefs and possibly other code that needs to
run when the updater process is shutting down.

BUG: 1061730
Change-Id: I0c293ced0e5c3ce66f921d1d65062de89f78bfc8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2103755Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Sorin Jianu <sorin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750955}

chrome/updater/BUILD.gn

@@ -179,6 +179,7 @@ if (is_win || is_mac) {
 
     sources = [
       "persisted_data_unittest.cc",
+      "prefs_unittest.cc",
       "run_all_unittests.cc",
       "updater_unittest.cc",
     ]

chrome/updater/app/app.cc

@@ -37,6 +37,8 @@ int App::Run() {
     runloop.Run();
   }
 
+  Uninitialize();
+
   // Shutting down the thread pool involves joining threads.
   base::ThreadPoolInstance::Get()->Shutdown();
   return exit_code;
@@ -46,6 +48,4 @@ void App::Shutdown(int exit_code) {
   std::move(quit_).Run(exit_code);
 }
 
-void App::Initialize() {}
-
 }  // namespace updater

chrome/updater/app/app.h

@@ -29,9 +29,12 @@ class App : public base::RefCountedThreadSafe<App> {
 
  private:
   // Implementations of App can override this to perform work on the main
-  // sequence while blocking is still allowed. The default implementation does
-  // nothing.
-  virtual void Initialize();
+  // sequence while blocking is still allowed.
+  virtual void Initialize() {}
+
+  // Called on the main sequence while blocking is allowed and before
+  // shutting down the thread pool.
+  virtual void Uninitialize() {}
 
   // Concrete implementations of App can execute their first task in this
   // method. It is called on the main sequence. Blocking is not allowed. It may

chrome/updater/app/app_update_all.cc

@@ -23,6 +23,7 @@ class AppUpdateAll : public App {
   // Overrides for App.
   void FirstTaskRun() override;
   void Initialize() override;
+  void Uninitialize() override;
 
   scoped_refptr<Configurator> config_;
   std::unique_ptr<UpdateService> update_service_;
@@ -32,6 +33,10 @@ void AppUpdateAll::Initialize() {
   config_ = base::MakeRefCounted<Configurator>();
 }
 
+void AppUpdateAll::Uninitialize() {
+  update_service_->Uninitialize();
+}
+
 // AppUpdateAll triggers an update of all registered applications.
 void AppUpdateAll::FirstTaskRun() {
   update_service_ = CreateUpdateService(config_);

chrome/updater/mac/update_service_out_of_process.h

@@ -45,6 +45,7 @@ class UpdateServiceOutOfProcess : public UpdateService {
               Priority priority,
               StateChangeCallback state_update,
               base::OnceCallback<void(Result)> done) override;
+  void Uninitialize() override;
 
  private:
   SEQUENCE_CHECKER(sequence_checker_);

chrome/updater/mac/update_service_out_of_process.mm

@@ -163,6 +163,10 @@ void UpdateServiceOutOfProcess::Update(const std::string& app_id,
                                    reply:reply];
 }
 
+void UpdateServiceOutOfProcess::Uninitialize() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
+
 UpdateServiceOutOfProcess::~UpdateServiceOutOfProcess() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 }

chrome/updater/prefs.cc

@@ -4,8 +4,12 @@
 
 #include "chrome/updater/prefs.h"
 
+#include <utility>
+
+#include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
+#include "base/run_loop.h"
 #include "chrome/updater/util.h"
 #include "components/prefs/json_pref_store.h"
 #include "components/prefs/pref_registry_simple.h"
@@ -30,4 +34,13 @@ std::unique_ptr<PrefService> CreatePrefService() {
   return pref_service_factory.Create(pref_registry);
 }
 
+void PrefsCommitPendingWrites(PrefService* pref_service) {
+  // Waits in the run loop until pending writes complete.
+  base::RunLoop runloop;
+  pref_service->CommitPendingWrite(base::BindOnce(
+      [](base::OnceClosure quit_closure) { std::move(quit_closure).Run(); },
+      runloop.QuitWhenIdleClosure()));
+  runloop.Run();
+}
+
 }  // namespace updater

chrome/updater/prefs.h

@@ -13,6 +13,11 @@ namespace updater {
 
 std::unique_ptr<PrefService> CreatePrefService();
 
+// Commits prefs changes to storage. This function should only be called
+// when the changes must be written immediately, for instance, during program
+// shutdown. The function must be called in the scope of a task executor.
+void PrefsCommitPendingWrites(PrefService* pref_service);
+
 }  // namespace updater
 
 #endif  // CHROME_UPDATER_PREFS_H_

chrome/updater/prefs_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 "base/test/task_environment.h"
+#include "chrome/updater/persisted_data.h"
+#include "chrome/updater/prefs.h"
+#include "chrome/updater/registration_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(PrefsTest, PrefsCommitPendingWrites) {
+  base::test::TaskEnvironment task_environment(
+      base::test::SingleThreadTaskEnvironment::MainThreadType::UI);
+  auto pref = std::make_unique<TestingPrefServiceSimple>();
+  update_client::RegisterPrefs(pref->registry());
+  auto metadata = base::MakeRefCounted<PersistedData>(pref.get());
+
+  // Writes something to prefs.
+  metadata->SetBrandCode("someappid", "brand");
+  EXPECT_STREQ(metadata->GetBrandCode("someappid").c_str(), "brand");
+
+  // Tests writing to storage completes.
+  PrefsCommitPendingWrites(pref.get());
+}
+
+}  // namespace updater

chrome/updater/update_service.h

@@ -105,6 +105,10 @@ class UpdateService {
           state_update,
       base::OnceCallback<void(Result)> done) = 0;
 
+  // Provides a way to commit data or clean up resources before the task
+  // scheduler is shutting down.
+  virtual void Uninitialize() = 0;
+
  protected:
   UpdateService() = default;
 };

chrome/updater/update_service_in_process.cc

@@ -18,6 +18,7 @@
 #include "chrome/updater/constants.h"
 #include "chrome/updater/installer.h"
 #include "chrome/updater/persisted_data.h"
+#include "chrome/updater/prefs.h"
 #include "chrome/updater/registration_data.h"
 #include "components/prefs/pref_service.h"
 #include "components/update_client/crx_update_item.h"
@@ -77,10 +78,16 @@ void UpdateServiceInProcess::Update(
     Priority priority,
     base::RepeatingCallback<void(UpdateState)> state_update,
     base::OnceCallback<void(Result)> done) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   // TODO(crbug.com/1059020): Implement.
   NOTREACHED();
 }
 
+void UpdateServiceInProcess::Uninitialize() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  PrefsCommitPendingWrites(config_->GetPrefService());
+}
+
 UpdateServiceInProcess::~UpdateServiceInProcess() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   config_->GetPrefService()->SchedulePendingLossyWrites();

chrome/updater/update_service_in_process.h

@@ -55,6 +55,8 @@ class UpdateServiceInProcess : public UpdateService {
               base::RepeatingCallback<void(UpdateState)> state_update,
               base::OnceCallback<void(Result)> done) override;
 
+  void Uninitialize() override;
+
  private:
   SEQUENCE_CHECKER(sequence_checker_);
 

chrome/updater/win/install_app.cc

@@ -37,6 +37,7 @@
 #include "chrome/updater/constants.h"
 #include "chrome/updater/installer.h"
 #include "chrome/updater/persisted_data.h"
+#include "chrome/updater/prefs.h"
 #include "chrome/updater/updater_version.h"
 #include "chrome/updater/win/install_progress_observer.h"
 #include "chrome/updater/win/setup/setup.h"
@@ -699,6 +700,7 @@ class AppInstall : public App {
  private:
   ~AppInstall() override = default;
   void Initialize() override;
+  void Uninitialize() override;
   void FirstTaskRun() override;
 
   void SetupDone(int result);
@@ -719,6 +721,10 @@ void AppInstall::Initialize() {
   config_ = base::MakeRefCounted<Configurator>();
 }
 
+void AppInstall::Uninitialize() {
+  PrefsCommitPendingWrites(config_->GetPrefService());
+}
+
 void AppInstall::FirstTaskRun() {
   DCHECK(base::ThreadTaskRunnerHandle::IsSet());
 

chrome/updater/win/update_service_out_of_process.cc

@@ -81,6 +81,10 @@ void UpdateServiceOutOfProcess::Update(const std::string& app_id,
   NOTREACHED();
 }
 
+void UpdateServiceOutOfProcess::Uninitialize() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+}
+
 void UpdateServiceOutOfProcess::UpdateAllOnSTA(
     base::OnceCallback<void(Result)> callback) {
   DCHECK(com_task_runner_->BelongsToCurrentThread());

chrome/updater/win/update_service_out_of_process.h

@@ -67,6 +67,7 @@ class UpdateServiceOutOfProcess : public UpdateService {
               Priority priority,
               StateChangeCallback state_update,
               base::OnceCallback<void(Result)> done) override;
+  void Uninitialize() override;
 
  private:
   UpdateServiceOutOfProcess();