Reland "Fix the AppWindowLauncherItemController::GetAppWindow crash issue.""

This reverts commit 93ae228e.

Reason for revert: <INSERT REASONING HERE>
This CL relands CL:2586647, which is reverted because breaks the test:
PluginVmFilesTest.LaunchPluginVmApp

The reason is |mock_window| is accessed after free, because after the
end of the test, when launcher_item_controller is destroyed,
|mock_window| is accessed.

Modify the test case, to set the controller, so that |mock_window| can
be removed from |launcher_item_controller|'s windows list.

Modify AppWindowBase, to check whether widget_ is null in
GetNativeWindow to avoid crash, because widget_ is null in the test
case.

So CL:2586647 can be relanded again.

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

chrome/browser/chromeos/plugin_vm/plugin_vm_files.cc

@@ -23,6 +23,7 @@
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/ash/launcher/app_window_base.h"
 #include "chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chromeos/dbus/cicerone/cicerone_service.pb.h"
@@ -30,7 +31,6 @@
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
-#include "ui/base/base_window.h"
 
 namespace plugin_vm {
 
@@ -73,7 +73,7 @@ void FocusAllPluginVmWindows() {
   if (!launcher_item_controller) {
     return;
   }
-  for (ui::BaseWindow* app_window : launcher_item_controller->windows()) {
+  for (auto* app_window : launcher_item_controller->windows()) {
     app_window->Activate();
   }
 }

chrome/browser/chromeos/plugin_vm/plugin_vm_files_unittest.cc

@@ -17,6 +17,7 @@
 #include "chrome/browser/chromeos/plugin_vm/mock_plugin_vm_manager.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_manager_factory.h"
 #include "chrome/browser/chromeos/plugin_vm/plugin_vm_util.h"
+#include "chrome/browser/ui/ash/launcher/app_window_base.h"
 #include "chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/test/base/testing_profile.h"
@@ -26,8 +27,23 @@
 #include "content/public/test/browser_task_environment.h"
 #include "storage/browser/file_system/external_mount_points.h"
 #include "storage/common/file_system/file_system_types.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "ui/base/test/mock_base_window.h"
+
+namespace {
+
+class MockAppWindowBase : public AppWindowBase {
+ public:
+  MockAppWindowBase(const ash::ShelfID& shelf_id, views::Widget* widget)
+      : AppWindowBase(shelf_id, widget) {}
+  ~MockAppWindowBase() = default;
+  MockAppWindowBase(const MockAppWindowBase&) = delete;
+  MockAppWindowBase& operator=(const MockAppWindowBase&) = delete;
+
+  MOCK_METHOD(void, Activate, (), ());
+};
+
+}  // namespace
 
 namespace plugin_vm {
 
@@ -156,11 +172,12 @@ TEST_F(PluginVmFilesTest, LaunchPluginVmApp) {
   std::move(launch_plugin_vm_callback).Run(/*success=*/true);
   ASSERT_FALSE(cicerone_response_callback.is_null());
 
+  ash::ShelfID shelf_id(kPluginVmShelfAppId);
   auto launcher_item_controller =
-      std::make_unique<AppWindowLauncherItemController>(
-          ash::ShelfID(kPluginVmShelfAppId));
-  ui::test::MockBaseWindow mock_window;
+      std::make_unique<AppWindowLauncherItemController>(shelf_id);
+  MockAppWindowBase mock_window(shelf_id, nullptr);
   launcher_item_controller->AddWindow(&mock_window);
+  mock_window.SetController(launcher_item_controller.get());
   shelf_model.SetShelfItemDelegate(ash::ShelfID(kPluginVmShelfAppId),
                                    std::move(launcher_item_controller));
   vm_tools::cicerone::LaunchContainerApplicationResponse response;

chrome/browser/ui/ash/chrome_new_window_client.cc

@@ -31,6 +31,7 @@
 #include "chrome/browser/prefs/incognito_mode_prefs.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
+#include "chrome/browser/ui/ash/launcher/app_window_base.h"
 #include "chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
@@ -68,7 +69,6 @@
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
 #include "ui/aura/window.h"
-#include "ui/base/base_window.h"
 #include "ui/base/page_transition_types.h"
 #include "ui/base/window_open_disposition.h"
 #include "ui/display/types/display_constants.h"

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

@@ -22,6 +22,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/ash/launcher/app_service/app_service_app_window_launcher_controller.h"
 #include "chrome/browser/ui/ash/launcher/app_service/app_service_app_window_launcher_item_controller.h"
+#include "chrome/browser/ui/ash/launcher/app_window_base.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/ash/launcher/shelf_spinner_controller.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_window_manager_helper.h"
@@ -33,7 +34,6 @@
 #include "components/exo/shell_surface_util.h"
 #include "components/user_manager/user_manager.h"
 #include "ui/aura/window.h"
-#include "ui/base/base_window.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
 #include "ui/wm/core/window_util.h"
@@ -182,7 +182,7 @@ void AppServiceAppWindowCrostiniTracker::OnAppLaunchRequested(
   // necessary.
   if (!launcher_item_controller)
     return;
-  for (ui::BaseWindow* app_window : launcher_item_controller->windows()) {
+  for (AppWindowBase* app_window : launcher_item_controller->windows()) {
     activation_permissions_.emplace(
         app_window->GetNativeWindow(),
         exo::GrantPermissionToActivate(app_window->GetNativeWindow(),

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

@@ -553,23 +553,25 @@ void AppServiceAppWindowLauncherController::AddAppWindowToShelf(
 
   AppWindowLauncherItemController* item_controller =
       owner()->shelf_model()->GetAppWindowLauncherItemController(shelf_id);
-  if (item_controller == nullptr) {
-    auto controller =
-        std::make_unique<AppServiceAppWindowLauncherItemController>(shelf_id,
-                                                                    this);
-    item_controller = controller.get();
-    if (!owner()->GetItem(shelf_id)) {
-      owner()->CreateAppLauncherItem(std::move(controller),
-                                     ash::STATUS_RUNNING);
-    } else {
-      owner()->shelf_model()->SetShelfItemDelegate(shelf_id,
-                                                   std::move(controller));
-      owner()->SetItemStatus(shelf_id, ash::STATUS_RUNNING);
-    }
+  if (item_controller) {
+    item_controller->AddWindow(app_window);
+    app_window->SetController(item_controller);
+    return;
   }
 
+  auto controller = std::make_unique<AppServiceAppWindowLauncherItemController>(
+      shelf_id, this);
+  item_controller = controller.get();
   item_controller->AddWindow(app_window);
   app_window->SetController(item_controller);
+
+  if (!owner()->GetItem(shelf_id)) {
+    owner()->CreateAppLauncherItem(std::move(controller), ash::STATUS_RUNNING);
+  } else {
+    owner()->shelf_model()->SetShelfItemDelegate(shelf_id,
+                                                 std::move(controller));
+    owner()->SetItemStatus(shelf_id, ash::STATUS_RUNNING);
+  }
 }
 
 void AppServiceAppWindowLauncherController::RemoveAppWindowFromShelf(

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

@@ -10,6 +10,7 @@
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_utils.h"
 #include "chrome/browser/ui/ash/launcher/app_service/app_service_app_window_launcher_controller.h"
+#include "chrome/browser/ui/ash/launcher/app_window_base.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chromeos/ui/base/window_properties.h"
 #include "chromeos/ui/base/window_state_type.h"
@@ -43,7 +44,7 @@ void AppServiceAppWindowLauncherItemController::ItemSelected(
     // Tapping the shelf icon of an app that's showing PIP means expanding PIP.
     // Even if the app contains multiple windows, we just expand PIP without
     // showing the menu on the shelf icon.
-    for (ui::BaseWindow* window : windows()) {
+    for (AppWindowBase* window : windows()) {
       aura::Window* native_window = window->GetNativeWindow();
       if (native_window->GetProperty(chromeos::kWindowStateTypeKey) ==
           chromeos::WindowStateType::kPip) {

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

@@ -43,7 +43,7 @@ bool AppWindowBase::IsFullscreen() const {
 }
 
 gfx::NativeWindow AppWindowBase::GetNativeWindow() const {
-  return widget_->GetNativeWindow();
+  return widget_ ? widget_->GetNativeWindow() : nullptr;
 }
 
 gfx::Rect AppWindowBase::GetRestoredBounds() const {

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

@@ -11,11 +11,11 @@
 #include "ash/public/cpp/shelf_types.h"
 #include "ash/public/cpp/window_properties.h"
 #include "chrome/browser/ui/ash/ash_util.h"
+#include "chrome/browser/ui/ash/launcher/app_window_base.h"
 #include "chrome/browser/ui/ash/launcher/chrome_launcher_controller.h"
 #include "chrome/browser/ui/ash/launcher/launcher_controller_helper.h"
 #include "chrome/browser/ui/ash/launcher/shelf_context_menu.h"
 #include "ui/aura/client/aura_constants.h"
-#include "ui/base/base_window.h"
 #include "ui/wm/core/window_util.h"
 
 namespace {
@@ -36,7 +36,7 @@ ash::ShelfAction ShowAndActivateOrMinimize(ui::BaseWindow* app_window,
 // Returns the action performed. Should be one of SHELF_ACTION_NONE,
 // SHELF_ACTION_WINDOW_ACTIVATED, or SHELF_ACTION_WINDOW_MINIMIZED.
 ash::ShelfAction ActivateOrAdvanceToNextAppWindow(
-    ui::BaseWindow* window_to_show,
+    AppWindowBase* window_to_show,
     const AppWindowLauncherItemController::WindowList& windows) {
   DCHECK(window_to_show);
 
@@ -63,9 +63,14 @@ AppWindowLauncherItemController::AppWindowLauncherItemController(
     const ash::ShelfID& shelf_id)
     : ash::ShelfItemDelegate(shelf_id) {}
 
-AppWindowLauncherItemController::~AppWindowLauncherItemController() {}
+AppWindowLauncherItemController::~AppWindowLauncherItemController() {
+  for (auto* window : windows_)
+    window->SetController(nullptr);
+  for (auto* window : hidden_windows_)
+    window->SetController(nullptr);
+}
 
-void AppWindowLauncherItemController::AddWindow(ui::BaseWindow* app_window) {
+void AppWindowLauncherItemController::AddWindow(AppWindowBase* app_window) {
   aura::Window* window = app_window->GetNativeWindow();
   if (window && !observed_windows_.IsObserving(window))
     observed_windows_.Add(window);
@@ -80,12 +85,12 @@ AppWindowLauncherItemController::WindowList::iterator
 AppWindowLauncherItemController::GetFromNativeWindow(aura::Window* window,
                                                      WindowList& list) {
   return std::find_if(list.begin(), list.end(),
-                      [window](ui::BaseWindow* base_window) {
+                      [window](AppWindowBase* base_window) {
                         return base_window->GetNativeWindow() == window;
                       });
 }
 
-void AppWindowLauncherItemController::RemoveWindow(ui::BaseWindow* app_window) {
+void AppWindowLauncherItemController::RemoveWindow(AppWindowBase* app_window) {
   DCHECK(app_window);
   aura::Window* window = app_window->GetNativeWindow();
   if (window && observed_windows_.IsObserving(window))
@@ -105,7 +110,7 @@ void AppWindowLauncherItemController::RemoveWindow(ui::BaseWindow* app_window) {
   UpdateShelfItemIcon();
 }
 
-ui::BaseWindow* AppWindowLauncherItemController::GetAppWindow(
+AppWindowBase* AppWindowLauncherItemController::GetAppWindow(
     aura::Window* window,
     bool include_hidden) {
   auto iter = GetFromNativeWindow(window, windows_);
@@ -121,7 +126,7 @@ ui::BaseWindow* AppWindowLauncherItemController::GetAppWindow(
 
 void AppWindowLauncherItemController::SetActiveWindow(aura::Window* window) {
   // If the window is hidden, do not set it as last_active_window
-  ui::BaseWindow* app_window = GetAppWindow(window, false);
+  AppWindowBase* app_window = GetAppWindow(window, false);
   if (app_window)
     last_active_window_ = app_window;
   UpdateShelfItemIcon();
@@ -157,7 +162,7 @@ void AppWindowLauncherItemController::ItemSelected(
     last_active = nullptr;
   }
 
-  ui::BaseWindow* window_to_show =
+  AppWindowBase* window_to_show =
       last_active ? last_active : filtered_windows.front();
   // If the event was triggered by a keystroke, we try to advance to the next
   // item if the window we are trying to activate is already active.
@@ -252,7 +257,7 @@ void AppWindowLauncherItemController::OnWindowPropertyChanged(
   }
 }
 
-ui::BaseWindow* AppWindowLauncherItemController::GetLastActiveWindow() {
+AppWindowBase* AppWindowLauncherItemController::GetLastActiveWindow() {
   if (last_active_window_)
     return last_active_window_;
   if (windows_.empty())
@@ -260,13 +265,12 @@ ui::BaseWindow* AppWindowLauncherItemController::GetLastActiveWindow() {
   return windows_.front();
 }
 
-
 void AppWindowLauncherItemController::UpdateShelfItemIcon() {
   // Set the shelf item icon from the kAppIconKey property of the current
   // (or most recently) active window. If there is no valid icon, ask
   // ChromeLauncherController to update the icon.
   const gfx::ImageSkia* app_icon = nullptr;
-  ui::BaseWindow* last_active_window = GetLastActiveWindow();
+  AppWindowBase* last_active_window = GetLastActiveWindow();
   if (last_active_window && last_active_window->GetNativeWindow()) {
     app_icon = last_active_window->GetNativeWindow()->GetProperty(
         aura::client::kAppIconKey);

chrome/browser/ui/ash/launcher/app_window_launcher_item_controller.h

@@ -15,10 +15,7 @@
 #include "ui/aura/window.h"
 #include "ui/aura/window_observer.h"
 
-namespace ui {
-class BaseWindow;
-}
-
+class AppWindowBase;
 class ShelfContextMenu;
 
 // This is a ShelfItemDelegate for abstract app windows (extension or ARC).
@@ -30,16 +27,16 @@ class ShelfContextMenu;
 class AppWindowLauncherItemController : public ash::ShelfItemDelegate,
                                         public aura::WindowObserver {
  public:
-  using WindowList = std::list<ui::BaseWindow*>;
+  using WindowList = std::list<AppWindowBase*>;
 
   explicit AppWindowLauncherItemController(const ash::ShelfID& shelf_id);
   ~AppWindowLauncherItemController() override;
 
-  void AddWindow(ui::BaseWindow* window);
-  void RemoveWindow(ui::BaseWindow* window);
+  void AddWindow(AppWindowBase* window);
+  void RemoveWindow(AppWindowBase* window);
 
   void SetActiveWindow(aura::Window* window);
-  ui::BaseWindow* GetAppWindow(aura::Window* window, bool include_hidden);
+  AppWindowBase* GetAppWindow(aura::Window* window, bool include_hidden);
 
   // ash::ShelfItemDelegate overrides:
   AppWindowLauncherItemController* AsAppWindowLauncherItemController() override;
@@ -74,7 +71,7 @@ class AppWindowLauncherItemController : public ash::ShelfItemDelegate,
 
  protected:
   // Returns last active window in the controller or first window.
-  ui::BaseWindow* GetLastActiveWindow();
+  AppWindowBase* GetLastActiveWindow();
 
  private:
   friend class ChromeLauncherControllerTest;
@@ -100,7 +97,7 @@ class AppWindowLauncherItemController : public ash::ShelfItemDelegate,
   // Pointer to the most recently active app window
   // TODO(khmel): Get rid of |last_active_window_| and provide more reliable
   // way to determine active window.
-  ui::BaseWindow* last_active_window_ = nullptr;
+  AppWindowBase* last_active_window_ = nullptr;
 
   // Scoped list of observed windows (for removal on destruction)
   ScopedObserver<aura::Window, aura::WindowObserver> observed_windows_{this};