Notify observers of node delete at end of AXTree::Unserialize

Previously, we notify an AXTreeObserver of a node removed from AXTree
before the node is actually removed from the tree. In this change, we
notify an AXTreeObserver of a node removed from AXTree after the tree
is stable and its nodes have been updated.

Previously, when in AXTree::Unserialize and before tree modification
takes place, we call NotifyNodeWillBeReparentedOrDeleted on the node
to be deleted. AXTreeObserver/
BrowserAccessibilityManager::OnNodeWillBeDeleted removes its
reference to the AxNode's BrowserAccessibility object and destroy it
as well. This takes place before the actual ax node is deleted from
the tree. The subsequent action, also before tree modification, such
as a call to BrowserAccessibility::InternalGetFirstChild() will reveal
the inconsistent state--retrieving the child node from ax tree,
however when we query through
BrowserAccessibilityManager::id_wrapper_map_ for its
BrowserAccessibility, it is not found because it is deleted in the
prior action. This inconsistent state can cause a null dereferencing
in BrowserAccessibility::PlatformIsLeafIncludingIgnored for
role::kButton.

In the following scenario, a crash would occur.
AXTree:
kRootWebArea chromium#1
++kButton chromium#5
++++kCheckBox chromium#2
++++kGenericContainer chromium#3 IGNORED
++++++kStaticText chromium#4 IGNORED

AXTreeUpdate:
1. remove node kCheckBox chromium#2
2. remove subtree kGenericContainer chromium#3 IGNORED
---
AXTree::Unserailize: {
  NotifyNodeWillBeReparentedOrDeleted(kCheckBox#2)
    OnNodeWillBeDeleted(kCheckBox#2)
      id_wrapper_map_.erase(kCheckBox#2)
      delete BrowserAccessibility of kCheckBox#2
  NotifySubTreeWillBeReparentedOrDeleted(#kGenericContainer#3)
    FireWinAccessibilityEvent(kGenericContainer#3)
      ...
      PlatformIsLeafIncludingIgnored(kButton#5)
        InternalGetFirstChild(kButton#5)
          GetUnignoredChildAtIndex(0) returns kCheckBox ax node
          id_wrapper_map.get(kCheckBox#2) -> null
        Dereferencing null, exception occurs.

  //--- then tree modification/ax node deletion happens --
}

Bug: 1020187
Change-Id: I8e83b3450ca6253e837f4854a1c90a4779b3d435
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1907774
Commit-Queue: Victor Fei <vicfei@microsoft.com>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Adam Ettenberger <adettenb@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#715341}

content/browser/accessibility/browser_accessibility_manager.cc

@@ -1222,8 +1222,6 @@ void BrowserAccessibilityManager::OnNodeWillBeDeleted(ui::AXTree* tree,
   if (BrowserAccessibility* wrapper = GetFromAXNode(node)) {
     if (wrapper == GetLastFocusedNode())
       SetLastFocusedNode(nullptr);
-    id_wrapper_map_.erase(node->id());
-    wrapper->Destroy();
   }
 }
 
@@ -1238,6 +1236,15 @@ void BrowserAccessibilityManager::OnNodeCreated(ui::AXTree* tree,
   wrapper->Init(this, node);
 }
 
+void BrowserAccessibilityManager::OnNodeDeleted(ui::AXTree* tree,
+                                                int32_t node_id) {
+  DCHECK_NE(node_id, ui::AXNode::kInvalidAXID);
+  if (BrowserAccessibility* wrapper = GetFromID(node_id)) {
+    id_wrapper_map_.erase(node_id);
+    wrapper->Destroy();
+  }
+}
+
 void BrowserAccessibilityManager::OnNodeReparented(ui::AXTree* tree,
                                                    ui::AXNode* node) {
   DCHECK(node);

content/browser/accessibility/browser_accessibility_manager.h

@@ -403,6 +403,7 @@ class CONTENT_EXPORT BrowserAccessibilityManager : public ui::AXTreeObserver,
   void OnNodeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
   void OnSubtreeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
   void OnNodeCreated(ui::AXTree* tree, ui::AXNode* node) override;
+  void OnNodeDeleted(ui::AXTree* tree, int32_t node_id) override;
   void OnNodeReparented(ui::AXTree* tree, ui::AXNode* node) override;
   void OnAtomicUpdateFinished(
       ui::AXTree* tree,

content/browser/accessibility/dump_accessibility_events_browsertest.cc

@@ -794,6 +794,11 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
   RunEventTest(FILE_PATH_LITERAL("button-click.html"));
 }
 
+IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
+                       AccessibilityEventsButtonRemoveChildren) {
+  RunEventTest(FILE_PATH_LITERAL("button-remove-children.html"));
+}
+
 IN_PROC_BROWSER_TEST_P(DumpAccessibilityEventsTest,
                        RangeValueIsReadonlyChanged) {
   RunEventTest(FILE_PATH_LITERAL("range-value-is-readonly-changed.html"));

content/test/data/accessibility/event/button-remove-children-expected-uia-win.txt

@@ -0,0 +1,2 @@
+StructureChanged/ChildRemoved on role=button
+StructureChanged/ChildrenReordered on role=button

content/test/data/accessibility/event/button-remove-children-expected-win.txt

@@ -0,0 +1,2 @@
+EVENT_OBJECT_HIDE on <input#checkbox> role=ROLE_SYSTEM_CHECKBUTTON FOCUSABLE IA2_STATE_CHECKABLE
+EVENT_OBJECT_REORDER on <button> role=ROLE_SYSTEM_PUSHBUTTON FOCUSABLE

content/test/data/accessibility/event/button-remove-children.html

@@ -0,0 +1,23 @@
+<!--
+@WIN-DENY:EVENT_OBJECT_LOCATIONCHANGE*
+@WIN-DENY:IA2_EVENT_TEXT_REMOVED*
+
+This test is for crbug.com/1020187.
+By removing 'checkbox' first then 'ignored_content', we should not cause AXTree
+to crash during AXTree unserialization.
+-->
+<!DOCTYPE html>
+<html>
+<body>
+<button>
+  <input id="checkbox" type="checkbox">
+  <div id="ignored_content" aria-hidden="true">text content ignored</div>
+</button>
+<script>
+  function go() {
+    document.getElementById('checkbox').remove();
+    document.getElementById('ignored_content').remove();
+  }
+</script>
+</body>
+</html>

ui/accessibility/ax_tree.cc

@@ -249,9 +249,14 @@ struct AXTreeUpdateState {
     return base::Contains(removed_node_ids, node->id());
   }
 
+  // Returns whether this update creates a node marked by |node_id|.
+  bool IsCreatedNode(AXNode::AXID node_id) const {
+    return base::Contains(new_node_ids, node_id);
+  }
+
   // Returns whether this update creates |node|.
   bool IsCreatedNode(const AXNode* node) const {
-    return base::Contains(new_node_ids, node->id());
+    return IsCreatedNode(node->id());
   }
 
   // If this node is removed, it should be considered reparented.
@@ -556,7 +561,7 @@ AXTree::AXTree(const AXTreeUpdate& initial_state) {
 
 AXTree::~AXTree() {
   if (root_) {
-    RecursivelyNotifyNodeWillBeDeleted(root_);
+    RecursivelyNotifyNodeDeletedForTreeTeardown(root_);
     base::AutoReset<bool> update_state_resetter(&tree_update_in_progress_,
                                                 true);
     DestroyNodeAndSubtree(root_, nullptr);
@@ -1017,11 +1022,17 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
       observer.OnTreeDataChanged(this, *update_state.old_tree_data, data_);
   }
 
+  // Now that the unignored cached values are up to date, update observers to
+  // the nodes that were deleted from the tree but not reparented.
+  for (AXNode::AXID node_id : update_state.removed_node_ids) {
+    if (!update_state.IsCreatedNode(node_id))
+      NotifyNodeHasBeenDeleted(node_id);
+  }
+
   // Now that the unignored cached values are up to date, update observers to
   // new nodes in the tree.
-  for (AXNode::AXID node_id : update_state.new_node_ids) {
+  for (AXNode::AXID node_id : update_state.new_node_ids)
     NotifyNodeHasBeenReparentedOrCreated(GetFromId(node_id), &update_state);
-  }
 
   // Now that the unignored cached values are up to date, update observers to
   // node changes.
@@ -1046,9 +1057,8 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
       observer.OnNodeChanged(this, node);
   }
 
-  for (AXTreeObserver& observer : observers_) {
+  for (AXTreeObserver& observer : observers_)
     observer.OnAtomicUpdateFinished(this, root_->id() != old_root_id, changes);
-  }
 
   return true;
 }
@@ -1405,15 +1415,25 @@ void AXTree::NotifyNodeWillBeReparentedOrDeleted(
   }
 }
 
-void AXTree::RecursivelyNotifyNodeWillBeDeleted(AXNode* node) {
+void AXTree::RecursivelyNotifyNodeDeletedForTreeTeardown(AXNode* node) {
   DCHECK(!GetTreeUpdateInProgressState());
   if (node->id() == AXNode::kInvalidAXID)
     return;
 
   for (AXTreeObserver& observer : observers_)
-    observer.OnNodeWillBeDeleted(this, node);
+    observer.OnNodeDeleted(this, node->id());
   for (auto* child : node->children())
-    RecursivelyNotifyNodeWillBeDeleted(child);
+    RecursivelyNotifyNodeDeletedForTreeTeardown(child);
+}
+
+void AXTree::NotifyNodeHasBeenDeleted(AXNode::AXID node_id) {
+  DCHECK(!GetTreeUpdateInProgressState());
+
+  if (node_id == AXNode::kInvalidAXID)
+    return;
+
+  for (AXTreeObserver& observer : observers_)
+    observer.OnNodeDeleted(this, node_id);
 }
 
 void AXTree::NotifyNodeHasBeenReparentedOrCreated(

ui/accessibility/ax_tree.h

@@ -217,8 +217,14 @@ class AX_EXPORT AXTree : public AXNode::OwnerTree {
       const AXTreeUpdateState* update_state);
 
   // Notify the delegate that |node| and all of its descendants will be
+  // destroyed. This function is called during AXTree teardown.
+  void RecursivelyNotifyNodeDeletedForTreeTeardown(AXNode* node);
+
+  // Notify the delegate that the node marked by |node_id| has been deleted.
+  // We are passing the node id instead of ax node is because by the time this
+  // function is called, the ax node in the tree will already have been
   // destroyed.
-  void RecursivelyNotifyNodeWillBeDeleted(AXNode* node);
+  void NotifyNodeHasBeenDeleted(AXNode::AXID node_id);
 
   // Notify the delegate that |node| has been created or reparented.
   void NotifyNodeHasBeenReparentedOrCreated(

ui/accessibility/ax_tree_observer.h

@@ -113,6 +113,10 @@ class AX_EXPORT AXTreeObserver : public base::CheckedObserver {
   // be valid, since the tree is in a stable state after updating.
   virtual void OnNodeCreated(AXTree* tree, AXNode* node) {}
 
+  // Called after all tree mutations have occurred or during tree teardown,
+  // notifying that a single node has been deleted from the tree.
+  virtual void OnNodeDeleted(AXTree* tree, int32_t node_id) {}
+
   // Same as |OnNodeCreated|, but called for nodes that have been reparented.
   virtual void OnNodeReparented(AXTree* tree, AXNode* node) {}
 

ui/accessibility/ax_tree_unittest.cc

@@ -95,7 +95,10 @@ class TestAXTreeObserver : public AXTreeObserver {
 
   base::Optional<AXNode::AXID> unignored_parent_id_before_node_deleted;
   void OnNodeWillBeDeleted(AXTree* tree, AXNode* node) override {
-    deleted_ids_.push_back(node->id());
+    // When this observer function is called in an update, the actual node
+    // deletion has not happened yet. Verify that node still exists in the tree.
+    ASSERT_NE(nullptr, tree->GetFromId(node->id()));
+
     if (unignored_parent_id_before_node_deleted) {
       ASSERT_NE(nullptr, node->GetUnignoredParent());
       ASSERT_EQ(*unignored_parent_id_before_node_deleted,
@@ -119,6 +122,13 @@ class TestAXTreeObserver : public AXTreeObserver {
     created_ids_.push_back(node->id());
   }
 
+  void OnNodeDeleted(AXTree* tree, int32_t node_id) override {
+    // When this observer function is called in an update, node has already been
+    // deleted from the tree. Verify that the node is absent from the tree.
+    ASSERT_EQ(nullptr, tree->GetFromId(node_id));
+    deleted_ids_.push_back(node_id);
+  }
+
   void OnNodeReparented(AXTree* tree, AXNode* node) override {
     node_reparented_ids_.push_back(node->id());
   }
@@ -3567,6 +3577,53 @@ TEST(AXTreeTest, OnNodeWillBeDeletedHasValidUnignoredParent) {
   ASSERT_TRUE(tree.Unserialize(tree_update));
 }
 
+TEST(AXTreeTest, OnNodeHasBeenDeleted) {
+  AXTreeUpdate initial_state;
+
+  initial_state.root_id = 1;
+  initial_state.nodes.resize(6);
+  initial_state.nodes[0].id = 1;
+  initial_state.nodes[0].role = ax::mojom::Role::kRootWebArea;
+  initial_state.nodes[0].child_ids = {2};
+  initial_state.nodes[1].id = 2;
+  initial_state.nodes[1].role = ax::mojom::Role::kButton;
+  initial_state.nodes[1].child_ids = {3, 4};
+  initial_state.nodes[2].id = 3;
+  initial_state.nodes[2].role = ax::mojom::Role::kCheckBox;
+  initial_state.nodes[3].id = 4;
+  initial_state.nodes[3].role = ax::mojom::Role::kStaticText;
+  initial_state.nodes[3].child_ids = {5, 6};
+  initial_state.nodes[4].id = 5;
+  initial_state.nodes[4].role = ax::mojom::Role::kInlineTextBox;
+  initial_state.nodes[5].id = 6;
+  initial_state.nodes[5].role = ax::mojom::Role::kInlineTextBox;
+
+  AXTree tree(initial_state);
+
+  AXTreeUpdate update;
+  update.nodes.resize(2);
+  update.nodes[0] = initial_state.nodes[1];
+  update.nodes[0].child_ids = {4};
+  update.nodes[1] = initial_state.nodes[3];
+  update.nodes[1].child_ids = {};
+
+  TestAXTreeObserver test_observer(&tree);
+  ASSERT_TRUE(tree.Unserialize(update));
+
+  EXPECT_EQ(3U, test_observer.deleted_ids().size());
+  EXPECT_EQ(3, test_observer.deleted_ids()[0]);
+  EXPECT_EQ(5, test_observer.deleted_ids()[1]);
+  EXPECT_EQ(6, test_observer.deleted_ids()[2]);
+
+  // Verify that the nodes we intend to delete in the update are actually
+  // absent from the tree.
+  for (auto id : test_observer.deleted_ids()) {
+    SCOPED_TRACE(testing::Message()
+                 << "Node with id=" << id << ", should not exist in the tree");
+    EXPECT_EQ(nullptr, tree.GetFromId(id));
+  }
+}
+
 // Tests a fringe scenario that may happen if multiple AXTreeUpdates are merged.
 // Make sure that we correctly Unserialize if a newly created node is deleted,
 // and possibly recreated later.

ui/accessibility/platform/ax_platform_node_auralinux_unittest.cc

@@ -2296,6 +2296,7 @@ TEST_F(AXPlatformNodeAuraLinuxTest, TestAtkObjectExpandRebuildsPlatformNode) {
   g_object_ref(original_atk_object);
 
   root_data = AXNodeData();
+  root_data.id = 1;
   root_data.role = ax::mojom::Role::kListBox;
   GetRootNode()->SetData(root_data);
 

ui/accessibility/platform/test_ax_node_wrapper.cc

@@ -21,7 +21,7 @@ namespace ui {
 namespace {
 
 // A global map from AXNodes to TestAXNodeWrappers.
-std::unordered_map<AXNode*, TestAXNodeWrapper*> g_node_to_wrapper_map;
+std::unordered_map<AXNode::AXID, TestAXNodeWrapper*> g_node_id_to_wrapper_map;
 
 // A global coordinate offset.
 gfx::Vector2d g_offset;
@@ -47,12 +47,12 @@ AXNode* g_node_from_last_default_action;
 // deleted so we can delete their wrappers.
 class TestAXTreeObserver : public AXTreeObserver {
  private:
-  void OnNodeWillBeDeleted(AXTree* tree, AXNode* node) override {
-    auto iter = g_node_to_wrapper_map.find(node);
-    if (iter != g_node_to_wrapper_map.end()) {
+  void OnNodeDeleted(AXTree* tree, int32_t node_id) override {
+    const auto iter = g_node_id_to_wrapper_map.find(node_id);
+    if (iter != g_node_id_to_wrapper_map.end()) {
       TestAXNodeWrapper* wrapper = iter->second;
       delete wrapper;
-      g_node_to_wrapper_map.erase(iter->first);
+      g_node_id_to_wrapper_map.erase(node_id);
     }
   }
 };
@@ -68,11 +68,11 @@ TestAXNodeWrapper* TestAXNodeWrapper::GetOrCreate(AXTree* tree, AXNode* node) {
 
   if (!tree->HasObserver(&g_ax_tree_observer))
     tree->AddObserver(&g_ax_tree_observer);
-  auto iter = g_node_to_wrapper_map.find(node);
-  if (iter != g_node_to_wrapper_map.end())
+  auto iter = g_node_id_to_wrapper_map.find(node->id());
+  if (iter != g_node_id_to_wrapper_map.end())
     return iter->second;
   TestAXNodeWrapper* wrapper = new TestAXNodeWrapper(tree, node);
-  g_node_to_wrapper_map[node] = wrapper;
+  g_node_id_to_wrapper_map[node->id()] = wrapper;
   return wrapper;
 }
 
@@ -290,14 +290,10 @@ AXPlatformNode* TestAXNodeWrapper::GetFromNodeID(int32_t id) {
   // Force creating all of the wrappers for this tree.
   BuildAllWrappers(tree_, node_);
 
-  for (auto it = g_node_to_wrapper_map.begin();
-       it != g_node_to_wrapper_map.end(); ++it) {
-    AXNode* node = it->first;
-    if (node->id() == id) {
-      TestAXNodeWrapper* wrapper = it->second;
-      return wrapper->ax_platform_node();
-    }
-  }
+  const auto iter = g_node_id_to_wrapper_map.find(id);
+  if (iter != g_node_id_to_wrapper_map.end())
+    return iter->second->ax_platform_node();
+
   return nullptr;
 }