arc-a11y: Prevent infinite loop in AX name computation

This CL adds a boolean argument to computeAXName(), and it is used when computing
the name, when the node is labeled by another node.
Without these changes, A11yNodeInfoDatWrapperTest.LabeledByLoop will go through an infinite
loop.

Also rename "labelled" to "labeled" for consistency.

Bug: b:161294907
Test: AccessibilityNodeInfoDataWrapperTest.LabeledByLoop
Change-Id: I09f6415064cd0f8a3a159b08c217e4f5784c1528
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2301555
Commit-Queue: Sara Kato <sarakato@chromium.org>
Reviewed-by: Hiroki Sato <hirokisato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789340}

chrome/browser/chromeos/arc/accessibility/accessibility_info_data_wrapper.h

@@ -45,7 +45,7 @@ class AccessibilityInfoDataWrapper {
   virtual void PopulateAXRole(ui::AXNodeData* out_data) const = 0;
   virtual void PopulateAXState(ui::AXNodeData* out_data) const = 0;
   virtual void Serialize(ui::AXNodeData* out_data) const = 0;
-  virtual std::string ComputeAXName() const = 0;
+  virtual std::string ComputeAXName(bool do_recursive) const = 0;
   virtual void GetChildren(
       std::vector<AccessibilityInfoDataWrapper*>* children) const = 0;
 

chrome/browser/chromeos/arc/accessibility/accessibility_node_info_data_wrapper.cc

@@ -92,7 +92,7 @@ bool AccessibilityNodeInfoDataWrapper::IsImportantInAndroid() const {
 bool AccessibilityNodeInfoDataWrapper::CanBeAccessibilityFocused() const {
   if (!IsAccessibilityFocusableContainer() && !HasAccessibilityFocusableText())
     return false;
-  return !ComputeAXName().empty();
+  return !ComputeAXName(true).empty();
 }
 
 bool AccessibilityNodeInfoDataWrapper::IsAccessibilityFocusableContainer()
@@ -320,7 +320,7 @@ void AccessibilityNodeInfoDataWrapper::Serialize(
   bool is_node_tree_root = tree_source_->IsRootOfNodeTree(GetId());
 
   // String properties.
-  const std::string name = ComputeAXName();
+  const std::string name = ComputeAXName(true);
   if (!name.empty())
     out_data->SetName(name);
 
@@ -478,9 +478,10 @@ void AccessibilityNodeInfoDataWrapper::Serialize(
   }
 }
 
-std::string AccessibilityNodeInfoDataWrapper::ComputeAXName() const {
+std::string AccessibilityNodeInfoDataWrapper::ComputeAXName(
+    bool do_recursive) const {
   // Accessible name computation is a concatenated string comprising of:
-  // content description, text, labelled by text, pane title, and cached name
+  // content description, text, labeled by text, pane title, and cached name
   // from previous events.
 
   // TODO(sarakato): Exposing all possible labels for a node, may result in
@@ -492,14 +493,12 @@ std::string AccessibilityNodeInfoDataWrapper::ComputeAXName() const {
   GetProperty(AXStringProperty::CONTENT_DESCRIPTION, &content_description);
   GetProperty(AXStringProperty::TEXT, &text);
 
-  int labelled_by = -1;
-  if (GetProperty(AXIntProperty::LABELED_BY, &labelled_by)) {
-    AccessibilityInfoDataWrapper* labelled_by_node =
-        tree_source_->GetFromId(labelled_by);
-    if (labelled_by_node && labelled_by_node->IsNode()) {
-      // TODO(sarakato): Fix potential bug to be an infinite loop.
-      label = labelled_by_node->ComputeAXName();
-    }
+  int labeled_by = -1;
+  if (do_recursive && GetProperty(AXIntProperty::LABELED_BY, &labeled_by)) {
+    AccessibilityInfoDataWrapper* labeled_by_node =
+        tree_source_->GetFromId(labeled_by);
+    if (labeled_by_node && labeled_by_node->IsNode())
+      label = labeled_by_node->ComputeAXName(false);
   }
 
   std::string pane_title;
@@ -668,7 +667,7 @@ void AccessibilityNodeInfoDataWrapper::ComputeNameFromContentsInternal(
     return;
 
   // Take the name from either content description or text. It's not clear
-  // whether labelled by should be taken into account here.
+  // whether labeled by should be taken into account here.
   std::string name;
   if (!GetProperty(AXStringProperty::CONTENT_DESCRIPTION, &name) ||
       name.empty()) {

chrome/browser/chromeos/arc/accessibility/accessibility_node_info_data_wrapper.h

@@ -39,7 +39,7 @@ class AccessibilityNodeInfoDataWrapper : public AccessibilityInfoDataWrapper {
   void PopulateAXRole(ui::AXNodeData* out_data) const override;
   void PopulateAXState(ui::AXNodeData* out_data) const override;
   void Serialize(ui::AXNodeData* out_data) const override;
-  std::string ComputeAXName() const override;
+  std::string ComputeAXName(bool do_recursive) const override;
   void GetChildren(
       std::vector<AccessibilityInfoDataWrapper*>* children) const override;
 

chrome/browser/chromeos/arc/accessibility/accessibility_node_info_data_wrapper_unittest.cc

@@ -39,10 +39,8 @@ void SetProperty(AXNodeInfoData* node, AXBooleanProperty prop, bool value) {
   arc::SetProperty(node->boolean_properties, prop, value);
 }
 
-void SetProperty(AXNodeInfoData* node,
-                 AXStringProperty prop,
-                 const std::string& value) {
-  arc::SetProperty(node->string_properties, prop, value);
+void SetProperty(AXNodeInfoData* node, AXIntProperty prop, int value) {
+  arc::SetProperty(node->int_properties, prop, value);
 }
 
 void SetProperty(AXNodeInfoData* node,
@@ -51,6 +49,12 @@ void SetProperty(AXNodeInfoData* node,
   arc::SetProperty(node->int_list_properties, prop, value);
 }
 
+void SetProperty(AXNodeInfoData* node,
+                 AXStringProperty prop,
+                 const std::string& value) {
+  arc::SetProperty(node->string_properties, prop, value);
+}
+
 }  // namespace
 
 class AccessibilityNodeInfoDataWrapperTest : public testing::Test,
@@ -569,4 +573,30 @@ TEST_F(AccessibilityNodeInfoDataWrapperTest, StateDescription) {
   EXPECT_EQ("state description", value);
 }
 
+TEST_F(AccessibilityNodeInfoDataWrapperTest, LabeledByLoop) {
+  AXNodeInfoData root;
+  root.id = 1;
+  SetProperty(&root, AXIntProperty::LABELED_BY, 2);
+  AccessibilityNodeInfoDataWrapper wrapper(tree_source(), &root);
+  SetIdToWrapper(&wrapper);
+
+  AXNodeInfoData node2;
+  node2.id = 2;
+  AccessibilityNodeInfoDataWrapper child1_wrapper(tree_source(), &node2);
+  SetIdToWrapper(&child1_wrapper);
+  SetProperty(&node2, AXStringProperty::CONTENT_DESCRIPTION, "node2");
+  SetProperty(&node2, AXIntProperty::LABELED_BY, 1);
+
+  ui::AXNodeData data = CallSerialize(wrapper);
+  std::string name;
+  ASSERT_TRUE(
+      data.GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
+  EXPECT_EQ("node2", name);
+
+  data = CallSerialize(child1_wrapper);
+  ASSERT_TRUE(
+      data.GetStringAttribute(ax::mojom::StringAttribute::kName, &name));
+  EXPECT_EQ("node2", name);
+}
+
 }  // namespace arc

chrome/browser/chromeos/arc/accessibility/accessibility_window_info_data_wrapper.cc

@@ -112,7 +112,7 @@ void AccessibilityWindowInfoDataWrapper::Serialize(
   AccessibilityInfoDataWrapper::Serialize(out_data);
 
   // String properties.
-  const std::string name = ComputeAXName();
+  const std::string name = ComputeAXName(true);
   if (!name.empty()) {
     out_data->SetName(name);
     out_data->SetNameFrom(ax::mojom::NameFrom::kTitle);
@@ -142,7 +142,8 @@ void AccessibilityWindowInfoDataWrapper::Serialize(
   // and LAYER_ORDER in ax::mojom::IntAttributes.
 }
 
-std::string AccessibilityWindowInfoDataWrapper::ComputeAXName() const {
+std::string AccessibilityWindowInfoDataWrapper::ComputeAXName(
+    bool do_recursive) const {
   std::string title;
   GetProperty(mojom::AccessibilityWindowStringProperty::TITLE, &title);
   return title;

chrome/browser/chromeos/arc/accessibility/accessibility_window_info_data_wrapper.h

@@ -37,7 +37,7 @@ class AccessibilityWindowInfoDataWrapper : public AccessibilityInfoDataWrapper {
   void PopulateAXRole(ui::AXNodeData* out_data) const override;
   void PopulateAXState(ui::AXNodeData* out_data) const override;
   void Serialize(ui::AXNodeData* out_data) const override;
-  std::string ComputeAXName() const override;
+  std::string ComputeAXName(bool do_recursive) const override;
   void GetChildren(
       std::vector<AccessibilityInfoDataWrapper*>* children) const override;
 

chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc

@@ -518,7 +518,7 @@ void AXTreeSourceArc::HandleLiveRegions(std::vector<ui::AXEvent>* events) {
       static_cast<AccessibilityNodeInfoDataWrapper*>(node)
           ->set_container_live_status(live_region_type);
 
-      new_live_region_map[node->GetId()] = node->ComputeAXName();
+      new_live_region_map[node->GetId()] = node->ComputeAXName(true);
 
       std::vector<int32_t> children;
       if (GetProperty(node->GetNode()->int_list_properties,