[Reland] Fix a crash when switching to tablet mode in Unified Desktop mode

1) The Home Launcher used to use the wrong display ID when in
   Unified Desktop mode.
2) If (1) is fixed, we hit https://crbug.com/902601. The captive
   portal dialog widget used to be leaked and never destroyed.
3) If (2) is fixed, we crash on the first attempt to press the
   app list button. The reason is tablet mode triggers a switch
   to mirror mode. This switch happens asynchronously after the
   Home Launcher had already been created. Switching from Unified
   to mirror mode destroys the Unified host and the Home Launcher.
   That's why we need to ensure that the Home Launcher is
   recreated.

TBR=oshima@chromium.org,weidongg@chromium.org
BUG=900956, 902601
TEST=Added a test that crashes without the fix.

Reviewed-on: https://chromium-review.googlesource.com/c/1325389
Commit-Queue: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Weidong Guo <weidongg@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#606687}
Change-Id: Ia0558bf1eba4234535b3826e671929e37db8dee1
Reviewed-on: https://chromium-review.googlesource.com/c/1329550Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606948}

ash/app_list/app_list_controller_impl.cc

@@ -76,9 +76,11 @@ AppListControllerImpl::AppListControllerImpl()
   }
 
   Shell::Get()->voice_interaction_controller()->AddLocalObserver(this);
+  Shell::Get()->window_tree_host_manager()->AddObserver(this);
 }
 
 AppListControllerImpl::~AppListControllerImpl() {
+  Shell::Get()->window_tree_host_manager()->RemoveObserver(this);
   keyboard::KeyboardController::Get()->RemoveObserver(this);
   Shell::Get()->RemoveShellObserver(this);
   Shell::Get()->wallpaper_controller()->RemoveObserver(this);
@@ -545,15 +547,8 @@ void AppListControllerImpl::OnTabletModeStarted() {
   if (!is_home_launcher_enabled_)
     return;
 
-  SessionController const* session_controller =
-      Shell::Get()->session_controller();
-  if (session_controller && !session_controller->IsActiveUserSessionStarted())
-    return;
-
   // Show the app list if the tablet mode starts.
-  Show(GetDisplayIdToShowAppListOn(), app_list::kTabletMode, base::TimeTicks());
-  UpdateHomeLauncherVisibility();
-  Shelf::ForWindow(presenter_.GetWindow())->MaybeUpdateShelfBackground();
+  ShowHomeLauncher();
 }
 
 void AppListControllerImpl::OnTabletModeEnded() {
@@ -598,6 +593,27 @@ void AppListControllerImpl::OnAssistantFeatureAllowedChanged(
   UpdateAssistantVisibility();
 }
 
+void AppListControllerImpl::OnDisplayConfigurationChanged() {
+  // Entering tablet mode triggers a display configuration change when we
+  // automatically switch to mirror mode. Switching to mirror mode happens
+  // asynchronously (see DisplayConfigurationObserver::OnTabletModeStarted()).
+  // This may result in the removal of a window tree host, as in the example of
+  // switching to tablet mode while Unified Desktop mode is on; the Unified host
+  // will be destroyed and the Home Launcher (which was created earlier when we
+  // entered tablet mode) will be dismissed.
+  // To avoid crashes, we must ensure that the Home Launcher shown status is as
+  // expected if it's enabled and we're still in tablet mode.
+  // https://crbug.com/900956.
+  const bool should_be_shown = IsHomeLauncherEnabledInTabletMode();
+  if (should_be_shown == GetTargetVisibility())
+    return;
+
+  if (should_be_shown)
+    ShowHomeLauncher();
+  else
+    DismissAppList();
+}
+
 bool AppListControllerImpl::IsHomeLauncherEnabledInTabletMode() const {
   return is_home_launcher_enabled_ && Shell::Get()
                                           ->tablet_mode_controller()
@@ -943,7 +959,8 @@ void AppListControllerImpl::UpdateAssistantVisibility() {
 }
 
 int64_t AppListControllerImpl::GetDisplayIdToShowAppListOn() {
-  if (IsHomeLauncherEnabledInTabletMode()) {
+  if (IsHomeLauncherEnabledInTabletMode() &&
+      !Shell::Get()->display_manager()->IsInUnifiedMode()) {
     return display::Display::HasInternalDisplay()
                ? display::Display::InternalDisplayId()
                : display::Screen::GetScreen()->GetPrimaryDisplay().id();
@@ -954,4 +971,15 @@ int64_t AppListControllerImpl::GetDisplayIdToShowAppListOn() {
       .id();
 }
 
+void AppListControllerImpl::ShowHomeLauncher() {
+  DCHECK(IsHomeLauncherEnabledInTabletMode());
+
+  if (!Shell::Get()->session_controller()->IsActiveUserSessionStarted())
+    return;
+
+  Show(GetDisplayIdToShowAppListOn(), app_list::kTabletMode, base::TimeTicks());
+  UpdateHomeLauncherVisibility();
+  Shelf::ForWindow(presenter_.GetWindow())->MaybeUpdateShelfBackground();
+}
+
 }  // namespace ash

ash/app_list/app_list_controller_impl.h

@@ -16,6 +16,7 @@
 #include "ash/app_list/model/search/search_model.h"
 #include "ash/app_list/presenter/app_list_presenter_impl.h"
 #include "ash/ash_export.h"
+#include "ash/display/window_tree_host_manager.h"
 #include "ash/public/cpp/app_list/app_list_constants.h"
 #include "ash/public/cpp/assistant/default_voice_interaction_observer.h"
 #include "ash/public/interfaces/app_list.mojom.h"
@@ -53,7 +54,8 @@ class ASH_EXPORT AppListControllerImpl
       public TabletModeObserver,
       public keyboard::KeyboardControllerObserver,
       public WallpaperControllerObserver,
-      public DefaultVoiceInteractionObserver {
+      public DefaultVoiceInteractionObserver,
+      public WindowTreeHostManager::Observer {
  public:
   using AppListItemMetadataPtr = mojom::AppListItemMetadataPtr;
   using SearchResultMetadataPtr = mojom::SearchResultMetadataPtr;
@@ -213,6 +215,9 @@ class ASH_EXPORT AppListControllerImpl
   void OnAssistantFeatureAllowedChanged(
       mojom::AssistantAllowedState state) override;
 
+  // WindowTreeHostManager::Observer:
+  void OnDisplayConfigurationChanged() override;
+
   bool onscreen_keyboard_shown() const { return onscreen_keyboard_shown_; }
 
   // Returns true if the home launcher is enabled in tablet mode.
@@ -245,6 +250,9 @@ class ASH_EXPORT AppListControllerImpl
 
   int64_t GetDisplayIdToShowAppListOn();
 
+  // Shows the home launcher in tablet mode.
+  void ShowHomeLauncher();
+
   base::string16 last_raw_query_;
 
   mojom::AppListClientPtr client_;

ash/display/display_configuration_observer.cc

@@ -60,8 +60,6 @@ void DisplayConfigurationObserver::StartMirrorMode() {
   // how to handle this scenario, and we shouldn't save this state.
   // https://crbug.com/733092.
   save_preference_ = false;
-  // TODO(oshima): Mirroring won't work with 3+ displays:
-  // https://crbug.com/737667.
   display::DisplayManager* display_manager = Shell::Get()->display_manager();
   was_in_mirror_mode_ = display_manager->IsInMirrorMode();
   display_manager->SetMirrorMode(display::MirrorMode::kNormal, base::nullopt);

ash/display/display_manager_unittest.cc

@@ -5,6 +5,7 @@
 #include "ui/display/manager/display_manager.h"
 
 #include "ash/accelerators/accelerator_commands.h"
+#include "ash/app_list/app_list_controller_impl.h"
 #include "ash/display/cursor_window_controller.h"
 #include "ash/display/display_configuration_controller.h"
 #include "ash/display/display_util.h"
@@ -3011,6 +3012,46 @@ TEST_F(DisplayManagerTest, UnifiedDesktopGridLayout3x2) {
                          .id());
 }
 
+TEST_F(DisplayManagerTest, UnifiedDesktopTabletMode) {
+  // Don't check root window destruction in unified mode.
+  Shell::GetPrimaryRootWindow()->RemoveObserver(this);
+
+  UpdateDisplay("400x300,800x800");
+  RunAllPendingInMessageLoop();
+
+  // Set the first display as internal display so that the tablet mode can be
+  // enabled.
+  display::test::DisplayManagerTestApi(display_manager())
+      .SetFirstDisplayAsInternalDisplay();
+
+  display_manager()->SetUnifiedDesktopEnabled(true);
+  EXPECT_TRUE(display_manager()->IsInUnifiedMode());
+
+  // Turn on tablet mode, expect that we switch to mirror mode without any
+  // crashes.
+  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(true);
+  RunAllPendingInMessageLoop();
+  EXPECT_TRUE(display_manager()->IsInSoftwareMirrorMode());
+
+  // The Home Launcher should be created and shown, not dismissed as a result of
+  // the destruction of the Unified host when we switched to mirror mode
+  // asynchronously.
+  auto* app_list_controller = Shell::Get()->app_list_controller();
+  EXPECT_TRUE(app_list_controller->IsHomeLauncherEnabledInTabletMode());
+  EXPECT_TRUE(app_list_controller->IsVisible());
+
+  // Exiting tablet mode should exit mirror mode and return back to Unified
+  // mode.
+  Shell::Get()->tablet_mode_controller()->EnableTabletModeWindowManager(false);
+  RunAllPendingInMessageLoop();
+  EXPECT_FALSE(display_manager()->IsInSoftwareMirrorMode());
+  EXPECT_TRUE(display_manager()->IsInUnifiedMode());
+
+  // Home Launcher should be dismissed.
+  EXPECT_FALSE(app_list_controller->IsHomeLauncherEnabledInTabletMode());
+  EXPECT_FALSE(app_list_controller->IsVisible());
+}
+
 TEST_F(DisplayManagerTest, DockMode) {
   const int64_t internal_id = 1;
   const int64_t external_id = 2;

chrome/browser/chromeos/login/ui/oobe_ui_dialog_delegate.cc

@@ -119,6 +119,8 @@ class CaptivePortalDialogDelegate
 
   void Hide() { widget_->Hide(); }
 
+  void Close() { widget_->Close(); }
+
   web_modal::WebContentsModalDialogHost* GetWebContentsModalDialogHost()
       override {
     return this;
@@ -180,12 +182,18 @@ class CaptivePortalDialogDelegate
 
   bool ShouldShowDialogTitle() const override { return false; }
 
+  base::WeakPtr<CaptivePortalDialogDelegate> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
  private:
   views::Widget* widget_ = nullptr;
   views::WebDialogView* view_ = nullptr;
   views::WebDialogView* host_view_ = nullptr;
   content::WebContents* web_contents_ = nullptr;
 
+  base::WeakPtrFactory<CaptivePortalDialogDelegate> weak_ptr_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(CaptivePortalDialogDelegate);
 };
 
@@ -220,7 +228,8 @@ OobeUIDialogDelegate::OobeUIDialogDelegate(
   extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
       dialog_view_->web_contents());
 
-  captive_portal_delegate_ = new CaptivePortalDialogDelegate(dialog_view_);
+  captive_portal_delegate_ =
+      (new CaptivePortalDialogDelegate(dialog_view_))->GetWeakPtr();
 
   GetOobeUI()->GetErrorScreen()->MaybeInitCaptivePortalWindowProxy(
       dialog_view_->web_contents());
@@ -229,6 +238,12 @@ OobeUIDialogDelegate::OobeUIDialogDelegate(
 }
 
 OobeUIDialogDelegate::~OobeUIDialogDelegate() {
+  // At shutdown, all widgets are closed. The order of destruction maybe
+  // different; i.e. the captive portal dialog might have been destroyed
+  // already. So we check the WeakPtr first.
+  if (captive_portal_delegate_)
+    captive_portal_delegate_->Close();
+
   if (controller_)
     controller_->OnDialogDestroyed(this);
 }

chrome/browser/chromeos/login/ui/oobe_ui_dialog_delegate.h

@@ -123,7 +123,7 @@ class OobeUIDialogDelegate : public display::DisplayObserver,
 
   base::WeakPtr<LoginDisplayHostMojo> controller_;
 
-  CaptivePortalDialogDelegate* captive_portal_delegate_ = nullptr;
+  base::WeakPtr<CaptivePortalDialogDelegate> captive_portal_delegate_;
 
   // This is owned by the underlying native widget.
   // Before its deletion, onDialogClosed will get called and delete this object.