borealis: add a callback to remove anonymous apps when windows close

Anonymous apps are generated when borealis identifies a window with an
app_id it does not recognize.

In this CL we add the logic to remove those anonymous apps. When all the
windows close associated with that anonymous app, we say the app is
"finished", allowing observers to clean it up.

Bug: b/172979315
Change-Id: I5c1c0cfd2ed15fcd35474c53011cbce6c0733d51
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2562624
Commit-Queue: Nic Hollingum <hollingum@google.com>
Reviewed-by: Daniel Ng <danielng@google.com>
Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833553}

chrome/browser/apps/app_service/borealis_apps.cc

@@ -66,7 +66,7 @@ BorealisApps::BorealisApps(
     : profile_(profile) {
   Registry()->AddObserver(this);
 
-  anonymous_observation_.Observe(
+  anonymous_app_observation_.Observe(
       &borealis::BorealisService::GetForProfile(profile_)->WindowManager());
 
   PublisherBase::Initialize(app_service, apps::mojom::AppType::kBorealis);
@@ -219,8 +219,8 @@ void BorealisApps::OnRegistryUpdated(
   }
 }
 
-void BorealisApps::NewAnonymousAppDetected(const std::string& shelf_app_id,
-                                           const std::string& shelf_app_name) {
+void BorealisApps::OnAnonymousAppAdded(const std::string& shelf_app_id,
+                                       const std::string& shelf_app_name) {
   apps::mojom::AppPtr app = apps::PublisherBase::MakeApp(
       apps::mojom::AppType::kBorealis, shelf_app_id,
       apps::mojom::Readiness::kReady, shelf_app_name,
@@ -238,9 +238,21 @@ void BorealisApps::NewAnonymousAppDetected(const std::string& shelf_app_id,
   Publish(std::move(app), subscribers_);
 }
 
-void BorealisApps::WindowManagerWillBeDeleted(
+void BorealisApps::OnAnonymousAppRemoved(const std::string& shelf_app_id) {
+  // First uninstall the anonymous app, then remove it.
+  for (auto readiness : {apps::mojom::Readiness::kUninstalledByUser,
+                         apps::mojom::Readiness::kRemoved}) {
+    apps::mojom::AppPtr app = apps::mojom::App::New();
+    app->app_type = apps::mojom::AppType::kBorealis;
+    app->app_id = shelf_app_id;
+    app->readiness = readiness;
+    Publish(std::move(app), subscribers_);
+  }
+}
+
+void BorealisApps::OnWindowManagerDeleted(
     borealis::BorealisWindowManager* window_manager) {
-  anonymous_observation_.Reset();
+  anonymous_app_observation_.Reset();
 }
 
 }  // namespace apps

chrome/browser/apps/app_service/borealis_apps.h

@@ -75,9 +75,10 @@ class BorealisApps
       const std::vector<std::string>& inserted_apps) override;
 
   // borealis::BorealisWindowManager::AnonymousAppObserver overrides.
-  void NewAnonymousAppDetected(const std::string& shelf_app_id,
-                               const std::string& shelf_app_name) override;
-  void WindowManagerWillBeDeleted(
+  void OnAnonymousAppAdded(const std::string& shelf_app_id,
+                           const std::string& shelf_app_name) override;
+  void OnAnonymousAppRemoved(const std::string& shelf_app_id) override;
+  void OnWindowManagerDeleted(
       borealis::BorealisWindowManager* window_manager) override;
 
   mojo::RemoteSet<apps::mojom::Subscriber> subscribers_;
@@ -88,7 +89,7 @@ class BorealisApps
 
   base::ScopedObservation<borealis::BorealisWindowManager,
                           borealis::BorealisWindowManager::AnonymousAppObserver>
-      anonymous_observation_{this};
+      anonymous_app_observation_{this};
 };
 
 }  // namespace apps

chrome/browser/chromeos/borealis/borealis_window_manager.cc

@@ -6,11 +6,14 @@
 
 #include <string>
 
+#include "base/containers/flat_map.h"
+#include "base/containers/flat_set.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "chrome/browser/chromeos/borealis/borealis_util.h"
 #include "chrome/browser/chromeos/crostini/crostini_shelf_utils.h"
 #include "components/exo/shell_surface_util.h"
+#include "ui/aura/window.h"
 
 namespace {
 
@@ -30,6 +33,10 @@ const std::string* GetWindowId(aura::Window* window) {
   return exo::GetShellStartupId(window);
 }
 
+std::string WindowToAnonymousAppId(aura::Window* window) {
+  return kBorealisAnonymousPrefix + *GetWindowId(window);
+}
+
 // Returns a name for the app with the given |anon_id|.
 std::string AnonymousIdentifierToName(const std::string& anon_id) {
   return anon_id.substr(anon_id.find(kBorealisWindowPrefix) +
@@ -52,8 +59,16 @@ BorealisWindowManager::BorealisWindowManager(Profile* profile)
     : profile_(profile) {}
 
 BorealisWindowManager::~BorealisWindowManager() {
+  for (auto& id_to_windows : anon_ids_to_windows_) {
+    for (aura::Window* window : id_to_windows.second) {
+      window->RemoveObserver(this);
+    }
+    for (auto& observer : observers_) {
+      observer.OnAnonymousAppRemoved(id_to_windows.first);
+    }
+  }
   for (auto& observer : observers_) {
-    observer.WindowManagerWillBeDeleted(this);
+    observer.OnWindowManagerDeleted(this);
   }
   DCHECK(!observers_.might_have_observers());
 }
@@ -84,18 +99,35 @@ std::string BorealisWindowManager::GetShelfAppId(aura::Window* window) {
     return crostini_equivalent_id;
 
   // The app has no registration, it is anonymous.
-  std::string anon_id = kBorealisAnonymousPrefix + *GetWindowId(window);
-  HandleAnonymousApp(anon_id);
+  std::string anon_id = WindowToAnonymousAppId(window);
+  if (!anon_ids_to_windows_.contains(anon_id)) {
+    std::string anon_name = AnonymousIdentifierToName(anon_id);
+    for (auto& observer : observers_)
+      observer.OnAnonymousAppAdded(anon_id, anon_name);
+  }
+  // Add the window to the tracking set, and if it wasn't already there, add an
+  // observer.
+  if (anon_ids_to_windows_[anon_id].emplace(window).second) {
+    window->AddObserver(this);
+  }
   return anon_id;
 }
 
-void BorealisWindowManager::HandleAnonymousApp(const std::string& anon_id) {
-  if (known_anon_ids_.contains(anon_id))
+void BorealisWindowManager::OnWindowDestroying(aura::Window* window) {
+  std::string anon_id = WindowToAnonymousAppId(window);
+  base::flat_map<std::string, base::flat_set<aura::Window*>>::iterator iter =
+      anon_ids_to_windows_.find(anon_id);
+
+  DCHECK(iter != anon_ids_to_windows_.end());
+  DCHECK(iter->second.contains(window));
+
+  iter->second.erase(window);
+  if (!iter->second.empty())
     return;
-  known_anon_ids_.emplace(anon_id);
-  std::string anon_name = AnonymousIdentifierToName(anon_id);
+
   for (auto& observer : observers_)
-    observer.NewAnonymousAppDetected(anon_id, anon_name);
+    observer.OnAnonymousAppRemoved(anon_id);
+  anon_ids_to_windows_.erase(iter);
 }
 
 }  // namespace borealis

chrome/browser/chromeos/borealis/borealis_window_manager.h

@@ -7,9 +7,11 @@
 
 #include <string>
 
+#include "base/containers/flat_map.h"
 #include "base/containers/flat_set.h"
 #include "base/observer_list.h"
 #include "base/observer_list_types.h"
+#include "ui/aura/window_observer.h"
 
 class Profile;
 
@@ -19,7 +21,7 @@ class Window;
 
 namespace borealis {
 
-class BorealisWindowManager {
+class BorealisWindowManager : public aura::WindowObserver {
  public:
   // Returns true if this window belongs to a borealis VM (based on its app_id
   // and startup_id).
@@ -32,17 +34,22 @@ class BorealisWindowManager {
     // belongs too. The |shelf_app_name| represents the system's best-guess for
     // what the app should be called. This us usually not a localized string but
     // something we read from the window's properties.
-    virtual void NewAnonymousAppDetected(const std::string& shelf_app_id,
-                                         const std::string& shelf_app_name) = 0;
+    virtual void OnAnonymousAppAdded(const std::string& shelf_app_id,
+                                     const std::string& shelf_app_name) = 0;
+
+    // Called when the last window for the anonymous app with |shelf_app_id| is
+    // closed, and the app is no longer relevant.
+    virtual void OnAnonymousAppRemoved(const std::string& shelf_app_id) = 0;
+
     // Called when the window manager is being deleted. Observers should
     // unregister themselves from it.
-    virtual void WindowManagerWillBeDeleted(
+    virtual void OnWindowManagerDeleted(
         BorealisWindowManager* window_manager) = 0;
   };
 
   explicit BorealisWindowManager(Profile* profile);
 
-  ~BorealisWindowManager();
+  ~BorealisWindowManager() override;
 
   void AddObserver(AnonymousAppObserver* observer);
   void RemoveObserver(AnonymousAppObserver* observer);
@@ -50,10 +57,12 @@ class BorealisWindowManager {
   std::string GetShelfAppId(aura::Window* window);
 
  private:
-  void HandleAnonymousApp(const std::string& anon_id);
+  // aura::WindowObserver overrides.
+  void OnWindowDestroying(aura::Window* window) override;
 
   Profile* const profile_;
-  base::flat_set<std::string> known_anon_ids_;
+  base::flat_map<std::string, base::flat_set<aura::Window*>>
+      anon_ids_to_windows_;
   base::ObserverList<AnonymousAppObserver> observers_;
 };
 

chrome/browser/chromeos/borealis/borealis_window_manager_unittest.cc

@@ -22,11 +22,13 @@ class MockAnonObserver
     : public borealis::BorealisWindowManager::AnonymousAppObserver {
  public:
   MOCK_METHOD(void,
-              NewAnonymousAppDetected,
+              OnAnonymousAppAdded,
               (const std::string&, const std::string&),
               ());
 
-  MOCK_METHOD(void, WindowManagerWillBeDeleted, (BorealisWindowManager*), ());
+  MOCK_METHOD(void, OnAnonymousAppRemoved, (const std::string&), ());
+
+  MOCK_METHOD(void, OnWindowManagerDeleted, (BorealisWindowManager*), ());
 };
 
 class BorealisWindowManagerTest : public testing::Test {
@@ -64,7 +66,7 @@ TEST_F(BorealisWindowManagerTest, ObserverNotifiedOnManagerShutdown) {
   BorealisWindowManager window_manager(profile());
   window_manager.AddObserver(&observer);
 
-  EXPECT_CALL(observer, WindowManagerWillBeDeleted(&window_manager))
+  EXPECT_CALL(observer, OnWindowManagerDeleted(&window_manager))
       .WillOnce(testing::Invoke([&observer](BorealisWindowManager* wm) {
         wm->RemoveObserver(&observer);
       }));
@@ -72,9 +74,9 @@ TEST_F(BorealisWindowManagerTest, ObserverNotifiedOnManagerShutdown) {
 
 TEST_F(BorealisWindowManagerTest, ObserverCalledForAnonymousApp) {
   testing::StrictMock<MockAnonObserver> observer;
-  EXPECT_CALL(observer,
-              NewAnonymousAppDetected(testing::ContainsRegex("anonymous_app"),
-                                      testing::_));
+  EXPECT_CALL(
+      observer,
+      OnAnonymousAppAdded(testing::ContainsRegex("anonymous_app"), testing::_));
 
   BorealisWindowManager window_manager(profile());
   window_manager.AddObserver(&observer);
@@ -82,6 +84,35 @@ TEST_F(BorealisWindowManagerTest, ObserverCalledForAnonymousApp) {
       MakeWindow("org.chromium.borealis.anonymous_app");
   window_manager.GetShelfAppId(window.get());
 
+  EXPECT_CALL(observer,
+              OnAnonymousAppRemoved(testing::ContainsRegex("anonymous_app")));
+  window.reset();
+
+  window_manager.RemoveObserver(&observer);
+}
+
+TEST_F(BorealisWindowManagerTest, HandlesMultipleWindows) {
+  testing::StrictMock<MockAnonObserver> observer;
+
+  BorealisWindowManager window_manager(profile());
+  window_manager.AddObserver(&observer);
+
+  // We add an anonymous window for the same app twice, but we should only see
+  // one observer call.
+  EXPECT_CALL(observer, OnAnonymousAppAdded(testing::_, testing::_)).Times(1);
+
+  std::unique_ptr<aura::Window> window1 =
+      MakeWindow("org.chromium.borealis.anonymous_app");
+  window_manager.GetShelfAppId(window1.get());
+  std::unique_ptr<aura::Window> window2 =
+      MakeWindow("org.chromium.borealis.anonymous_app");
+  window_manager.GetShelfAppId(window2.get());
+
+  // We only expect to see the app removed after the last window closes.
+  window1.reset();
+  EXPECT_CALL(observer, OnAnonymousAppRemoved(testing::_)).Times(1);
+  window2.reset();
+
   window_manager.RemoveObserver(&observer);
 }