Fix and test default web app uninstall_and_replace config parameter

This CL adds a basic test for the uninstall_and_replace feature of
ExternalWebAppManager and fixes two issues:
 - On Chrome OS the uninstall was getting blocked on a user prompt.
 - On non-Chrome OS the uninstall never happened.

The changes in this CL are:
 - Use AppServiceProxy::UninstallSilently() to avoid user prompt.
 - Move ExtensionAppsChromeOs::Uninstall() to ExtensionAppsBase.
 - Instantiate ExtensionApps publisher for non-Chrome OS app service.
 - Refactor ExternalWebAppManager's ParseConfig() out of ScanDir()
   and expose it for testing.

Bug: 809304, 1058265
Change-Id: I86caeeb072ec18a4c16a181a01f3c795d2735648
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2306139
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Glen Robertson <glenrob@chromium.org>
Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#790233}

chrome/browser/apps/app_service/app_service_proxy.cc

@@ -201,6 +201,8 @@ void AppServiceProxy::Initialize() {
       extension_web_apps_ = std::make_unique<ExtensionApps>(
           app_service_, profile_, apps::mojom::AppType::kWeb);
     }
+    extension_apps_ = std::make_unique<ExtensionApps>(
+        app_service_, profile_, apps::mojom::AppType::kExtension);
 #endif
 
     // Asynchronously add app icon source, so we don't do too much work in the

chrome/browser/apps/app_service/app_service_proxy.h

@@ -435,6 +435,7 @@ class AppServiceProxy : public KeyedService,
   // TODO(crbug.com/877898): Erase extension_web_apps_ when BMO is on.
   std::unique_ptr<ExtensionApps> extension_web_apps_;
   std::unique_ptr<WebApps> web_apps_;
+  std::unique_ptr<ExtensionApps> extension_apps_;
 #endif
 
   Profile* profile_;

chrome/browser/apps/app_service/extension_apps_base.cc

@@ -51,6 +51,7 @@
 #include "components/content_settings/core/common/content_settings_pattern.h"
 #include "components/content_settings/core/common/content_settings_types.h"
 #include "components/services/app_service/public/cpp/intent_filter_util.h"
+#include "content/public/browser/clear_site_data_utils.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/ui_util.h"
@@ -602,6 +603,82 @@ void ExtensionAppsBase::SetPermission(const std::string& app_id,
       url, url, permission_type, std::string() /* resource identifier */,
       permission_value);
 }
+void ExtensionAppsBase::Uninstall(const std::string& app_id,
+                                  bool clear_site_data,
+                                  bool report_abuse) {
+  // TODO(crbug.com/1009248): We need to add the error code, which could be used
+  // by ExtensionFunction, ManagementUninstallFunctionBase on the callback
+  // OnExtensionUninstallDialogClosed
+  scoped_refptr<const extensions::Extension> extension =
+      extensions::ExtensionRegistry::Get(profile())->GetInstalledExtension(
+          app_id);
+  if (!extension.get()) {
+    return;
+  }
+
+  base::string16 error;
+  extensions::ExtensionSystem::Get(profile())
+      ->extension_service()
+      ->UninstallExtension(app_id, extensions::UNINSTALL_REASON_USER_INITIATED,
+                           &error);
+
+  if (extension->from_bookmark()) {
+    if (!clear_site_data) {
+      UMA_HISTOGRAM_ENUMERATION(
+          "Webapp.UninstallDialogAction",
+          extensions::ExtensionUninstallDialog::CLOSE_ACTION_UNINSTALL,
+          extensions::ExtensionUninstallDialog::CLOSE_ACTION_LAST);
+      return;
+    }
+
+    UMA_HISTOGRAM_ENUMERATION(
+        "Webapp.UninstallDialogAction",
+        extensions::ExtensionUninstallDialog::
+            CLOSE_ACTION_UNINSTALL_AND_CHECKBOX_CHECKED,
+        extensions::ExtensionUninstallDialog::CLOSE_ACTION_LAST);
+
+    constexpr bool kClearCookies = true;
+    constexpr bool kClearStorage = true;
+    constexpr bool kClearCache = true;
+    constexpr bool kAvoidClosingConnections = false;
+    content::ClearSiteData(
+        base::BindRepeating(
+            [](content::BrowserContext* browser_context) {
+              return browser_context;
+            },
+            base::Unretained(profile())),
+        url::Origin::Create(
+            extensions::AppLaunchInfo::GetFullLaunchURL(extension.get())),
+        kClearCookies, kClearStorage, kClearCache, kAvoidClosingConnections,
+        base::DoNothing());
+  } else {
+    if (!report_abuse) {
+      UMA_HISTOGRAM_ENUMERATION(
+          "Extensions.UninstallDialogAction",
+          extensions::ExtensionUninstallDialog::CLOSE_ACTION_UNINSTALL,
+          extensions::ExtensionUninstallDialog::CLOSE_ACTION_LAST);
+      return;
+    }
+
+    UMA_HISTOGRAM_ENUMERATION(
+        "Extensions.UninstallDialogAction",
+        extensions::ExtensionUninstallDialog::
+            CLOSE_ACTION_UNINSTALL_AND_CHECKBOX_CHECKED,
+        extensions::ExtensionUninstallDialog::CLOSE_ACTION_LAST);
+
+    // If the extension specifies a custom uninstall page via
+    // chrome.runtime.setUninstallURL, then at uninstallation its uninstall
+    // page opens. To ensure that the CWS Report Abuse page is the active
+    // tab at uninstallation, navigates to the url to report abuse.
+    constexpr char kReferrerId[] = "chrome-remove-extension-dialog";
+    NavigateParams params(
+        profile(),
+        extension_urls::GetWebstoreReportAbuseUrl(app_id, kReferrerId),
+        ui::PAGE_TRANSITION_LINK);
+    params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+    Navigate(&params);
+  }
+}
 
 void ExtensionAppsBase::OpenNativeSettings(const std::string& app_id) {
   const auto* extension = MaybeGetExtension(app_id);

chrome/browser/apps/app_service/extension_apps_base.h

@@ -145,6 +145,9 @@ class ExtensionAppsBase : public apps::PublisherBase,
                            int64_t display_id) override;
   void SetPermission(const std::string& app_id,
                      apps::mojom::PermissionPtr permission) override;
+  void Uninstall(const std::string& app_id,
+                 bool clear_site_data,
+                 bool report_abuse) override;
   void OpenNativeSettings(const std::string& app_id) override;
 
   // content_settings::Observer overrides.

chrome/browser/apps/app_service/extension_apps_chromeos.cc

@@ -201,83 +201,6 @@ void ExtensionAppsChromeOs::LaunchAppWithIntent(
   }
 }
 
-void ExtensionAppsChromeOs::Uninstall(const std::string& app_id,
-                                      bool clear_site_data,
-                                      bool report_abuse) {
-  // TODO(crbug.com/1009248): We need to add the error code, which could be used
-  // by ExtensionFunction, ManagementUninstallFunctionBase on the callback
-  // OnExtensionUninstallDialogClosed
-  scoped_refptr<const extensions::Extension> extension =
-      extensions::ExtensionRegistry::Get(profile())->GetInstalledExtension(
-          app_id);
-  if (!extension.get()) {
-    return;
-  }
-
-  base::string16 error;
-  extensions::ExtensionSystem::Get(profile())
-      ->extension_service()
-      ->UninstallExtension(app_id, extensions::UNINSTALL_REASON_USER_INITIATED,
-                           &error);
-
-  if (extension->from_bookmark()) {
-    if (!clear_site_data) {
-      UMA_HISTOGRAM_ENUMERATION(
-          "Webapp.UninstallDialogAction",
-          extensions::ExtensionUninstallDialog::CLOSE_ACTION_UNINSTALL,
-          extensions::ExtensionUninstallDialog::CLOSE_ACTION_LAST);
-      return;
-    }
-
-    UMA_HISTOGRAM_ENUMERATION(
-        "Webapp.UninstallDialogAction",
-        extensions::ExtensionUninstallDialog::
-            CLOSE_ACTION_UNINSTALL_AND_CHECKBOX_CHECKED,
-        extensions::ExtensionUninstallDialog::CLOSE_ACTION_LAST);
-
-    constexpr bool kClearCookies = true;
-    constexpr bool kClearStorage = true;
-    constexpr bool kClearCache = true;
-    constexpr bool kAvoidClosingConnections = false;
-    content::ClearSiteData(
-        base::BindRepeating(
-            [](content::BrowserContext* browser_context) {
-              return browser_context;
-            },
-            base::Unretained(profile())),
-        url::Origin::Create(
-            extensions::AppLaunchInfo::GetFullLaunchURL(extension.get())),
-        kClearCookies, kClearStorage, kClearCache, kAvoidClosingConnections,
-        base::DoNothing());
-  } else {
-    if (!report_abuse) {
-      UMA_HISTOGRAM_ENUMERATION(
-          "Extensions.UninstallDialogAction",
-          extensions::ExtensionUninstallDialog::CLOSE_ACTION_UNINSTALL,
-          extensions::ExtensionUninstallDialog::CLOSE_ACTION_LAST);
-      return;
-    }
-
-    UMA_HISTOGRAM_ENUMERATION(
-        "Extensions.UninstallDialogAction",
-        extensions::ExtensionUninstallDialog::
-            CLOSE_ACTION_UNINSTALL_AND_CHECKBOX_CHECKED,
-        extensions::ExtensionUninstallDialog::CLOSE_ACTION_LAST);
-
-    // If the extension specifies a custom uninstall page via
-    // chrome.runtime.setUninstallURL, then at uninstallation its uninstall
-    // page opens. To ensure that the CWS Report Abuse page is the active
-    // tab at uninstallation, navigates to the url to report abuse.
-    constexpr char kReferrerId[] = "chrome-remove-extension-dialog";
-    NavigateParams params(
-        profile(),
-        extension_urls::GetWebstoreReportAbuseUrl(app_id, kReferrerId),
-        ui::PAGE_TRANSITION_LINK);
-    params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
-    Navigate(&params);
-  }
-}
-
 void ExtensionAppsChromeOs::PauseApp(const std::string& app_id) {
   if (paused_apps_.MaybeAddApp(app_id)) {
     SetIconEffect(app_id);

chrome/browser/apps/app_service/extension_apps_chromeos.h

@@ -78,9 +78,6 @@ class ExtensionAppsChromeOs : public ExtensionAppsBase,
                            apps::mojom::IntentPtr intent,
                            apps::mojom::LaunchSource launch_source,
                            int64_t display_id) override;
-  void Uninstall(const std::string& app_id,
-                 bool clear_site_data,
-                 bool report_abuse) override;
   void PauseApp(const std::string& app_id) override;
   void UnpauseApps(const std::string& app_id) override;
   void GetMenuModel(const std::string& app_id,

chrome/browser/ui/web_applications/web_app_ui_manager_impl.cc

@@ -134,7 +134,7 @@ void WebAppUiManagerImpl::UninstallAndReplace(
     apps::AppServiceProxy* proxy =
         apps::AppServiceProxyFactory::GetForProfile(profile_);
     DCHECK(proxy);
-    proxy->Uninstall(from_app, nullptr /* parent_window */);
+    proxy->UninstallSilently(from_app);
   }
 }
 

chrome/browser/web_applications/BUILD.gn

@@ -245,6 +245,7 @@ source_set("web_applications_browser_tests") {
   testonly = true
 
   sources = [
+    "external_web_app_manager_browsertest.cc",
     "manifest_update_manager_browsertest.cc",
     "pending_app_manager_impl_browsertest.cc",
     "system_web_app_manager_browsertest.cc",

chrome/browser/web_applications/external_web_app_manager.h

@@ -11,6 +11,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/web_applications/components/external_install_options.h"
+#include "chrome/browser/web_applications/components/pending_app_manager.h"
 
 namespace base {
 class FilePath;
@@ -45,6 +46,12 @@ class ExternalWebAppManager {
 
   void ScanForExternalWebApps(ScanCallback callback);
 
+  static void SkipStartupScanForTesting();
+
+  void SynchronizeAppsForTesting(
+      std::vector<std::string> app_configs,
+      PendingAppManager::SynchronizeCallback callback);
+
  private:
   void OnScanForExternalWebApps(std::vector<ExternalInstallOptions>);
 

chrome/browser/web_applications/external_web_app_manager_browsertest.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/external_web_app_manager.h"
+
+#include "base/strings/string_util.h"
+#include "base/test/bind_test_util.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/test_launcher.h"
+#include "extensions/browser/extension_registry.h"
+#include "extensions/browser/test_extension_registry_observer.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+const char kChromeAppDirectory[] = "app";
+const char kChromeAppName[] = "App Test";
+
+}  // namespace
+
+namespace web_app {
+
+class ExternalWebAppManagerBrowserTest
+    : public extensions::ExtensionBrowserTest {
+ public:
+  ExternalWebAppManagerBrowserTest() {
+    ExternalWebAppManager::SkipStartupScanForTesting();
+  }
+
+  GURL GetAppUrl() const {
+    return embedded_test_server()->GetURL("/web_apps/basic.html");
+  }
+
+  ~ExternalWebAppManagerBrowserTest() override = default;
+};
+
+IN_PROC_BROWSER_TEST_F(ExternalWebAppManagerBrowserTest, UninstallAndReplace) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  Profile* profile = browser()->profile();
+
+  // Install Chrome app to be replaced.
+  const extensions::Extension* app = InstallExtensionWithSourceAndFlags(
+      test_data_dir_.AppendASCII(kChromeAppDirectory), 1,
+      extensions::Manifest::INTERNAL, extensions::Extension::NO_FLAGS);
+  EXPECT_EQ(app->name(), kChromeAppName);
+
+  // Start listening for Chrome app uninstall.
+  extensions::TestExtensionRegistryObserver uninstall_observer(
+      extensions::ExtensionRegistry::Get(profile));
+
+  // Trigger default web app install.
+  base::RunLoop sync_run_loop;
+  WebAppProvider::Get(profile)
+      ->external_web_app_manager_for_testing()
+      .SynchronizeAppsForTesting(
+          {base::ReplaceStringPlaceholders(
+              R"({
+                "app_url": "$1",
+                "launch_container": "window",
+                "user_type": ["unmanaged"],
+                "uninstall_and_replace": ["$2"]
+              })",
+              {GetAppUrl().spec(), app->id()}, nullptr)},
+          base::BindLambdaForTesting(
+              [&](std::map<GURL, InstallResultCode> install_results,
+                  std::map<GURL, bool> uninstall_results) {
+                EXPECT_EQ(install_results.at(GetAppUrl()),
+                          InstallResultCode::kSuccessNewInstall);
+                sync_run_loop.Quit();
+              }));
+  sync_run_loop.Run();
+
+  // Chrome app should get uninstalled.
+  scoped_refptr<const extensions::Extension> uninstalled_app =
+      uninstall_observer.WaitForExtensionUninstalled();
+  EXPECT_EQ(app, uninstalled_app.get());
+}
+
+}  // namespace web_app

chrome/browser/web_applications/web_app_provider.h

@@ -94,6 +94,10 @@ class WebAppProvider : public WebAppProviderBase {
     return on_registry_ready_;
   }
 
+  ExternalWebAppManager& external_web_app_manager_for_testing() {
+    return *external_web_app_manager_;
+  }
+
  protected:
   virtual void StartImpl();
   void OnDatabaseMigrationCompleted(bool success);