Fix default web app migration failing after revert

The default web app migration process guards its uninstall_and_replace
logic on whether the web app has been installed before. Instead it
should check whether the app to migrate exists or not.

Without this CL the default web app would not perform the migration if
it had previously been installed then reverted.

Bug: 1104696
Change-Id: Iedc8752a9824e85080d227c818307dbbfdd5248b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2389740
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804206}

chrome/browser/ui/web_applications/web_app_ui_manager_impl.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
+#include "components/services/app_service/public/cpp/app_registry_cache.h"
 #include "extensions/browser/app_sorting.h"
 #include "extensions/browser/extension_system.h"
 
@@ -36,6 +37,20 @@
 
 namespace web_app {
 
+namespace {
+
+bool IsAppInstalled(apps::AppServiceProxy* proxy, const AppId& app_id) {
+  bool installed = false;
+  proxy->AppRegistryCache().ForOneApp(
+      app_id, [&installed](const apps::AppUpdate& update) {
+        installed =
+            update.Readiness() != apps::mojom::Readiness::kUninstalledByUser;
+      });
+  return installed;
+}
+
+}  // namespace
+
 // static
 std::unique_ptr<WebAppUiManager> WebAppUiManager::Create(Profile* profile) {
   return std::make_unique<WebAppUiManagerImpl>(profile);
@@ -123,11 +138,16 @@ void WebAppUiManagerImpl::NotifyOnAllAppWindowsClosed(
   windows_closed_requests_map_[app_id].push_back(std::move(callback));
 }
 
-void WebAppUiManagerImpl::UninstallAndReplace(
+void WebAppUiManagerImpl::UninstallAndReplaceIfExists(
     const std::vector<AppId>& from_apps,
     const AppId& to_app) {
   bool has_migrated = false;
   for (const AppId& from_app : from_apps) {
+    apps::AppServiceProxy* proxy =
+        apps::AppServiceProxyFactory::GetForProfile(profile_);
+    if (!IsAppInstalled(proxy, from_app))
+      continue;
+
     if (!has_migrated) {
 #if defined(OS_CHROMEOS)
       // Grid position in app list.
@@ -173,8 +193,6 @@ void WebAppUiManagerImpl::UninstallAndReplace(
       }
     }
 
-    apps::AppServiceProxy* proxy =
-        apps::AppServiceProxyFactory::GetForProfile(profile_);
     proxy->UninstallSilently(from_app,
                              apps::mojom::UninstallSource::kMigration);
   }

chrome/browser/ui/web_applications/web_app_ui_manager_impl.h

@@ -45,8 +45,8 @@ class WebAppUiManagerImpl : public BrowserListObserver, public WebAppUiManager {
   size_t GetNumWindowsForApp(const AppId& app_id) override;
   void NotifyOnAllAppWindowsClosed(const AppId& app_id,
                                    base::OnceClosure callback) override;
-  void UninstallAndReplace(const std::vector<AppId>& from_apps,
-                           const AppId& to_app) override;
+  void UninstallAndReplaceIfExists(const std::vector<AppId>& from_apps,
+                                   const AppId& to_app) override;
   bool CanAddAppToQuickLaunchBar() const override;
   void AddAppToQuickLaunchBar(const AppId& app_id) override;
   bool IsInAppWindow(content::WebContents* web_contents,

chrome/browser/ui/web_applications/web_app_ui_manager_impl_browsertest.cc

@@ -7,6 +7,8 @@
 #include "base/barrier_closure.h"
 #include "base/test/bind_test_util.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/apps/app_service/app_service_proxy.h"
+#include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
 #include "chrome/browser/apps/app_service/built_in_chromeos_apps.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
@@ -15,6 +17,7 @@
 #include "chrome/browser/ui/web_applications/test/web_app_browsertest_util.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
 #include "chrome/browser/web_applications/system_web_app_manager.h"
+#include "chrome/browser/web_applications/test/web_app_uninstall_waiter.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
 #include "chrome/common/chrome_features.h"
 #include "chrome/common/web_application_info.h"
@@ -184,90 +187,44 @@ IN_PROC_BROWSER_TEST_F(WebAppUiManagerImplBrowserTest,
 }
 
 #if defined(OS_CHROMEOS)
-class WebAppUiManagerMigrationBrowserTest
-    : public WebAppUiManagerImplBrowserTest {
- public:
-  void SetUp() override {
-    hide_settings_app_for_testing_ =
-        apps::BuiltInChromeOsApps::SetHideSettingsAppForTesting(true);
-    // Disable System Web Apps so that the Internal Apps are installed.
-    scoped_feature_list_.InitAndDisableFeature(features::kSystemWebApps);
-    WebAppUiManagerImplBrowserTest::SetUp();
-  }
-
-  void TearDown() override {
-    WebAppUiManagerImplBrowserTest::TearDown();
-    apps::BuiltInChromeOsApps::SetHideSettingsAppForTesting(
-        hide_settings_app_for_testing_);
-  }
-
- private:
-  base::test::ScopedFeatureList scoped_feature_list_;
-  bool hide_settings_app_for_testing_ = false;
-};
-
-// Tests that the Settings app migrates the launcher and app list details from
-// the Settings internal app.
-//
-// TODO(https://crbug.com/1012967): Find a way to implement this that does not
-// depend on unsupported behavior of the FeatureList API.
-IN_PROC_BROWSER_TEST_F(WebAppUiManagerMigrationBrowserTest,
-                       DISABLED_SettingsSystemWebAppMigration) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndEnableFeature(features::kSystemWebApps);
-
-  auto& system_web_app_manager =
-      WebAppProvider::Get(browser()->profile())->system_web_app_manager();
-
-  auto* app_list_service =
+// Tests that app migrations use the UI preferences of the replaced app but only
+// if it's present.
+IN_PROC_BROWSER_TEST_F(WebAppUiManagerImplBrowserTest, DoubleMigration) {
+  app_list::AppListSyncableService* app_list_service =
       app_list::AppListSyncableServiceFactory::GetForProfile(
           browser()->profile());
 
-  // Pin the Settings Internal App.
-  syncer::StringOrdinal pin_position =
-      syncer::StringOrdinal::CreateInitialOrdinal();
-  pin_position = pin_position.CreateAfter().CreateAfter();
-  app_list_service->SetPinPosition(ash::kInternalAppIdSettings, pin_position);
-
-  // Add the Settings Internal App to a folder.
-  AppListModelUpdater* updater =
-      test::GetModelUpdater(test::GetAppListClient());
-  updater->MoveItemToFolder(ash::kInternalAppIdSettings, "asdf");
-
-  // Install the Settings System Web App, which should be immediately migrated
-  // to the Settings Internal App's details.
-  system_web_app_manager.InstallSystemAppsForTesting();
-  std::string settings_system_web_app_id =
-      *system_web_app_manager.GetAppIdForSystemApp(SystemAppType::SETTINGS);
-  {
-    const app_list::AppListSyncableService::SyncItem* web_app_item =
-        app_list_service->GetSyncItem(settings_system_web_app_id);
-    const app_list::AppListSyncableService::SyncItem* internal_app_item =
-        app_list_service->GetSyncItem(ash::kInternalAppIdSettings);
-
-    EXPECT_TRUE(internal_app_item->item_pin_ordinal.Equals(
-        web_app_item->item_pin_ordinal));
-    EXPECT_TRUE(
-        internal_app_item->item_ordinal.Equals(web_app_item->item_ordinal));
-    EXPECT_EQ(internal_app_item->parent_id, web_app_item->parent_id);
-  }
+  // Install an old app to be replaced.
+  AppId old_app_id = InstallWebApp(GURL("https://old.app.com"));
+  app_list_service->SetPinPosition(old_app_id,
+                                   syncer::StringOrdinal("positionold"));
 
-  // Change Settings System Web App properties.
-  app_list_service->SetPinPosition(
-      settings_system_web_app_id,
-      syncer::StringOrdinal::CreateInitialOrdinal());
-  updater->MoveItemToFolder(settings_system_web_app_id, std::string());
-
-  // Do migration again with the already-installed app. Should be a no-op.
-  system_web_app_manager.InstallSystemAppsForTesting();
+  // Install a new app to migrate the old one to.
+  AppId new_app_id = InstallWebApp(GURL("https://new.app.com"));
   {
-    const app_list::AppListSyncableService::SyncItem* web_app_item =
-        app_list_service->GetSyncItem(settings_system_web_app_id);
-
-    EXPECT_TRUE(syncer::StringOrdinal::CreateInitialOrdinal().Equals(
-        web_app_item->item_pin_ordinal));
-    EXPECT_EQ(std::string(), web_app_item->parent_id);
+    WebAppUninstallWaiter waiter(browser()->profile(), old_app_id);
+    ui_manager().UninstallAndReplaceIfExists({old_app_id}, new_app_id);
+    waiter.Wait();
+    apps::AppServiceProxyFactory::GetForProfile(browser()->profile())
+        ->FlushMojoCallsForTesting();
   }
+
+  // New app should acquire old app's pin position.
+  EXPECT_EQ(app_list_service->GetSyncItem(new_app_id)
+                ->item_pin_ordinal.ToDebugString(),
+            "positionold");
+
+  // Change the new app's pin position.
+  app_list_service->SetPinPosition(new_app_id,
+                                   syncer::StringOrdinal("positionnew"));
+
+  // Do migration again. New app should not move.
+  ui_manager().UninstallAndReplaceIfExists({old_app_id}, new_app_id);
+  apps::AppServiceProxyFactory::GetForProfile(browser()->profile())
+      ->FlushMojoCallsForTesting();
+  EXPECT_EQ(app_list_service->GetSyncItem(new_app_id)
+                ->item_pin_ordinal.ToDebugString(),
+            "positionnew");
 }
 #endif  // defined(OS_CHROMEOS)
 

chrome/browser/web_applications/BUILD.gn

@@ -174,6 +174,8 @@ source_set("web_applications_test_support") {
     "test/web_app_registration_waiter.h",
     "test/web_app_test.cc",
     "test/web_app_test.h",
+    "test/web_app_uninstall_waiter.cc",
+    "test/web_app_uninstall_waiter.h",
   ]
 
   deps = [

chrome/browser/web_applications/components/web_app_ui_manager.h

@@ -49,8 +49,8 @@ class WebAppUiManager {
   // Uninstalls the the apps in |from_apps| and migrates an |to_app|'s OS
   // attributes (e.g pin position, app list folder/position, shortcuts) to the
   // first |from_app| found.
-  virtual void UninstallAndReplace(const std::vector<AppId>& from_apps,
-                                   const AppId& to_app) = 0;
+  virtual void UninstallAndReplaceIfExists(const std::vector<AppId>& from_apps,
+                                           const AppId& to_app) = 0;
 
   virtual bool CanAddAppToQuickLaunchBar() const = 0;
   virtual void AddAppToQuickLaunchBar(const AppId& app_id) = 0;

chrome/browser/web_applications/extensions/pending_app_install_task_unittest.cc

@@ -853,7 +853,7 @@ TEST_F(PendingAppInstallTaskTest, UninstallAndReplace) {
     run_loop.Run();
   }
   {
-    // Migration shouldn't run on subsequent installs of the same app.
+    // Migration should run on every install of the app.
     options.uninstall_and_replace = {"app3"};
 
     base::RunLoop run_loop;
@@ -864,7 +864,7 @@ TEST_F(PendingAppInstallTaskTest, UninstallAndReplace) {
           EXPECT_EQ(InstallResultCode::kSuccessNewInstall, result.code);
           EXPECT_EQ(app_id, *result.app_id);
 
-          EXPECT_FALSE(ui_manager()->DidUninstallAndReplace("app3", app_id));
+          EXPECT_TRUE(ui_manager()->DidUninstallAndReplace("app3", app_id));
 
           run_loop.Quit();
         }));

chrome/browser/web_applications/external_web_app_migration_browsertest.cc

@@ -207,7 +207,8 @@ class ExternalWebAppMigrationBrowserTest : public InProcessBrowserTest {
   std::unique_ptr<extensions::ExtensionCacheFake> test_extension_cache_;
 };
 
-IN_PROC_BROWSER_TEST_F(ExternalWebAppMigrationBrowserTest, MigrateAndRevert) {
+IN_PROC_BROWSER_TEST_F(ExternalWebAppMigrationBrowserTest,
+                       MigrateRevertMigrate) {
   // Set up pre-migration state.
   {
     ASSERT_FALSE(IsExternalAppInstallFeatureEnabled(kMigrationFlag));
@@ -253,6 +254,24 @@ IN_PROC_BROWSER_TEST_F(ExternalWebAppMigrationBrowserTest, MigrateAndRevert) {
     EXPECT_TRUE(IsExtensionAppInstalled());
     EXPECT_FALSE(IsWebAppInstalled());
   }
+
+  // Re-run migration.
+  {
+    base::AutoReset<bool> testing_scope =
+        SetExternalAppInstallFeatureAlwaysEnabledForTesting();
+    ASSERT_TRUE(IsExternalAppInstallFeatureEnabled(kMigrationFlag));
+
+    extensions::TestExtensionRegistryObserver uninstall_observer(
+        extensions::ExtensionRegistry::Get(profile()));
+
+    SyncExternalWebApps(/*expect_install=*/true, /*expect_uninstall=*/false);
+    EXPECT_TRUE(IsWebAppInstalled());
+
+    scoped_refptr<const extensions::Extension> uninstalled_app =
+        uninstall_observer.WaitForExtensionUninstalled();
+    EXPECT_EQ(uninstalled_app->id(), kExtensionId);
+    EXPECT_FALSE(IsExtensionAppInstalled());
+  }
 }
 
 IN_PROC_BROWSER_TEST_F(ExternalWebAppMigrationBrowserTest, MigratePreferences) {

chrome/browser/web_applications/pending_app_install_task.cc

@@ -240,13 +240,8 @@ void PendingAppInstallTask::OnWebAppInstalled(bool is_placeholder,
     return;
   }
 
-  // If this is the first time the app has been installed, run a migration. This
-  // will not happen again, even if the app is uninstalled and reinstalled.
-  if (!externally_installed_app_prefs_.LookupAppId(
-          install_options_.install_url)) {
-    ui_manager_->UninstallAndReplace(install_options().uninstall_and_replace,
-                                     app_id);
-  }
+  ui_manager_->UninstallAndReplaceIfExists(
+      install_options().uninstall_and_replace, app_id);
 
   externally_installed_app_prefs_.Insert(install_options_.install_url, app_id,
                                          install_options_.install_source);

chrome/browser/web_applications/test/test_web_app_ui_manager.cc

@@ -54,7 +54,7 @@ void TestWebAppUiManager::NotifyOnAllAppWindowsClosed(
                      }));
 }
 
-void TestWebAppUiManager::UninstallAndReplace(
+void TestWebAppUiManager::UninstallAndReplaceIfExists(
     const std::vector<AppId>& from_apps,
     const AppId& to_app) {
   for (const AppId& from_app : from_apps) {

chrome/browser/web_applications/test/test_web_app_ui_manager.h

@@ -29,8 +29,8 @@ class TestWebAppUiManager : public WebAppUiManager {
   size_t GetNumWindowsForApp(const AppId& app_id) override;
   void NotifyOnAllAppWindowsClosed(const AppId& app_id,
                                    base::OnceClosure callback) override;
-  void UninstallAndReplace(const std::vector<AppId>& from_apps,
-                           const AppId& to_app) override;
+  void UninstallAndReplaceIfExists(const std::vector<AppId>& from_apps,
+                                   const AppId& to_app) override;
   bool CanAddAppToQuickLaunchBar() const override;
   void AddAppToQuickLaunchBar(const AppId& app_id) override;
   bool IsInAppWindow(content::WebContents* web_contents,

chrome/browser/web_applications/test/web_app_uninstall_waiter.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/browser/web_applications/test/web_app_uninstall_waiter.h"
+
+#include "chrome/browser/web_applications/components/web_app_provider_base.h"
+
+namespace web_app {
+
+WebAppUninstallWaiter::WebAppUninstallWaiter(Profile* profile, AppId app_id)
+    : app_id_(std::move(app_id)) {
+  observer_.Add(&WebAppProviderBase::GetProviderBase(profile)->registrar());
+}
+WebAppUninstallWaiter::~WebAppUninstallWaiter() = default;
+
+void WebAppUninstallWaiter::Wait() {
+  run_loop_.Run();
+}
+
+void WebAppUninstallWaiter::OnWebAppUninstalled(const AppId& app_id) {
+  if (app_id == app_id_)
+    run_loop_.Quit();
+}
+
+}  // namespace web_app

chrome/browser/web_applications/test/web_app_uninstall_waiter.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_BROWSER_WEB_APPLICATIONS_TEST_WEB_APP_UNINSTALL_WAITER_H_
+#define CHROME_BROWSER_WEB_APPLICATIONS_TEST_WEB_APP_UNINSTALL_WAITER_H_
+
+#include "base/run_loop.h"
+#include "base/scoped_observer.h"
+#include "chrome/browser/web_applications/components/app_registrar.h"
+#include "chrome/browser/web_applications/components/app_registrar_observer.h"
+#include "chrome/browser/web_applications/components/web_app_id.h"
+
+namespace web_app {
+
+class WebAppUninstallWaiter final : public AppRegistrarObserver {
+ public:
+  WebAppUninstallWaiter(Profile* profile, AppId app_id);
+  ~WebAppUninstallWaiter() final;
+  void Wait();
+
+  // AppRegistrarObserver:
+  void OnWebAppUninstalled(const AppId& app_id) final;
+
+ private:
+  AppId app_id_;
+  base::RunLoop run_loop_;
+  ScopedObserver<AppRegistrar, AppRegistrarObserver> observer_{this};
+};
+
+}  // namespace web_app
+
+#endif  // CHROME_BROWSER_WEB_APPLICATIONS_TEST_WEB_APP_UNINSTALL_WAITER_H_