Abandon installation after 3 failures at the same version

This change is to avoid bootlooping on SWA installation. With this
change, we will attempt the same version 3 times and then stop
attempting reinstallation. We will try installation again when a
new version is pushed.
Formatting fixes

Fix up the tests

Abandon failed SWA install after 3 at same version

First test code

Bug: 1095523
Change-Id: I3139652767d94586e5dd6fc0c8070eae8a25cce5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2268207
Commit-Queue: Dominic Schulz <dominicschulz@google.com>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#791837}

chrome/browser/web_applications/components/pending_app_manager.cc

@@ -24,8 +24,9 @@ PendingAppManager::SynchronizeRequest::SynchronizeRequest(
 
 PendingAppManager::SynchronizeRequest::~SynchronizeRequest() = default;
 
-PendingAppManager::SynchronizeRequest& PendingAppManager::SynchronizeRequest::
-operator=(PendingAppManager::SynchronizeRequest&&) = default;
+PendingAppManager::SynchronizeRequest&
+PendingAppManager::SynchronizeRequest::operator=(
+    PendingAppManager::SynchronizeRequest&&) = default;
 
 PendingAppManager::SynchronizeRequest::SynchronizeRequest(
     SynchronizeRequest&& other) = default;
@@ -165,5 +166,9 @@ void PendingAppManager::OnAppSynchronized(ExternalInstallSource source,
     synchronize_requests_.erase(source);
   }
 }
+void PendingAppManager::ClearSynchronizeRequestsForTesting() {
+  synchronize_requests_.erase(synchronize_requests_.begin(),
+                              synchronize_requests_.end());
+}
 
 }  // namespace web_app

chrome/browser/web_applications/components/pending_app_manager.h

@@ -114,6 +114,7 @@ class PendingAppManager {
 
   void SetRegistrationCallbackForTesting(RegistrationCallback callback);
   void ClearRegistrationCallbackForTesting();
+  void ClearSynchronizeRequestsForTesting();
 
   virtual void Shutdown() = 0;
 

chrome/browser/web_applications/system_web_app_manager.cc

@@ -68,6 +68,10 @@ namespace {
 // complex dependencies.
 const char kFileHandlingOriginTrial[] = "FileHandling";
 
+// Number of attempts to install a given version & locale of the SWAs before
+// bailing out.
+const int kInstallFailureAttempts = 3;
+
 // Use #if defined to avoid compiler error on unused function.
 #if defined(OS_CHROMEOS)
 
@@ -361,21 +365,29 @@ void SystemWebAppManager::Start() {
 #endif  // defined(OS_CHROMEOS)
 
   std::vector<ExternalInstallOptions> install_options_list;
+  const bool needs_update = NeedsUpdate();
+  if (needs_update) {
+    UpdateLastAttemptedInfo();
+  }
   if (IsEnabled()) {
     const auto disabled_system_apps = GetDisabledSystemWebApps();
 
     // Skipping this will uninstall all System Apps currently installed.
     for (const auto& app : system_app_infos_) {
       install_options_list.push_back(CreateInstallOptionsForSystemApp(
-          app.second, NeedsUpdate(),
+          app.second, needs_update,
           base::Contains(disabled_system_apps, app.first)));
     }
   }
-  pending_app_manager_->SynchronizeInstalledApps(
-      std::move(install_options_list), ExternalInstallSource::kSystemInstalled,
-      base::BindOnce(&SystemWebAppManager::OnAppsSynchronized,
-                     weak_ptr_factory_.GetWeakPtr(), install_start_time));
 
+  const bool exceeded_retries = CheckAndIncrementRetryAttempts();
+  if (!exceeded_retries) {
+    pending_app_manager_->SynchronizeInstalledApps(
+        std::move(install_options_list),
+        ExternalInstallSource::kSystemInstalled,
+        base::BindOnce(&SystemWebAppManager::OnAppsSynchronized,
+                       weak_ptr_factory_.GetWeakPtr(), install_start_time));
+  }
 #if defined(OS_CHROMEOS)
   PrefService* const local_state = g_browser_process->local_state();
   if (local_state) {  // Sometimes it's not available in tests.
@@ -552,6 +564,9 @@ void SystemWebAppManager::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterStringPref(prefs::kSystemWebAppLastUpdateVersion, "");
   registry->RegisterStringPref(prefs::kSystemWebAppLastInstalledLocale, "");
+  registry->RegisterStringPref(prefs::kSystemWebAppLastAttemptedVersion, "");
+  registry->RegisterStringPref(prefs::kSystemWebAppLastAttemptedLocale, "");
+  registry->RegisterIntegerPref(prefs::kSystemWebAppInstallFailureCount, 0);
 }
 
 const base::Version& SystemWebAppManager::CurrentVersion() const {
@@ -633,14 +648,13 @@ void SystemWebAppManager::OnAppsSynchronized(
   const base::TimeDelta install_duration =
       base::TimeTicks::Now() - install_start_time;
 
-  if (IsEnabled()) {
-    // TODO(qjw): Figure out where install_results come from, decide if
-    // installation failures need to be handled
-    pref_service_->SetString(prefs::kSystemWebAppLastUpdateVersion,
-                             CurrentVersion().GetString());
-    pref_service_->SetString(prefs::kSystemWebAppLastInstalledLocale,
-                             CurrentLocale());
-  }
+  // TODO(qjw): Figure out where install_results come from, decide if
+  // installation failures need to be handled
+  pref_service_->SetString(prefs::kSystemWebAppLastUpdateVersion,
+                           CurrentVersion().GetString());
+  pref_service_->SetString(prefs::kSystemWebAppLastInstalledLocale,
+                           CurrentLocale());
+  pref_service_->SetInteger(prefs::kSystemWebAppInstallFailureCount, 0);
 
   RecordSystemWebAppInstallMetrics(install_results, install_duration);
 
@@ -661,25 +675,67 @@ void SystemWebAppManager::OnAppsSynchronized(
 }
 
 bool SystemWebAppManager::NeedsUpdate() const {
-  if (update_policy_ == UpdatePolicy::kAlwaysUpdate)
-    return true;
-
-  base::Version last_update_version(
+  base::Version current_installed_version(
       pref_service_->GetString(prefs::kSystemWebAppLastUpdateVersion));
 
-  const std::string& last_installed_locale(
+  const std::string& current_installed_locale(
       pref_service_->GetString(prefs::kSystemWebAppLastInstalledLocale));
 
   // If Chrome version rolls back for some reason, ensure System Web Apps are
   // always in sync with Chrome version.
-  bool versionIsDifferent =
-      !last_update_version.IsValid() || last_update_version != CurrentVersion();
+  const bool versionIsDifferent = !current_installed_version.IsValid() ||
+                                  current_installed_version != CurrentVersion();
 
   // If system language changes, ensure System Web Apps launcher localization
   // are in sync with current language.
-  bool localeIsDifferent = last_installed_locale != CurrentLocale();
+  const bool localeIsDifferent = current_installed_locale != CurrentLocale();
+
+  const bool should_update = versionIsDifferent || localeIsDifferent;
+
+  if (should_update) {
+    return true;
+  }
+
+  if (update_policy_ == UpdatePolicy::kAlwaysUpdate) {
+    return true;
+  }
+
+  return false;
+}
 
-  return versionIsDifferent || localeIsDifferent;
+void SystemWebAppManager::UpdateLastAttemptedInfo() {
+  base::Version last_attempted_version(
+      pref_service_->GetString(prefs::kSystemWebAppLastAttemptedVersion));
+
+  const std::string& last_attempted_locale(
+      pref_service_->GetString(prefs::kSystemWebAppLastAttemptedLocale));
+
+  const bool is_retry = last_attempted_version.IsValid() &&
+                        last_attempted_version == CurrentVersion() &&
+                        last_attempted_locale == CurrentLocale();
+  if (!is_retry) {
+    pref_service_->SetInteger(prefs::kSystemWebAppInstallFailureCount, 0);
+  }
+
+  pref_service_->SetString(prefs::kSystemWebAppLastAttemptedVersion,
+                           CurrentVersion().GetString());
+  pref_service_->SetString(prefs::kSystemWebAppLastAttemptedLocale,
+                           CurrentLocale());
+  pref_service_->CommitPendingWrite();
+}
+
+bool SystemWebAppManager::CheckAndIncrementRetryAttempts() {
+  int installation_failures =
+      pref_service_->GetInteger(prefs::kSystemWebAppInstallFailureCount);
+  bool reached_retry_limit = installation_failures > kInstallFailureAttempts;
+
+  if (!reached_retry_limit) {
+    pref_service_->SetInteger(prefs::kSystemWebAppInstallFailureCount,
+                              installation_failures + 1);
+    pref_service_->CommitPendingWrite();
+    return false;
+  }
+  return true;
 }
 
 void SystemWebAppManager::OnAppsPolicyChanged() {

chrome/browser/web_applications/system_web_app_manager.h

@@ -233,6 +233,10 @@ class SystemWebAppManager {
                           std::map<GURL, InstallResultCode> install_results,
                           std::map<GURL, bool> uninstall_results);
   bool NeedsUpdate() const;
+  void UpdateLastAttemptedInfo();
+  // Returns if we have exceeded the number of retry attempts allowed for this
+  // version.
+  bool CheckAndIncrementRetryAttempts();
 
   void RecordSystemWebAppInstallMetrics(
       const std::map<GURL, InstallResultCode>& install_results,

chrome/browser/web_applications/system_web_app_manager_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/callback.h"
 #include "base/run_loop.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task/task_traits.h"
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "chrome/browser/web_applications/components/externally_installed_web_app_prefs.h"
@@ -668,4 +669,225 @@ TEST_F(SystemWebAppManagerTest, UpdateOnLocaleChange) {
   EXPECT_FALSE(install_requests[2].force_reinstall);
 }
 
+TEST_F(SystemWebAppManagerTest, AbandonFailedInstalls) {
+  const std::vector<ExternalInstallOptions>& install_requests =
+      pending_app_manager().install_requests();
+
+  system_web_app_manager().SetUpdatePolicy(
+      SystemWebAppManager::UpdatePolicy::kOnVersionChange);
+
+  InitEmptyRegistrar();
+
+  PrepareSystemAppDataToRetrieve({{AppUrl1(), AppIconUrl1()}});
+  PrepareLoadUrlResults({AppUrl1()});
+  base::flat_map<SystemAppType, SystemAppInfo> system_apps;
+  system_apps.emplace(SystemAppType::SETTINGS,
+                      SystemAppInfo(kSettingsAppNameForLogging, AppUrl1()));
+  system_web_app_manager().SetSystemAppsForTesting(system_apps);
+
+  system_web_app_manager().set_current_version(base::Version("1.0.0.0"));
+  StartAndWaitForAppsToSynchronize();
+
+  EXPECT_EQ(1u, install_requests.size());
+  EXPECT_TRUE(install_requests[0].force_reinstall);
+  EXPECT_TRUE(IsInstalled(AppUrl1()));
+
+  // Bump the version number, and an update will trigger, and force
+  // reinstallation of both apps.
+  PrepareSystemAppDataToRetrieve({{AppUrl1(), AppIconUrl1()}});
+  PrepareLoadUrlResults({AppUrl1()});
+  system_web_app_manager().set_current_version(base::Version("2.0.0.0"));
+  pending_app_manager().SetDropRequestsForTesting(true);
+  // Can't use the normal method because RunLoop::Run goes until
+  // on_app_synchronized is called, and this fails, never calling that.
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+
+  // 1 successful, 1 abandoned, and 3 more abanonded retries is 5.
+  EXPECT_EQ(5u, install_requests.size());
+  EXPECT_TRUE(install_requests[1].force_reinstall);
+  EXPECT_TRUE(install_requests[2].force_reinstall);
+  EXPECT_TRUE(install_requests[3].force_reinstall);
+  EXPECT_TRUE(install_requests[4].force_reinstall);
+
+  EXPECT_TRUE(IsInstalled(AppUrl1()));
+
+  // If we don't abandon at the same version, it doesn't even attempt another
+  // request
+  pending_app_manager().SetDropRequestsForTesting(false);
+  system_web_app_manager().set_current_version(base::Version("2.0.0.0"));
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+  EXPECT_EQ(5u, install_requests.size());
+
+  // Bump the version, and it works.
+  system_web_app_manager().set_current_version(base::Version("3.0.0.0"));
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+
+  EXPECT_EQ(6u, install_requests.size());
+}
+
+// Same test, but for locale change.
+TEST_F(SystemWebAppManagerTest, AbandonFailedInstallsLocaleChange) {
+  const std::vector<ExternalInstallOptions>& install_requests =
+      pending_app_manager().install_requests();
+
+  system_web_app_manager().SetUpdatePolicy(
+      SystemWebAppManager::UpdatePolicy::kOnVersionChange);
+
+  InitEmptyRegistrar();
+
+  PrepareSystemAppDataToRetrieve({{AppUrl1(), AppIconUrl1()}});
+  PrepareLoadUrlResults({AppUrl1()});
+  base::flat_map<SystemAppType, SystemAppInfo> system_apps;
+  system_apps.emplace(SystemAppType::SETTINGS,
+                      SystemAppInfo(kSettingsAppNameForLogging, AppUrl1()));
+  system_web_app_manager().SetSystemAppsForTesting(system_apps);
+
+  system_web_app_manager().set_current_version(base::Version("1.0.0.0"));
+  system_web_app_manager().set_current_locale("en/us");
+  StartAndWaitForAppsToSynchronize();
+
+  EXPECT_EQ(1u, install_requests.size());
+  EXPECT_TRUE(install_requests[0].force_reinstall);
+  EXPECT_TRUE(IsInstalled(AppUrl1()));
+
+  // Bump the version number, and an update will trigger, and force
+  // reinstallation of both apps.
+  PrepareSystemAppDataToRetrieve({{AppUrl1(), AppIconUrl1()}});
+  PrepareLoadUrlResults({AppUrl1()});
+  system_web_app_manager().set_current_locale("en/au");
+  pending_app_manager().SetDropRequestsForTesting(true);
+  // Can't use the normal method because RunLoop::Run goes until
+  // on_app_synchronized is called, and this fails, never calling that.
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+
+  // 1 successful, 1 abandoned, and 3 more abanonded retries is 5.
+  EXPECT_EQ(5u, install_requests.size());
+  EXPECT_TRUE(install_requests[1].force_reinstall);
+  EXPECT_TRUE(install_requests[2].force_reinstall);
+  EXPECT_TRUE(install_requests[3].force_reinstall);
+  EXPECT_TRUE(install_requests[4].force_reinstall);
+
+  EXPECT_TRUE(IsInstalled(AppUrl1()));
+
+  // If we don't abandon at the same version, it doesn't even attempt another
+  // request
+  pending_app_manager().SetDropRequestsForTesting(false);
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+  EXPECT_EQ(5u, install_requests.size());
+
+  // Bump the version, and it works.
+  system_web_app_manager().set_current_locale("fr/fr");
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+}
+
+TEST_F(SystemWebAppManagerTest, SucceedsAfterOneRetry) {
+  const std::vector<ExternalInstallOptions>& install_requests =
+      pending_app_manager().install_requests();
+
+  system_web_app_manager().SetUpdatePolicy(
+      SystemWebAppManager::UpdatePolicy::kOnVersionChange);
+
+  InitEmptyRegistrar();
+
+  PrepareSystemAppDataToRetrieve({{AppUrl1(), AppIconUrl1()}});
+  PrepareLoadUrlResults({AppUrl1()});
+  // Set up and install a baseline
+  base::flat_map<SystemAppType, SystemAppInfo> system_apps;
+  system_apps.emplace(SystemAppType::SETTINGS,
+                      SystemAppInfo(kSettingsAppNameForLogging, AppUrl1()));
+  system_web_app_manager().SetSystemAppsForTesting(system_apps);
+
+  system_web_app_manager().set_current_version(base::Version("1.0.0.0"));
+  StartAndWaitForAppsToSynchronize();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+
+  EXPECT_EQ(1u, install_requests.size());
+  EXPECT_TRUE(install_requests[0].force_reinstall);
+  EXPECT_TRUE(IsInstalled(AppUrl1()));
+  // Bump the version number, and an update will trigger, and force
+  // reinstallation. But, this fails!
+  system_web_app_manager().set_current_version(base::Version("2.0.0.0"));
+  pending_app_manager().SetDropRequestsForTesting(true);
+
+  PrepareSystemAppDataToRetrieve({{AppUrl1(), AppIconUrl1()}});
+  PrepareLoadUrlResults({AppUrl1()});
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+
+  EXPECT_EQ(2u, install_requests.size());
+  EXPECT_TRUE(install_requests[1].force_reinstall);
+  EXPECT_TRUE(IsInstalled(AppUrl1()));
+  system_web_app_manager().Start();
+  base::RunLoop().RunUntilIdle();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+
+  // Retry a few times, but not until abandonment.
+  EXPECT_EQ(3u, install_requests.size());
+  EXPECT_TRUE(install_requests[0].force_reinstall);
+  EXPECT_TRUE(install_requests[1].force_reinstall);
+  EXPECT_TRUE(install_requests[2].force_reinstall);
+  EXPECT_TRUE(IsInstalled(AppUrl1()));
+
+  // Now we succeed at the same version
+  pending_app_manager().SetDropRequestsForTesting(false);
+  StartAndWaitForAppsToSynchronize();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+  PrepareSystemAppDataToRetrieve({{AppUrl1(), AppIconUrl1()}});
+  PrepareLoadUrlResults({AppUrl1()});
+
+  StartAndWaitForAppsToSynchronize();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+  EXPECT_EQ(5u, install_requests.size());
+  EXPECT_TRUE(install_requests[3].force_reinstall);
+  EXPECT_FALSE(install_requests[4].force_reinstall);
+
+  // Bump the version number, and an update will trigger, and force
+  // reinstallation of both apps. This succeeds, everything works.
+  system_web_app_manager().set_current_version(base::Version("3.0.0.0"));
+
+  StartAndWaitForAppsToSynchronize();
+  pending_app_manager().ClearSynchronizeRequestsForTesting();
+  EXPECT_EQ(6u, install_requests.size());
+  EXPECT_TRUE(install_requests[5].force_reinstall);
+
+  PrepareSystemAppDataToRetrieve({{AppUrl1(), AppIconUrl1()}});
+  PrepareLoadUrlResults({AppUrl1()});
+  StartAndWaitForAppsToSynchronize();
+  EXPECT_EQ(7u, install_requests.size());
+  EXPECT_FALSE(install_requests[6].force_reinstall);
+}
+
 }  // namespace web_app

chrome/browser/web_applications/test/test_pending_app_manager_impl.cc

@@ -24,7 +24,10 @@ void TestPendingAppManagerImpl::InstallApps(
     const RepeatingInstallCallback& callback) {
   std::copy(install_options_list.begin(), install_options_list.end(),
             std::back_inserter(install_requests_));
-  PendingAppManagerImpl::InstallApps(install_options_list, std::move(callback));
+  if (!drop_requests_for_testing_) {
+    PendingAppManagerImpl::InstallApps(install_options_list,
+                                       std::move(callback));
+  }
 }
 
 void TestPendingAppManagerImpl::UninstallApps(

chrome/browser/web_applications/test/test_pending_app_manager_impl.h

@@ -31,9 +31,14 @@ class TestPendingAppManagerImpl : public PendingAppManagerImpl {
     return uninstall_requests_;
   }
 
+  void SetDropRequestsForTesting(bool drop_requests_for_testing) {
+    drop_requests_for_testing_ = drop_requests_for_testing;
+  }
+
  private:
   std::vector<ExternalInstallOptions> install_requests_;
   std::vector<GURL> uninstall_requests_;
+  bool drop_requests_for_testing_ = false;
 };
 
 }  // namespace web_app

chrome/common/pref_names.cc

@@ -1900,6 +1900,26 @@ const char kSystemWebAppLastUpdateVersion[] =
 const char kSystemWebAppLastInstalledLocale[] =
     "web_apps.system_web_app_last_installed_language";
 
+// An int representing the number of failures to install SWAs for a given
+// version & locale pair. After 3 failures, we'll abandon this version to avoid
+// bootlooping, and wait for a new version to come along.
+const char kSystemWebAppInstallFailureCount[] =
+    "web_apps.system_web_app_failure_count";
+
+// A string representing the latest Chrome version where an attempt was made
+// to install. In the case of success, this and LastUpdateVersion will be the
+// same. If there is an installation failure, they will diverge until a
+// successful installation is made.
+extern const char kSystemWebAppLastAttemptedVersion[] =
+    "web_apps.system_web_app_last_attempted_update";
+
+// A string representing the most recent locale that was attempted to be
+// installed. In the case of success, this and LastUpdateVersion will be the
+// same. If there is an installation failure, they will diverge until a
+// successful installation is made.
+extern const char kSystemWebAppLastAttemptedLocale[] =
+    "web_apps.system_web_app_last_attempted_language";
+
 // The default audio capture device used by the Media content setting.
 const char kDefaultAudioCaptureDevice[] = "media.default_audio_capture_device";
 

chrome/common/pref_names.h

@@ -644,6 +644,9 @@ extern const char kWebAppsExtensionIDs[];
 extern const char kWebAppsPreferences[];
 extern const char kSystemWebAppLastUpdateVersion[];
 extern const char kSystemWebAppLastInstalledLocale[];
+extern const char kSystemWebAppInstallFailureCount[];
+extern const char kSystemWebAppLastAttemptedVersion[];
+extern const char kSystemWebAppLastAttemptedLocale[];
 
 extern const char kDefaultAudioCaptureDevice[];
 extern const char kDefaultVideoCaptureDevice[];