Reland: cros: Cleanup ShelfWidget shutdown

Make it use DeleteDelegate() to be more consistent with
StatusAreaWidgetDelegate. Defer closing the widget until all the
top-level windows are closed. Don't try to close all windows more than
once during shutdown, which simplifies the shelf cleanup code.

Originally landed as cc4be1cb.

That version exposed a use-after-free in ash drag and drop code,
which was fixed separately. See https://crbug.com/818603

TBR=sadrul@chromium.org

Bug: 628655, 818603
Test: ash_unittests
Change-Id: I4a8ade351456b476a19225f2c740e031f6b0e122
Reviewed-on: https://chromium-review.googlesource.com/956924
Commit-Queue: James Cook <jamescook@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542308}

ash/magnifier/magnification_controller.cc

@@ -555,6 +555,9 @@ void MagnificationControllerImpl::SwitchTargetRootWindow(
 
   // Unmagnify the previous root window.
   root_window_->RemoveObserver(this);
+  // TODO: This may need to remove the IME observer from the old root window
+  // and add it to the new root window. https://crbug.com/820464
+
   // Do not move mouse back to its original position (point at border of the
   // root window) after redrawing as doing so will trigger root window switch
   // again.

ash/root_window_controller.cc

@@ -471,17 +471,19 @@ void RootWindowController::Shutdown() {
 }
 
 void RootWindowController::CloseChildWindows() {
-  // NOTE: this may be called multiple times.
+  // Child windows can be closed by secondary monitor disconnection, Shell
+  // shutdown, or both. Avoid running the related cleanup code twice.
+  if (did_close_child_windows_)
+    return;
+  did_close_child_windows_ = true;
 
   // Deactivate keyboard container before closing child windows and shutting
   // down associated layout managers.
   DeactivateKeyboard(keyboard::KeyboardController::GetInstance());
 
   // |panel_layout_manager_| needs to be shut down before windows are destroyed.
-  if (panel_layout_manager_) {
-    panel_layout_manager_->Shutdown();
-    panel_layout_manager_ = nullptr;
-  }
+  panel_layout_manager_->Shutdown();
+  panel_layout_manager_ = nullptr;
 
   shelf_->ShutdownShelfWidget();
 
@@ -506,6 +508,7 @@ void RootWindowController::CloseChildWindows() {
     while (!toplevel_windows.windows().empty())
       delete toplevel_windows.Pop();
   }
+
   // And then remove the containers.
   while (!root->children().empty()) {
     aura::Window* child = root->children()[0];
@@ -515,6 +518,8 @@ void RootWindowController::CloseChildWindows() {
       root->RemoveChild(child);
   }
 
+  // Removing the containers destroys ShelfLayoutManager. ShelfWidget outlives
+  // ShelfLayoutManager because ShelfLayoutManager holds a pointer to it.
   shelf_->DestroyShelfWidget();
 
   ::wm::SetTooltipClient(GetRootWindow(), nullptr);

ash/root_window_controller.h

@@ -327,6 +327,10 @@ class ASH_EXPORT RootWindowController {
   std::unique_ptr<LockScreenActionBackgroundController>
       lock_screen_action_background_controller_;
 
+  // Whether child windows have been closed during shutdown. Exists to avoid
+  // calling related cleanup code more than once.
+  bool did_close_child_windows_ = false;
+
   static std::vector<RootWindowController*>* root_window_controllers_;
 
   DISALLOW_COPY_AND_ASSIGN(RootWindowController);

ash/shelf/shelf.cc

@@ -90,12 +90,11 @@ void Shelf::CreateShelfWidget(aura::Window* root) {
 }
 
 void Shelf::ShutdownShelfWidget() {
-  if (shelf_widget_)
-    shelf_widget_->Shutdown();
+  shelf_widget_->Shutdown();
 }
 
 void Shelf::DestroyShelfWidget() {
-  // May be called multiple times during shutdown.
+  DCHECK(shelf_widget_);
   shelf_widget_.reset();
 }
 
@@ -334,11 +333,6 @@ LoginShelfView* Shelf::GetLoginShelfViewForTesting() {
 }
 
 void Shelf::WillDeleteShelfLayoutManager() {
-  if (Shell::GetAshConfig() == Config::MASH) {
-    // TODO(sky): this should be removed once Shell is used everywhere.
-    ShutdownShelfWidget();
-  }
-
   // Clear event handlers that might forward events to the destroyed instance.
   auto_hide_event_handler_.reset();
   bezel_event_handler_.reset();

ash/shelf/shelf_layout_manager_unittest.cc

@@ -2063,7 +2063,7 @@ TEST_F(ShelfLayoutManagerTest, ShutdownHandlesWindowActivation) {
   window2->Show();
   wm::ActivateWindow(window1);
 
-  GetShelfWidget()->Shutdown();
+  GetShelfLayoutManager()->PrepareForShutdown();
 
   // Deleting a focused maximized window will switch focus to |window2|. This
   // would normally cause the ShelfLayoutManager to update its state. However

ash/shelf/shelf_widget.cc

@@ -73,7 +73,8 @@ class ShelfWidget::DelegateView : public views::WidgetDelegate,
     default_last_focusable_child_ = default_last_focusable_child;
   }
 
-  // views::WidgetDelegateView:
+  // views::WidgetDelegate:
+  void DeleteDelegate() override { delete this; }
   views::Widget* GetWidget() override { return View::GetWidget(); }
   const views::Widget* GetWidget() const override { return View::GetWidget(); }
 
@@ -106,6 +107,8 @@ ShelfWidget::DelegateView::DelegateView(ShelfWidget* shelf_widget)
       focus_cycler_(nullptr),
       opaque_background_(ui::LAYER_SOLID_COLOR) {
   DCHECK(shelf_widget_);
+  set_owned_by_client();  // Deleted by DeleteDelegate().
+
   SetLayoutManager(std::make_unique<views::FillLayout>());
   set_allow_deactivate_on_esc(true);
   opaque_background_.SetBounds(GetLocalBounds());
@@ -217,8 +220,24 @@ ShelfWidget::ShelfWidget(aura::Window* shelf_container, Shelf* shelf)
 ShelfWidget::~ShelfWidget() {
   // Must call Shutdown() before destruction.
   DCHECK(!status_area_widget_);
+}
+
+void ShelfWidget::Shutdown() {
+  // Shutting down the status area widget may cause some widgets (e.g. bubbles)
+  // to close, so uninstall the ShelfLayoutManager event filters first. Don't
+  // reset the pointer until later because other widgets (e.g. app list) may
+  // access it later in shutdown.
+  shelf_layout_manager_->PrepareForShutdown();
+
+  background_animator_.RemoveObserver(status_area_widget_.get());
+  Shell::Get()->focus_cycler()->RemoveWidget(status_area_widget_.get());
+  status_area_widget_.reset();
+
+  // Don't need to update the shelf background during shutdown.
   background_animator_.RemoveObserver(delegate_view_);
   background_animator_.RemoveObserver(this);
+
+  // Don't need to observe focus/activation during shutdown.
   Shell::Get()->focus_cycler()->RemoveWidget(this);
   SetFocusCycler(nullptr);
   RemoveObserver(this);
@@ -290,23 +309,6 @@ FocusCycler* ShelfWidget::GetFocusCycler() {
   return delegate_view_->focus_cycler();
 }
 
-void ShelfWidget::Shutdown() {
-  // Shutting down the status area widget may cause some widgets (e.g. bubbles)
-  // to close, so uninstall the ShelfLayoutManager event filters first. Don't
-  // reset the pointer until later because other widgets (e.g. app list) may
-  // access it later in shutdown.
-  if (shelf_layout_manager_)
-    shelf_layout_manager_->PrepareForShutdown();
-
-  if (status_area_widget_) {
-    background_animator_.RemoveObserver(status_area_widget_.get());
-    Shell::Get()->focus_cycler()->RemoveWidget(status_area_widget_.get());
-    status_area_widget_.reset();
-  }
-
-  CloseNow();
-}
-
 void ShelfWidget::UpdateIconPositionForPanel(aura::Window* panel) {
   ShelfID id = ShelfID::Deserialize(panel->GetProperty(kShelfIDKey));
   if (id.IsNull())

ash/shelf/shelf_widget.h

@@ -43,6 +43,9 @@ class ASH_EXPORT ShelfWidget : public views::Widget,
   ShelfWidget(aura::Window* shelf_container, Shelf* shelf);
   ~ShelfWidget() override;
 
+  // Clean up prior to deletion.
+  void Shutdown();
+
   // Returns true if the views-based shelf is being shown.
   static bool IsUsingViewsShelf();
 
@@ -73,9 +76,6 @@ class ASH_EXPORT ShelfWidget : public views::Widget,
   void SetFocusCycler(FocusCycler* focus_cycler);
   FocusCycler* GetFocusCycler();
 
-  // Clean up prior to deletion.
-  void Shutdown();
-
   // See Shelf::UpdateIconPositionForPanel().
   void UpdateIconPositionForPanel(aura::Window* panel);
 

ash/shell.cc

@@ -724,8 +724,6 @@ Shell::~Shell() {
 
   toast_manager_.reset();
 
-  for (aura::Window* root : GetAllRootWindows())
-    Shelf::ForWindow(root)->ShutdownShelfWidget();
   tray_bluetooth_helper_.reset();
 
   // Accesses root window containers.
@@ -759,6 +757,7 @@ Shell::~Shell() {
   // |window_selector_controller_|.
   split_view_controller_.reset();
 
+  // Close all widgets (including the shelf) and destroy all window containers.
   CloseAllRootWindowChildWindows();
 
   // MruWindowTracker must be destroyed after all windows have been deleted to

testing/buildbot/filters/mash.ash_unittests.filter

@@ -128,6 +128,12 @@
 -MagnificationControllerTest.PanWindowToLeft
 -MagnificationControllerTest.PanWindowToRight
 
+# Mash has one InputMethodBase per root window, whereas classic ash has a
+# single shared instance. This causes a shutdown use-after-free because the
+# magnifier controller adds and removes IME observers assuming there is a single
+# IME instance. https://crbug.com/820464
+-MagnificationControllerTest.MoveToSecondDisplayWithTouch
+
 # TODO: CursorManagerTestApi. http://crbug.com/780637
 -MirrorWindowControllerTest.MirrorCursorBasic
 -MirrorWindowControllerTest.MirrorCursorLocations

ui/aura/mus/window_tree_host_mus.cc

@@ -87,6 +87,8 @@ WindowTreeHostMus::WindowTreeHostMus(WindowTreeHostMusInitParams init_params)
       this, use_default_accelerated_widget, bounds_in_pixels));
 
   if (!init_params.use_classic_ime) {
+    // NOTE: This creates one InputMethodMus per display, despite the
+    // call to SetSharedInputMethod() below.
     input_method_ = std::make_unique<InputMethodMus>(this, window());
     input_method_->Init(init_params.window_tree_client->connector());
     SetSharedInputMethod(input_method_.get());