Fix the unit tests error in ASAN bots.

This CL is used to fix the two unit tests in ASAN bots:

The reason is when the window is closed, the item controller is not
removed, because it can't find the app type from AppService. When the
unit test exits, the window is deleted, then the item controller is
removed, which accesses the removed window.

Update extension_apps to SetWindowOwner when OnAppWindowAdded,
otherwise MultiUserWindowManagerHelper::ShowWindowForUser could fail
at ui side, because no owner for the window.

Update the OnInstances to find the corrent proxy, which might has the
instance info, because the window coud be teleported to from the
inactive user to the active user, but the instance should still be saved
in the inactive user's InstanceRegistry.

Add kHidden state to avoid reusing kStarted state.

BUG=1036648

Change-Id: I40b494dcf9fa5ea6febf29dad90150b51096e3d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1978342Reviewed-by: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Nancy Wang <nancylingwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#727931}

chrome/browser/apps/app_service/extension_apps.cc

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "ash/public/cpp/app_list/app_list_metrics.h"
+#include "ash/public/cpp/multi_user_window_manager.h"
 #include "ash/public/cpp/shelf_types.h"
 #include "base/bind.h"
 #include "base/callback.h"
@@ -29,6 +30,9 @@
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
 #include "chrome/browser/ui/app_list/extension_app_utils.h"
 #include "chrome/browser/ui/app_list/extension_uninstaller.h"
+#include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
+#include "chrome/browser/ui/ash/multi_user/multi_user_window_manager_helper.h"
+#include "chrome/browser/ui/ash/session_controller_client_impl.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
@@ -718,6 +722,14 @@ void ExtensionApps::OnAppWindowAdded(extensions::AppWindow* app_window) {
       app_window->GetNativeWindow(),
       [](const apps::InstanceUpdate& update) {}));
   app_window_to_aura_window_[app_window] = app_window->GetNativeWindow();
+
+  // Attach window to multi-user manager now to let it manage visibility state
+  // of the window correctly.
+  if (SessionControllerClientImpl::IsMultiProfileAvailable()) {
+    MultiUserWindowManagerHelper::GetWindowManager()->SetWindowOwner(
+        app_window->GetNativeWindow(),
+        multi_user_util::GetAccountIdFromProfile(profile_));
+  }
   RegisterInstance(app_window, InstanceState::kStarted);
 }
 
@@ -731,8 +743,12 @@ void ExtensionApps::OnAppWindowShown(extensions::AppWindow* app_window,
   instance_registry_->ForOneInstance(
       app_window->GetNativeWindow(),
       [&state](const apps::InstanceUpdate& update) { state = update.State(); });
+
+  // If the window is shown, it should be started, running and not hidden.
   state = static_cast<apps::InstanceState>(
       state | apps::InstanceState::kStarted | apps::InstanceState::kRunning);
+  state =
+      static_cast<apps::InstanceState>(state & ~apps::InstanceState::kHidden);
   RegisterInstance(app_window, state);
 }
 
@@ -741,9 +757,9 @@ void ExtensionApps::OnAppWindowHidden(extensions::AppWindow* app_window) {
     return;
   }
 
-  // For hidden |app_window|, the other state bit, running, active, and visible
-  // should be cleared, and the state is set back to the started state.
-  RegisterInstance(app_window, InstanceState::kStarted);
+  // For hidden |app_window|, the other state bit, started, running, active, and
+  // visible should be cleared.
+  RegisterInstance(app_window, InstanceState::kHidden);
 }
 
 void ExtensionApps::OnAppWindowRemoved(extensions::AppWindow* app_window) {

chrome/browser/ui/ash/launcher/app_service/app_service_app_window_launcher_controller.cc

@@ -40,8 +40,7 @@ AppServiceAppWindowLauncherController::AppServiceAppWindowLauncherController(
     : AppWindowLauncherController(owner),
       proxy_(apps::AppServiceProxyFactory::GetForProfile(owner->profile())),
       app_service_instance_helper_(
-          std::make_unique<AppServiceInstanceRegistryHelper>(
-              owner->profile())) {
+          std::make_unique<AppServiceInstanceRegistryHelper>(this)) {
   aura::Env::GetInstance()->AddObserver(this);
   DCHECK(proxy_);
   Observe(&proxy_->InstanceRegistry());
@@ -170,7 +169,7 @@ void AppServiceAppWindowLauncherController::OnWindowVisibilityChanged(
   if (arc_tracker_)
     arc_tracker_->OnWindowVisibilityChanged(window);
 
-  ash::ShelfID shelf_id = GetShelfId(window);
+  ash::ShelfID shelf_id = GetShelfId(window, false /*search_profile_list*/);
   if (shelf_id.IsNull())
     return;
 
@@ -205,7 +204,12 @@ void AppServiceAppWindowLauncherController::OnWindowDestroying(
   if (arc_tracker_)
     arc_tracker_->RemoveCandidateWindow(window);
 
-  const ash::ShelfID shelf_id = GetShelfId(window);
+  // When the window is destroyed, we should search all proxies, because the
+  // window could be teleported from the inactive user, and isn't saved in the
+  // proxy of the active user's profile, but it should still be removed from
+  // the controller, and the shelf, so search all the proxies.
+  const ash::ShelfID shelf_id =
+      GetShelfId(window, true /*search_profile_list*/);
   if (shelf_id.IsNull())
     return;
 
@@ -271,6 +275,12 @@ void AppServiceAppWindowLauncherController::OnInstanceUpdate(
     window->SetProperty(ash::kShelfIDKey, shelf_id.Serialize());
     window->SetProperty<int>(ash::kShelfItemTypeKey, ash::TYPE_APP);
 
+    // When an extension app window is added, calls UserHasAppOnActiveDesktop to
+    // handle teleport function.
+    if (update.BrowserContext() &&
+        (update.State() == apps::InstanceState::kStarted)) {
+      UserHasAppOnActiveDesktop(window, shelf_id, update.BrowserContext());
+    }
     // Apps opened in browser are managed by browser, so skip them.
     if (app_service_instance_helper_->IsOpenedInBrowser(shelf_id.app_id,
                                                         window) ||
@@ -281,33 +291,16 @@ void AppServiceAppWindowLauncherController::OnInstanceUpdate(
     return;
   }
 
-  // For Chrome apps, when it is shown, call UserHasAppOnActiveDesktop to handle
-  // teleport function.
-  if (update.BrowserContext() &&
-      (update.State() & apps::InstanceState::kStarted) !=
-          apps::InstanceState::kUnknown &&
-      (update.State() & apps::InstanceState::kRunning) !=
-          apps::InstanceState::kUnknown) {
-    UserHasAppOnActiveDesktop(window, shelf_id, update.BrowserContext());
-  }
-
   // Launch id is updated, so constructs a new shelf id.
   if (update.LaunchIdChanged()) {
     window->SetProperty(ash::kShelfIDKey, shelf_id.Serialize());
     window->SetProperty<int>(ash::kShelfItemTypeKey, ash::TYPE_APP);
   }
 
-  if (update.StateChanged() &&
-      update.State() == apps::InstanceState::kStarted) {
-    // For Chrome apps, when the app window is added or hidden, the state is set
-    // as kStarted, no kRunning/kActive/kVisible. For all other app types, it is
-    // impossible that the state is kStarted, because kStarted and kRunning are
-    // set together. So if the state is started, that means the Chrome app
-    // window is just added or hidden, and we don't need to add the app window
-    // to Shelf. When the app window is visible or activeted, it can be added to
-    // Shelf.
+  if (update.State() == apps::InstanceState::kHidden) {
+    // When the app window is hidden, it should be removed from Shelf.
     //
-    // The the window is teleported to the current user could be hidden as
+    // The window is teleported to the current user could be hidden as
     // well. But we only remove the window added for the active user, and skip
     // the window teleported to the current user, because
     // MultiUserWindowManagerHelper manages those windows.
@@ -390,7 +383,8 @@ void AppServiceAppWindowLauncherController::SetWindowActivated(
   if (!window)
     return;
 
-  const ash::ShelfID shelf_id = GetShelfId(window);
+  const ash::ShelfID shelf_id =
+      GetShelfId(window, false /*search_profile_list*/);
   if (shelf_id.IsNull())
     return;
 
@@ -511,7 +505,8 @@ void AppServiceAppWindowLauncherController::OnItemDelegateDiscarded(
 }
 
 ash::ShelfID AppServiceAppWindowLauncherController::GetShelfId(
-    aura::Window* window) const {
+    aura::Window* window,
+    bool search_profile_list) const {
   if (crostini_tracker_) {
     std::string shelf_app_id;
     shelf_app_id = crostini_tracker_->GetShelfAppId(window);
@@ -523,21 +518,50 @@ ash::ShelfID AppServiceAppWindowLauncherController::GetShelfId(
 
   // If the window exists in InstanceRegistry, get the shelf id from
   // InstanceRegistry.
-  bool exist_in_instance = proxy_->InstanceRegistry().ForOneInstance(
-      window, [&shelf_id](const apps::InstanceUpdate& update) {
-        shelf_id = ash::ShelfID(update.AppId(), update.LaunchId());
-      });
-  if (!exist_in_instance) {
-    shelf_id = ash::ShelfID::Deserialize(window->GetProperty(ash::kShelfIDKey));
-  }
-
-  if (!shelf_id.IsNull()) {
-    if (proxy_->AppRegistryCache().GetAppType(shelf_id.app_id) ==
-            apps::mojom::AppType::kUnknown &&
-        shelf_id.app_id != extension_misc::kChromeAppId) {
+  if (!search_profile_list) {
+    // Search from the proxy of the active user's profile, and verify whether
+    // the app exists in the proxy.
+    proxy_->InstanceRegistry().ForOneInstance(
+        window, [&shelf_id](const apps::InstanceUpdate& update) {
+          shelf_id = ash::ShelfID(update.AppId(), update.LaunchId());
+        });
+    if (shelf_id.IsNull()) {
+      shelf_id =
+          ash::ShelfID::Deserialize(window->GetProperty(ash::kShelfIDKey));
+    }
+    if (!shelf_id.IsNull()) {
+      if (proxy_->AppRegistryCache().GetAppType(shelf_id.app_id) ==
+              apps::mojom::AppType::kUnknown &&
+          shelf_id.app_id != extension_misc::kChromeAppId) {
+        return ash::ShelfID();
+      }
+      return shelf_id;
+    }
+  } else {
+    for (auto* profile : profile_list_) {
+      auto* proxy = apps::AppServiceProxyFactory::GetForProfile(profile);
+      if (proxy->InstanceRegistry().ForOneInstance(
+              window, [&shelf_id](const apps::InstanceUpdate& update) {
+                shelf_id = ash::ShelfID(update.AppId(), update.LaunchId());
+              })) {
+        break;
+      }
+    }
+    if (shelf_id.IsNull()) {
+      shelf_id =
+          ash::ShelfID::Deserialize(window->GetProperty(ash::kShelfIDKey));
+    }
+    if (!shelf_id.IsNull()) {
+      for (auto* profile : profile_list_) {
+        auto* proxy = apps::AppServiceProxyFactory::GetForProfile(profile);
+        if (proxy->AppRegistryCache().GetAppType(shelf_id.app_id) !=
+                apps::mojom::AppType::kUnknown ||
+            shelf_id.app_id == extension_misc::kChromeAppId) {
+          return shelf_id;
+        }
+      }
       return ash::ShelfID();
     }
-    return shelf_id;
   }
 
   // For null shelf id, it could be VM window or ARC apps window.

chrome/browser/ui/ash/launcher/app_service/app_service_app_window_launcher_controller.h

@@ -42,6 +42,8 @@ class AppServiceAppWindowLauncherController
       public apps::InstanceRegistry::Observer,
       public ArcAppWindowDelegate {
  public:
+  using ProfileList = std::vector<Profile*>;
+
   explicit AppServiceAppWindowLauncherController(
       ChromeLauncherController* owner);
   ~AppServiceAppWindowLauncherController() override;
@@ -97,10 +99,11 @@ class AppServiceAppWindowLauncherController
 
   AppWindowBase* GetAppWindow(aura::Window* window);
 
+  ProfileList& GetProfileList() { return profile_list_; }
+
  private:
   using AuraWindowToAppWindow =
       std::map<aura::Window*, std::unique_ptr<AppWindowBase>>;
-  using ProfileList = std::vector<Profile*>;
   using WindowList = std::vector<aura::Window*>;
 
   void SetWindowActivated(aura::Window* window, bool active);
@@ -118,7 +121,15 @@ class AppServiceAppWindowLauncherController
   // AppWindowLauncherController:
   void OnItemDelegateDiscarded(ash::ShelfItemDelegate* delegate) override;
 
-  ash::ShelfID GetShelfId(aura::Window* window) const;
+  // Returns the shelf id for |window|. The window could be teleported from the
+  // inactive user to the active user. When |search_profile_list| is true, we
+  // should check the all proxies for all profiles, otherwise, only check the
+  // current active user profile's proxy.
+  //
+  // When |window|'s visibility or activate status is changed,
+  // |search_profile_list| is false to check the active user profile only. When
+  // |window| is destroyed, |search_profile_list| is true to check all proxies.
+  ash::ShelfID GetShelfId(aura::Window* window, bool search_profile_list) const;
 
   // Register |window| if the owner of the given |window| has a window
   // teleported of the |window|'s application type to the current desktop.

chrome/browser/ui/ash/launcher/app_service/app_service_instance_registry_helper.cc

@@ -13,6 +13,8 @@
 #include "chrome/browser/apps/app_service/app_service_proxy_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/ash/launcher/app_service/app_service_app_window_launcher_controller.h"
+#include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_list.h"
@@ -24,10 +26,15 @@
 #include "extensions/common/constants.h"
 
 AppServiceInstanceRegistryHelper::AppServiceInstanceRegistryHelper(
-    Profile* profile)
-    : proxy_(apps::AppServiceProxyFactory::GetForProfile(profile)),
-      launcher_controller_helper_(
-          std::make_unique<LauncherControllerHelper>(profile)) {}
+    AppServiceAppWindowLauncherController* controller)
+    : controller_(controller),
+      proxy_(apps::AppServiceProxyFactory::GetForProfile(
+          controller->owner()->profile())),
+      launcher_controller_helper_(std::make_unique<LauncherControllerHelper>(
+          controller->owner()->profile())) {
+  DCHECK(controller_);
+  DCHECK(proxy_);
+}
 
 AppServiceInstanceRegistryHelper::~AppServiceInstanceRegistryHelper() = default;
 
@@ -162,7 +169,22 @@ void AppServiceInstanceRegistryHelper::OnInstances(const std::string& app_id,
 
   std::vector<std::unique_ptr<apps::Instance>> deltas;
   deltas.push_back(std::move(instance));
-  proxy_->InstanceRegistry().OnInstances(std::move(deltas));
+
+  // The window could be teleported from the inactive user's profile to the
+  // current active user, so search all proxies. If the instance is found from a
+  // proxy, still save to that proxy, otherwise, save to the current active user
+  // profile's proxy.
+  auto* proxy = proxy_;
+  for (auto* profile : controller_->GetProfileList()) {
+    auto* proxy_for_profile =
+        apps::AppServiceProxyFactory::GetForProfile(profile);
+    if (proxy_for_profile->InstanceRegistry().ForOneInstance(
+            window, [](const apps::InstanceUpdate& update) {})) {
+      proxy = proxy_for_profile;
+      break;
+    }
+  }
+  proxy->InstanceRegistry().OnInstances(std::move(deltas));
 }
 
 void AppServiceInstanceRegistryHelper::OnWindowVisibilityChanged(

chrome/browser/ui/ash/launcher/app_service/app_service_instance_registry_helper.h

@@ -22,12 +22,13 @@ namespace content {
 class WebContents;
 }
 
-class Profile;
+class AppServiceAppWindowLauncherController;
 
 // The helper class to operate the App Service Instance Registry.
 class AppServiceInstanceRegistryHelper {
  public:
-  explicit AppServiceInstanceRegistryHelper(Profile* profile);
+  explicit AppServiceInstanceRegistryHelper(
+      AppServiceAppWindowLauncherController* controller);
   ~AppServiceInstanceRegistryHelper();
 
   void ActiveUserChanged();
@@ -97,6 +98,8 @@ class AppServiceInstanceRegistryHelper {
   // Removes the tab's |window| from |browser_window_to_tab_window_|.
   void RemoveTabWindow(const std::string& app_id, aura::Window* window);
 
+  AppServiceAppWindowLauncherController* controller_ = nullptr;
+
   apps::AppServiceProxy* proxy_ = nullptr;
 
   // Used to get app info for tabs.

chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc

@@ -2568,6 +2568,7 @@ TEST_F(ChromeLauncherControllerWithArcTest, DISABLED_ArcCustomAppIcon) {
 TEST_F(ChromeLauncherControllerWithArcTest, ArcWindowPackageName) {
   InitLauncherController();
   SendListOfArcApps();
+  app_service_test().WaitForAppService();
 
   std::string window_app_id1("org.chromium.arc.1");
   std::string window_app_id2("org.chromium.arc.2");

chrome/services/app_service/public/cpp/instance.h

@@ -20,6 +20,7 @@ enum InstanceState {
   kRunning = 0x02,
   kActive = 0x04,
   kVisible = 0x08,
+  kHidden = 0x10,
   kDestroyed = 0x80,
 };