Fix AXPosition::AtEndOfTextSpan to not rely on CreateParentPosition

AXPosition::AtEndOfTextSpan relies on CreateParentPosition to
determine if an inline text box marks the end of text span. Since
CreateParentPosition calls into AtStartOfLine which calls
AtEndOfTextSpan so we end up with an infinite loop and stack overflow.

https://crrev.com/c/2245611 fixed most cases with this infinite loop
of AtStartOfLine/CreateParentPosition/AtEndOfTextSpan by checking for
AtEndOfAnchor first before calling AtEndOfTextSpan.

However consider the following example:
<style>
  .required-label::after {
    content: " *";
  }
</style>
<label class="required-label">Required </label>

labelText #1
++staticText name='Required ' nextOnLineId=inlineTextBox  #2
++++inlineTextBox name='Required ' nextOnLineId=inlineTextBox #3
++staticText name=' *' previousOnLineId=inlineTextBox #4
++++inlineTextBox name='*' previousOnLineId=inlineTextBox #5

AtEndOfAnchor check would not prevent this infinite loop, if
AtStartOfLine is called on staticText #4 name=' *' as the max text
offsets differ between staticText #4 and its inlineText #5.

This CL fixes the above issue by making AtEndOfTextSpan not to call
CreateParentPosition. Instead it determines end of text span by
directly looking at the current position's parent role and its index
among its siblings.

AX-Relnotes: Fixes hangs in certain accessibility scenarios.

Bug: 1120636
Change-Id: Ic99c27b069f220f26da736b3946560ef23d6b622
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2369681
Commit-Queue: Victor Fei <vicfei@microsoft.com>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804531}

content/browser/accessibility/browser_accessibility_position.cc

@@ -104,6 +104,13 @@ int BrowserAccessibilityPosition::AnchorIndexInParent() const {
                      : AXPosition::INVALID_INDEX;
 }
 
+int BrowserAccessibilityPosition::AnchorSiblingCount() const {
+  BrowserAccessibility* parent = GetAnchor()->PlatformGetParent();
+  if (parent)
+    return static_cast<int>(parent->InternalChildCount());
+  return 0;
+}
+
 base::stack<BrowserAccessibility*>
 BrowserAccessibilityPosition::GetAncestorAnchors() const {
   base::stack<BrowserAccessibility*> anchors;
@@ -186,11 +193,16 @@ bool BrowserAccessibilityPosition::IsInLineBreakingObject() const {
          !GetAnchor()->IsInListMarker();
 }
 
-ax::mojom::Role BrowserAccessibilityPosition::GetRole() const {
+ax::mojom::Role BrowserAccessibilityPosition::GetAnchorRole() const {
   if (IsNullPosition())
     return ax::mojom::Role::kNone;
   DCHECK(GetAnchor());
-  return GetAnchor()->GetRole();
+  return GetRole(GetAnchor());
+}
+
+ax::mojom::Role BrowserAccessibilityPosition::GetRole(
+    BrowserAccessibility* node) const {
+  return node->GetRole();
 }
 
 ui::AXNodeTextStyles BrowserAccessibilityPosition::GetTextStyles() const {

content/browser/accessibility/browser_accessibility_position.h

@@ -43,6 +43,7 @@ class CONTENT_EXPORT BrowserAccessibilityPosition
   int AnchorChildCount() const override;
   int AnchorUnignoredChildCount() const override;
   int AnchorIndexInParent() const override;
+  int AnchorSiblingCount() const override;
   base::stack<BrowserAccessibility*> GetAncestorAnchors() const override;
   BrowserAccessibility* GetLowestUnignoredAncestor() const override;
   void AnchorParent(AXTreeID* tree_id,
@@ -55,7 +56,8 @@ class CONTENT_EXPORT BrowserAccessibilityPosition
   bool IsEmbeddedObjectInParent() const override;
 
   bool IsInLineBreakingObject() const override;
-  ax::mojom::Role GetRole() const override;
+  ax::mojom::Role GetAnchorRole() const override;
+  ax::mojom::Role GetRole(BrowserAccessibility* node) const override;
   ui::AXNodeTextStyles GetTextStyles() const override;
   std::vector<int32_t> GetWordStartOffsets() const override;
   std::vector<int32_t> GetWordEndOffsets() const override;

ui/accessibility/ax_node_position.cc

@@ -99,6 +99,14 @@ int AXNodePosition::AnchorIndexInParent() const {
   return GetAnchor() ? int{GetAnchor()->index_in_parent()} : INVALID_INDEX;
 }
 
+int AXNodePosition::AnchorSiblingCount() const {
+  AXNode* parent = GetAnchor()->GetUnignoredParent();
+  if (parent)
+    return static_cast<int>(parent->GetUnignoredChildCount());
+
+  return 0;
+}
+
 base::stack<AXNode*> AXNodePosition::GetAncestorAnchors() const {
   base::stack<AXNode*> anchors;
   AXNode* current_anchor = GetAnchor();
@@ -283,11 +291,15 @@ bool AXNodePosition::IsInLineBreakingObject() const {
          !GetAnchor()->IsInListMarker();
 }
 
-ax::mojom::Role AXNodePosition::GetRole() const {
+ax::mojom::Role AXNodePosition::GetAnchorRole() const {
   if (IsNullPosition())
     return ax::mojom::Role::kNone;
   DCHECK(GetAnchor());
-  return GetAnchor()->data().role;
+  return GetRole(GetAnchor());
+}
+
+ax::mojom::Role AXNodePosition::GetRole(AXNode* node) const {
+  return node->data().role;
 }
 
 AXNodeTextStyles AXNodePosition::GetTextStyles() const {

ui/accessibility/ax_node_position.h

@@ -49,6 +49,7 @@ class AX_EXPORT AXNodePosition : public AXPosition<AXNodePosition, AXNode> {
   int AnchorChildCount() const override;
   int AnchorUnignoredChildCount() const override;
   int AnchorIndexInParent() const override;
+  int AnchorSiblingCount() const override;
   base::stack<AXNode*> GetAncestorAnchors() const override;
   AXNode* GetLowestUnignoredAncestor() const override;
   void AnchorParent(AXTreeID* tree_id, AXNode::AXID* parent_id) const override;
@@ -58,7 +59,8 @@ class AX_EXPORT AXNodePosition : public AXPosition<AXNodePosition, AXNode> {
 
   bool IsEmbeddedObjectInParent() const override;
   bool IsInLineBreakingObject() const override;
-  ax::mojom::Role GetRole() const override;
+  ax::mojom::Role GetAnchorRole() const override;
+  ax::mojom::Role GetRole(AXNode* node) const override;
   AXNodeTextStyles GetTextStyles() const override;
   std::vector<int32_t> GetWordStartOffsets() const override;
   std::vector<int32_t> GetWordEndOffsets() const override;

ui/accessibility/ax_node_position_unittest.cc

@@ -1260,6 +1260,59 @@ TEST_F(AXPositionTest, AtStartOfLineWithTextPosition) {
   EXPECT_FALSE(text_position->AtStartOfLine());
 }
 
+TEST_F(AXPositionTest, AtStartOfLineStaticTextExtraPrecedingSpace) {
+  // Consider the following web content:
+  //   <style>
+  //     .required-label::after {
+  //       content: " *";
+  //     }
+  //   </style>
+  //   <label class="required-label">Required </label>
+  //
+  // Which has the following AXTree, where the static text (#3)
+  // contains an extra preceding space compared to its inline text (#4).
+  // ++1 kRootWebArea
+  // ++++2 kLabelText
+  // ++++++3 kStaticText      name=" *"
+  // ++++++++4 kInlineTextBox name="*"
+  // This test ensures that this difference between static text and its inline
+  // text box does not cause a hang when AtStartOfLine is called on static text
+  // with text position " <*>".
+
+  AXNodeData root;
+  root.id = 1;
+  root.role = ax::mojom::Role::kRootWebArea;
+  // "kIsLineBreakingObject" is not strictly necessary but is added for
+  // completeness.
+  root.AddBoolAttribute(ax::mojom::BoolAttribute::kIsLineBreakingObject, true);
+  AXNodeData label_text;
+  label_text.id = 2;
+  label_text.role = ax::mojom::Role::kLabelText;
+
+  AXNodeData static_text1;
+  static_text1.id = 3;
+  static_text1.role = ax::mojom::Role::kStaticText;
+  static_text1.SetName(" *");
+
+  AXNodeData inline_text1;
+  inline_text1.id = 4;
+  inline_text1.role = ax::mojom::Role::kInlineTextBox;
+  inline_text1.SetName("*");
+
+  static_text1.child_ids = {inline_text1.id};
+  root.child_ids = {static_text1.id};
+
+  SetTree(CreateAXTree({root, static_text1, inline_text1}));
+
+  // Calling AtStartOfLine on |static_text1| with position " <*>",
+  // text_offset_=1, should not get into an infinite loop; it should be
+  // guaranteed to terminate.
+  TestPositionType text_position = AXNodePosition::CreateTextPosition(
+      GetTreeID(), static_text1.id, 1 /* child_index */,
+      ax::mojom::TextAffinity::kDownstream);
+  ASSERT_FALSE(text_position->AtStartOfLine());
+}
+
 TEST_F(AXPositionTest, AtEndOfLineWithTextPosition) {
   TestPositionType text_position = AXNodePosition::CreateTextPosition(
       GetTreeID(), inline_box1_.id, 5 /* text_offset */,

ui/accessibility/ax_position.h

@@ -968,7 +968,7 @@ class AXPosition {
   bool AtStartOfDocument() const {
     if (IsNullPosition())
       return false;
-    return IsDocument(GetRole()) && AtStartOfAnchor();
+    return IsDocument(GetAnchorRole()) && AtStartOfAnchor();
   }
 
   bool AtEndOfDocument() const {
@@ -3148,8 +3148,8 @@ class AXPosition {
     // infinite loop. However, GetAnchor()->IsIgnored() is sufficient here
     // because we know that the anchor at this position doesn't have an
     // unignored child, making this a leaf tree or text position.
-    return !GetAnchor()->IsIgnored() && !IsDocument(GetRole()) &&
-           !IsInTextObject() && !IsIframe(GetRole());
+    return !GetAnchor()->IsIgnored() && !IsDocument(GetAnchorRole()) &&
+           !IsInTextObject() && !IsIframe(GetAnchorRole());
   }
 
   bool IsInDescendantOfEmptyObject() const {
@@ -3343,6 +3343,7 @@ class AXPosition {
   // When we call the following method on TextField, it would return 1.
   virtual int AnchorUnignoredChildCount() const = 0;
   virtual int AnchorIndexInParent() const = 0;
+  virtual int AnchorSiblingCount() const = 0;
   virtual base::stack<AXNodeType*> GetAncestorAnchors() const = 0;
   virtual AXNodeType* GetLowestUnignoredAncestor() const = 0;
   virtual void AnchorParent(AXTreeID* tree_id, int32_t* parent_id) const = 0;
@@ -3366,7 +3367,8 @@ class AXPosition {
   // break in the text representation, e.g. a block level element or a <br>.
   virtual bool IsInLineBreakingObject() const = 0;
 
-  virtual ax::mojom::Role GetRole() const = 0;
+  virtual ax::mojom::Role GetAnchorRole() const = 0;
+  virtual ax::mojom::Role GetRole(AXNodeType* node) const = 0;
   virtual AXNodeTextStyles GetTextStyles() const = 0;
   virtual std::vector<int32_t> GetWordStartOffsets() const = 0;
   virtual std::vector<int32_t> GetWordEndOffsets() const = 0;
@@ -3400,12 +3402,17 @@ class AXPosition {
   // A text span is defined by a series of inline text boxes that make up a
   // single static text object.
   bool AtEndOfTextSpan() const {
-    if (GetRole() != ax::mojom::Role::kInlineTextBox || !AtEndOfAnchor())
+    if (GetAnchorRole() != ax::mojom::Role::kInlineTextBox || !AtEndOfAnchor())
       return false;
 
-    AXPositionInstance parent_position = CreateParentPosition();
-    return parent_position->GetRole() == ax::mojom::Role::kStaticText &&
-           parent_position->AtEndOfAnchor();
+    // We are at the end of text span if |this| position has
+    // role::kInlineTextBox, the parent of |this| has role::kStaticText, and the
+    // anchor node of |this| is the last child of parent's children.
+    const bool is_last_child =
+        AnchorIndexInParent() == (AnchorSiblingCount() - 1);
+
+    return is_last_child && GetRole(GetLowestUnignoredAncestor()) ==
+                                ax::mojom::Role::kStaticText;
   }
 
   // Uses depth-first pre-order traversal.
@@ -3650,8 +3657,8 @@ class AXPosition {
     }
 
     // Treat moving into or out of nodes with certain roles as a format break.
-    ax::mojom::Role from_role = move_from.GetRole();
-    ax::mojom::Role to_role = move_to.GetRole();
+    ax::mojom::Role from_role = move_from.GetAnchorRole();
+    ax::mojom::Role to_role = move_to.GetAnchorRole();
     if (from_role != to_role) {
       if (IsFormatBoundary(from_role) || IsFormatBoundary(to_role))
         return true;
@@ -3833,7 +3840,7 @@ class AXPosition {
   AXPositionInstance CreateDocumentAncestorPosition() const {
     AXPositionInstance iterator = Clone();
     while (!iterator->IsNullPosition()) {
-      if (IsDocument(iterator->GetRole()) &&
+      if (IsDocument(iterator->GetAnchorRole()) &&
           iterator->CreateParentPosition()->IsNullPosition()) {
         break;
       }