Guarantee AXViewObjWrapper only dereferences valid views

- observe lifetime of the view and invalidate our weak pointer to it when it is being deleted
- clean up strange useage of AXUniqueId

TBR=slan@chromium.org

Bug: 859190
Change-Id: I15b45750ddecde7d20c4c91502411690e4025c07
Reviewed-on: https://chromium-review.googlesource.com/c/1316440
Commit-Queue: David Tseng <dtseng@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605484}

chrome/browser/ui/ash/accessibility/ax_tree_source_aura_unittest.cc

@@ -88,7 +88,7 @@ TEST_F(AXTreeSourceAuraTest, Accessors) {
   ASSERT_TRUE(ax_tree.GetRoot());
 
   // ID's should be > 0.
-  ASSERT_GE(ax_tree.GetRoot()->GetUniqueId().Get(), 1);
+  ASSERT_GE(ax_tree.GetRoot()->GetUniqueId(), 1);
 
   // Grab the content view directly from cache to avoid walking down the tree.
   AXAuraObjWrapper* content =
@@ -131,7 +131,7 @@ TEST_F(AXTreeSourceAuraTest, DoDefault) {
   ASSERT_FALSE(textfield_->HasFocus());
   ui::AXActionData action_data;
   action_data.action = ax::mojom::Action::kDoDefault;
-  action_data.target_node_id = textfield_wrapper->GetUniqueId().Get();
+  action_data.target_node_id = textfield_wrapper->GetUniqueId();
   textfield_wrapper->HandleAccessibleAction(action_data);
   ASSERT_TRUE(textfield_->HasFocus());
 }
@@ -147,7 +147,7 @@ TEST_F(AXTreeSourceAuraTest, Focus) {
   ASSERT_FALSE(textfield_->HasFocus());
   ui::AXActionData action_data;
   action_data.action = ax::mojom::Action::kFocus;
-  action_data.target_node_id = textfield_wrapper->GetUniqueId().Get();
+  action_data.target_node_id = textfield_wrapper->GetUniqueId();
   textfield_wrapper->HandleAccessibleAction(action_data);
   ASSERT_TRUE(textfield_->HasFocus());
 }
@@ -184,7 +184,7 @@ TEST_F(AXTreeSourceAuraTest, Serialize) {
 
   int text_field_update_index = -1;
   for (size_t i = 0; i < node_count; ++i) {
-    if (textfield_wrapper->GetUniqueId().Get() == out_update2.nodes[i].id)
+    if (textfield_wrapper->GetUniqueId() == out_update2.nodes[i].id)
       text_field_update_index = i;
   }
 

chrome/browser/ui/aura/accessibility/automation_manager_aura.cc

@@ -95,7 +95,7 @@ void AutomationManagerAura::HandleEvent(views::View* view,
   // This helps us avoid firing accessibility events for transient changes.
   // because there's a chance that the underlying object being wrapped could
   // be deleted, pass the ID of the object rather than the object pointer.
-  int32_t id = obj->GetUniqueId().Get();
+  int32_t id = obj->GetUniqueId();
   base::ThreadTaskRunnerHandle::Get()->PostTask(
       FROM_HERE,
       base::BindOnce(&AutomationManagerAura::SendEventOnObjectById,
@@ -214,7 +214,7 @@ void AutomationManagerAura::SendEvent(views::AXAuraObjWrapper* aura_obj,
   // ancestor) but we shouldn't fire the event on the node not in the tree.
   if (current_tree_serializer_->IsInClientTree(aura_obj)) {
     ui::AXEvent event;
-    event.id = aura_obj->GetUniqueId().Get();
+    event.id = aura_obj->GetUniqueId();
     event.event_type = event_type;
     event_bundle.events.push_back(event);
   }

chrome/browser/ui/aura/accessibility/automation_manager_aura_browsertest.cc

@@ -176,21 +176,21 @@ IN_PROC_BROWSER_TEST_F(AutomationManagerAuraBrowserTest,
   // Focus view1, then block until we get an accessibility event that
   // shows this view is focused.
   view1->RequestFocus();
-  waiter.WaitForNodeIdToBeFocused(wrapper1->GetUniqueId().Get());
+  waiter.WaitForNodeIdToBeFocused(wrapper1->GetUniqueId());
 
-  EXPECT_TRUE(waiter.WasNodeIdFocused(wrapper1->GetUniqueId().Get()));
-  EXPECT_FALSE(waiter.WasNodeIdFocused(wrapper2->GetUniqueId().Get()));
-  EXPECT_FALSE(waiter.WasNodeIdFocused(wrapper3->GetUniqueId().Get()));
+  EXPECT_TRUE(waiter.WasNodeIdFocused(wrapper1->GetUniqueId()));
+  EXPECT_FALSE(waiter.WasNodeIdFocused(wrapper2->GetUniqueId()));
+  EXPECT_FALSE(waiter.WasNodeIdFocused(wrapper3->GetUniqueId()));
 
   // Now focus view2 and then view3. We shouldn't ever get an event
   // showing view2 as focused, just view3.
   view2->RequestFocus();
   view3->RequestFocus();
-  waiter.WaitForNodeIdToBeFocused(wrapper3->GetUniqueId().Get());
+  waiter.WaitForNodeIdToBeFocused(wrapper3->GetUniqueId());
 
-  EXPECT_TRUE(waiter.WasNodeIdFocused(wrapper1->GetUniqueId().Get()));
-  EXPECT_FALSE(waiter.WasNodeIdFocused(wrapper2->GetUniqueId().Get()));
-  EXPECT_TRUE(waiter.WasNodeIdFocused(wrapper3->GetUniqueId().Get()));
+  EXPECT_TRUE(waiter.WasNodeIdFocused(wrapper1->GetUniqueId()));
+  EXPECT_FALSE(waiter.WasNodeIdFocused(wrapper2->GetUniqueId()));
+  EXPECT_TRUE(waiter.WasNodeIdFocused(wrapper3->GetUniqueId()));
 
   cache->set_focused_widget_for_testing(nullptr);
 

chromecast/browser/ui/aura/accessibility/automation_manager_aura.cc

@@ -165,7 +165,7 @@ void AutomationManagerAura::SendEvent(BrowserContext* context,
   }
 
   ui::AXEvent event;
-  event.id = aura_obj->GetUniqueId().Get();
+  event.id = aura_obj->GetUniqueId();
   event.event_type = event_type;
   event_bundle.events.push_back(event);
 

ui/views/accessibility/ax_aura_obj_cache.cc

@@ -208,7 +208,7 @@ AXAuraObjWrapper* AXAuraObjCache::CreateInternal(
     return Get(it->second);
 
   auto wrapper = std::make_unique<AuraViewWrapper>(aura_view);
-  int32_t id = wrapper->GetUniqueId().Get();
+  int32_t id = wrapper->GetUniqueId();
   aura_view_to_id_map[aura_view] = id;
   cache_[id] = std::move(wrapper);
   return cache_[id].get();

ui/views/accessibility/ax_aura_obj_wrapper.h

@@ -17,7 +17,6 @@
 namespace ui {
 struct AXActionData;
 struct AXNodeData;
-class AXUniqueId;
 }  // namespace ui
 
 namespace views {
@@ -36,7 +35,7 @@ class VIEWS_EXPORT AXAuraObjWrapper {
   virtual void GetChildren(
       std::vector<AXAuraObjWrapper*>* out_children) = 0;
   virtual void Serialize(ui::AXNodeData* out_node_data) = 0;
-  virtual const ui::AXUniqueId& GetUniqueId() const = 0;
+  virtual int32_t GetUniqueId() const = 0;
 
   // Actions.
   virtual bool HandleAccessibleAction(const ui::AXActionData& action);

ui/views/accessibility/ax_root_obj_wrapper.cc

@@ -10,6 +10,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "ui/accessibility/ax_enums.mojom.h"
 #include "ui/accessibility/ax_node_data.h"
+#include "ui/accessibility/platform/ax_unique_id.h"
 #include "ui/aura/env.h"
 #include "ui/aura/window.h"
 #include "ui/display/display.h"
@@ -98,8 +99,8 @@ void AXRootObjWrapper::Serialize(ui::AXNodeData* out_node_data) {
 #endif
 }
 
-const ui::AXUniqueId& AXRootObjWrapper::GetUniqueId() const {
-  return unique_id_;
+int32_t AXRootObjWrapper::GetUniqueId() const {
+  return unique_id_.Get();
 }
 
 void AXRootObjWrapper::OnDisplayMetricsChanged(const display::Display& display,

ui/views/accessibility/ax_root_obj_wrapper.h

@@ -37,7 +37,7 @@ class VIEWS_EXPORT AXRootObjWrapper : public views::AXAuraObjWrapper,
   void GetChildren(
       std::vector<views::AXAuraObjWrapper*>* out_children) override;
   void Serialize(ui::AXNodeData* out_node_data) override;
-  const ui::AXUniqueId& GetUniqueId() const override;
+  int32_t GetUniqueId() const override;
 
  private:
   // display::DisplayObserver:

ui/views/accessibility/ax_tree_source_views.cc

@@ -38,20 +38,20 @@ bool AXTreeSourceViews::GetTreeData(ui::AXTreeData* tree_data) const {
   tree_data->loading_progress = 1.0;
   AXAuraObjWrapper* focus = AXAuraObjCache::GetInstance()->GetFocus();
   if (focus)
-    tree_data->focus_id = focus->GetUniqueId().Get();
+    tree_data->focus_id = focus->GetUniqueId();
   return true;
 }
 
 AXAuraObjWrapper* AXTreeSourceViews::GetFromId(int32_t id) const {
   AXAuraObjWrapper* root = GetRoot();
   // Root might not be in the cache.
-  if (id == root->GetUniqueId().Get())
+  if (id == root->GetUniqueId())
     return root;
   return AXAuraObjCache::GetInstance()->Get(id);
 }
 
 int32_t AXTreeSourceViews::GetId(AXAuraObjWrapper* node) const {
-  return node->GetUniqueId().Get();
+  return node->GetUniqueId();
 }
 
 void AXTreeSourceViews::GetChildren(
@@ -100,7 +100,7 @@ void AXTreeSourceViews::SerializeNode(AXAuraObjWrapper* node,
   ui::AXNodeData parent_data;
   parent->Serialize(&parent_data);
   out_data->location.Offset(-parent_data.location.OffsetFromOrigin());
-  out_data->offset_container_id = parent->GetUniqueId().Get();
+  out_data->offset_container_id = parent->GetUniqueId();
 }
 
 std::string AXTreeSourceViews::ToString(AXAuraObjWrapper* root,

ui/views/accessibility/ax_tree_source_views_unittest.cc

@@ -109,16 +109,16 @@ TEST_F(AXTreeSourceViewsTest, Basics) {
   EXPECT_EQ(root, tree.GetParent(textfield));
 
   // IDs match the ones in the cache.
-  EXPECT_EQ(root->GetUniqueId().Get(), tree.GetId(root));
-  EXPECT_EQ(label1->GetUniqueId().Get(), tree.GetId(label1));
-  EXPECT_EQ(label2->GetUniqueId().Get(), tree.GetId(label2));
-  EXPECT_EQ(textfield->GetUniqueId().Get(), tree.GetId(textfield));
+  EXPECT_EQ(root->GetUniqueId(), tree.GetId(root));
+  EXPECT_EQ(label1->GetUniqueId(), tree.GetId(label1));
+  EXPECT_EQ(label2->GetUniqueId(), tree.GetId(label2));
+  EXPECT_EQ(textfield->GetUniqueId(), tree.GetId(textfield));
 
   // Reverse ID lookups work.
-  EXPECT_EQ(root, tree.GetFromId(root->GetUniqueId().Get()));
-  EXPECT_EQ(label1, tree.GetFromId(label1->GetUniqueId().Get()));
-  EXPECT_EQ(label2, tree.GetFromId(label2->GetUniqueId().Get()));
-  EXPECT_EQ(textfield, tree.GetFromId(textfield->GetUniqueId().Get()));
+  EXPECT_EQ(root, tree.GetFromId(root->GetUniqueId()));
+  EXPECT_EQ(label1, tree.GetFromId(label1->GetUniqueId()));
+  EXPECT_EQ(label2, tree.GetFromId(label2->GetUniqueId()));
+  EXPECT_EQ(textfield, tree.GetFromId(textfield->GetUniqueId()));
 
   // Validity.
   EXPECT_TRUE(tree.IsValid(root));

ui/views/accessibility/ax_view_obj_wrapper.cc

@@ -6,6 +6,7 @@
 
 #include "ui/accessibility/ax_action_data.h"
 #include "ui/accessibility/ax_node_data.h"
+#include "ui/accessibility/platform/ax_unique_id.h"
 #include "ui/views/accessibility/ax_aura_obj_cache.h"
 #include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/view.h"
@@ -16,15 +17,24 @@ namespace views {
 AXViewObjWrapper::AXViewObjWrapper(View* view)  : view_(view) {
   if (view->GetWidget())
     AXAuraObjCache::GetInstance()->GetOrCreate(view->GetWidget());
+  view->AddObserver(this);
 }
 
-AXViewObjWrapper::~AXViewObjWrapper() {}
+AXViewObjWrapper::~AXViewObjWrapper() {
+  if (view_) {
+    view_->RemoveObserver(this);
+    view_ = nullptr;
+  }
+}
 
 bool AXViewObjWrapper::IsIgnored() {
-  return view_->GetViewAccessibility().IsIgnored();
+  return view_ ? view_->GetViewAccessibility().IsIgnored() : true;
 }
 
 AXAuraObjWrapper* AXViewObjWrapper::GetParent() {
+  if (!view_)
+    return nullptr;
+
   AXAuraObjCache* cache = AXAuraObjCache::GetInstance();
   if (view_->parent())
     return cache->GetOrCreate(view_->parent());
@@ -37,6 +47,9 @@ AXAuraObjWrapper* AXViewObjWrapper::GetParent() {
 
 void AXViewObjWrapper::GetChildren(
     std::vector<AXAuraObjWrapper*>* out_children) {
+  if (!view_)
+    return;
+
   if (view_->GetViewAccessibility().IsLeaf())
     return;
 
@@ -52,16 +65,23 @@ void AXViewObjWrapper::GetChildren(
 }
 
 void AXViewObjWrapper::Serialize(ui::AXNodeData* out_node_data) {
+  if (!view_)
+    return;
+
   view_->GetViewAccessibility().GetAccessibleNodeData(out_node_data);
-  out_node_data->id = GetUniqueId().Get();
+  out_node_data->id = GetUniqueId();
 }
 
-const ui::AXUniqueId& AXViewObjWrapper::GetUniqueId() const {
-  return view_->GetViewAccessibility().GetUniqueId();
+int32_t AXViewObjWrapper::GetUniqueId() const {
+  return view_ ? view_->GetViewAccessibility().GetUniqueId() : -1;
 }
 
 bool AXViewObjWrapper::HandleAccessibleAction(const ui::AXActionData& action) {
-  return view_->HandleAccessibleAction(action);
+  return view_ ? view_->HandleAccessibleAction(action) : false;
+}
+
+void AXViewObjWrapper::OnViewIsDeleting(View* observed_view) {
+  view_ = nullptr;
 }
 
 }  // namespace views

ui/views/accessibility/ax_view_obj_wrapper.h

@@ -9,12 +9,13 @@
 
 #include "base/macros.h"
 #include "ui/views/accessibility/ax_aura_obj_wrapper.h"
+#include "ui/views/view_observer.h"
 
 namespace views {
 class View;
 
 // Describes a |View| for use with other AX classes.
-class AXViewObjWrapper : public AXAuraObjWrapper {
+class AXViewObjWrapper : public AXAuraObjWrapper, public ViewObserver {
  public:
   explicit AXViewObjWrapper(View* view);
   ~AXViewObjWrapper() override;
@@ -26,11 +27,14 @@ class AXViewObjWrapper : public AXAuraObjWrapper {
   AXAuraObjWrapper* GetParent() override;
   void GetChildren(std::vector<AXAuraObjWrapper*>* out_children) override;
   void Serialize(ui::AXNodeData* out_node_data) override;
-  const ui::AXUniqueId& GetUniqueId() const final;
+  int32_t GetUniqueId() const final;
   bool HandleAccessibleAction(const ui::AXActionData& action) override;
 
+  // ViewObserver overrides.
+  void OnViewIsDeleting(View* observed_view) override;
+
  private:
-  View* const view_;
+  View* view_;
 
   DISALLOW_COPY_AND_ASSIGN(AXViewObjWrapper);
 };

ui/views/accessibility/ax_widget_obj_wrapper.cc

@@ -46,7 +46,7 @@ void AXWidgetObjWrapper::GetChildren(
 }
 
 void AXWidgetObjWrapper::Serialize(ui::AXNodeData* out_node_data) {
-  out_node_data->id = GetUniqueId().Get();
+  out_node_data->id = GetUniqueId();
   out_node_data->role = widget_->widget_delegate()->GetAccessibleWindowRole();
   out_node_data->AddStringAttribute(
       ax::mojom::StringAttribute::kName,
@@ -56,8 +56,8 @@ void AXWidgetObjWrapper::Serialize(ui::AXNodeData* out_node_data) {
   out_node_data->state = 0;
 }
 
-const ui::AXUniqueId& AXWidgetObjWrapper::GetUniqueId() const {
-  return unique_id_;
+int32_t AXWidgetObjWrapper::GetUniqueId() const {
+  return unique_id_.Get();
 }
 
 void AXWidgetObjWrapper::OnWidgetDestroying(Widget* widget) {

ui/views/accessibility/ax_widget_obj_wrapper.h

@@ -29,7 +29,7 @@ class AXWidgetObjWrapper : public AXAuraObjWrapper,
   AXAuraObjWrapper* GetParent() override;
   void GetChildren(std::vector<AXAuraObjWrapper*>* out_children) override;
   void Serialize(ui::AXNodeData* out_node_data) override;
-  const ui::AXUniqueId& GetUniqueId() const final;
+  int32_t GetUniqueId() const final;
 
   // WidgetObserver overrides.
   void OnWidgetDestroying(Widget* widget) override;

ui/views/accessibility/ax_window_obj_wrapper.cc

@@ -11,7 +11,6 @@
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/accessibility/ax_tree_id.h"
 #include "ui/accessibility/platform/aura_window_properties.h"
-#include "ui/accessibility/platform/ax_unique_id.h"
 #include "ui/aura/client/focus_client.h"
 #include "ui/aura/window.h"
 #include "ui/views/accessibility/ax_aura_obj_cache.h"
@@ -85,7 +84,7 @@ void AXWindowObjWrapper::GetChildren(
 }
 
 void AXWindowObjWrapper::Serialize(ui::AXNodeData* out_node_data) {
-  out_node_data->id = GetUniqueId().Get();
+  out_node_data->id = GetUniqueId();
   ax::mojom::Role role = window_->GetProperty(ui::kAXRoleOverride);
   if (role != ax::mojom::Role::kNone)
     out_node_data->role = role;
@@ -116,8 +115,8 @@ void AXWindowObjWrapper::Serialize(ui::AXNodeData* out_node_data) {
   }
 }
 
-const ui::AXUniqueId& AXWindowObjWrapper::GetUniqueId() const {
-  return unique_id_;
+int32_t AXWindowObjWrapper::GetUniqueId() const {
+  return unique_id_.Get();
 }
 
 void AXWindowObjWrapper::OnWindowDestroyed(aura::Window* window) {

ui/views/accessibility/ax_window_obj_wrapper.h

@@ -36,7 +36,7 @@ class AXWindowObjWrapper : public AXAuraObjWrapper,
   AXAuraObjWrapper* GetParent() override;
   void GetChildren(std::vector<AXAuraObjWrapper*>* out_children) override;
   void Serialize(ui::AXNodeData* out_node_data) override;
-  const ui::AXUniqueId& GetUniqueId() const final;
+  int32_t GetUniqueId() const final;
 
   // WindowObserver overrides.
   void OnWindowDestroyed(aura::Window* window) override;

ui/views/mus/ax_remote_host.cc

@@ -247,7 +247,7 @@ void AXRemoteHost::SendEvent(AXAuraObjWrapper* aura_obj,
   }
 
   ui::AXEvent event;
-  event.id = aura_obj->GetUniqueId().Get();
+  event.id = aura_obj->GetUniqueId();
   event.event_type = event_type;
   // Other fields are not used.
 

ui/views/mus/ax_tree_source_mus_unittest.cc

@@ -86,7 +86,7 @@ TEST_F(AXTreeSourceMusTest, Serialize) {
 
   // Child has relative position with the root as the container.
   EXPECT_EQ(gfx::RectF(1, 1, 111, 111), node_data.location);
-  EXPECT_EQ(root->GetUniqueId().Get(), node_data.offset_container_id);
+  EXPECT_EQ(root->GetUniqueId(), node_data.offset_container_id);
 }
 
 TEST_F(AXTreeSourceMusTest, ScaleFactor) {