Adjust AXTree::Unserialize to notify events outside of UpdateNode [2/3]

This change separates updating AXNodeData in AXTree::UpdateNode from
notifying that the node data will change, and node data has changed.

---

Problem :
  AXTree notifies AXTreeObservers of node and subtree changes in the middle
  of a tree update, which means observers may be seeing a tree state that's
  incomplete. This can lead to subtle bugs when observers access nodes in the
  AXTree other than the node being updated.
  e.g. |GetUnignoredParent| may be invalid during a notification, but
  correct afterwards.

Proposed solution :
  Separate notifications from updates in AXTree::Unserialize.

  This requires the following steps :
    1. Record all operations that would be performed on the tree.
    2. Fire all on destroy and OnNodeDataWillChange callbacks.
    3. Apply all update operations.
    4. Fire all created and node data changed callbacks.


  For additional context, see these comments by Nektarios and Dominic :
  https://chromium-review.googlesource.com/c/chromium/src/+/1650222/20#message-ffe9bf96b616a915bebf2e69d7803bcae6a18b24
  https://chromium-review.googlesource.com/c/chromium/src/+/1535171/69#message-7b62970b193439a1878d7339cdc94d2556d0a416

  I  plan to submit approximately 3 CLs to resolve this :
    1. Handle Pre/Post Tree changes (create node / destroy subtree)
    2. Handle Pre/Post Node data changes (attributes will / have changes)
    3. Add DCHECKs to AXNode::*Unignored* accessors, along with any remaining
       work required to do so, to help harden these paths.

Bug: 974444
Change-Id: Iade343a572003e31d79039b08cced689fb6e9cbe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1710128
Commit-Queue: Adam Ettenberger <adettenb@microsoft.com>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#684391}

content/browser/accessibility/browser_accessibility_manager_auralinux.cc

@@ -234,20 +234,20 @@ static void EstablishEmbeddedRelationship(AtkObject* document_object) {
   document_platform_node->SetEmbeddingWindow(window);
 }
 
-void BrowserAccessibilityManagerAuraLinux::OnStateChanged(
+void BrowserAccessibilityManagerAuraLinux::OnNodeDataWillChange(
     ui::AXTree* tree,
-    ui::AXNode* node,
-    ax::mojom::State state,
-    bool new_value) {
+    const ui::AXNodeData& old_node_data,
+    const ui::AXNodeData& new_node_data) {
   DCHECK_EQ(ax_tree(), tree);
 
   // Since AuraLinux needs to send the children-changed::remove event with the
   // index in parent, the event must be fired before the node becomes ignored.
   // children-changed:add is handled with the generated Event::IGNORED_CHANGED.
-  if (state == ax::mojom::State::kIgnored && new_value) {
-    DCHECK(!node->data().HasState(ax::mojom::State::kIgnored));
-    BrowserAccessibility* obj = GetFromAXNode(node);
+  if (!old_node_data.HasState(ax::mojom::State::kIgnored) &&
+      new_node_data.HasState(ax::mojom::State::kIgnored)) {
+    BrowserAccessibility* obj = GetFromID(old_node_data.id);
     if (obj && obj->IsNative() && obj->GetParent()) {
+      DCHECK(!obj->HasState(ax::mojom::State::kIgnored));
       g_signal_emit_by_name(obj->GetParent(), "children-changed::remove",
                             obj->GetIndexInParent(),
                             obj->GetNativeViewAccessible());

content/browser/accessibility/browser_accessibility_manager_auralinux.h

@@ -42,10 +42,9 @@ class CONTENT_EXPORT BrowserAccessibilityManagerAuraLinux
 
  protected:
   // AXTreeObserver methods.
-  void OnStateChanged(ui::AXTree* tree,
-                      ui::AXNode* node,
-                      ax::mojom::State state,
-                      bool new_value) override;
+  void OnNodeDataWillChange(ui::AXTree* tree,
+                            const ui::AXNodeData& old_node_data,
+                            const ui::AXNodeData& new_node_data) override;
   void OnSubtreeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
   void OnAtomicUpdateFinished(
       ui::AXTree* tree,

extensions/renderer/api/automation/automation_ax_tree_wrapper.cc

@@ -421,7 +421,7 @@ AutomationAXTreeWrapper::GetChildTreeIDReverseMap() {
   return *child_tree_id_reverse_map;
 }
 
-void AutomationAXTreeWrapper::OnNodeDataWillChange(
+void AutomationAXTreeWrapper::OnNodeDataChanged(
     ui::AXTree* tree,
     const ui::AXNodeData& old_node_data,
     const ui::AXNodeData& new_node_data) {

extensions/renderer/api/automation/automation_ax_tree_wrapper.h

@@ -52,9 +52,9 @@ class AutomationAXTreeWrapper : public ui::AXTreeObserver {
 
  private:
   // AXTreeObserver overrides.
-  void OnNodeDataWillChange(ui::AXTree* tree,
-                            const ui::AXNodeData& old_node_data,
-                            const ui::AXNodeData& new_node_data) override;
+  void OnNodeDataChanged(ui::AXTree* tree,
+                         const ui::AXNodeData& old_node_data,
+                         const ui::AXNodeData& new_node_data) override;
   void OnNodeWillBeDeleted(ui::AXTree* tree, ui::AXNode* node) override;
   void OnAtomicUpdateFinished(ui::AXTree* tree,
                               bool root_changed,

ui/accessibility/ax_event_generator.cc

@@ -143,9 +143,9 @@ void AXEventGenerator::AddEvent(AXNode* node, AXEventGenerator::Event event) {
   node_events.emplace(event, ax::mojom::EventFrom::kNone);
 }
 
-void AXEventGenerator::OnNodeDataWillChange(AXTree* tree,
-                                            const AXNodeData& old_node_data,
-                                            const AXNodeData& new_node_data) {
+void AXEventGenerator::OnNodeDataChanged(AXTree* tree,
+                                         const AXNodeData& old_node_data,
+                                         const AXNodeData& new_node_data) {
   DCHECK_EQ(tree_, tree);
   // Fire CHILDREN_CHANGED events when the list of children updates.
   // Internally we store inline text box nodes as children of a static text
@@ -179,8 +179,6 @@ void AXEventGenerator::OnStateChanged(AXTree* tree,
     case ax::mojom::State::kExpanded:
       AddEvent(node, new_value ? Event::EXPANDED : Event::COLLAPSED);
 
-      // TODO(dtseng): tree in the midst of updates. Disallow access to
-      // |node|.
       if (node->data().role == ax::mojom::Role::kRow ||
           node->data().role == ax::mojom::Role::kTreeItem) {
         AXNode* container = node;
@@ -248,8 +246,6 @@ void AXEventGenerator::OnStringAttributeChanged(AXTree* tree,
 
       // Fire a LIVE_REGION_CREATED if the previous value was off, and the new
       // value is not-off.
-      // TODO(dtseng): tree in the midst of updates. Disallow access to
-      // |node|.
       if (!IsAlert(node->data().role)) {
         bool old_state = !old_value.empty() && old_value != "off";
         bool new_state = !new_value.empty() && new_value != "off";
@@ -263,8 +259,6 @@ void AXEventGenerator::OnStringAttributeChanged(AXTree* tree,
       if (node != tree->root())
         AddEvent(node, Event::NAME_CHANGED);
 
-      // TODO(dtseng): tree in the midst of updates. Disallow
-      // access to |node|.
       if (node->data().HasStringAttribute(
               ax::mojom::StringAttribute::kContainerLiveStatus)) {
         FireLiveRegionEvents(node);

ui/accessibility/ax_event_generator.h

@@ -165,9 +165,9 @@ class AX_EXPORT AXEventGenerator : public AXTreeObserver {
 
  protected:
   // AXTreeObserver overrides.
-  void OnNodeDataWillChange(AXTree* tree,
-                            const AXNodeData& old_node_data,
-                            const AXNodeData& new_node_data) override;
+  void OnNodeDataChanged(AXTree* tree,
+                         const AXNodeData& old_node_data,
+                         const AXNodeData& new_node_data) override;
   void OnRoleChanged(AXTree* tree,
                      AXNode* node,
                      ax::mojom::Role old_role,

ui/accessibility/ax_node.cc

@@ -649,6 +649,10 @@ base::Optional<int> AXNode::GetPosInSet() {
     return base::nullopt;
   }
 
+  if (data().HasState(ax::mojom::State::kIgnored)) {
+    return base::nullopt;
+  }
+
   const AXNode* ordered_set = GetOrderedSet();
   if (!ordered_set) {
     return base::nullopt;
@@ -669,6 +673,10 @@ base::Optional<int> AXNode::GetSetSize() {
   if (!(IsOrderedSetItem() || IsOrderedSet()))
     return base::nullopt;
 
+  if (data().HasState(ax::mojom::State::kIgnored)) {
+    return base::nullopt;
+  }
+
   // If node is item-like, find its outerlying ordered set. Otherwise,
   // this node is the ordered set.
   const AXNode* ordered_set = this;

ui/accessibility/ax_tree.h

@@ -214,9 +214,14 @@ class AX_EXPORT AXTree : public AXNode::OwnerTree {
       AXNode* node,
       const AXTreeUpdateState* update_state);
 
-  void CallNodeChangeCallbacks(AXNode* node,
-                               const AXNodeData& old_data,
-                               const AXNodeData& new_data);
+  // Notify the delegate that a node will change its data.
+  void NotifyNodeDataWillChange(const AXNodeData& old_data,
+                                const AXNodeData& new_data);
+
+  // Notify the delegate that |node| has changed its data.
+  void NotifyNodeDataHasBeenChanged(AXNode* node,
+                                    const AXNodeData& old_data,
+                                    const AXNodeData& new_data);
 
   void UpdateReverseRelations(AXNode* node, const AXNodeData& new_data);
 

ui/accessibility/ax_tree_observer.h

@@ -18,33 +18,34 @@ struct AXTreeData;
 
 // Used when you want to be notified when changes happen to an AXTree.
 //
-// Some of the notifications are called in the middle of an update operation.
-// Be careful, as the tree may be in an inconsistent state at this time;
-// don't walk the parents and children at this time:
-//   OnNodeWillBeDeleted
-//   OnSubtreeWillBeDeleted
-//   OnNodeWillBeReparented
-//   OnSubtreeWillBeReparented
-//   OnNodeCreated
-//   OnNodeReparented
-//   OnNodeChanged
-//
-// In addition, one additional notification is fired at the end of an
-// atomic update, and it provides a vector of nodes that were added or
-// changed, for final postprocessing:
-//   OnAtomicUpdateFinished
-//
+// |OnAtomicUpdateFinished| is notified at the end of an atomic update.
+// It provides a vector of nodes that were added or changed, for final
+// postprocessing.
 class AX_EXPORT AXTreeObserver : public base::CheckedObserver {
  public:
   AXTreeObserver();
   ~AXTreeObserver() override;
 
-  // Called before a node's data gets updated.
+  // Called before any tree modifications have occurred, notifying that a single
+  // node will change its data. Its id and data will be valid, but its links to
+  // parents and children are only valid within this callstack. Do not hold a
+  // reference to the node or any relative nodes such as ancestors or
+  // descendants described by the node data outside of this event.
   virtual void OnNodeDataWillChange(AXTree* tree,
                                     const AXNodeData& old_node_data,
                                     const AXNodeData& new_node_data) {}
 
+  // Called after all tree modifications have occurred, notifying that a single
+  // node has changed its data. Its id, data, and links to parent and children
+  // will all be valid, since the tree is in a stable state after updating.
+  virtual void OnNodeDataChanged(AXTree* tree,
+                                 const AXNodeData& old_node_data,
+                                 const AXNodeData& new_node_data) {}
+
   // Individual callbacks for every attribute of AXNodeData that can change.
+  // Called after all tree mutations have occurred, notifying that a single node
+  // changed its data. Its id, data, and links to parent and children will all
+  // be valid, since the tree is in a stable state after updating.
   virtual void OnRoleChanged(AXTree* tree,
                              AXNode* node,
                              ax::mojom::Role old_role,
@@ -115,8 +116,10 @@ class AX_EXPORT AXTreeObserver : public base::CheckedObserver {
   // Same as |OnNodeCreated|, but called for nodes that have been reparented.
   virtual void OnNodeReparented(AXTree* tree, AXNode* node) {}
 
-  // Called when a node changes its data or children. The tree may be in
-  // the middle of an update, don't walk the parents and children now.
+  // Called after all tree mutations have occurred, notifying that a single node
+  // has updated its data or children. Its id, data, and links to parent and
+  // children will all be valid, since the tree is in a stable state after
+  // updating.
   virtual void OnNodeChanged(AXTree* tree, AXNode* node) {}
 
   enum ChangeType {

ui/accessibility/ax_tree_unittest.cc

@@ -66,6 +66,9 @@ class TestAXTreeObserver : public AXTreeObserver {
   void OnNodeDataWillChange(AXTree* tree,
                             const AXNodeData& old_node_data,
                             const AXNodeData& new_node_data) override {}
+  void OnNodeDataChanged(AXTree* tree,
+                         const AXNodeData& old_node_data,
+                         const AXNodeData& new_node_data) override {}
   void OnTreeDataChanged(AXTree* tree,
                          const ui::AXTreeData& old_data,
                          const ui::AXTreeData& new_data) override {
@@ -104,8 +107,6 @@ class TestAXTreeObserver : public AXTreeObserver {
 
   void OnNodeChanged(AXTree* tree, AXNode* node) override {
     changed_ids_.push_back(node->id());
-    if (call_posinset_and_setsize)
-      AssertPosinsetAndSetsizeNotSet(node);
   }
 
   void OnAtomicUpdateFinished(AXTree* tree,
@@ -236,12 +237,6 @@ class TestAXTreeObserver : public AXTreeObserver {
     return attribute_change_log_;
   }
 
-  bool call_posinset_and_setsize = false;
-  void AssertPosinsetAndSetsizeNotSet(AXNode* node) {
-    ASSERT_FALSE(node->GetPosInSet());
-    ASSERT_FALSE(node->GetSetSize());
-  }
-
  private:
   AXTree* tree_;
   bool tree_data_changed_;
@@ -2992,20 +2987,78 @@ TEST(AXTreeTest, TestSetSizePosInSetSubtreeDeleted) {
   initial_state.nodes[2].role = ax::mojom::Role::kTreeItem;
   AXTree tree(initial_state);
 
-  // This should work normally.
+  AXNode* tree_node = tree.GetFromId(1);
   AXNode* item = tree.GetFromId(3);
+
+  // This should work normally.
   EXPECT_OPTIONAL_EQ(2, item->GetPosInSet());
   EXPECT_OPTIONAL_EQ(2, item->GetSetSize());
 
-  // Use test observer to assert posinset and setsize are 0.
-  TestAXTreeObserver test_observer(&tree);
-  test_observer.call_posinset_and_setsize = true;
   // Remove item from tree.
   AXTreeUpdate tree_update = initial_state;
   tree_update.nodes.resize(1);
   tree_update.nodes[0].child_ids = {2};
 
   ASSERT_TRUE(tree.Unserialize(tree_update));
+
+  // These values are lazily created, so to test that they fail when
+  // called in the middle of a tree update, fake the update state.
+  tree.SetTreeUpdateInProgressState(true);
+  ASSERT_FALSE(tree_node->GetPosInSet());
+  ASSERT_FALSE(tree_node->GetSetSize());
+
+  // Then reset the state to make sure we have the expected values
+  // after |Unserialize|.
+  tree.SetTreeUpdateInProgressState(false);
+  ASSERT_FALSE(tree_node->GetPosInSet());
+  EXPECT_OPTIONAL_EQ(1, tree_node->GetSetSize());
+}
+
+// Tests that GetPosInSet and GetSetSize work when there are ignored nodes.
+TEST(AXTreeTest, TestSetSizePosInSetIgnoredItem) {
+  AXTreeUpdate initial_state;
+  initial_state.root_id = 1;
+  initial_state.nodes.resize(3);
+  initial_state.nodes[0].id = 1;
+  initial_state.nodes[0].role = ax::mojom::Role::kTree;
+  initial_state.nodes[0].child_ids = {2, 3};
+  initial_state.nodes[1].id = 2;
+  initial_state.nodes[1].role = ax::mojom::Role::kTreeItem;
+  initial_state.nodes[2].id = 3;
+  initial_state.nodes[2].role = ax::mojom::Role::kTreeItem;
+  AXTree tree(initial_state);
+
+  AXNode* tree_node = tree.GetFromId(1);
+  AXNode* item1 = tree.GetFromId(2);
+  AXNode* item2 = tree.GetFromId(3);
+
+  // This should work normally.
+  ASSERT_FALSE(tree_node->GetPosInSet());
+  EXPECT_OPTIONAL_EQ(2, tree_node->GetSetSize());
+
+  EXPECT_OPTIONAL_EQ(1, item1->GetPosInSet());
+  EXPECT_OPTIONAL_EQ(2, item1->GetSetSize());
+
+  EXPECT_OPTIONAL_EQ(2, item2->GetPosInSet());
+  EXPECT_OPTIONAL_EQ(2, item2->GetSetSize());
+
+  // Remove item from tree.
+  AXTreeUpdate tree_update;
+  tree_update.nodes.resize(1);
+  tree_update.nodes[0] = initial_state.nodes[1];
+  tree_update.nodes[0].AddState(ax::mojom::State::kIgnored);
+
+  ASSERT_TRUE(tree.Unserialize(tree_update));
+
+  ASSERT_FALSE(tree_node->GetPosInSet());
+  EXPECT_OPTIONAL_EQ(1, tree_node->GetSetSize());
+
+  // Ignored nodes are not part of ordered sets.
+  EXPECT_FALSE(item1->GetPosInSet());
+  EXPECT_FALSE(item1->GetSetSize());
+
+  EXPECT_OPTIONAL_EQ(1, item2->GetPosInSet());
+  EXPECT_OPTIONAL_EQ(1, item2->GetSetSize());
 }
 
 // Tests that kPopUpButtons are assigned the SetSize of the wrapped