MacPWAs: Close shims when it is uninstalled for all profiles

Change AppShimManager::AppState::ShouldDeleteAppState to return true
if the app is not installed for any profiles. Previously, the app
would continue running even after it had been deleted, resulting in
all sorts of cool behaviors. This winds up being hit in two paths.

First path is the AppShimManager::OnAppDeactivated call that is made
when a profile closes its last app window. This fixes crbug.com/1139254
where an app kept running even after it was uninstalled.

Second path is AppShimManager::OnProfileMarkedForPermanentDeletion,
for when a profile is deleted. This is for crbug.com/1132223 and is
a bit more involved.
* Listening for chrome::NOTIFICATION_PROFILE_CREATED/DESTROYED has
  been deprecated for a half-decade, so change AppShimManager to be
  a ProfileManagerObserver instead.
* Inline AppShimManager::CloseShimsForProfile to be a lambda inside
  of OnProfileMarkedForPermanentDeletion.
* Of note about ProfileManagerObserver is that closing app shims
  inside OnProfileMarkedForPermanentDeletion can cause crashes
  because it is not expected that Browser instances will be closed
  inside that call. For that reason, post a task to do the work.

Update tests (because now they're affected by the AppShimRegistry)
and add tests for both paths (shim installed vs uninstalled).

Since AppShimManager is now a ProfileManagerObserver we have to
make sure that the AppShimManager stops observing the ProfileManager
before the ProfileManager is destroyed. To do this, a new call,
BrowserProcessPlatformPart::BeginStartTearDown, which happens at
the beginning of BrowserProcessPlatformPart::StartTearDown.

Bug: 1139254,1132223
Change-Id: I19990f6ae5ff8d8727e1fb0ecf46d1142b543764
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2481790Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#818594}

chrome/browser/apps/app_shim/app_shim_manager_mac.cc

@@ -219,11 +219,14 @@ bool AppShimManager::AppState::ShouldDeleteAppState() const {
   // The new behavior for multi-profile apps is to not close the app based on
   // which windows are open. Rather, the app must be explicitly closed via
   // the Quit menu, which will terminate the app (and the browser will be
-  // notified of the closed mojo pipe).
-  // https://crbug.com/1080729
+  // notified of the closed mojo pipe). The app is closed automatically when
+  // it has been uninstalled for all profiles.
+  // https://crbug.com/1080729 for new behavior.
+  // https://crbug.com/1139254,1132223 for closing when profiles close.
   if (IsMultiProfile() &&
       base::FeatureList::IsEnabled(features::kAppShimNewCloseBehavior)) {
-    return false;
+    return profiles.empty() &&
+           AppShimRegistry::Get()->GetInstalledProfilesForApp(app_id).empty();
   }
 
   // The old behavior, and the behavior for single-profile apps, is to close
@@ -243,16 +246,12 @@ void AppShimManager::AppState::SaveLastActiveProfiles() const {
 }
 
 AppShimManager::AppShimManager(std::unique_ptr<Delegate> delegate)
-    : delegate_(std::move(delegate)), weak_factory_(this) {
+    : delegate_(std::move(delegate)),
+      profile_manager_(g_browser_process->profile_manager()),
+      weak_factory_(this) {
   AppShimHostBootstrap::SetClient(this);
-  // This is instantiated in BrowserProcessImpl::PreMainMessageLoopRun with
-  // AppShimListener. Since PROFILE_CREATED is not fired until
-  // ProfileManager::GetLastUsedProfile/GetLastOpenedProfiles, this should catch
-  // notifications for all profiles.
-  registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
-                 content::NotificationService::AllBrowserContextsAndSources());
-  registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED,
-                 content::NotificationService::AllBrowserContextsAndSources());
+  if (profile_manager_)
+    profile_manager_->AddObserver(this);
   BrowserList::AddObserver(this);
 }
 
@@ -261,6 +260,14 @@ AppShimManager::~AppShimManager() {
   AppShimHostBootstrap::SetClient(nullptr);
 }
 
+void AppShimManager::OnBeginTearDown() {
+  avatar_menu_.reset();
+  if (profile_manager_)
+    profile_manager_->RemoveObserver(this);
+  profile_manager_ = nullptr;
+  weak_factory_.InvalidateWeakPtrs();
+}
+
 AppShimHost* AppShimManager::FindHost(Profile* profile,
                                       const web_app::AppId& app_id) {
   auto found_app = apps_.find(app_id);
@@ -626,18 +633,6 @@ AppShimManager* AppShimManager::Get() {
   return g_browser_process->platform_part()->app_shim_manager();
 }
 
-void AppShimManager::CloseShimsForProfile(Profile* profile) {
-  for (auto iter_app = apps_.begin(); iter_app != apps_.end();) {
-    AppState* app_state = iter_app->second.get();
-    app_state->profiles.erase(profile);
-
-    if (app_state->ShouldDeleteAppState())
-      iter_app = apps_.erase(iter_app);
-    else
-      ++iter_app;
-  }
-}
-
 void AppShimManager::LoadProfileAndApp(const base::FilePath& profile_path,
                                        const web_app::AppId& app_id,
                                        LoadProfileAndAppCallback callback) {
@@ -710,19 +705,19 @@ bool AppShimManager::IsAcceptablyCodeSigned(pid_t pid) const {
 }
 
 Profile* AppShimManager::ProfileForPath(const base::FilePath& full_path) {
-  ProfileManager* profile_manager = g_browser_process->profile_manager();
-  Profile* profile = profile_manager->GetProfileByPath(full_path);
+  if (!profile_manager_)
+    return nullptr;
+  Profile* profile = profile_manager_->GetProfileByPath(full_path);
 
   // Use IsValidProfile to check if the profile has been created.
-  return profile && profile_manager->IsValidProfile(profile) ? profile
-                                                             : nullptr;
+  return profile && profile_manager_->IsValidProfile(profile) ? profile
+                                                              : nullptr;
 }
 
 void AppShimManager::LoadProfileAsync(
     const base::FilePath& full_path,
     base::OnceCallback<void(Profile*)> callback) {
-  ProfileManager* profile_manager = g_browser_process->profile_manager();
-  profile_manager->LoadProfileByPath(full_path, false, std::move(callback));
+  profile_manager_->LoadProfileByPath(full_path, false, std::move(callback));
 }
 
 void AppShimManager::WaitForAppRegistryReadyAsync(
@@ -755,7 +750,7 @@ void AppShimManager::OpenAppURLInBrowserWindow(
   Profile* profile =
       profile_path.empty() ? nullptr : ProfileForPath(profile_path);
   if (!profile)
-    profile = g_browser_process->profile_manager()->GetLastUsedProfile();
+    profile = profile_manager_->GetLastUsedProfile();
   if (!profile)
     return;
   Browser* browser =
@@ -858,34 +853,39 @@ void AppShimManager::OnShimSelectedProfile(AppShimHost* host,
                    std::vector<base::FilePath>(), base::DoNothing());
 }
 
-void AppShimManager::Observe(int type,
-                             const content::NotificationSource& source,
-                             const content::NotificationDetails& details) {
-  switch (type) {
-    case chrome::NOTIFICATION_PROFILE_CREATED: {
-      Profile* profile = content::Source<Profile>(source).ptr();
-      if (profile->IsOffTheRecord())
-        return;
-
-      AppLifetimeMonitorFactory::GetForBrowserContext(profile)->AddObserver(
-          this);
-      break;
-    }
-    case chrome::NOTIFICATION_PROFILE_DESTROYED: {
-      Profile* profile = content::Source<Profile>(source).ptr();
-      if (profile->IsOffTheRecord())
-        return;
-
-      AppLifetimeMonitorFactory::GetForBrowserContext(profile)->RemoveObserver(
-          this);
-      CloseShimsForProfile(profile);
-      break;
-    }
-    default: {
-      NOTREACHED();  // Unexpected notification.
-      break;
+void AppShimManager::OnProfileAdded(Profile* profile) {
+  if (profile->IsOffTheRecord())
+    return;
+
+  AppLifetimeMonitorFactory::GetForBrowserContext(profile)->AddObserver(this);
+}
+
+void AppShimManager::OnProfileMarkedForPermanentDeletion(Profile* profile) {
+  if (profile->IsOffTheRecord())
+    return;
+
+  AppLifetimeMonitorFactory::GetForBrowserContext(profile)->RemoveObserver(
+      this);
+
+  // Close app shims that were kept alive only for this profile. Note that this
+  // must be done as a posted task because closing shims may result in closing
+  // windows midway through BrowserList::TryToCloseBrowserList, which does not
+  // expect that behavior, and may result in crashes.
+  auto close_shims_lambda = [](base::WeakPtr<AppShimManager> manager) {
+    if (!manager)
+      return;
+    for (auto iter_app = manager->apps_.begin();
+         iter_app != manager->apps_.end();) {
+      AppState* app_state = iter_app->second.get();
+      if (app_state->ShouldDeleteAppState())
+        iter_app = manager->apps_.erase(iter_app);
+      else
+        ++iter_app;
     }
-  }
+  };
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(close_shims_lambda, weak_factory_.GetWeakPtr()));
 }
 
 void AppShimManager::OnAppStart(content::BrowserContext* context,
@@ -978,9 +978,8 @@ void AppShimManager::UpdateAllProfileMenus() {
 
 void AppShimManager::RebuildProfileMenuItemsFromAvatarMenu() {
   if (!avatar_menu_) {
-    ProfileManager* profile_manager = g_browser_process->profile_manager();
     avatar_menu_ = std::make_unique<AvatarMenu>(
-        &profile_manager->GetProfileAttributesStorage(), this, nullptr);
+        &profile_manager_->GetProfileAttributesStorage(), this, nullptr);
   }
   avatar_menu_->RebuildMenu();
   profile_menu_items_.clear();

chrome/browser/apps/app_shim/app_shim_manager_mac.h

@@ -18,13 +18,13 @@
 #include "chrome/browser/apps/app_shim/app_shim_host_bootstrap_mac.h"
 #include "chrome/browser/apps/app_shim/app_shim_host_mac.h"
 #include "chrome/browser/profiles/avatar_menu_observer.h"
+#include "chrome/browser/profiles/profile_manager_observer.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 
 class Profile;
+class ProfileManager;
 
 namespace base {
 class FilePath;
@@ -40,10 +40,10 @@ namespace apps {
 // extension.
 class AppShimManager : public AppShimHostBootstrap::Client,
                        public AppShimHost::Client,
-                       public content::NotificationObserver,
                        public AppLifetimeMonitor::Observer,
                        public BrowserListObserver,
-                       public AvatarMenuObserver {
+                       public AvatarMenuObserver,
+                       public ProfileManagerObserver {
  public:
   class Delegate {
    public:
@@ -113,6 +113,10 @@ class AppShimManager : public AppShimHostBootstrap::Client,
   explicit AppShimManager(std::unique_ptr<Delegate> delegate);
   ~AppShimManager() override;
 
+  // Called at the beginning of browser shut down. Is used to remove |this| as
+  // a ProfileManager and AvatarMenuObserver observer.
+  void OnBeginTearDown();
+
   // Get the host corresponding to a profile and app id, or null if there is
   // none.
   AppShimHost* FindHost(Profile* profile, const web_app::AppId& app_id);
@@ -155,10 +159,9 @@ class AppShimManager : public AppShimHostBootstrap::Client,
   void OnAppStop(content::BrowserContext* context,
                  const std::string& app_id) override;
 
-  // content::NotificationObserver overrides:
-  void Observe(int type,
-               const content::NotificationSource& source,
-               const content::NotificationDetails& details) override;
+  // ProfileManagerObserver overrides:
+  void OnProfileAdded(Profile* profile) override;
+  void OnProfileMarkedForPermanentDeletion(Profile* profile) override;
 
   // BrowserListObserver overrides;
   void OnBrowserAdded(Browser* browser) override;
@@ -213,9 +216,6 @@ class AppShimManager : public AppShimHostBootstrap::Client,
   // quitting due to apps being open.
   virtual void MaybeTerminate();
 
-  // Exposed for testing.
-  content::NotificationRegistrar& registrar() { return registrar_; }
-
   // Called when profile menu items may have changed. Rebuilds the profile
   // menu item list and sends updated lists to all apps.
   void UpdateAllProfileMenus();
@@ -311,11 +311,12 @@ class AppShimManager : public AppShimHostBootstrap::Client,
   ProfileState* GetOrCreateProfileState(Profile* profile,
                                         const web_app::AppId& app_id);
 
+  // Weak, reset during OnBeginTearDown.
+  ProfileManager* profile_manager_ = nullptr;
+
   // Map from extension id to the state for that app.
   std::map<std::string, std::unique_ptr<AppState>> apps_;
 
-  content::NotificationRegistrar registrar_;
-
   // The avatar menu instance used by all app shims.
   std::unique_ptr<AvatarMenu> avatar_menu_;
 

chrome/browser/apps/app_shim/app_shim_manager_mac_unittest.cc

@@ -159,8 +159,6 @@ class TestingAppShimManager : public AppShimManager {
     return load_profile_callbacks_.erase(path);
   }
 
-  content::NotificationRegistrar& GetRegistrar() { return registrar(); }
-
  private:
   std::map<base::FilePath, base::OnceCallback<void(Profile*)>>
       load_profile_callbacks_;
@@ -621,6 +619,10 @@ TEST_F(AppShimManagerTest, AppLifetime) {
   scoped_feature_list_.InitWithFeatures({features::kAppShimNewCloseBehavior},
                                         {});
 
+  // This app is installed for profile A throughout this test.
+  AppShimRegistry::Get()->OnAppInstalledForProfile(kTestAppIdA,
+                                                   profile_path_a_);
+
   // When the app activates, a host is created. If there is no shim, one is
   // launched.
   manager_->SetHostForCreate(std::move(host_aa_unique_));
@@ -821,6 +823,11 @@ TEST_F(AppShimManagerTest, MaybeTerminate) {
   scoped_feature_list_.InitWithFeatures({features::kAppShimNewCloseBehavior},
                                         {});
 
+  AppShimRegistry::Get()->OnAppInstalledForProfile(kTestAppIdA,
+                                                   profile_path_a_);
+  AppShimRegistry::Get()->OnAppInstalledForProfile(kTestAppIdB,
+                                                   profile_path_a_);
+
   // Launch shims, adding entries in the map.
   RegisterOnlyLaunch(bootstrap_aa_, std::move(host_aa_unique_));
   EXPECT_EQ(chrome::mojom::AppShimLaunchResult::kSuccess,
@@ -836,8 +843,42 @@ TEST_F(AppShimManagerTest, MaybeTerminate) {
   EXPECT_CALL(*manager_, MaybeTerminate()).Times(0);
   manager_->OnAppDeactivated(&profile_a_, kTestAppIdA);
 
-  // Quitting when it's the last shim should terminate.
+  // Quitting when it's the last shim should not terminate in the new behavior.
+  EXPECT_CALL(*manager_, MaybeTerminate()).Times(0);
+  manager_->OnAppDeactivated(&profile_a_, kTestAppIdB);
+}
+
+TEST_F(AppShimManagerTest, MaybeTerminateOnUninstall) {
+  scoped_feature_list_.InitWithFeatures({features::kAppShimNewCloseBehavior},
+                                        {});
+
+  AppShimRegistry::Get()->OnAppInstalledForProfile(kTestAppIdA,
+                                                   profile_path_a_);
+  AppShimRegistry::Get()->OnAppInstalledForProfile(kTestAppIdB,
+                                                   profile_path_a_);
+
+  // Launch shims, adding entries in the map.
+  RegisterOnlyLaunch(bootstrap_aa_, std::move(host_aa_unique_));
+  EXPECT_EQ(chrome::mojom::AppShimLaunchResult::kSuccess,
+            *bootstrap_aa_result_);
+  EXPECT_EQ(host_aa_.get(), manager_->FindHost(&profile_a_, kTestAppIdA));
+
+  RegisterOnlyLaunch(bootstrap_ab_, std::move(host_ab_unique_));
+  EXPECT_EQ(chrome::mojom::AppShimLaunchResult::kSuccess,
+            *bootstrap_ab_result_);
+  EXPECT_EQ(host_ab_.get(), manager_->FindHost(&profile_a_, kTestAppIdB));
+
+  // Quitting when there's another shim should not terminate.
+  AppShimRegistry::Get()->OnAppUninstalledForProfile(kTestAppIdA,
+                                                     profile_path_a_);
   EXPECT_CALL(*manager_, MaybeTerminate()).Times(0);
+  manager_->OnAppDeactivated(&profile_a_, kTestAppIdA);
+
+  // Quitting when it's the last shim and the app is uninstalled should
+  // terminate.
+  AppShimRegistry::Get()->OnAppUninstalledForProfile(kTestAppIdB,
+                                                     profile_path_a_);
+  EXPECT_CALL(*manager_, MaybeTerminate()).Times(1);
   manager_->OnAppDeactivated(&profile_a_, kTestAppIdB);
 }
 

chrome/browser/browser_process_impl.cc

@@ -370,6 +370,8 @@ void BrowserProcessImpl::StartTearDown() {
   tearing_down_ = true;
   DCHECK(IsShuttingDown());
 
+  platform_part()->BeginStartTearDown();
+
   metrics_services_manager_.reset();
   intranet_redirect_detector_.reset();
   if (safe_browsing_service_.get())

chrome/browser/browser_process_platform_part_base.cc

@@ -18,6 +18,8 @@ void BrowserProcessPlatformPartBase::PlatformSpecificCommandLineProcessing(
     const base::CommandLine& /* command_line */) {
 }
 
+void BrowserProcessPlatformPartBase::BeginStartTearDown() {}
+
 void BrowserProcessPlatformPartBase::StartTearDown() {
 }
 

chrome/browser/browser_process_platform_part_base.h

@@ -26,7 +26,10 @@ class BrowserProcessPlatformPartBase {
   virtual void PlatformSpecificCommandLineProcessing(
       const base::CommandLine& command_line);
 
-  // Called from BrowserProcessImpl::StartTearDown().
+  // Called at the very beginning of BrowserProcessImpl::StartTearDown().
+  virtual void BeginStartTearDown();
+
+  // Called in the middle of BrowserProcessImpl::StartTearDown().
   virtual void StartTearDown();
 
   // Called from AttemptExitInternal().

chrome/browser/browser_process_platform_part_mac.h

@@ -23,6 +23,7 @@ class BrowserProcessPlatformPart : public BrowserProcessPlatformPartBase {
   ~BrowserProcessPlatformPart() override;
 
   // Overridden from BrowserProcessPlatformPartBase:
+  void BeginStartTearDown() override;
   void StartTearDown() override;
   void AttemptExit(bool try_to_quit_application) override;
   void PreMainMessageLoopRun() override;

chrome/browser/browser_process_platform_part_mac.mm

@@ -21,6 +21,11 @@ BrowserProcessPlatformPart::BrowserProcessPlatformPart() {
 BrowserProcessPlatformPart::~BrowserProcessPlatformPart() {
 }
 
+void BrowserProcessPlatformPart::BeginStartTearDown() {
+  if (app_shim_manager_)
+    app_shim_manager_->OnBeginTearDown();
+}
+
 void BrowserProcessPlatformPart::StartTearDown() {
   app_shim_listener_ = nullptr;
 }