[MSan] Fix use-after-dtor issues with Assistant app list integration

There were several places that registered observers for child views but
never unregistered. Such observers do not behave quite as expected at
destruction time, since observer methods are virtual, and the child
views are not destroyed until the View base class is destroyed. In
practice, this is mostly benign, but per the C++ standard, this is
undefined behavior and MSan's use-after-dtor error is capable of
catching such mistakes.

Bug: 40222690
Change-Id: I88b0276c6ea0d4c12d070e7ab13496736fe6fe6b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5646994
Reviewed-by: Yuki Awano <yawano@google.com>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1320122}

ash/app_list/views/assistant/assistant_main_stage.cc

@@ -192,7 +192,7 @@ void AppListAssistantMainStage::InitLayoutWithIph() {
 
   // The layout container stacks two views.
   // On top is a main content container including the line separator, progress
-  // indicator query view, `ui_element_container_` and `footer_`.
+  // indicator query view, `ui_element_container()` and `footer()`.
   // The `zero_state_view_` is laid out above of the main content container. As
   // such, it floats above and does not cause repositioning to any of content
   // layout's underlying views.
@@ -223,7 +223,7 @@ std::unique_ptr<views::View>
 AppListAssistantMainStage::CreateContentLayoutContainer() {
   // The content layout container stacks two views.
   // On top is a main content container including the line separator, progress
-  // indicator query view and |ui_element_container_|.
+  // indicator query view and |ui_element_container()|.
   // The |zero_state_view_| is laid out above of the main content container. As
   // such, it floats above and does not cause repositioning to any of content
   // layout's underlying views.
@@ -266,17 +266,17 @@ AppListAssistantMainStage::CreateMainContentLayoutContainer() {
   content_layout_container->AddChildView(CreateDividerLayoutContainer());
 
   // Query view. Will be animated on its own layer.
-  query_view_ = content_layout_container->AddChildView(
+  auto* query_view = content_layout_container->AddChildView(
       std::make_unique<AssistantQueryView>());
-  query_view_->SetPaintToLayer();
-  query_view_->layer()->SetFillsBoundsOpaquely(false);
-  query_view_->AddObserver(this);
+  query_view->SetPaintToLayer();
+  query_view->layer()->SetFillsBoundsOpaquely(false);
+  query_view_observation_.Observe(query_view);
 
   // UI element container.
-  ui_element_container_ = content_layout_container->AddChildView(
-      std::make_unique<UiElementContainerView>(delegate_));
-  ui_element_container_->AddObserver(this);
-  content_layout->SetFlexForView(ui_element_container_, 1,
+  ui_element_container_observation_.Observe(
+      content_layout_container->AddChildView(
+          std::make_unique<UiElementContainerView>(delegate_)));
+  content_layout->SetFlexForView(ui_element_container(), 1,
                                  /*use_min_size=*/true);
 
   return content_layout_container;
@@ -318,20 +318,19 @@ AppListAssistantMainStage::CreateDividerLayoutContainer() {
 std::unique_ptr<views::View>
 AppListAssistantMainStage::CreateFooterLayoutContainer() {
   // Footer.
-  // Note that the |footer_| is placed within its own view container so that as
-  // its visibility changes, its parent container will still reserve the same
-  // layout space. This prevents jank that would otherwise occur due to
-  // |ui_element_container_| claiming that empty space.
+  // Note that the |footer()| is placed within its own view container so that
+  // as its visibility changes, its parent container will still reserve the
+  // same layout space. This prevents jank that would otherwise occur due to
+  // |ui_element_container()| claiming that empty space.
   auto footer_container = std::make_unique<FooterContainer>();
   footer_container->SetLayoutManager(std::make_unique<views::FillLayout>());
 
-  footer_ = footer_container->AddChildView(
-      std::make_unique<AssistantFooterView>(delegate_));
-  footer_->AddObserver(this);
+  footer_observation_.Observe(footer_container->AddChildView(
+      std::make_unique<AssistantFooterView>(delegate_)));
 
   // The footer will be animated on its own layer.
-  footer_->SetPaintToLayer();
-  footer_->layer()->SetFillsBoundsOpaquely(false);
+  footer()->SetPaintToLayer();
+  footer()->layer()->SetFillsBoundsOpaquely(false);
 
   return footer_container;
 }
@@ -365,11 +364,11 @@ void AppListAssistantMainStage::AnimateInZeroState() {
 
 void AppListAssistantMainStage::AnimateInFooter() {
   // Set up our pre-animation values.
-  footer_->layer()->SetOpacity(0.f);
-  footer_->SetVisible(true);
+  footer()->layer()->SetOpacity(0.f);
+  footer()->SetVisible(true);
 
   // Animate the footer to 100% opacity with delay.
-  footer_->layer()->GetAnimator()->StartAnimation(CreateLayerAnimationSequence(
+  footer()->layer()->GetAnimator()->StartAnimation(CreateLayerAnimationSequence(
       ui::LayerAnimationElement::CreatePauseElement(
           ui::LayerAnimationElement::AnimatableProperty::OPACITY,
           kFooterEntryAnimationFadeInDelay),
@@ -387,7 +386,7 @@ void AppListAssistantMainStage::OnAssistantControllerDestroying() {
 void AppListAssistantMainStage::OnCommittedQueryChanged(
     const AssistantQuery& query) {
   // Update the view.
-  query_view_->SetQuery(query);
+  query_view()->SetQuery(query);
 
   // If query is empty and we are showing zero state, do not update the Ui.
   if (query.Empty() && IsShown(zero_state_view_))
@@ -414,7 +413,7 @@ void AppListAssistantMainStage::OnCommittedQueryChanged(
 void AppListAssistantMainStage::OnPendingQueryChanged(
     const AssistantQuery& query) {
   // Update the view.
-  query_view_->SetQuery(query);
+  query_view()->SetQuery(query);
 
   if (!IsShown(zero_state_view_))
     return;
@@ -423,8 +422,8 @@ void AppListAssistantMainStage::OnPendingQueryChanged(
   // animation duration to avoid the two views displaying at the same time.
   constexpr base::TimeDelta kQueryAnimationFadeInDuration =
       base::Milliseconds(433);
-  query_view_->layer()->SetOpacity(0.f);
-  query_view_->layer()->GetAnimator()->StartAnimation(
+  query_view()->layer()->SetOpacity(0.f);
+  query_view()->layer()->GetAnimator()->StartAnimation(
       CreateLayerAnimationSequence(
           ui::LayerAnimationElement::CreatePauseElement(
               ui::LayerAnimationElement::AnimatableProperty::OPACITY,
@@ -440,7 +439,7 @@ void AppListAssistantMainStage::OnPendingQueryCleared(bool due_to_commit) {
   // cancelled, or because the query was committed. If the query was committed,
   // reseting the query here will have no visible effect. If the interaction was
   // cancelled, we set the query here to restore the previously committed query.
-  query_view_->SetQuery(
+  query_view()->SetQuery(
       AssistantInteractionController::Get()->GetModel()->committed_query());
 }
 
@@ -476,7 +475,7 @@ void AppListAssistantMainStage::OnUiVisibilityChanged(
     return;
   }
 
-  query_view_->SetQuery(AssistantNullQuery());
+  query_view()->SetQuery(AssistantNullQuery());
 }
 
 void AppListAssistantMainStage::InitializeUIForBubbleView() {
@@ -503,7 +502,7 @@ void AppListAssistantMainStage::InitializeUIForStartingSession(
   progress_indicator_->layer()->SetOpacity(0.f);
   horizontal_separator_->layer()->SetOpacity(from_search ? 1.f : 0.f);
 
-  footer_->InitializeUIForBubbleView();
+  footer()->InitializeUIForBubbleView();
   if (from_search) {
     zero_state_view_->SetVisible(false);
     AnimateInFooter();
@@ -512,7 +511,7 @@ void AppListAssistantMainStage::InitializeUIForStartingSession(
 
     if (base::FeatureList::IsEnabled(
             feature_engagement::kIPHLauncherSearchHelpUiFeature)) {
-      footer_->SetVisible(false);
+      footer()->SetVisible(false);
     } else {
       AnimateInFooter();
     }

ash/app_list/views/assistant/assistant_main_stage.h

@@ -86,15 +86,40 @@ class ASH_EXPORT AppListAssistantMainStage
   void MaybeHideZeroStateAndShowFooter();
   void InitializeUIForStartingSession(bool from_search);
 
+  AssistantQueryView* query_view() {
+    return query_view_observation_.GetSource();
+  }
+  const AssistantQueryView* query_view() const {
+    return query_view_observation_.GetSource();
+  }
+
+  UiElementContainerView* ui_element_container() {
+    return ui_element_container_observation_.GetSource();
+  }
+  const UiElementContainerView* ui_element_container() const {
+    return ui_element_container_observation_.GetSource();
+  }
+
+  AssistantFooterView* footer() { return footer_observation_.GetSource(); }
+  const AssistantFooterView* footer() const {
+    return footer_observation_.GetSource();
+  }
+
   const raw_ptr<AssistantViewDelegate> delegate_;  // Owned by Shell.
 
   // Owned by view hierarchy.
   raw_ptr<AssistantProgressIndicator> progress_indicator_;
   raw_ptr<views::Separator> horizontal_separator_;
-  raw_ptr<AssistantQueryView> query_view_;
-  raw_ptr<UiElementContainerView> ui_element_container_;
+  // The observed views are owned by the view hierarchy. These could be a
+  // raw_ptr to the view + ScopedObservation, but accessing the view through
+  // the ScopedObservation saves a pointer.
+  base::ScopedObservation<AssistantQueryView, ViewObserver>
+      query_view_observation_{this};
+  base::ScopedObservation<UiElementContainerView, ViewObserver>
+      ui_element_container_observation_{this};
   raw_ptr<AssistantZeroStateView> zero_state_view_;
-  raw_ptr<AssistantFooterView> footer_;
+  base::ScopedObservation<AssistantFooterView, ViewObserver>
+      footer_observation_{this};
 
   base::ScopedObservation<AssistantController, AssistantControllerObserver>
       assistant_controller_observation_{this};

ash/assistant/ui/base/assistant_scroll_view.cc

@@ -63,10 +63,10 @@ AssistantScrollView::AssistantScrollView() {
 AssistantScrollView::~AssistantScrollView() = default;
 
 void AssistantScrollView::OnViewPreferredSizeChanged(views::View* view) {
-  DCHECK_EQ(content_view_, view);
+  DCHECK_EQ(content_view(), view);
 
   for (auto& observer : observers_)
-    observer.OnContentsPreferredSizeChanged(content_view_);
+    observer.OnContentsPreferredSizeChanged(content_view());
 
   PreferredSizeChanged();
 }
@@ -84,9 +84,8 @@ void AssistantScrollView::InitLayout() {
   SetDrawOverflowIndicator(false);
 
   // Content view.
-  auto content_view = std::make_unique<ContentView>();
-  content_view->AddObserver(this);
-  content_view_ = SetContents(std::move(content_view));
+  content_view_observation_.Observe(
+      SetContents(std::make_unique<ContentView>()));
 
   // Scroll bars.
   SetVerticalScrollBarMode(views::ScrollView::ScrollBarMode::kHiddenButEnabled);

ash/assistant/ui/base/assistant_scroll_view.h

@@ -8,6 +8,7 @@
 #include "base/component_export.h"
 #include "base/memory/raw_ptr.h"
 #include "base/observer_list.h"
+#include "base/scoped_observation.h"
 #include "ui/base/metadata/metadata_header_macros.h"
 #include "ui/views/controls/scroll_view.h"
 #include "ui/views/view_observer.h"
@@ -42,15 +43,21 @@ class COMPONENT_EXPORT(ASSISTANT_UI) AssistantScrollView
   void AddScrollViewObserver(Observer* observer);
   void RemoveScrollViewObserver(Observer* observer);
 
-  views::View* content_view() { return content_view_; }
-  const views::View* content_view() const { return content_view_; }
+  views::View* content_view() { return content_view_observation_.GetSource(); }
+  const views::View* content_view() const {
+    return content_view_observation_.GetSource();
+  }
 
  private:
   void InitLayout();
 
   base::ObserverList<Observer> observers_;
 
-  raw_ptr<views::View> content_view_;  // Owned by view hierarchy.
+  // The observed view is owned by the view hierarchy. This could be a raw_ptr
+  // to the view + ScopedObservation, but accessing the view through the
+  // ScopedObservation saves a pointer.
+  base::ScopedObservation<views::View, views::ViewObserver>
+      content_view_observation_{this};
 };
 
 }  // namespace ash