Handle more corner cases for textrange endpoints node deletion

Previous CLs (crrev.com/c/3331356, crrev.com/c/3293769) missed a couple
of cases:

- Due to how we handle parent positions, the leaf tree position after
normalization could end up being ignored. If that is the case, the
position will be moved forward, leaving us in the case where
start > end. In those cases, we left start pointing at end, which could
be a pointer to a soon-to-be-deleted node. Instead, we now move the
degenerate range to a position we know will not be deleted.

- In trees where the ancestor chain of the deleted subtree is completely
ignored, we might not find a parent position (or at least normalizing
to unignored might put us back in the unignored subtree to be deleted).
In these cases, make sure we end up with a null position so we don't
have a dangling pointer.

One other performance-related fix to the previous code: we should use
a cheaper mechanism to say which nodes we care about. In the common
case, deleted subtrees won't affect a given text range so do a node-
based walk instead of trying to create the parent/ancestor positions
up-front.

This CL also removes the DumpWithoutCrashing since its location is a
bit late to understand what operations led to the conditions where it
currently fires.

(cherry picked from commit 8b2eacc)

Bug: 1278304, 1265638
Change-Id: I1aa318e54f9f9b4c8a1b13f71f6fdc678e510f39
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3351043
Commit-Queue: Daniel Libby <dlibby@microsoft.com>
Reviewed-by: Jacques Newman <janewman@microsoft.com>
Cr-Original-Commit-Position: refs/heads/main@{#953289}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3453248
Reviewed-by: Srinivas Sista <srinivassista@chromium.org>
Commit-Queue: Srinivas Sista <srinivassista@chromium.org>
Owners-Override: Srinivas Sista <srinivassista@chromium.org>
Cr-Commit-Position: refs/branch-heads/4758@{#1143}
Cr-Branched-From: 4a2cf4b-refs/heads/main@{#950365}

ui/accessibility/ax_node.cc

@@ -1510,6 +1510,10 @@ AXNode* AXNode::GetOrderedSet() const {
 
   return result;
 }
+
+bool AXNode::IsDataValid() const {
+  return !is_data_still_uninitialized_ && !has_data_been_taken_;
+}
 
 AXNode* AXNode::ComputeLastUnignoredChildRecursive() const {
   DCHECK(!tree_->GetTreeUpdateInProgressState());

ui/accessibility/ax_node.h

@@ -695,6 +695,10 @@ class AX_EXPORT AXNode final {
   // Finds and returns a pointer to ordered set containing node.
   AXNode* GetOrderedSet() const;
 
+  // Returns false if the |data_| is uninitialized or has been taken. Returns
+  // true otherwise.
+  bool IsDataValid() const;
+
  private:
   AXTableInfo* GetAncestorTableInfo() const;
   void IdVectorToNodeVector(const std::vector<AXNodeID>& ids,

ui/accessibility/platform/ax_platform_node_textrangeprovider_win.cc

@@ -1619,19 +1619,22 @@ void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::
   // When the subtree of the root node will be deleted, we can be certain that
   // our endpoint should be invalidated. We know it's the root node when the
   // node doesn't have a parent.
-  if (!node->GetParent()) {
+  AXNode* endpoint_anchor = endpoint->GetAnchor();
+  if (!node->GetParent() || !endpoint_anchor) {
     is_start_endpoint ? SetStart(AXNodePosition::CreateNullPosition())
                       : SetEnd(AXNodePosition::CreateNullPosition());
     return;
   }
 
+  // Fast check for the common case - there are many tree updates and the
+  // endpoints probably are not in the deleted subtree. Note that
+  // CreateAncestorPosition/GetParentPosition can be expensive for text
+  // positions.
+  if (!endpoint_anchor->IsDescendantOfCrossingTreeBoundary(node))
+    return;
+
   AXPositionInstance new_endpoint = endpoint->CreateAncestorPosition(
       node, ax::mojom::MoveDirection::kForward);
-  // When a null position is created from CreateAncestorPosition, it means that
-  // |node| wasn't an ancestor of |new_endpoint| or the anchor it's on. This
-  // means that endpoint is unaffected by the node deletion.
-  if (new_endpoint->IsNullPosition())
-    return;
 
   // Obviously, we want the position to be on the parent of |node| and not on
   // |node| itself since it's about to be deleted.
@@ -1646,17 +1649,30 @@ void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::
   DCHECK(!new_endpoint->IsIgnored());
   DCHECK(!other_endpoint->IsIgnored());
 
-  // Create a degenerate range at |end_| if we have an inverted range - which
-  // occurs when the |end_| comes before the |start_|.
+  // If after all the above operations we're still left with a new endpoint that
+  // is a descendant of the subtree root being deleted, just point at a null
+  // position and don't crash later on. This can happen when the entire parent
+  // chain of the subtree is ignored.
+  endpoint_anchor = new_endpoint->GetAnchor();
+  if (!endpoint_anchor ||
+      endpoint_anchor->IsDescendantOfCrossingTreeBoundary(node))
+    new_endpoint = AXNodePosition::CreateNullPosition();
+
+  // Create a degenerate range at the new position if we have an inverted range
+  // - which occurs when the |end_| comes before the |start_|. This could have
+  // happened due to the new endpoint walking forwards or backwards when
+  // normalizing above. If we don't set the opposite endpoint to something that
+  // we know will be safe (i.e. not in a deleted subtree) we'll crash later on
+  // when trying to create a valid position.
   if (is_start_endpoint) {
     if (*other_endpoint < *new_endpoint)
-      new_endpoint = other_endpoint->Clone();
+      SetEnd(new_endpoint->Clone());
 
     SetStart(std::move(new_endpoint));
     validation_necessary_for_start_ = {tree->GetAXTreeID(), node->id()};
   } else {
     if (*new_endpoint < *other_endpoint)
-      new_endpoint = other_endpoint->Clone();
+      SetStart(new_endpoint->Clone());
 
     SetEnd(std::move(new_endpoint));
     validation_necessary_for_end_ = {tree->GetAXTreeID(), node->id()};
@@ -1673,13 +1689,22 @@ void AXPlatformNodeTextRangeProviderWin::TextRangeEndpoints::OnNodeDeleted(
   if (validation_necessary_for_start_.has_value() &&
       validation_necessary_for_start_->tree_id == tree->GetAXTreeID() &&
       validation_necessary_for_start_->node_id == node_id) {
-    SetStart(start_->AsValidPosition());
+    if (!start_->IsNullPosition() && start_->GetAnchor()->IsDataValid())
+      SetStart(start_->AsValidPosition());
+    else
+      SetStart(AXNodePosition::CreateNullPosition());
+
     validation_necessary_for_start_ = absl::nullopt;
   }
+
   if (validation_necessary_for_end_.has_value() &&
       validation_necessary_for_end_->tree_id == tree->GetAXTreeID() &&
       validation_necessary_for_end_->node_id == node_id) {
-    SetEnd(end_->AsValidPosition());
+    if (!end_->IsNullPosition() && end_->GetAnchor()->IsDataValid())
+      SetEnd(end_->AsValidPosition());
+    else
+      SetEnd(AXNodePosition::CreateNullPosition());
+
     validation_necessary_for_end_ = absl::nullopt;
   }
 }

ui/accessibility/platform/ax_platform_node_textrangeprovider_win_unittest.cc

@@ -6509,7 +6509,7 @@ TEST_F(AXPlatformNodeTextRangeProviderTest,
   // a change to the range.
   //
   // ++1 kRootWebArea
-  // ++++++2 kStaticText "one"
+  // ++++2 kStaticText "one"
   // ++++3 kGenericContainer
   // ++++++4 kGenericContainer
   // ++++++++5 kStaticText " two"
@@ -6595,6 +6595,258 @@ TEST_F(AXPlatformNodeTextRangeProviderTest,
   EXPECT_EQ(3, GetEnd(range.Get())->text_offset());
 }
 
+TEST_F(AXPlatformNodeTextRangeProviderTest,
+       TestDeleteSubtreeWithIgnoredAncestors) {
+  // This test updates the tree structure to ensure that the text range doesn't
+  // crash and points to null positions after a subtree that includes the text
+  // range is deleted and all ancestors are ignored.
+  //
+  // ++1 kRootWebArea ignored
+  // ++++2 kStaticText "one"
+  // ++++3 kGenericContainer ignored
+  // ++++++4 kGenericContainer
+  // ++++++++5 kGenericContainer
+  // ++++++++++6 kStaticText " two"
+  // ++++++++7 kGenericContainer ignored
+  // ++++++++++8 kStaticText " ignored" ignored
+  // ++++++++9 kGenericContainer
+  // ++++++++++10 kStaticText " three"
+  // ++++11 kGenericContainer
+  // ++++++12 kStaticText "four"
+  AXNodeData root_1;
+  AXNodeData text_2;
+  AXNodeData gc_3;
+  AXNodeData gc_4;
+  AXNodeData gc_5;
+  AXNodeData text_6;
+  AXNodeData gc_7;
+  AXNodeData text_8;
+  AXNodeData gc_9;
+  AXNodeData text_10;
+  AXNodeData gc_11;
+  AXNodeData text_12;
+
+  root_1.id = 1;
+  text_2.id = 2;
+  gc_3.id = 3;
+  gc_4.id = 4;
+  gc_5.id = 5;
+  text_6.id = 6;
+  gc_7.id = 7;
+  text_8.id = 8;
+  gc_9.id = 9;
+  text_10.id = 10;
+  gc_11.id = 11;
+  text_12.id = 12;
+
+  root_1.role = ax::mojom::Role::kRootWebArea;
+  root_1.child_ids = {text_2.id, gc_3.id, gc_11.id};
+  root_1.AddState(ax::mojom::State::kIgnored);
+
+  text_2.role = ax::mojom::Role::kStaticText;
+  text_2.SetName("one");
+
+  gc_3.role = ax::mojom::Role::kGenericContainer;
+  gc_3.AddState(ax::mojom::State::kIgnored);
+  gc_3.child_ids = {gc_4.id};
+
+  gc_4.role = ax::mojom::Role::kGenericContainer;
+  gc_4.child_ids = {gc_5.id, gc_7.id, gc_9.id};
+
+  gc_5.role = ax::mojom::Role::kGenericContainer;
+  gc_5.child_ids = {text_6.id};
+
+  text_6.role = ax::mojom::Role::kStaticText;
+  text_6.SetName(" two");
+
+  gc_7.role = ax::mojom::Role::kGenericContainer;
+  gc_7.AddState(ax::mojom::State::kIgnored);
+  gc_7.child_ids = {text_8.id};
+
+  text_8.role = ax::mojom::Role::kStaticText;
+  text_8.AddState(ax::mojom::State::kIgnored);
+  text_8.SetName(" ignored");
+
+  gc_9.role = ax::mojom::Role::kGenericContainer;
+  gc_9.child_ids = {text_10.id};
+
+  text_10.role = ax::mojom::Role::kStaticText;
+  text_10.SetName(" three");
+
+  gc_11.role = ax::mojom::Role::kGenericContainer;
+  gc_11.child_ids = {text_12.id};
+
+  text_12.role = ax::mojom::Role::kStaticText;
+  text_12.SetName("four");
+
+  ui::AXTreeUpdate update;
+  ui::AXTreeID tree_id = ui::AXTreeID::CreateNewAXTreeID();
+  update.root_id = root_1.id;
+  update.tree_data.tree_id = tree_id;
+  update.has_tree_data = true;
+  update.nodes = {root_1, text_2, gc_3, gc_4,    gc_5,  text_6,
+                  gc_7,   text_8, gc_9, text_10, gc_11, text_12};
+  Init(update);
+
+  // Making |owner| AXID:1 so that |TestAXNodeWrapper::BuildAllWrappers|
+  // will build the entire tree.
+  AXPlatformNodeWin* owner = static_cast<AXPlatformNodeWin*>(
+      AXPlatformNodeFromNode(GetNodeFromTree(tree_id, 1)));
+
+  // Create a range that spans " two three" located on the leaf nodes.
+
+  // start: TextPosition, anchor_id=5, text_offset=0
+  // end  : TextPosition, anchor_id=7, text_offset=6
+  ComPtr<AXPlatformNodeTextRangeProviderWin> range;
+  CreateTextRangeProviderWin(
+      range, owner, tree_id,
+      /*start_anchor_id*/ text_6.id, /*start_offset*/ 2,
+      /*start_affinity*/ ax::mojom::TextAffinity::kDownstream,
+      /*end_anchor_id*/ text_10.id, /*end_offset*/ 6,
+      /*end_affinity*/ ax::mojom::TextAffinity::kDownstream);
+
+  EXPECT_UIA_TEXTRANGE_EQ(range, /*expected_text*/ L"wo three");
+
+  // Delete |gc_3|, which will delete the entire subtree where both of our
+  // endpoints are.
+  AXTreeUpdate test_update;
+  gc_3.child_ids = {};
+  test_update.nodes = {gc_3};
+  ASSERT_TRUE(GetTree()->Unserialize(test_update));
+
+  // There was no unignored position in which to place the start and end - they
+  // should now be null positions.
+  EXPECT_TRUE(GetStart(range.Get())->IsNullPosition());
+  EXPECT_TRUE(GetEnd(range.Get())->IsNullPosition());
+}
+
+TEST_F(AXPlatformNodeTextRangeProviderTest,
+       TestDeleteSubtreeThatIncludesEndpointsNormalizeMoves) {
+  // This test updates the tree structure to ensure that the text range is still
+  // valid after a subtree that includes the text range is deleted, resulting in
+  // a change to the range that is adjusted forwards due to an ignored node.
+  //
+  // ++1 kRootWebArea
+  // ++++2 kStaticText "one"
+  // ++++3 kGenericContainer ignored
+  // ++++++4 kGenericContainer
+  // ++++++++5 kGenericContainer
+  // ++++++++++6 kStaticText " two"
+  // ++++++++7 kGenericContainer
+  // ++++++++++8 kStaticText " three"
+  // ++++++++9 kGenericContainer ignored
+  // ++++++++++10 kStaticText " ignored" ignored
+  // ++++11 kGenericContainer
+  // ++++++12 kStaticText "four"
+  AXNodeData root_1;
+  AXNodeData text_2;
+  AXNodeData gc_3;
+  AXNodeData gc_4;
+  AXNodeData gc_5;
+  AXNodeData text_6;
+  AXNodeData gc_7;
+  AXNodeData text_8;
+  AXNodeData gc_9;
+  AXNodeData text_10;
+  AXNodeData gc_11;
+  AXNodeData text_12;
+
+  root_1.id = 1;
+  text_2.id = 2;
+  gc_3.id = 3;
+  gc_4.id = 4;
+  gc_5.id = 5;
+  text_6.id = 6;
+  gc_7.id = 7;
+  text_8.id = 8;
+  gc_9.id = 9;
+  text_10.id = 10;
+  gc_11.id = 11;
+  text_12.id = 12;
+
+  root_1.role = ax::mojom::Role::kRootWebArea;
+  root_1.child_ids = {text_2.id, gc_3.id, gc_11.id};
+
+  text_2.role = ax::mojom::Role::kStaticText;
+  text_2.SetName("one");
+
+  gc_3.role = ax::mojom::Role::kGenericContainer;
+  gc_3.AddState(ax::mojom::State::kIgnored);
+  gc_3.child_ids = {gc_4.id};
+
+  gc_4.role = ax::mojom::Role::kGenericContainer;
+  gc_4.child_ids = {gc_5.id, gc_7.id, gc_9.id};
+
+  gc_5.role = ax::mojom::Role::kGenericContainer;
+  gc_5.child_ids = {text_6.id};
+
+  text_6.role = ax::mojom::Role::kStaticText;
+  text_6.SetName(" two");
+
+  gc_7.role = ax::mojom::Role::kGenericContainer;
+  gc_7.child_ids = {text_8.id};
+
+  text_8.role = ax::mojom::Role::kStaticText;
+  text_8.SetName(" three");
+
+  gc_9.role = ax::mojom::Role::kGenericContainer;
+  gc_9.AddState(ax::mojom::State::kIgnored);
+  gc_9.child_ids = {text_10.id};
+
+  text_10.role = ax::mojom::Role::kStaticText;
+  text_10.AddState(ax::mojom::State::kIgnored);
+  text_10.SetName(" ignored");
+
+  gc_11.role = ax::mojom::Role::kGenericContainer;
+  gc_11.child_ids = {text_12.id};
+
+  text_12.role = ax::mojom::Role::kStaticText;
+  text_12.SetName("four");
+
+  ui::AXTreeUpdate update;
+  ui::AXTreeID tree_id = ui::AXTreeID::CreateNewAXTreeID();
+  update.root_id = root_1.id;
+  update.tree_data.tree_id = tree_id;
+  update.has_tree_data = true;
+  update.nodes = {root_1, text_2, gc_3, gc_4,    gc_5,  text_6,
+                  gc_7,   text_8, gc_9, text_10, gc_11, text_12};
+  Init(update);
+
+  // Making |owner| AXID:1 so that |TestAXNodeWrapper::BuildAllWrappers|
+  // will build the entire tree.
+  AXPlatformNodeWin* owner = static_cast<AXPlatformNodeWin*>(
+      AXPlatformNodeFromNode(GetNodeFromTree(tree_id, 1)));
+
+  // Create a range that spans " two three" located on the leaf nodes.
+
+  // start: TextPosition, anchor_id=5, text_offset=0
+  // end  : TextPosition, anchor_id=7, text_offset=6
+  ComPtr<AXPlatformNodeTextRangeProviderWin> range;
+  CreateTextRangeProviderWin(
+      range, owner, tree_id,
+      /*start_anchor_id*/ text_6.id, /*start_offset*/ 2,
+      /*start_affinity*/ ax::mojom::TextAffinity::kDownstream,
+      /*end_anchor_id*/ text_8.id, /*end_offset*/ 6,
+      /*end_affinity*/ ax::mojom::TextAffinity::kDownstream);
+
+  EXPECT_UIA_TEXTRANGE_EQ(range, /*expected_text*/ L"wo three");
+
+  // Delete |gc_3|, which will delete the entire subtree where both of our
+  // endpoints are.
+  AXTreeUpdate test_update;
+  gc_3.child_ids = {};
+  test_update.nodes = {gc_3};
+  ASSERT_TRUE(GetTree()->Unserialize(test_update));
+
+  // The text range should now be a degenerate range positioned at the end of
+  // root, the parent of |gc_3|, since |gc_3| has been deleted.
+  EXPECT_EQ(text_12.id, GetStart(range.Get())->anchor_id());
+  EXPECT_EQ(0, GetStart(range.Get())->text_offset());
+
+  EXPECT_EQ(text_12.id, GetEnd(range.Get())->anchor_id());
+  EXPECT_EQ(0, GetEnd(range.Get())->text_offset());
+}
+
 TEST_F(AXPlatformNodeTextRangeProviderTest,
        TestReplaceStartAndEndEndpointRepeatRemoval) {
   // This test updates the tree structure to ensure that the text range is still