Ensure View entries are removed from the AXAuraObjCache when destroyed

This CL addresses an issue where the map of View objects to AXNodeIDs in
the AXAuraObjectCache gets populated with stale elements. When a View is
destructed, we remove it from this cache, but if the View was focused,
we incorrectly add a new entry back (see stack traces below). This
causes issues if later on a View is created at the same address at this
deleted view (manifesting as a flaky test failure on the linked bug).

We fix this by having the AXViewObjWrapper remove the cache entry when
the View is destroyed (OnViewIsDeleting).

Stack trace for removing AXAuraObjCache entry
#0 views::AXAuraObjCache::Remove()
#1 views::AXAuraObjCache::RemoveViewSubtree()
#2 views::Widget::NotifyWillRemoveView()
#3 views::View::DoRemoveChildView()
#4 views::View::~View()

Stack trace for re-adding AXAuraObjCache entry
#0 views::AXAuraObjCache::GetOrCreate()
#1 AutomationManagerAura::OnViewEvent()
#2 views::AXEventManager::NotifyViewEvent()
#3 views::View::NotifyAccessibilityEvent()
#4 views::View::SetVisible()
#5 views::FocusRing::RefreshLayer()
#6 views::View::Blur()
#7 views::FocusManager::SetFocusedViewWithReason()
#8 views::Widget::ViewHierarchyChanged()
#9 views::internal::RootView::ViewHierarchyChanged()
#10 views::View::ViewHierarchyChangedImpl()
#11 views::View::PropagateRemoveNotifications()
#12 views::View::DoRemoveChildView()
#13 views::View::~View()

AX-Relnotes: n/a.

Bug: b/159074662
Change-Id: Iaf787af321da7de5448e88c036a556a3fc4e1032
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2912232
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#886102}

ui/views/accessibility/ax_aura_obj_cache_unittest.cc

@@ -98,6 +98,59 @@ TEST_F(AXAuraObjCacheTest, TestViewRemoval) {
   delete parent;
 }
 
+// Helper for the ViewDestruction test.
+class ViewBlurObserver : public ViewObserver {
+ public:
+  ViewBlurObserver(AXAuraObjCache* cache, View* view) : cache_(cache) {
+    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), 0);
+    cache_->GetOrCreate(view);
+  }
+
+  bool was_called() { return !observation_.IsObserving(); }
+
+ private:
+  AXAuraObjCache* cache_;
+  base::ScopedObservation<View, ViewObserver> observation_{this};
+};
+
+// Test that stale cache entries are not left behind if a cache entry is
+// re-created during View destruction.
+TEST_F(AXAuraObjCacheTest, ViewDestruction) {
+  AXAuraObjCache cache;
+
+  WidgetAutoclosePtr widget(CreateTopLevelPlatformWidget());
+  auto* button = new LabelButton(Button::PressedCallback(), u"button");
+  widget->GetRootView()->AddChildView(button);
+  widget->Activate();
+  button->RequestFocus();
+  base::RunLoop().RunUntilIdle();
+  ASSERT_TRUE(button->HasFocus());
+
+  cache.GetOrCreate(widget.get());
+  cache.GetOrCreate(button);
+  // Everything should have an ID, indicating it's in the cache.
+  EXPECT_GT(cache.GetID(widget.get()), 0);
+  EXPECT_GT(cache.GetID(button), 0);
+
+  ViewBlurObserver observer(&cache, button);
+  delete button;
+
+  // The button object is destroyed, so there should be no stale cache entries.
+  EXPECT_NE(button, nullptr);
+  EXPECT_EQ(ui::kInvalidAXNodeID, cache.GetID(button));
+  EXPECT_TRUE(observer.was_called());
+}
+
 TEST_F(AXAuraObjCacheTest, ValidTree) {
   // Create a parent window.
   auto parent_widget = std::make_unique<Widget>();

ui/views/accessibility/ax_view_obj_wrapper.cc

@@ -27,9 +27,6 @@ AXViewObjWrapper::AXViewObjWrapper(AXAuraObjCache* aura_obj_cache, View* view)
 AXViewObjWrapper::~AXViewObjWrapper() = default;
 
 AXAuraObjWrapper* AXViewObjWrapper::GetParent() {
-  if (!view_)
-    return nullptr;
-
   if (view_->parent()) {
     if (view_->parent()->GetViewAccessibility().GetChildTreeID() !=
         ui::AXTreeIDUnknown())
@@ -46,9 +43,6 @@ AXAuraObjWrapper* AXViewObjWrapper::GetParent() {
 
 void AXViewObjWrapper::GetChildren(
     std::vector<AXAuraObjWrapper*>* out_children) {
-  if (!view_)
-    return;
-
   const ViewAccessibility& view_accessibility = view_->GetViewAccessibility();
 
   // Ignore this view's descendants if it has a child tree.
@@ -69,9 +63,6 @@ void AXViewObjWrapper::GetChildren(
 }
 
 void AXViewObjWrapper::Serialize(ui::AXNodeData* out_node_data) {
-  if (!view_)
-    return;
-
   ViewAccessibility& view_accessibility = view_->GetViewAccessibility();
   view_accessibility.GetAccessibleNodeData(out_node_data);
 
@@ -89,21 +80,22 @@ void AXViewObjWrapper::Serialize(ui::AXNodeData* out_node_data) {
 }
 
 ui::AXNodeID AXViewObjWrapper::GetUniqueId() const {
-  return view_ ? view_->GetViewAccessibility().GetUniqueId()
-               : ui::kInvalidAXNodeID;
+  return view_->GetViewAccessibility().GetUniqueId();
 }
 
 bool AXViewObjWrapper::HandleAccessibleAction(const ui::AXActionData& action) {
-  return view_ ? view_->HandleAccessibleAction(action) : false;
+  return view_->HandleAccessibleAction(action);
 }
 
 std::string AXViewObjWrapper::ToString() const {
-  return std::string(view_ ? view_->GetClassName() : "Null view");
+  return std::string(view_->GetClassName());
 }
 
 void AXViewObjWrapper::OnViewIsDeleting(View* observed_view) {
+  DCHECK_EQ(view_, observed_view);
   observation_.Reset();
-  view_ = nullptr;
+  // Remove() deletes |this|, so this should be the last line in the function.
+  aura_obj_cache_->Remove(observed_view);
 }
 
 }  // namespace views

ui/views/accessibility/ax_view_obj_wrapper.h

@@ -41,7 +41,8 @@ class AXViewObjWrapper : public AXAuraObjWrapper, public ViewObserver {
   void OnViewIsDeleting(View* observed_view) override;
 
  private:
-  View* view_;
+  // This is never null, as we destroy ourselves when the view is deleted.
+  View* const view_;
 
   base::ScopedObservation<View, ViewObserver> observation_{this};
 };