Revert "Fix a crash when switching to tablet mode in Unified Desktop mode"

This reverts commit dcb55ce2.

Reason for revert: Appears to cause crashes in LoginCursorTest.CursorHidden. See https://chromium-swarm.appspot.com/task?id=410f91874d359810&refresh=10&show_raw=1:
BrowserTestBase received signal: Segmentation fault. Backtrace:
#0 0x55c75309100f base::debug::StackTrace::StackTrace()
#1 0x55c752aa8075 content::(anonymous namespace)::DumpStackTraceSignalHandler()
#2 0x7f17e8b21cb0 <unknown>
#3 0x55c751991a1d chromeos::OobeUIDialogDelegate::~OobeUIDialogDelegate()
#4 0x55c751991c8e chromeos::OobeUIDialogDelegate::~OobeUIDialogDelegate()
#5 0x55c7553a6063 views::WebDialogView::OnDialogClosed()
#6 0x55c7553a5d92 views::WebDialogView::WindowClosing()
#7 0x55c752f81eb0 views::Widget::OnNativeWidgetDestroying()
#8 0x55c7548080ff views::DesktopWindowTreeHostMus::CloseNow()

Original change's description:
> 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.
> 
> BUG=900956, 902601
> TEST=Added a test that crashes without the fix.
> 
> Change-Id: If6eb9c2255dfa9d442aa115a3274db2d8a4110d7
> 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-Commit-Position: refs/heads/master@{#606687}

TBR=xiyuan@chromium.org,oshima@chromium.org,afakhry@chromium.org,jdufault@chromium.org,weidongg@chromium.org

Change-Id: I2e0cacc2c29bbc44e8e8c9dfcb86fd8106c008ff
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 900956, 902601
Reviewed-on: https://chromium-review.googlesource.com/c/1329004Reviewed-by: Patrik Höglund <phoglund@chromium.org>
Commit-Queue: Patrik Höglund <phoglund@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606808}

ash/app_list/app_list_controller_impl.cc

@@ -79,11 +79,9 @@ AppListControllerImpl::AppListControllerImpl()
   mojom::VoiceInteractionObserverPtr ptr;
   voice_interaction_binding_.Bind(mojo::MakeRequest(&ptr));
   Shell::Get()->voice_interaction_controller()->AddObserver(std::move(ptr));
-  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);
@@ -549,8 +547,15 @@ 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.
-  ShowHomeLauncher();
+  Show(GetDisplayIdToShowAppListOn(), app_list::kTabletMode, base::TimeTicks());
+  UpdateHomeLauncherVisibility();
+  Shelf::ForWindow(presenter_.GetWindow())->MaybeUpdateShelfBackground();
 }
 
 void AppListControllerImpl::OnTabletModeEnded() {
@@ -595,27 +600,6 @@ 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()
@@ -961,8 +945,7 @@ void AppListControllerImpl::UpdateAssistantVisibility() {
 }
 
 int64_t AppListControllerImpl::GetDisplayIdToShowAppListOn() {
-  if (IsHomeLauncherEnabledInTabletMode() &&
-      !Shell::Get()->display_manager()->IsInUnifiedMode()) {
+  if (IsHomeLauncherEnabledInTabletMode()) {
     return display::Display::HasInternalDisplay()
                ? display::Display::InternalDisplayId()
                : display::Screen::GetScreen()->GetPrimaryDisplay().id();
@@ -973,15 +956,4 @@ 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,7 +16,6 @@
 #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"
@@ -54,8 +53,7 @@ class ASH_EXPORT AppListControllerImpl
       public TabletModeObserver,
       public keyboard::KeyboardControllerObserver,
       public WallpaperControllerObserver,
-      public DefaultVoiceInteractionObserver,
-      public WindowTreeHostManager::Observer {
+      public DefaultVoiceInteractionObserver {
  public:
   using AppListItemMetadataPtr = mojom::AppListItemMetadataPtr;
   using SearchResultMetadataPtr = mojom::SearchResultMetadataPtr;
@@ -215,9 +213,6 @@ 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.
@@ -250,9 +245,6 @@ 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,6 +60,8 @@ 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,7 +5,6 @@
 #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"
@@ -3012,46 +3011,6 @@ 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,8 +119,6 @@ class CaptivePortalDialogDelegate
 
   void Hide() { widget_->Hide(); }
 
-  void Close() { widget_->Close(); }
-
   web_modal::WebContentsModalDialogHost* GetWebContentsModalDialogHost()
       override {
     return this;
@@ -231,8 +229,6 @@ OobeUIDialogDelegate::OobeUIDialogDelegate(
 }
 
 OobeUIDialogDelegate::~OobeUIDialogDelegate() {
-  captive_portal_delegate_->Close();
-
   if (controller_)
     controller_->OnDialogDestroyed(this);
 }