Make AppRegistrar observe profile deletion

Bug:1080703

Currently, web app subsystems in BMO mode are not notified of profile
deletion because AppRegistrar::NotifyWebAppProfileWillBeDeleted is never
called on WebAppRegistrar.

In contrast, NotifyWebAppProfileWillBeDeleted is properly called on
BookmarkAppRegistrar during profile deletion through the following call
stack:

BookmarkAppRegistrar::NotifyWebAppProfileWillBeDeleted
BookmarkAppRegistrar::OnExtensionUnloaded
ExtensionRegistry::TriggerOnUnloaded
ExtensionRegistrar::DeactivateExtension
ExtensionRegistrar::RemoveExtension
ExtensionService::UnloadExtension
ExtensionService::OnProfileMarkedForPermanentDeletion

This CL makes WebAppRegistrar observe profile deletion through the
ProfileManagerObserver interface and call
NotifyWebAppProfileWillBeDeleted accordingly.

NOTE: An alternate implementation that consolidated
ProfileManagerObserver subscription into AppRegistrar (base class) was
also explored. However, because ExtensionRegistry (the backend storage
for BookmarkAppRegistrar) is also a ProfileManagerObsever,
ExtensionRegistry may be notified of profile deletion first and clean up
its internal app state before AppRegistrar handles profile deletion and
queries installed apps. BookmarkAppRegistrar and ExtensionService cannot
both be ProfileManagerObservers that interact with one another
during profile deletion.


Change-Id: Ic2244e10a3e1d7cc1b0f76ccb18cd1a397d06fff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2191112Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Reviewed-by: David Bienvenu <davidbienvenu@chromium.org>
Commit-Queue: Connor Moody <connor.moody@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#772121}

chrome/browser/ui/web_applications/web_app_profile_deletion_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 "base/bind_helpers.h"
+#include "base/run_loop.h"
+#include "base/scoped_observer.h"
+#include "base/test/bind_test_util.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/web_applications/web_app_controller_browsertest.h"
+#include "chrome/browser/web_applications/components/app_registrar.h"
+#include "chrome/browser/web_applications/components/web_app_id.h"
+#include "chrome/browser/web_applications/components/web_app_provider_base.h"
+#include "chrome/browser/web_applications/test/web_app_install_observer.h"
+#include "chrome/browser/web_applications/test/web_app_test.h"
+#include "content/public/test/browser_test.h"
+#include "url/gurl.h"
+
+namespace web_app {
+
+class WebAppProfileDeletionBrowserTest : public WebAppControllerBrowserTest {
+ public:
+  AppRegistrar& registrar() {
+    auto* provider = WebAppProviderBase::GetProviderBase(profile());
+    CHECK(provider);
+    return provider->registrar();
+  }
+
+  void ScheduleCurrentProfileForDeletion() {
+    g_browser_process->profile_manager()->ScheduleProfileForDeletion(
+        profile()->GetPath(), base::DoNothing());
+  }
+};
+
+IN_PROC_BROWSER_TEST_P(WebAppProfileDeletionBrowserTest,
+                       AppRegistrarNotifiesProfileDeletion) {
+  GURL app_url(GetInstallableAppURL());
+  const AppId app_id = InstallPWA(app_url);
+
+  base::RunLoop run_loop;
+  WebAppInstallObserver observer(&registrar());
+  observer.SetWebAppProfileWillBeDeletedDelegate(
+      base::BindLambdaForTesting([&](const AppId& app_to_be_uninstalled) {
+        EXPECT_EQ(app_to_be_uninstalled, app_id);
+        run_loop.Quit();
+      }));
+
+  ScheduleCurrentProfileForDeletion();
+  run_loop.Run();
+}
+
+INSTANTIATE_TEST_SUITE_P(All,
+                         WebAppProfileDeletionBrowserTest,
+                         ::testing::Values(ProviderType::kBookmarkApps,
+                                           ProviderType::kWebApps),
+                         ProviderTypeParamToString);
+
+}  // namespace web_app

chrome/browser/web_applications/components/app_registrar.h

@@ -36,6 +36,9 @@ class AppRegistrar {
   explicit AppRegistrar(Profile* profile);
   virtual ~AppRegistrar();
 
+  virtual void Start() {}
+  virtual void Shutdown() {}
+
   // Returns whether the app with |app_id| is currently listed in the registry.
   // ie. we have data for web app manifest and icons, and this |app_id| can be
   // used in other registrar methods.

chrome/browser/web_applications/test/web_app_install_observer.cc

@@ -33,6 +33,11 @@ void WebAppInstallObserver::SetWebAppUninstalledDelegate(
   app_uninstalled_delegate_ = delegate;
 }
 
+void WebAppInstallObserver::SetWebAppProfileWillBeDeletedDelegate(
+    WebAppProfileWillBeDeletedDelegate delegate) {
+  app_profile_will_be_deleted_delegate_ = delegate;
+}
+
 void WebAppInstallObserver::OnWebAppInstalled(const AppId& app_id) {
   if (app_installed_delegate_)
     app_installed_delegate_.Run(app_id);
@@ -46,4 +51,9 @@ void WebAppInstallObserver::OnWebAppUninstalled(const AppId& app_id) {
     app_uninstalled_delegate_.Run(app_id);
 }
 
+void WebAppInstallObserver::OnWebAppProfileWillBeDeleted(const AppId& app_id) {
+  if (app_profile_will_be_deleted_delegate_)
+    app_profile_will_be_deleted_delegate_.Run(app_id);
+}
+
 }  // namespace web_app

chrome/browser/web_applications/test/web_app_install_observer.h

@@ -33,9 +33,15 @@ class WebAppInstallObserver final : public AppRegistrarObserver {
       base::RepeatingCallback<void(const AppId& app_id)>;
   void SetWebAppUninstalledDelegate(WebAppUninstalledDelegate delegate);
 
+  using WebAppProfileWillBeDeletedDelegate =
+      base::RepeatingCallback<void(const AppId& app_id)>;
+  void SetWebAppProfileWillBeDeletedDelegate(
+      WebAppProfileWillBeDeletedDelegate delegate);
+
   // AppRegistrarObserver:
   void OnWebAppInstalled(const AppId& app_id) override;
   void OnWebAppUninstalled(const AppId& app_id) override;
+  void OnWebAppProfileWillBeDeleted(const AppId& app_id) override;
 
  private:
   base::RunLoop run_loop_;
@@ -43,6 +49,7 @@ class WebAppInstallObserver final : public AppRegistrarObserver {
 
   WebAppInstalledDelegate app_installed_delegate_;
   WebAppUninstalledDelegate app_uninstalled_delegate_;
+  WebAppProfileWillBeDeletedDelegate app_profile_will_be_deleted_delegate_;
 
   ScopedObserver<AppRegistrar, AppRegistrarObserver> observer_{this};
 

chrome/browser/web_applications/web_app_provider.cc

@@ -156,6 +156,7 @@ void WebAppProvider::Shutdown() {
   manifest_update_manager_->Shutdown();
   system_web_app_manager_->Shutdown();
   install_finalizer_->Shutdown();
+  registrar_->Shutdown();
 }
 
 void WebAppProvider::StartImpl() {
@@ -271,6 +272,7 @@ void WebAppProvider::StartRegistryController() {
 void WebAppProvider::OnRegistryControllerReady() {
   DCHECK(!on_registry_ready_.is_signaled());
 
+  registrar_->Start();
   install_finalizer_->Start();
   external_web_app_manager_->Start();
   web_app_policy_manager_->Start();

chrome/browser/web_applications/web_app_registrar.cc

@@ -11,6 +11,8 @@
 #include "base/bind.h"
 #include "base/check_op.h"
 #include "base/strings/string_util.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/web_applications/web_app.h"
 
 namespace web_app {
@@ -24,6 +26,17 @@ const WebApp* WebAppRegistrar::GetAppById(const AppId& app_id) const {
   return it == registry_.end() ? nullptr : it->second.get();
 }
 
+void WebAppRegistrar::Start() {
+  // Profile manager can be null in unit tests.
+  if (g_browser_process->profile_manager())
+    g_browser_process->profile_manager()->AddObserver(this);
+}
+
+void WebAppRegistrar::Shutdown() {
+  if (g_browser_process->profile_manager())
+    g_browser_process->profile_manager()->RemoveObserver(this);
+}
+
 bool WebAppRegistrar::IsInstalled(const AppId& app_id) const {
   return GetAppById(app_id) != nullptr;
 }
@@ -125,6 +138,15 @@ WebAppRegistrar* WebAppRegistrar::AsWebAppRegistrar() {
   return this;
 }
 
+void WebAppRegistrar::OnProfileMarkedForPermanentDeletion(
+    Profile* profile_to_be_deleted) {
+  if (profile() != profile_to_be_deleted)
+    return;
+
+  for (const auto& app : AllApps())
+    NotifyWebAppProfileWillBeDeleted(app.app_id());
+}
+
 WebAppRegistrar::AppSet::AppSet(const WebAppRegistrar* registrar)
     : registrar_(registrar)
 #if DCHECK_IS_ON()

chrome/browser/web_applications/web_app_registrar.h

@@ -12,6 +12,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "chrome/browser/profiles/profile_manager_observer.h"
 #include "chrome/browser/web_applications/components/app_registrar.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
@@ -23,7 +24,7 @@ class WebApp;
 using Registry = std::map<AppId, std::unique_ptr<WebApp>>;
 
 // A registry model. This is a read-only container, which owns WebApp objects.
-class WebAppRegistrar : public AppRegistrar {
+class WebAppRegistrar : public AppRegistrar, public ProfileManagerObserver {
  public:
   explicit WebAppRegistrar(Profile* profile);
   ~WebAppRegistrar() override;
@@ -33,6 +34,8 @@ class WebAppRegistrar : public AppRegistrar {
   const WebApp* GetAppById(const AppId& app_id) const;
 
   // AppRegistrar:
+  void Start() override;
+  void Shutdown() override;
   bool IsInstalled(const AppId& app_id) const override;
   bool IsLocallyInstalled(const AppId& app_id) const override;
   bool WasInstalledByUser(const AppId& app_id) const override;
@@ -51,6 +54,10 @@ class WebAppRegistrar : public AppRegistrar {
   std::vector<AppId> GetAppIds() const override;
   WebAppRegistrar* AsWebAppRegistrar() override;
 
+  // ProfileManagerObserver:
+  void OnProfileMarkedForPermanentDeletion(
+      Profile* profile_to_be_deleted) override;
+
   // Only range-based |for| loop supported. Don't use AppSet directly.
   // Doesn't support registration and unregistration of WebApp while iterating.
   class AppSet {

chrome/test/BUILD.gn

@@ -1321,6 +1321,7 @@ if (!is_android) {
       "../browser/ui/web_applications/web_app_link_capturing_browsertest.cc",
       "../browser/ui/web_applications/web_app_metrics_browsertest.cc",
       "../browser/ui/web_applications/web_app_navigate_browsertest.cc",
+      "../browser/ui/web_applications/web_app_profile_deletion_browsertest.cc",
       "../browser/ui/web_applications/web_app_ui_manager_impl_browsertest.cc",
       "../browser/ui/web_applications/web_app_uninstall_browsertest.cc",
       "../browser/ui/webauthn/authenticator_dialog_browsertest.cc",