Improve pos_in_set and set_size cache robustness

Write and pass a test case for an invalid AXTree. Test exposed
error handling not previously considered. This change list depends
on https://chromium-review.googlesource.com/c/chromium/src/+/1351782.

Change-Id: Ie5e65f6583a9baf6861a95e7db4969bd3ec0e979
Reviewed-on: https://chromium-review.googlesource.com/c/1357530
Commit-Queue: Akihiro Ota <akihiroota@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613388}

ui/accessibility/ax_node.cc

@@ -585,13 +585,19 @@ bool AXNode::SetRoleMatchesItemRole(const AXNode* ordered_set) const {
 // Is not required for set's role to match node's role.
 AXNode* AXNode::GetOrderedSet() const {
   AXNode* result = parent();
+
   // Continue walking up while parent is invalid, ignored, or is a generic
   // container.
-  while ((result && result->data().HasState(ax::mojom::State::kIgnored)) ||
-         result->data().role == ax::mojom::Role::kGenericContainer ||
-         result->data().role == ax::mojom::Role::kIgnored) {
+  while (result && (result->data().HasState(ax::mojom::State::kIgnored) ||
+                    result->data().role == ax::mojom::Role::kGenericContainer ||
+                    result->data().role == ax::mojom::Role::kIgnored)) {
     result = result->parent();
   }
+
+  // If the parent of this node isn't a valid ordered set, return nullptr
+  if (result && !IsSetLike(result->data().role))
+    return nullptr;
+
   return result;
 }
 

ui/accessibility/ax_tree.cc

@@ -882,7 +882,8 @@ int32_t AXTree::GetNextNegativeInternalNodeId() {
   return return_value;
 }
 
-// Populates items vector with all items within ordered set whose roles match.
+// Populates items vector with all items within ordered_set.
+// Will only add items whose roles match the role of the ordered_set.
 void AXTree::PopulateOrderedSetItems(const AXNode* ordered_set,
                                      const AXNode* local_parent,
                                      std::vector<const AXNode*>& items) const {
@@ -908,7 +909,10 @@ void AXTree::PopulateOrderedSetItems(const AXNode* ordered_set,
 
 // Given an ordered_set, compute pos_in_set and set_size for all of its items
 // and store values in cache.
+// Ordered_set should never be nullptr.
 void AXTree::ComputeSetSizePosInSetAndCache(const AXNode* ordered_set) {
+  DCHECK(ordered_set);
+
   // Default ordered_set's pos_in_set and set_size to 0.
   ordered_set_info_map_[ordered_set->id()] = OrderedSetInfo();
 
@@ -983,9 +987,11 @@ void AXTree::ComputeSetSizePosInSetAndCache(const AXNode* ordered_set) {
 // pos_in_set values, minimizing the size of the cache.
 int32_t AXTree::GetPosInSet(const AXNode& item) {
   // If item's id is not in the cache, compute it.
-  if (ordered_set_info_map_.find(item.id()) == ordered_set_info_map_.end())
-    ComputeSetSizePosInSetAndCache(item.GetOrderedSet());
-
+  if (ordered_set_info_map_.find(item.id()) == ordered_set_info_map_.end()) {
+    const AXNode* ordered_set = item.GetOrderedSet();
+    if (ordered_set)
+      ComputeSetSizePosInSetAndCache(ordered_set);
+  }
   return ordered_set_info_map_[item.id()].pos_in_set;
 }
 
@@ -996,17 +1002,14 @@ int32_t AXTree::GetPosInSet(const AXNode& item) {
 // This function is guaranteed to be only called on nodes that can hold
 // set_size values, minimizing the size of the cache.
 int32_t AXTree::GetSetSize(const AXNode& node) {
-  const AXNode* ordered_set;
-
   // If node's id is not in the cache, compute it.
   if (ordered_set_info_map_.find(node.id()) == ordered_set_info_map_.end()) {
+    const AXNode* ordered_set = &node;
     // If node is item-like, find its outerlying ordered set
     if (IsItemLike(node.data().role))
       ordered_set = node.GetOrderedSet();
-    // If its set-like, then it is the ordered set
-    else
-      ordered_set = &node;
-    ComputeSetSizePosInSetAndCache(ordered_set);
+    if (ordered_set)
+      ComputeSetSizePosInSetAndCache(ordered_set);
   }
   return ordered_set_info_map_[node.id()].set_size;
 }

ui/accessibility/ax_tree.h

@@ -362,7 +362,8 @@ class AX_EXPORT AXTree : public AXNode::OwnerTree {
     ~OrderedSetInfo() {}
   };
 
-  // Populates items vector with all items within ordered_set whose roles match.
+  // Populates items vector with all items within ordered_set.
+  // Will only add items whose roles match the role of the ordered_set.
   void PopulateOrderedSetItems(const AXNode* ordered_set,
                                const AXNode* local_parent,
                                std::vector<const AXNode*>& items) const;
@@ -370,8 +371,10 @@ class AX_EXPORT AXTree : public AXNode::OwnerTree {
   // values of all items in ordered_set and caches those values.
   void ComputeSetSizePosInSetAndCache(const AXNode* ordered_set);
 
-  // Map from node ID to OrderedSetInfo, if the given node
-  // is an element within an ordered set.
+  // Map from node ID to OrderedSetInfo.
+  // Item-like and ordered-set-like objects will map to populated OrderedSetInfo
+  // objects.
+  // All other objects will map to default-constructed OrderedSetInfo objects.
   // Invalidated every time the tree is updated.
   mutable std::unordered_map<int32_t, OrderedSetInfo> ordered_set_info_map_;
 

ui/accessibility/ax_tree_unittest.cc

@@ -1943,7 +1943,7 @@ TEST(AXTreeTest, TestSetSizePosInSetAddItem) {
   EXPECT_EQ(new_item4->GetSetSize(), 4);
 }
 
-// Tests that the outerlying ordered set reports its set_size. Ordered sets
+// Tests that the outerlying ordered set reports a set_size. Ordered sets
 // should not report a pos_in_set value other than 0, since they are not
 // considered to be items within a set (even when nested).
 TEST(AXTreeTest, TestOrderedSetReportsSetSize) {
@@ -2027,4 +2027,31 @@ TEST(AXTreeTest, TestOrderedSetReportsSetSize) {
   EXPECT_EQ(inner_list4->GetSetSize(), 5);
 }
 
+// Tests GetPosInSet and GetSetSize code on invalid input
+TEST(AXTreeTest, TestSetSizePosInSetInvalid) {
+  AXTreeUpdate tree_update;
+  tree_update.root_id = 1;
+  tree_update.nodes.resize(3);
+  tree_update.nodes[0].id = 1;
+  tree_update.nodes[0].role = ax::mojom::Role::kListItem;  // 0 of 0
+  tree_update.nodes[0].child_ids = {2, 3};
+  tree_update.nodes[1].id = 2;
+  tree_update.nodes[1].role = ax::mojom::Role::kListItem;
+  tree_update.nodes[1].AddIntAttribute(ax::mojom::IntAttribute::kPosInSet,
+                                       4);  // 0 of 0
+  tree_update.nodes[2].id = 3;
+  tree_update.nodes[2].role = ax::mojom::Role::kListItem;
+  AXTree tree(tree_update);
+
+  AXNode* item1 = tree.GetFromId(1);
+  EXPECT_EQ(item1->GetPosInSet(), 0);
+  EXPECT_EQ(item1->GetSetSize(), 0);
+  AXNode* item2 = tree.GetFromId(2);
+  EXPECT_EQ(item2->GetPosInSet(), 0);
+  EXPECT_EQ(item2->GetSetSize(), 0);
+  AXNode* item3 = tree.GetFromId(3);
+  EXPECT_EQ(item3->GetPosInSet(), 0);
+  EXPECT_EQ(item3->GetSetSize(), 0);
+}
+
 }  // namespace ui