Fix bug in AXTreeSerializer::Reset

In a few places in the code we call AXTreeSerializer::Reset
when there's a tricky reparenting issue or when we really
want the accessibility tree to be re-serialized from
scratch (like when the mode changes).

However, this can lead to a corner case where the next
update from the serializer can't be unserialized, because
the client tries to unserialize it as an incremental
update, but it contains some reparenting.

The trick is to just ensure that we always set
node_id_to_clear on the next update after a reset.
That also requires us to relax the previous requirement
that node_id_to_clear must be valid. Now the
serializer sets node_id_to_clear to be equal to the
root after a reset, to guarantee that the client ends
up with a fresh start.

Bug: 878325
Change-Id: Iff255a3f62e47363be0b3d9c2f6809c229a6a00f
Reviewed-on: https://chromium-review.googlesource.com/1198009Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589017}

ui/accessibility/ax_tree.cc

@@ -349,16 +349,11 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
   bool root_updated = false;
   if (update.node_id_to_clear != 0) {
     AXNode* node = GetFromId(update.node_id_to_clear);
-    if (!node) {
-      error_ = base::StringPrintf("Bad node_id_to_clear: %d",
-                                  update.node_id_to_clear);
-      return false;
-    }
 
     // Only destroy the root if the root was replaced and not if it's simply
     // updated. To figure out if  the root was simply updated, we compare the ID
     // of the new root with the existing root ID.
-    if (node == root_) {
+    if (node && node == root_) {
       if (update.root_id != old_root_id) {
         // Clear root_ before calling DestroySubtree so that root_ doesn't ever
         // point to an invalid node.
@@ -372,7 +367,7 @@ bool AXTree::Unserialize(const AXTreeUpdate& update) {
 
     // If the root has simply been updated, we treat it like an update to any
     // other node.
-    if (root_ && (node != root_ || root_updated)) {
+    if (node && root_ && (node != root_ || root_updated)) {
       for (int i = 0; i < node->child_count(); ++i)
         DestroySubtree(node->ChildAtIndex(i), &update_state);
       std::vector<AXNode*> children;

ui/accessibility/ax_tree_serializer.h

@@ -162,6 +162,10 @@ class AXTreeSerializer {
   // Visit all of the descendants of |node| once.
   void WalkAllDescendants(AXSourceNode node);
 
+  // Delete the entire client subtree but don't set the did_reset_ flag
+  // like when Reset() is called.
+  void InternalReset();
+
   // The tree source.
   AXTreeSource<AXSourceNode, AXNodeData, AXTreeData>* tree_;
 
@@ -177,6 +181,11 @@ class AXTreeSerializer {
   // The maximum number of nodes to serialize in a given call to
   // SerializeChanges, or 0 if there's no maximum.
   size_t max_node_count_ = 0;
+
+  // Keeps track of if Reset() was called. If so, we need to always
+  // explicitly set node_id_to_clear to ensure that the next serialized
+  // tree is treated as a completely new tree and not a partial update.
+  bool did_reset_ = false;
 };
 
 // In order to keep track of what nodes the client knows about, we keep a
@@ -203,6 +212,12 @@ AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::~AXTreeSerializer() {
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
 void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::Reset() {
+  InternalReset();
+  did_reset_ = true;
+}
+
+template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
+void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::InternalReset() {
   client_tree_data_ = AXTreeData();
 
   // Normally we use DeleteClientSubtree to remove nodes from the tree,
@@ -371,7 +386,7 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges(
         // If there's no LCA, just tell the client to destroy the whole
         // tree and then we'll serialize everything from the new root.
         out_update->node_id_to_clear = client_root_->id;
-        Reset();
+        InternalReset();
       } else if (need_delete) {
         // Otherwise, if we need to reserialize a subtree, first we need
         // to delete those nodes in our client tree so that
@@ -394,7 +409,19 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges(
   //     DumpAccessibilityTreeTest.AccessibilityAriaOwns.
   WalkAllDescendants(lca);
 
-  return SerializeChangedNodes(lca, out_update);
+  if (!SerializeChangedNodes(lca, out_update))
+    return false;
+
+  // If we had a reset, ensure that the old tree is cleared before the client
+  // unserializes this update. If we didn't do this, there's a chance that
+  // treating this update as an incremental update could result in some
+  // reparenting.
+  if (did_reset_) {
+    out_update->node_id_to_clear = tree_->GetId(lca);
+    did_reset_ = false;
+  }
+
+  return true;
 }
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
@@ -444,7 +471,8 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
   int id = tree_->GetId(node);
   ClientTreeNode* client_node = ClientTreeNodeById(id);
   if (!client_node) {
-    Reset();
+    if (client_root_)
+      Reset();
     client_root_ = new ClientTreeNode();
     client_node = client_root_;
     client_node->id = id;

ui/accessibility/ax_tree_serializer_unittest.cc

@@ -389,4 +389,76 @@ TEST_F(AXTreeSerializerTest, DuplicateIdsReturnsErrorAndFlushes) {
   ASSERT_EQ(static_cast<size_t>(5), update.nodes.size());
 }
 
+// If a tree serializer is reset, that means it doesn't know about
+// the state of the client tree anymore. The safest thing to do in
+// that circumstance is to force the client to clear everything.
+TEST_F(AXTreeSerializerTest, ResetUpdatesNodeIdToClear) {
+  // (1 (2 (3 (4 (5)))))
+  treedata0_.root_id = 1;
+  treedata0_.nodes.resize(5);
+  treedata0_.nodes[0].id = 1;
+  treedata0_.nodes[0].child_ids.push_back(2);
+  treedata0_.nodes[1].id = 2;
+  treedata0_.nodes[1].child_ids.push_back(3);
+  treedata0_.nodes[2].id = 3;
+  treedata0_.nodes[2].child_ids.push_back(4);
+  treedata0_.nodes[3].id = 4;
+  treedata0_.nodes[3].child_ids.push_back(5);
+  treedata0_.nodes[4].id = 5;
+
+  // Node 5 has been reparented from being a child of node 4,
+  // to a child of node 2.
+  // (1 (2 (3 (4) 5)))
+  treedata1_.root_id = 1;
+  treedata1_.nodes.resize(5);
+  treedata1_.nodes[0].id = 1;
+  treedata1_.nodes[0].child_ids.push_back(2);
+  treedata1_.nodes[1].id = 2;
+  treedata1_.nodes[1].child_ids.push_back(3);
+  treedata1_.nodes[1].child_ids.push_back(5);
+  treedata1_.nodes[2].id = 3;
+  treedata1_.nodes[2].child_ids.push_back(4);
+  treedata1_.nodes[3].id = 4;
+  treedata1_.nodes[4].id = 5;
+
+  CreateTreeSerializer();
+
+  serializer_->Reset();
+
+  AXTreeUpdate update;
+  ASSERT_TRUE(serializer_->SerializeChanges(tree1_->GetFromId(4), &update));
+
+  // The update should unserialize without errors.
+  AXTree dst_tree(treedata0_);
+  EXPECT_TRUE(dst_tree.Unserialize(update)) << dst_tree.error();
+}
+
+// Ensure that calling Reset doesn't cause any problems if
+// the root changes.
+TEST_F(AXTreeSerializerTest, ResetWorksWithNewRootId) {
+  // (1 (2))
+  treedata0_.root_id = 1;
+  treedata0_.nodes.resize(2);
+  treedata0_.nodes[0].id = 1;
+  treedata0_.nodes[0].child_ids.push_back(2);
+  treedata0_.nodes[1].id = 2;
+
+  // (3 (4))
+  treedata1_.root_id = 3;
+  treedata1_.nodes.resize(2);
+  treedata1_.nodes[0].id = 3;
+  treedata1_.nodes[0].child_ids.push_back(4);
+  treedata1_.nodes[1].id = 4;
+
+  CreateTreeSerializer();
+  serializer_->Reset();
+
+  AXTreeUpdate update;
+  ASSERT_TRUE(serializer_->SerializeChanges(tree1_->GetFromId(4), &update));
+
+  // The update should unserialize without errors.
+  AXTree dst_tree(treedata0_);
+  EXPECT_TRUE(dst_tree.Unserialize(update)) << dst_tree.error();
+}
+
 }  // namespace ui

ui/accessibility/ax_tree_unittest.cc

@@ -323,25 +323,6 @@ TEST(AXTreeTest, SerializeAXTreeUpdate) {
       update.ToString());
 }
 
-TEST(AXTreeTest, DeleteUnknownSubtreeFails) {
-  AXNodeData root;
-  root.id = 1;
-
-  AXTreeUpdate initial_state;
-  initial_state.root_id = 1;
-  initial_state.nodes.push_back(root);
-  AXTree tree(initial_state);
-
-  // This should fail because we're asking it to delete
-  // a subtree rooted at id=2, which doesn't exist.
-  AXTreeUpdate update;
-  update.node_id_to_clear = 2;
-  update.nodes.resize(1);
-  update.nodes[0].id = 1;
-  EXPECT_FALSE(tree.Unserialize(update));
-  ASSERT_EQ("Bad node_id_to_clear: 2", tree.error());
-}
-
 TEST(AXTreeTest, LeaveOrphanedDeletedSubtreeFails) {
   AXTreeUpdate initial_state;
   initial_state.root_id = 1;