Instrument View::~View to ascertain recreation of AXViewObjWrapper

This change adds additional nuance to View::Lifecycle, makes it public,
and uses it to DCHECK calls to AXAuraObjCache::GetOrCreate.

Specifically, accessibility never expects GetOrCreate on a view which is
destroying or destroyed.

The change also adds an early return to GetOrCreate, if the view is
detached. This occurs in ~View, when the view is removed as a child of
its parent.

Thus, the added DCHECK in AXAuraObjCache::GetOrCreate triggers narrowly
for any codepaths from the start of ~View up to almost all of
parent_->RemoveChildView(this) where view's parent_ is nullified in
DoRemoveChildView.

Bug: 1355560
Change-Id: If8817f88b90a0633898d6a6e073b85ab91fbbe26
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3868127
Reviewed-by: Robert Liao <robliao@chromium.org>
Commit-Queue: David Tseng <dtseng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1047200}

ui/views/accessibility/ax_aura_obj_cache.cc

@@ -74,6 +74,11 @@ AXAuraObjWrapper* AXAuraObjCache::GetOrCreate(View* view) {
   // Avoid problems with transient focus events. https://crbug.com/729449
   if (!view->GetWidget())
     return nullptr;
+
+  DCHECK(view_to_id_map_.find(view) != view_to_id_map_.end() ||
+         // This is either a new view or we're erroneously here during ~View.
+         view->life_cycle_state() == View::LifeCycleState::kAlive);
+
   return CreateInternal<AXViewObjWrapper>(view, &view_to_id_map_);
 }
 

ui/views/accessibility/ax_aura_obj_cache_unittest.cc

@@ -101,15 +101,12 @@ class ViewBlurObserver : public ViewObserver {
     observation_.Observe(view);
   }
 
-  // This is fired while the view is being destroyed, after the cache entry is
-  // removed by the AXWidgetObjWrapper. Re-create the cache entry so we can
-  // test that it will also be removed.
   void OnViewBlurred(View* view) override {
     ASSERT_FALSE(was_called());
     observation_.Reset();
 
-    ASSERT_EQ(cache_->GetID(view), ui::kInvalidAXNodeID);
-    cache_->GetOrCreate(view);
+    // The cache entry gets deleted in
+    // AXViewObjWrapper::OnViewIsDeleting which occurs later in ~View.
   }
 
   bool was_called() { return !observation_.IsObserving(); }

ui/views/accessibility/ax_widget_obj_wrapper.cc

@@ -20,7 +20,6 @@ AXWidgetObjWrapper::AXWidgetObjWrapper(AXAuraObjCache* aura_obj_cache,
     : AXAuraObjWrapper(aura_obj_cache), widget_(widget) {
   DCHECK(widget->GetNativeView());
   widget_observation_.Observe(widget);
-  widget_removals_observation_.Observe(widget);
 }
 
 AXWidgetObjWrapper::~AXWidgetObjWrapper() = default;
@@ -80,8 +79,4 @@ void AXWidgetObjWrapper::OnWidgetVisibilityChanged(Widget*, bool) {
   aura_obj_cache_->OnFocusedViewChanged();
 }
 
-void AXWidgetObjWrapper::OnWillRemoveView(Widget* widget, View* view) {
-  aura_obj_cache_->RemoveViewSubtree(view);
-}
-
 }  // namespace views

ui/views/accessibility/ax_widget_obj_wrapper.h

@@ -16,15 +16,12 @@
 #include "ui/views/accessibility/ax_aura_obj_wrapper.h"
 #include "ui/views/widget/widget.h"
 #include "ui/views/widget/widget_observer.h"
-#include "ui/views/widget/widget_removals_observer.h"
 
 namespace views {
 class AXAuraObjCache;
 
 // Describes a |Widget| for use with other AX classes.
-class AXWidgetObjWrapper : public AXAuraObjWrapper,
-                           public WidgetObserver,
-                           public WidgetRemovalsObserver {
+class AXWidgetObjWrapper : public AXAuraObjWrapper, public WidgetObserver {
  public:
   // |aura_obj_cache| must outlive this object.
   AXWidgetObjWrapper(AXAuraObjCache* aura_obj_cache, Widget* widget);
@@ -44,20 +41,12 @@ class AXWidgetObjWrapper : public AXAuraObjWrapper,
   void OnWidgetDestroyed(Widget* widget) override;
   void OnWidgetVisibilityChanged(Widget*, bool) override;
 
-  // WidgetRemovalsObserver overrides.
-  void OnWillRemoveView(Widget* widget, View* view) override;
-
  private:
   raw_ptr<Widget> widget_;
 
   const ui::AXUniqueId unique_id_;
 
   base::ScopedObservation<Widget, WidgetObserver> widget_observation_{this};
-  base::ScopedObservation<Widget,
-                          WidgetRemovalsObserver,
-                          &Widget::AddRemovalsObserver,
-                          &Widget::RemoveRemovalsObserver>
-      widget_removals_observation_{this};
 };
 
 }  // namespace views

ui/views/view.cc

@@ -227,6 +227,8 @@ View::View() {
 }
 
 View::~View() {
+  life_cycle_state_ = LifeCycleState::kDestroying;
+
   if (parent_)
     parent_->RemoveChildView(this);
 

ui/views/view.h

@@ -1441,6 +1441,16 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   void RemoveObserver(ViewObserver* observer);
   bool HasObserver(const ViewObserver* observer) const;
 
+  // http://crbug.com/1162949 : Instrumentation that indicates if this is alive.
+  // Callers should not depend on this as it is meant to be temporary.
+  enum class LifeCycleState : uint32_t {
+    kAlive = 0x600D600D,
+    kDestroying = 0x90141013,
+    kDestroyed = 0xBAADBAAD,
+  };
+
+  LifeCycleState life_cycle_state() const { return life_cycle_state_; }
+
  protected:
   // Used to track a drag. RootView passes this into
   // ProcessMousePressed/Dragged.
@@ -1702,12 +1712,6 @@ class VIEWS_EXPORT View : public ui::LayerDelegate,
   FRIEND_TEST_ALL_PREFIXES(ViewTest, PaintWithMovedViewUsesCacheInRTL);
   FRIEND_TEST_ALL_PREFIXES(ViewTest, PaintWithUnknownInvalidation);
 
-  // http://crbug.com/1162949 : Instrumentation that indicates if this is alive.
-  enum class LifeCycleState : uint32_t {
-    kAlive = 0x600D600D,
-    kDestroyed = 0xBAADBAAD,
-  };
-
   // This is the default view layout. It is a very simple version of FillLayout,
   // which merely sets the bounds of the children to the content bounds. The
   // actual FillLayout isn't used here because it supports a couple of features