Fix crashes caused by destruction order in Assistant.

It was previously assumed that the WebViews owned by ManagedWebContents
would not be deleted before the Assistant view hierarchy in ash. As it
turns out, this is not always the case as identified by the reproduction
steps outlined in the attached bugs.

As such, we need to perform clean up safely whether ManagedWebContents
WebViews are destroyed before or after Assistant views in ash.

Bug: b:116821400, b:116814451
Change-Id: I982b327d45b22bcacef6e07489cb967eb36bcaf1
Reviewed-on: https://chromium-review.googlesource.com/c/1249569Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: David Black <dmblack@google.com>
Cr-Commit-Position: refs/heads/master@{#596032}

ash/assistant/ui/assistant_web_view.cc

@@ -101,11 +101,38 @@ void AssistantWebView::ChildPreferredSizeChanged(views::View* child) {
   SchedulePaint();
 }
 
-void AssistantWebView::OnViewBoundsChanged(views::View* view) {
+void AssistantWebView::OnViewIsDeleting(views::View* view) {
   DCHECK_EQ(content_view_, view);
 
-  // The mask layer should always match the bounds of the content view.
-  content_view_mask_->layer()->SetBounds(content_view_->GetLocalBounds());
+  // It's possible for |content_view_| to be deleted before AssistantWebView.
+  // When this happens, we need to perform clean up on |content_view_| before
+  // it is destroyed and clear references so that we don't try to perform
+  // clean up on the destroyed instance when destroying AssistantWebView.
+  content_view_->RemoveObserver(this);
+  content_view_ = nullptr;
+}
+
+void AssistantWebView::OnWindowBoundsChanged(aura::Window* window,
+                                             const gfx::Rect& old_bounds,
+                                             const gfx::Rect& new_bounds,
+                                             ui::PropertyChangeReason reason) {
+  DCHECK_EQ(native_content_view_, window);
+
+  // The mask layer should always match the bounds of the native content view.
+  content_view_mask_->layer()->SetBounds(gfx::Rect(new_bounds.size()));
+}
+
+void AssistantWebView::OnWindowDestroying(aura::Window* window) {
+  DCHECK_EQ(native_content_view_, window);
+
+  // It's possible for |native_content_view_| to be deleted before
+  // AssistantWebView. When this happens, we need to perform clean up on
+  // |native_content_view_| before it is destroyed and clear references so that
+  // we don't try to perform clean up on the destroyed instance when destroying
+  // AssistantWebView.
+  native_content_view_->RemoveObserver(this);
+  native_content_view_->layer()->SetMaskLayer(nullptr);
+  native_content_view_ = nullptr;
 }
 
 void AssistantWebView::InitLayout() {
@@ -190,15 +217,18 @@ void AssistantWebView::OnWebContentsReady(
         embed_token.value());
     content_view_->AddObserver(this);
 
-    // The mask layer should always match the bounds of the content view. We
-    // enforce this prior to applying the mask to the |native_content_view_|
-    // layer to prevent a DCHECK failure in cc::Layer.
-    content_view_mask_->layer()->SetBounds(content_view_->GetLocalBounds());
-
-    // Apply our layer mask which enforces corner radius.
+    // Cache a reference to the content's native view and observe it so that
+    // we can monitor changes to bounds and lifecycle.
     native_content_view_ =
         app_list::AnswerCardContentsRegistry::Get()->GetNativeView(
             embed_token.value());
+    native_content_view_->AddObserver(this);
+
+    // Apply a mask layer to enforce corner radius. The mask bounds must always
+    // match the bounds of |native_content_view_|. We sync bounds prior to
+    // applying the mask to prevent a DCHECK failure in cc::Layer.
+    content_view_mask_->layer()->SetBounds(
+        gfx::Rect(native_content_view_->GetBoundsInScreen().size()));
     native_content_view_->layer()->SetMaskLayer(content_view_mask_->layer());
 
     AddChildView(content_view_);
@@ -226,6 +256,7 @@ void AssistantWebView::ReleaseWebContents() {
   }
 
   if (native_content_view_) {
+    native_content_view_->RemoveObserver(this);
     native_content_view_->layer()->SetMaskLayer(nullptr);
     native_content_view_ = nullptr;
   }

ash/assistant/ui/assistant_web_view.h

@@ -13,6 +13,7 @@
 #include "ash/assistant/ui/caption_bar.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "ui/aura/window_observer.h"
 #include "ui/views/view.h"
 #include "ui/views/view_observer.h"
 
@@ -30,6 +31,7 @@ class AssistantController;
 // web contents.
 class AssistantWebView : public views::View,
                          public views::ViewObserver,
+                         public aura::WindowObserver,
                          public AssistantControllerObserver,
                          public CaptionBarDelegate {
  public:
@@ -43,7 +45,14 @@ class AssistantWebView : public views::View,
   void ChildPreferredSizeChanged(views::View* child) override;
 
   // views::ViewObserver:
-  void OnViewBoundsChanged(views::View* view) override;
+  void OnViewIsDeleting(views::View* view) override;
+
+  // views::WindowObserver:
+  void OnWindowBoundsChanged(aura::Window* window,
+                             const gfx::Rect& old_bounds,
+                             const gfx::Rect& new_bounds,
+                             ui::PropertyChangeReason reason) override;
+  void OnWindowDestroying(aura::Window* window) override;
 
   // CaptionBarDelegate:
   bool OnCaptionButtonPressed(CaptionButtonId id) override;

ash/assistant/ui/main_stage/ui_element_container_view.cc

@@ -114,7 +114,8 @@ class CardElementViewHolder : public views::NativeViewHost,
   }
 
   ~CardElementViewHolder() override {
-    card_element_view_->RemoveObserver(this);
+    if (card_element_view_)
+      card_element_view_->RemoveObserver(this);
   }
 
   // views::NativeViewHost:
@@ -164,7 +165,21 @@ class CardElementViewHolder : public views::NativeViewHost,
   }
 
   // views::ViewObserver:
+  void OnViewIsDeleting(views::View* view) override {
+    DCHECK_EQ(card_element_view_, view);
+
+    // It's possible for |card_element_view_| to be destroyed before
+    // CardElementViewHolder. When this happens, we need to perform clean up
+    // prior to |card_element_view_| being destroyed and remove our cached
+    // reference to prevent additional clean up attempts on the destroyed
+    // instance when destroying CardElementViewHolder.
+    card_element_view_->RemoveObserver(this);
+    card_element_view_ = nullptr;
+  }
+
   void OnViewPreferredSizeChanged(views::View* view) override {
+    DCHECK_EQ(card_element_view_, view);
+
     gfx::Size preferred_size = view->GetPreferredSize();
 
     if (border()) {
@@ -197,7 +212,7 @@ class CardElementViewHolder : public views::NativeViewHost,
   }
 
  private:
-  views::View* const card_element_view_;  // Owned by WebContentsManager.
+  views::View* card_element_view_;  // Owned by WebContentsManager.
 
   std::unique_ptr<views::Widget> child_widget_;