Fix --install for Windows.

After introducing the App instance, --install is broken on Windows.
* pv is not handled correctly and it is not clear if this ever worked at
all, even before refactoring.
* the splash screen object has an incorrect life time.
* the prefs are not save correctly due to threading and run loop issues.

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

chrome/updater/app/app.cc

@@ -37,17 +37,21 @@ int App::Run() {
   base::ThreadPoolInstance::CreateAndStartWithDefaultParams("Updater");
   base::SingleThreadTaskExecutor main_task_executor(base::MessagePumpType::UI);
   Initialize();
-  base::RunLoop runloop;
-  base::ScopedDisallowBlocking no_blocking_allowed_on_ui_thread;
   int exit_code = 0;
-  quit_ = base::BindOnce(
-      [](base::OnceClosure quit, int* exit_code_out, int exit_code) {
-        *exit_code_out = exit_code;
-        std::move(quit).Run();
-      },
-      runloop.QuitWhenIdleClosure(), &exit_code);
-  FirstTaskRun();
-  runloop.Run();
+  {
+    base::ScopedDisallowBlocking no_blocking_allowed_on_ui_thread;
+    base::RunLoop runloop;
+    quit_ = base::BindOnce(
+        [](base::OnceClosure quit, int* exit_code_out, int exit_code) {
+          *exit_code_out = exit_code;
+          std::move(quit).Run();
+        },
+        runloop.QuitWhenIdleClosure(), &exit_code);
+    FirstTaskRun();
+    runloop.Run();
+  }
+
+  // Shutting down the thread pool involves joining threads.
   base::ThreadPoolInstance::Get()->Shutdown();
   return exit_code;
 }

chrome/updater/constants.h

@@ -15,6 +15,8 @@ extern const char kUpdaterAppId[];
 // Chrome's app ID.
 extern const char kChromeAppId[];
 
+// "0.0.0.0". Historically, a null version has been used to indicate a
+// new install.
 extern const char kNullVersion[];
 
 // Command line switches.

chrome/updater/installer.cc

@@ -61,9 +61,12 @@ update_client::CrxComponent Installer::MakeCrxComponent() {
   // |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())
+  if (pv.IsValid()) {
     pv_ = pv;
-  fingerprint_ = persisted_data_->GetFingerprint(app_id_);
+    fingerprint_ = persisted_data_->GetFingerprint(app_id_);
+  } else {
+    pv_ = base::Version(kNullVersion);
+  }
 
   update_client::CrxComponent component;
   component.installer = scoped_refptr<Installer>(this);

chrome/updater/win/install_app.cc

@@ -487,7 +487,6 @@ InstallAppController::InstallAppController(
 
 InstallAppController::~InstallAppController() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  FlushPrefs();
 }
 
 void InstallAppController::InstallApp(const std::string& app_id) {
@@ -550,7 +549,7 @@ void InstallAppController::DoInstallApp() {
 // by calling UpdateClient::GetCrxUpdateState.
 void InstallAppController::InstallComplete() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-
+  FlushPrefs();
   install_progress_observer_ipc_ = nullptr;
   update_client_->RemoveObserver(this);
   update_client_ = nullptr;
@@ -692,11 +691,7 @@ BOOL InstallAppController::PreTranslateMessage(MSG* msg) {
 
 void InstallAppController::FlushPrefs() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  base::RunLoop runloop;
-  config_->GetPrefService()->CommitPendingWrite(base::BindOnce(
-      [](base::OnceClosure quit_closure) { std::move(quit_closure).Run(); },
-      runloop.QuitWhenIdleClosure()));
-  runloop.Run();
+  config_->GetPrefService()->SchedulePendingLossyWrites();
 }
 
 DWORD InstallAppController::GetUIThreadID() const {
@@ -706,6 +701,7 @@ DWORD InstallAppController::GetUIThreadID() const {
 
 }  // namespace
 
+// Installs the updater and one application specified by |app_id|.
 class AppInstall : public App {
  public:
   explicit AppInstall(const std::string& app_id);
@@ -720,6 +716,10 @@ class AppInstall : public App {
   std::string app_id_;
   scoped_refptr<Configurator> config_;
   std::unique_ptr<InstallAppController> app_install_controller_;
+
+  // The splash screen has a fading effect. That means that the splash screen
+  // needs to be alive for a while, until the fading effect is over.
+  std::unique_ptr<ui::SplashScreen> splash_screen_;
 };
 
 AppInstall::AppInstall(const std::string& app_id) : app_id_(app_id) {}
@@ -732,8 +732,8 @@ void AppInstall::Initialize() {
 void AppInstall::FirstTaskRun() {
   DCHECK(base::ThreadTaskRunnerHandle::IsSet());
 
-  ui::SplashScreen splash_screen(kAppNameChrome);
-  splash_screen.Show();
+  splash_screen_ = std::make_unique<ui::SplashScreen>(kAppNameChrome);
+  splash_screen_->Show();
 
   base::ThreadPool::PostTaskAndReplyWithResult(
       FROM_HERE,
@@ -745,7 +745,7 @@ void AppInstall::FirstTaskRun() {
              base::OnceCallback<void(int)> done, int result) {
             splash_screen->Dismiss(base::BindOnce(std::move(done), result));
           },
-          &splash_screen, base::BindOnce(&AppInstall::SetupDone, this)));
+          splash_screen_.get(), base::BindOnce(&AppInstall::SetupDone, this)));
 }
 
 void AppInstall::SetupDone(int result) {

chrome/updater/win/ui/splash_screen.cc

@@ -41,6 +41,9 @@ SplashScreen::SplashScreen(const base::string16& bundle_name)
 
 SplashScreen::~SplashScreen() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  // TODO(crbug.com/1059094) this assert may fire when the dtor is called
+  // while the window is fading out.
   DCHECK(state_ == WindowState::STATE_CREATED ||
          state_ == WindowState::STATE_CLOSED);
 }
@@ -58,6 +61,7 @@ void SplashScreen::Show() {
 }
 
 void SplashScreen::Dismiss(base::OnceClosure on_close_closure) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   on_close_closure_ = std::move(on_close_closure);
   switch (state_) {
     case WindowState::STATE_CREATED: