Fix bug in serializing accessibility trees with invalidation.

In http://crrev.com/c/1152180, we introduced
AXTreeSerializer::InvalidateSubtree. This replaced
AXTreeSerializer::DeleteClientSubtree, which was problematic and led
to subtle, hard-to-catch unserialization errors.

This patch adds comprehensive tests for InvalidateSubtree by introducing it
into the "generated tree" tests, which try every permutation of trees with
up to a certain number of nodes and every possible change to those trees.
Now those tests also try every possible way to invalidate a subtree before
serializing.

This uncovered one subtle bug: the code to detect reparenting also needed
to check the 'invalid' flag and recursively check invalid children.

Bug: 870661, 866293
Change-Id: I45313b712e50db4fba16a5d55f72ecd440f04335
Reviewed-on: https://chromium-review.googlesource.com/1180091Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584643}

ui/accessibility/ax_generated_tree_unittest.cc

@@ -148,42 +148,49 @@ TEST(AXGeneratedTreeTest, SerializeGeneratedTrees) {
 
       // Now iterate over which node to update first, |k|.
       for (int k = 0; k < tree_size; k++) {
-        SCOPED_TRACE("i=" + base::IntToString(i) +
-                     " j=" + base::IntToString(j) +
-                     " k=" + base::IntToString(k));
-
-        // Start by serializing tree0 and unserializing it into a new
-        // empty tree |dst_tree|.
-        std::unique_ptr<AXTreeSource<const AXNode*, AXNodeData, AXTreeData>>
-            tree0_source(tree0.CreateTreeSource());
-        AXTreeSerializer<const AXNode*, AXNodeData, AXTreeData> serializer(
-            tree0_source.get());
-        AXTreeUpdate update0;
-        ASSERT_TRUE(serializer.SerializeChanges(tree0.root(), &update0));
-
-        AXTree dst_tree;
-        ASSERT_TRUE(dst_tree.Unserialize(update0));
-
-        // At this point, |dst_tree| should now be identical to |tree0|.
-        EXPECT_EQ(TreeToString(tree0), TreeToString(dst_tree));
-
-        // Next, pretend that tree0 turned into tree1, and serialize
-        // a sequence of updates to |dst_tree| to match.
-        std::unique_ptr<AXTreeSource<const AXNode*, AXNodeData, AXTreeData>>
-            tree1_source(tree1.CreateTreeSource());
-        serializer.ChangeTreeSourceForTesting(tree1_source.get());
-
-        for (int k_index = 0; k_index < tree_size; ++k_index) {
-          int id = 1 + (k + k_index) % tree_size;
-          AXTreeUpdate update;
-          ASSERT_TRUE(
-              serializer.SerializeChanges(tree1.GetFromId(id), &update));
-          ASSERT_TRUE(dst_tree.Unserialize(update));
+        // Iterate over a node to invalidate, |l| (zero means no invalidation).
+        for (int l = 0; l <= tree_size; l++) {
+          SCOPED_TRACE(
+              "i=" + base::IntToString(i) + " j=" + base::IntToString(j) +
+              " k=" + base::IntToString(k) + " l=" + base::IntToString(l));
+
+          // Start by serializing tree0 and unserializing it into a new
+          // empty tree |dst_tree|.
+          std::unique_ptr<AXTreeSource<const AXNode*, AXNodeData, AXTreeData>>
+              tree0_source(tree0.CreateTreeSource());
+          AXTreeSerializer<const AXNode*, AXNodeData, AXTreeData> serializer(
+              tree0_source.get());
+          AXTreeUpdate update0;
+          ASSERT_TRUE(serializer.SerializeChanges(tree0.root(), &update0));
+
+          AXTree dst_tree;
+          ASSERT_TRUE(dst_tree.Unserialize(update0));
+
+          // At this point, |dst_tree| should now be identical to |tree0|.
+          EXPECT_EQ(TreeToString(tree0), TreeToString(dst_tree));
+
+          // Next, pretend that tree0 turned into tree1.
+          std::unique_ptr<AXTreeSource<const AXNode*, AXNodeData, AXTreeData>>
+              tree1_source(tree1.CreateTreeSource());
+          serializer.ChangeTreeSourceForTesting(tree1_source.get());
+
+          // Invalidate a subtree rooted at one of the nodes.
+          if (l > 0)
+            serializer.InvalidateSubtree(tree1.GetFromId(l));
+
+          // Serialize a sequence of updates to |dst_tree| to match.
+          for (int k_index = 0; k_index < tree_size; ++k_index) {
+            int id = 1 + (k + k_index) % tree_size;
+            AXTreeUpdate update;
+            ASSERT_TRUE(
+                serializer.SerializeChanges(tree1.GetFromId(id), &update));
+            ASSERT_TRUE(dst_tree.Unserialize(update));
+          }
+
+          // After the sequence of updates, |dst_tree| should now be
+          // identical to |tree1|.
+          EXPECT_EQ(TreeToString(tree1), TreeToString(dst_tree));
         }
-
-        // After the sequence of updates, |dst_tree| should now be
-        // identical to |tree1|.
-        EXPECT_EQ(TreeToString(tree1), TreeToString(dst_tree));
       }
     }
   }

ui/accessibility/ax_tree_serializer.h

@@ -306,8 +306,8 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
         // and return true (reparenting was found).
         *out_lca = LeastCommonAncestor(*out_lca, client_child);
         result = true;
-      } else {
-        // This child is already in the client tree, we won't
+      } else if (!client_child->invalid) {
+        // This child is already in the client tree and valid, we won't
         // recursively serialize it so we don't need to check this
         // subtree recursively for reparenting.
         continue;

ui/accessibility/ax_tree_serializer_unittest.cc

@@ -180,6 +180,10 @@ TEST_F(AXTreeSerializerTest, ReparentingUpdatesSubtree) {
   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();
+
   // The update should delete the subtree rooted at node id=2, and
   // then include nodes 2...5.
   EXPECT_EQ(2, update.node_id_to_clear);
@@ -190,6 +194,48 @@ TEST_F(AXTreeSerializerTest, ReparentingUpdatesSubtree) {
   EXPECT_EQ(5, update.nodes[3].id);
 }
 
+// Similar to ReparentingUpdatesSubtree, except that InvalidateSubtree is
+// called on id=1 - we need to make sure that the reparenting is still
+// detected.
+TEST_F(AXTreeSerializerTest, ReparentingWithInvalidationUpdatesSubtree) {
+  // (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();
+  AXTreeUpdate update;
+  serializer_->InvalidateSubtree(tree1_->GetFromId(1));
+  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();
+}
+
 // A variant of AXTreeSource that returns true for IsValid() for one
 // particular id.
 class AXTreeSourceWithInvalidId