Fix crash due to destruction order.

We have dependencies on TabletModeController and need to be destroyed
before it.

Previously we assumed that the view hierarchy would be destroyed before
our Assistant controllers. This will not actually be the case since I
am moving our reset above destruction of the view hierarchy.

As such, we now destroy our view hierarchy at the beginning of our
controller destruction sequence.

Also fixes some outdated comments.

Bug: b:112781572
Change-Id: Iecdce642790c0c8278d489734ea735370e98912c
Reviewed-on: https://chromium-review.googlesource.com/1180656
Commit-Queue: David Black <dmblack@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584831}

ash/assistant/assistant_controller.cc

@@ -252,11 +252,6 @@ void AssistantController::OpenUrl(const GURL& url) {
   NotifyUrlOpened(url);
 }
 
-void AssistantController::ShutDown() {
-  // Controllers can be added to handle this event as needed.
-  assistant_ui_controller_->ShutDown();
-}
-
 void AssistantController::NotifyConstructed() {
   for (AssistantControllerObserver& observer : observers_)
     observer.OnAssistantControllerConstructed();

ash/assistant/assistant_controller.h

@@ -123,9 +123,6 @@ class ASH_EXPORT AssistantController
   // TODO(dmblack): Support opening specific URLs in the Assistant container.
   void OpenUrl(const GURL& url);
 
-  // Called before dtor to deregister services and avoid life cycle issues.
-  void ShutDown();
-
   AssistantCacheController* cache_controller() {
     DCHECK(assistant_cache_controller_);
     return assistant_cache_controller_.get();

ash/assistant/assistant_ui_controller.cc

@@ -14,7 +14,6 @@
 #include "ash/system/toast/toast_data.h"
 #include "ash/system/toast/toast_manager.h"
 #include "ash/voice_interaction/voice_interaction_controller.h"
-#include "ash/wm/tablet_mode/tablet_mode_controller.h"
 #include "base/optional.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 #include "ui/base/l10n/l10n_util.h"
@@ -45,16 +44,12 @@ AssistantUiController::AssistantUiController(
   AddModelObserver(this);
   assistant_controller_->AddObserver(this);
   Shell::Get()->highlighter_controller()->AddObserver(this);
-  Shell::Get()->tablet_mode_controller()->AddObserver(this);
 }
 
 AssistantUiController::~AssistantUiController() {
   Shell::Get()->highlighter_controller()->RemoveObserver(this);
   assistant_controller_->RemoveObserver(this);
   RemoveModelObserver(this);
-
-  if (container_view_)
-    container_view_->GetWidget()->RemoveObserver(this);
 }
 
 void AssistantUiController::SetAssistant(
@@ -93,16 +88,6 @@ void AssistantUiController::OnWidgetDestroying(views::Widget* widget) {
   container_view_ = nullptr;
 }
 
-void AssistantUiController::OnAssistantControllerConstructed() {
-  assistant_controller_->interaction_controller()->AddModelObserver(this);
-  assistant_controller_->screen_context_controller()->AddModelObserver(this);
-}
-
-void AssistantUiController::OnAssistantControllerDestroying() {
-  assistant_controller_->screen_context_controller()->RemoveModelObserver(this);
-  assistant_controller_->interaction_controller()->RemoveModelObserver(this);
-}
-
 void AssistantUiController::OnInputModalityChanged(
     InputModality input_modality) {
   UpdateUiMode();
@@ -188,6 +173,22 @@ void AssistantUiController::OnHighlighterEnabledChanged(
   }
 }
 
+void AssistantUiController::OnAssistantControllerConstructed() {
+  assistant_controller_->interaction_controller()->AddModelObserver(this);
+  assistant_controller_->screen_context_controller()->AddModelObserver(this);
+}
+
+void AssistantUiController::OnAssistantControllerDestroying() {
+  assistant_controller_->screen_context_controller()->RemoveModelObserver(this);
+  assistant_controller_->interaction_controller()->RemoveModelObserver(this);
+
+  if (container_view_) {
+    // Our view hierarchy should not outlive our controllers.
+    container_view_->GetWidget()->CloseNow();
+    DCHECK_EQ(nullptr, container_view_);
+  }
+}
+
 void AssistantUiController::OnDeepLinkReceived(
     assistant::util::DeepLinkType type,
     const std::map<std::string, std::string>& params) {
@@ -265,20 +266,6 @@ void AssistantUiController::ToggleUi(AssistantSource source) {
     ShowUi(source);
 }
 
-void AssistantUiController::OnTabletModeStarted() {
-  if (container_view_)
-    container_view_->OnTabletModeChanged();
-}
-
-void AssistantUiController::OnTabletModeEnded() {
-  if (container_view_)
-    container_view_->OnTabletModeChanged();
-}
-
-void AssistantUiController::ShutDown() {
-  Shell::Get()->tablet_mode_controller()->RemoveObserver(this);
-}
-
 void AssistantUiController::UpdateUiMode(
     base::Optional<AssistantUiMode> ui_mode) {
   // If a UI mode is provided, we will use it in lieu of updating UI mode on the

ash/assistant/assistant_ui_controller.h

@@ -15,7 +15,6 @@
 #include "ash/assistant/ui/caption_bar.h"
 #include "ash/assistant/ui/dialog_plate/dialog_plate.h"
 #include "ash/highlighter/highlighter_controller.h"
-#include "ash/wm/tablet_mode/tablet_mode_observer.h"
 #include "base/macros.h"
 #include "ui/views/widget/widget_observer.h"
 
@@ -45,8 +44,7 @@ class ASH_EXPORT AssistantUiController
       public AssistantMiniViewDelegate,
       public CaptionBarDelegate,
       public DialogPlateObserver,
-      public HighlighterController::Observer,
-      public TabletModeObserver {
+      public HighlighterController::Observer {
  public:
   explicit AssistantUiController(AssistantController* assistant_controller);
   ~AssistantUiController() override;
@@ -102,13 +100,6 @@ class ASH_EXPORT AssistantUiController
   void HideUi(AssistantSource source);
   void ToggleUi(AssistantSource source);
 
-  // TabletModeObserver:
-  void OnTabletModeStarted() override;
-  void OnTabletModeEnded() override;
-
-  // Called before dtor to clean up service dependencies.
-  void ShutDown();
-
   AssistantContainerView* GetViewForTest();
 
  private:

ash/assistant/ui/assistant_container_view.cc

@@ -32,12 +32,11 @@ namespace ash {
 
 namespace {
 
-constexpr int kClamshellMarginBottomDip = 8;  // Margin in clamshell mode.
-constexpr int kTabletMarginTopDip = 24;       // Margin in tablet mode.
-
 // Appearance.
 constexpr SkColor kBackgroundColor = SK_ColorWHITE;
 constexpr int kCornerRadiusDip = 20;
+constexpr int kClamshellMarginBottomDip = 8;  // Margin in clamshell mode.
+constexpr int kTabletMarginTopDip = 24;       // Margin in tablet mode.
 
 // Animation.
 constexpr int kResizeAnimationDurationMs = 250;
@@ -163,9 +162,11 @@ AssistantContainerView::AssistantContainerView(
   // AssistantContainerView belongs so is guaranteed to outlive it.
   assistant_controller_->ui_controller()->AddModelObserver(this);
   display::Screen::GetScreen()->AddObserver(this);
+  Shell::Get()->tablet_mode_controller()->AddObserver(this);
 }
 
 AssistantContainerView::~AssistantContainerView() {
+  Shell::Get()->tablet_mode_controller()->RemoveObserver(this);
   display::Screen::GetScreen()->RemoveObserver(this);
   assistant_controller_->ui_controller()->RemoveModelObserver(this);
 }
@@ -281,9 +282,12 @@ void AssistantContainerView::RequestFocus() {
 }
 
 void AssistantContainerView::SetAnchor(aura::Window* root_window) {
+  // If |root_window| is not specified, we'll use the root window corresponding
+  // to where new windows will be opened.
   if (!root_window)
     root_window = Shell::Get()->GetRootWindowForNewWindows();
-  // Anchor to the display matching where new windows will be opened.
+
+  // Anchor to the display matching |root_window|.
   display::Display display = display::Screen::GetScreen()->GetDisplayMatching(
       root_window->GetBoundsInScreen());
 
@@ -291,7 +295,8 @@ void AssistantContainerView::SetAnchor(aura::Window* root_window) {
                          ->tablet_mode_controller()
                          ->IsTabletModeWindowManagerEnabled();
 
-  // Anchor to the bottom center of the work area.
+  // Align to the horizontal center of the work area. We are top aligned in
+  // tablet mode, bottom aligned in clamshell.
   gfx::Rect work_area = display.work_area();
   gfx::Rect anchor =
       gfx::Rect(work_area.x(),
@@ -335,8 +340,9 @@ void AssistantContainerView::AnimationProgressed(
   // Retrieve current bounds.
   gfx::Rect bounds = GetWidget()->GetWindowBoundsInScreen();
 
-  // Our view is horizontally centered and bottom aligned. As such, we should
-  // retain the same |bottom| and |center_x| position after resizing.
+  // Our view is horizontally centered. As such, we should retain the same
+  // |center_x| position after resizing. In clamshell mode, we are bottom
+  // aligned so we cache |bottom| position here as well.
   const int bottom = bounds.bottom();
   const int center_x = bounds.CenterPoint().x();
 
@@ -350,21 +356,17 @@ void AssistantContainerView::AnimationProgressed(
   if (!Shell::Get()
            ->tablet_mode_controller()
            ->IsTabletModeWindowManagerEnabled()) {
-    // Maintain our original |bottom| positions in clamshell mode. In tablet
-    // mode, they should grow downwards, which is the default behavior.
+    // Maintain our original |bottom| position in clamshell mode. In tablet
+    // mode we grow downwards which is the default behavior.
     bounds.set_y(bottom - bounds.height());
   }
+
   // Maintain |center_x| position.
   bounds.set_x(center_x - (bounds.width() / 2));
 
   GetWidget()->SetBounds(bounds);
 }
 
-void AssistantContainerView::OnTabletModeChanged() {
-  DCHECK(GetWidget());
-  SetAnchor(GetWidget()->GetNativeWindow()->GetRootWindow());
-}
-
 void AssistantContainerView::OnDisplayMetricsChanged(
     const display::Display& display,
     uint32_t changed_metrics) {
@@ -373,4 +375,14 @@ void AssistantContainerView::OnDisplayMetricsChanged(
     SetAnchor(root_window);
 }
 
+void AssistantContainerView::OnTabletModeStarted() {
+  DCHECK(GetWidget());
+  SetAnchor(GetWidget()->GetNativeWindow()->GetRootWindow());
+}
+
+void AssistantContainerView::OnTabletModeEnded() {
+  DCHECK(GetWidget());
+  SetAnchor(GetWidget()->GetNativeWindow()->GetRootWindow());
+}
+
 }  // namespace ash

ash/assistant/ui/assistant_container_view.h

@@ -6,6 +6,7 @@
 #define ASH_ASSISTANT_UI_ASSISTANT_CONTAINER_VIEW_H_
 
 #include "ash/assistant/model/assistant_ui_model_observer.h"
+#include "ash/wm/tablet_mode/tablet_mode_observer.h"
 #include "base/macros.h"
 #include "ui/display/display_observer.h"
 #include "ui/gfx/animation/animation_delegate.h"
@@ -29,7 +30,8 @@ class AssistantWebView;
 class AssistantContainerView : public views::BubbleDialogDelegateView,
                                public AssistantUiModelObserver,
                                public display::DisplayObserver,
-                               public gfx::AnimationDelegate {
+                               public gfx::AnimationDelegate,
+                               public TabletModeObserver {
  public:
   explicit AssistantContainerView(AssistantController* assistant_controller);
   ~AssistantContainerView() override;
@@ -57,14 +59,12 @@ class AssistantContainerView : public views::BubbleDialogDelegateView,
   void AnimationProgressed(const gfx::Animation* animation) override;
 
   // display::DisplayObserver:
-  void OnWillProcessDisplayChanges() override {}
-  void OnDidProcessDisplayChanges() override {}
-  void OnDisplayAdded(const display::Display& new_display) override {}
-  void OnDisplayRemoved(const display::Display& old_display) override {}
   void OnDisplayMetricsChanged(const display::Display& display,
                                uint32_t changed_metrics) override;
 
-  void OnTabletModeChanged();
+  // TabletModeObserver:
+  void OnTabletModeStarted() override;
+  void OnTabletModeEnded() override;
 
  private:
   // Sets anchor rect to |root_window|. If it's null,

ash/shell.cc

@@ -792,10 +792,11 @@ Shell::~Shell() {
   // the former may use the latter before destruction.
   app_list_controller_.reset();
 
-  // Shutdown |assistant_controller_| to properly remove observer on
-  // |tablet_mode_controller_|.
+  // Destroy |assistant_controller_| earlier than |tablet_mode_controller_| so
+  // that the former will destroy the Assistant view hierarchy which has a
+  // dependency on the latter.
   if (chromeos::switches::IsAssistantEnabled())
-    assistant_controller_->ShutDown();
+    assistant_controller_.reset();
 
   // Destroy tablet mode controller early on since it has some observers which
   // need to be removed.
@@ -860,7 +861,6 @@ Shell::~Shell() {
   // These need a valid Shell instance to clean up properly, so explicitly
   // delete them before invalidating the instance.
   // Alphabetical. TODO(oshima): sort.
-  assistant_controller_.reset();
   magnification_controller_.reset();
   tooltip_controller_.reset();
   event_client_.reset();