split view: Fix adding/removing observers, transitioning to/from unified

When the MultiDisplayOverviewAndSplitView feature flag is disabled,
during transition to unified desktop mode, the destructors of
BackdropController and ShelfLayoutManager should call
SplitViewController::RemoveObserver on the split view controller for the
old primary root window rather than for the new primary root window. The
present CL addresses the problem by adjusting the timing of when
Shell::GetPrimaryRootWindow stops returning the old primary root and
starts returning the new primary root. The child CL has some LOG(ERROR)
code useful for verifying that the present CL serves its purpose.

Bug: 970013
Change-Id: I81074d4883a7d252d1da6a808976b47ad22cfc32
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869838
Commit-Queue: Avery Musbach <amusbach@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#708745}

ash/display/display_manager_unittest.cc

@@ -22,6 +22,7 @@
 #include "ash/screen_util.h"
 #include "ash/shelf/shelf.h"
 #include "ash/shell.h"
+#include "ash/shell_state.h"
 #include "ash/strings/grit/ash_strings.h"
 #include "ash/test/ash_test_base.h"
 #include "ash/wm/tablet_mode/tablet_mode_controller.h"
@@ -4619,4 +4620,37 @@ TEST_F(DisplayManagerTest, PanelOrientation) {
             screen_orientation_controller->GetCurrentOrientation());
 }
 
+TEST_F(DisplayManagerTest, UpdateRootWindowForNewWindows) {
+  Shell::GetPrimaryRootWindow()->RemoveObserver(this);
+
+  // Sets display configuration with three displays, sets the "root window for
+  // new windows" to the one at index before, removes the one at index 2, and
+  // checks that the "root window for new windows" is the one at index after.
+  const auto test_removing_secondary = [this](size_t before, size_t after) {
+    UpdateDisplay("800x600,800x600,800x600");
+    aura::Window::Windows root_windows = Shell::GetAllRootWindows();
+    Shell::Get()->shell_state()->SetRootWindowForNewWindows(
+        root_windows[before]);
+    UpdateDisplay("800x600,800x600");
+    EXPECT_EQ(root_windows[after], Shell::GetRootWindowForNewWindows());
+  };
+  test_removing_secondary(0u, 0u);
+  test_removing_secondary(1u, 1u);
+  test_removing_secondary(2u, 0u);
+
+  // Each iteration sets display configuration with three displays, sets the
+  // "root window for new windows" to the one at index before, enters unified
+  // desktop mode, and checks that the "root window for new windows" is the
+  // primary one.
+  for (size_t before = 0u; before < 3u; ++before) {
+    UpdateDisplay("800x600,800x600,800x600");
+    Shell::Get()->shell_state()->SetRootWindowForNewWindows(
+        Shell::GetAllRootWindows()[before]);
+    display_manager()->SetUnifiedDesktopEnabled(true);
+    EXPECT_EQ(Shell::GetPrimaryRootWindow(),
+              Shell::GetRootWindowForNewWindows());
+    display_manager()->SetUnifiedDesktopEnabled(false);
+  }
+}
+
 }  // namespace ash

ash/display/window_tree_host_manager.cc

@@ -22,6 +22,7 @@
 #include "ash/root_window_controller.h"
 #include "ash/root_window_settings.h"
 #include "ash/shell.h"
+#include "ash/shell_state.h"
 #include "ash/system/status_area_widget.h"
 #include "ash/system/unified/unified_system_tray.h"
 #include "ash/wm/window_util.h"
@@ -225,6 +226,7 @@ void WindowTreeHostManager::Shutdown() {
   }
   CHECK(primary_rwc);
 
+  Shell::Get()->shell_state()->SetRootWindowForNewWindows(nullptr);
   for (auto* rwc : to_delete)
     delete rwc;
   delete primary_rwc;
@@ -466,7 +468,8 @@ void WindowTreeHostManager::OnDisplayAdded(const display::Display& display) {
 
     AshWindowTreeHost* ash_host =
         AddWindowTreeHostForDisplay(display, AshWindowTreeHostInitParams());
-    RootWindowController::CreateForSecondaryDisplay(ash_host);
+    RootWindowController* new_root_window_controller =
+        RootWindowController::CreateForSecondaryDisplay(ash_host);
 
     // Magnifier controllers keep pointers to the current root window.
     // Update them here to avoid accessing them later.
@@ -478,15 +481,12 @@ void WindowTreeHostManager::OnDisplayAdded(const display::Display& display) {
             ash_host->AsWindowTreeHost()->window());
 
     AshWindowTreeHost* to_delete = primary_tree_host_for_replace_;
-    primary_tree_host_for_replace_ = nullptr;
 
     // Show the shelf if the original WTH had a visible system
     // tray. It may or may not be visible depending on OOBE state.
     RootWindowController* old_root_window_controller =
         RootWindowController::ForWindow(
             to_delete->AsWindowTreeHost()->window());
-    RootWindowController* new_root_window_controller =
-        ash::Shell::Get()->GetPrimaryRootWindowController();
     TrayBackgroundView* old_tray =
         old_root_window_controller->GetStatusAreaWidget()
             ->unified_system_tray();
@@ -498,16 +498,15 @@ void WindowTreeHostManager::OnDisplayAdded(const display::Display& display) {
       new_tray->GetWidget()->Show();
     }
 
-    DeleteHost(to_delete);
-#ifndef NDEBUG
-    auto iter = std::find_if(
-        window_tree_hosts_.begin(), window_tree_hosts_.end(),
+    // |to_delete| has already been removed from |window_tree_hosts_|.
+    DCHECK(std::none_of(
+        window_tree_hosts_.cbegin(), window_tree_hosts_.cend(),
         [to_delete](const std::pair<int64_t, AshWindowTreeHost*>& pair) {
           return pair.second == to_delete;
-        });
-    DCHECK(iter == window_tree_hosts_.end());
-#endif
-    // the host has already been removed from the window_tree_host_.
+        }));
+
+    DeleteHost(to_delete);
+    DCHECK(!primary_tree_host_for_replace_);
   } else if (primary_tree_host_for_replace_) {
     // TODO(oshima): It should be possible to consolidate logic for
     // unified and non unified, but I'm keeping them separated to minimize
@@ -537,15 +536,23 @@ void WindowTreeHostManager::OnDisplayAdded(const display::Display& display) {
 
 void WindowTreeHostManager::DeleteHost(AshWindowTreeHost* host_to_delete) {
   ClearDisplayPropertiesOnHost(host_to_delete);
+  aura::Window* root_being_deleted = GetWindow(host_to_delete);
   RootWindowController* controller =
-      RootWindowController::ForWindow(GetWindow(host_to_delete));
+      RootWindowController::ForWindow(root_being_deleted);
   DCHECK(controller);
-  controller->MoveWindowsTo(GetPrimaryRootWindow());
+  aura::Window* primary_root_after_host_deletion =
+      GetRootWindowForDisplayId(GetPrimaryDisplayId());
+  controller->MoveWindowsTo(primary_root_after_host_deletion);
   // Delete most of root window related objects, but don't delete
   // root window itself yet because the stack may be using it.
   controller->Shutdown();
   if (primary_tree_host_for_replace_ == host_to_delete)
     primary_tree_host_for_replace_ = nullptr;
+  DCHECK_EQ(primary_root_after_host_deletion, Shell::GetPrimaryRootWindow());
+  if (Shell::GetRootWindowForNewWindows() == root_being_deleted) {
+    Shell::Get()->shell_state()->SetRootWindowForNewWindows(
+        primary_root_after_host_deletion);
+  }
   // NOTE: ShelfWidget is gone, but Shelf still exists until this task runs.
   base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, controller);
 }

ash/root_window_controller.cc

@@ -39,7 +39,6 @@
 #include "ash/shelf/shelf_widget.h"
 #include "ash/shelf/shelf_window_targeter.h"
 #include "ash/shell.h"
-#include "ash/shell_state.h"
 #include "ash/system/status_area_layout_manager.h"
 #include "ash/system/status_area_widget.h"
 #include "ash/system/tray/tray_background_view.h"
@@ -423,14 +422,18 @@ RootWindowController::~RootWindowController() {
                                             this));
 }
 
-void RootWindowController::CreateForPrimaryDisplay(AshWindowTreeHost* host) {
+RootWindowController* RootWindowController::CreateForPrimaryDisplay(
+    AshWindowTreeHost* host) {
   RootWindowController* controller = new RootWindowController(host, nullptr);
   controller->Init(RootWindowType::PRIMARY);
+  return controller;
 }
 
-void RootWindowController::CreateForSecondaryDisplay(AshWindowTreeHost* host) {
+RootWindowController* RootWindowController::CreateForSecondaryDisplay(
+    AshWindowTreeHost* host) {
   RootWindowController* controller = new RootWindowController(host, nullptr);
   controller->Init(RootWindowType::SECONDARY);
+  return controller;
 }
 
 // static
@@ -577,9 +580,6 @@ void RootWindowController::Shutdown() {
       std::make_unique<aura::NullWindowTargeter>());
 
   touch_exploration_manager_.reset();
-
-  ResetRootForNewWindowsIfNecessary();
-
   wallpaper_widget_controller_.reset();
 
   CloseChildWindows();
@@ -1163,21 +1163,6 @@ void RootWindowController::CreateSystemWallpaper(
       new SystemWallpaperController(GetRootWindow(), color));
 }
 
-void RootWindowController::ResetRootForNewWindowsIfNecessary() {
-  // Change the target root window before closing child windows. If any child
-  // being removed triggers a relayout of the shelf it will try to build a
-  // window list adding windows from the target root window's containers which
-  // may have already gone away.
-  aura::Window* root = GetRootWindow();
-  if (Shell::GetRootWindowForNewWindows() == root) {
-    // The root window for new windows is being destroyed. Switch to the primary
-    // root window if possible.
-    aura::Window* primary_root = Shell::GetPrimaryRootWindow();
-    Shell::Get()->shell_state()->SetRootWindowForNewWindows(
-        primary_root == root ? nullptr : primary_root);
-  }
-}
-
 AccessibilityPanelLayoutManager*
 RootWindowController::GetAccessibilityPanelLayoutManager() const {
   aura::Window* container = const_cast<aura::Window*>(

ash/root_window_controller.h

@@ -75,10 +75,13 @@ class ASH_EXPORT RootWindowController {
   ~RootWindowController();
 
   // Creates and Initialize the RootWindowController for primary display.
-  static void CreateForPrimaryDisplay(AshWindowTreeHost* host);
+  // Returns a pointer to the newly created controller.
+  static RootWindowController* CreateForPrimaryDisplay(AshWindowTreeHost* host);
 
   // Creates and Initialize the RootWindowController for secondary displays.
-  static void CreateForSecondaryDisplay(AshWindowTreeHost* host);
+  // Returns a pointer to the newly created controller.
+  static RootWindowController* CreateForSecondaryDisplay(
+      AshWindowTreeHost* host);
 
   // Returns a RootWindowController of the window's root window.
   static RootWindowController* ForWindow(const aura::Window* window);
@@ -260,11 +263,6 @@ class ASH_EXPORT RootWindowController {
   // not this is the first boot.
   void CreateSystemWallpaper(RootWindowType root_window_type);
 
-  // Resets Shell::GetRootWindowForNewWindows() if appropriate. This is called
-  // during shutdown to make sure GetRootWindowForNewWindows() isn't referencing
-  // this.
-  void ResetRootForNewWindowsIfNecessary();
-
   // Callback for MenuRunner.
   void OnMenuClosed();