When serializing accessibility tree, skip invalid children.

ui/accessibility/ax_tree_serializer.h

Index: ui/accessibility/ax_tree_serializer.h
diff --git a/ui/accessibility/ax_tree_serializer.h b/ui/accessibility/ax_tree_serializer.h
index 53e515aaa484a99ed9b2d6cdc28bcb166c07f80b..5435bbe2bae702eb274fcfc18b160a2fee67fe49 100644
--- a/ui/accessibility/ax_tree_serializer.h
+++ b/ui/accessibility/ax_tree_serializer.h
@@ -417,34 +417,42 @@ void AXTreeSerializer<AXSourceNode>::SerializeChangedNodes(
 
   // Serialize this node. This fills in all of the fields in
   // AXNodeData except child_ids, which we handle below.
+  size_t serialized_node_index = out_update->nodes.size();

[aboxhall, 2015/06/01 16:46:46]

Why do this _and_ put serialized_node inside a scoped block?

[dmazzoni, 2015/06/01 16:53:18]

So we can reference out_update->nodes[serialized_node_index] below

   out_update->nodes.push_back(AXNodeData());
-  AXNodeData* serialized_node = &out_update->nodes.back();
-  tree_->SerializeNode(node, serialized_node);
-  // TODO(dmazzoni/dtseng): Make the serializer not depend on roles to identify
-  // the root.
-  if (serialized_node->id == client_root_->id &&
-      (serialized_node->role != AX_ROLE_ROOT_WEB_AREA &&
-       serialized_node->role != AX_ROLE_DESKTOP)) {
-    serialized_node->role = AX_ROLE_ROOT_WEB_AREA;
+  {
+    // Take the address of an element in a vector only within a limited
+    // scope because otherwise the pointer can become invalid if the
+    // vector is resized.

[aboxhall, 2015/06/01 16:46:46]

How can the vector get resized? Could we come up with a test for that situation?

[dmazzoni, 2015/06/01 16:53:18]

Because we call this function recursively on each child, and then finish writing
|this| last.

Imagine you have this tree:

    A
  B C D

This function serializes A, so out_update->nodes is: [A]
  Recursively serialize B, so out_update->nodes is: [A B]
  Recursively serialize C, so out_update->nodes is: [A B C]
  Recursively serialize D, so out_update->nodes is: [A B C D]
After doing the children, we update the first element's list of child_ids - but
a pointer to out_update->nodes[0] could be invalid because we resized
out_update->nodes.

ASAN already caught this error.

[aboxhall, 2015/06/01 21:25:46]

Got it, thanks.

+    AXNodeData* serialized_node = &out_update->nodes[serialized_node_index];
+
+    tree_->SerializeNode(node, serialized_node);
+    // TODO(dmazzoni/dtseng): Make the serializer not depend on roles to
+    // identify the root.
+    if (serialized_node->id == client_root_->id &&
+        (serialized_node->role != AX_ROLE_ROOT_WEB_AREA &&
+         serialized_node->role != AX_ROLE_DESKTOP)) {
+      serialized_node->role = AX_ROLE_ROOT_WEB_AREA;
+    }
   }
-  serialized_node->child_ids.clear();
 
-  // Iterate over the children, make note of the ones that are new
-  // and need to be serialized, and update the ClientTreeNode
+  // Iterate over the children, serialize them, and update the ClientTreeNode
   // data structure to reflect the new tree.
-  std::vector<AXSourceNode> children_to_serialize;
+  std::vector<int32> actual_serialized_node_child_ids;
   client_node->children.reserve(children.size());
   for (size_t i = 0; i < children.size(); ++i) {
     AXSourceNode& child = children[i];
     int child_id = tree_->GetId(child);
 
-    // No need to do anything more with children that aren't new;
-    // the client will reuse its existing object.
+    // Skip if the child isn't valid.
+    if (!tree_->IsValid(child))
+      continue;
+
+    // Skip if the same child is included more than once.

[dmazzoni, 2015/05/26 23:03:12]

Note: the code below this comment was correct before, the but the comment above
it ("No need to do anything more with children that aren't new") was wrong -
new_child_ids isn't the set of child ids that are newly added, it's just the new
set of child ids after this update. The purpose of this line wasn't to avoid
serializing nodes that haven't changed, it was to avoid serializing the same
child twice if a parent lists the same child twice in its child list.

     if (new_child_ids.find(child_id) == new_child_ids.end())
       continue;
 
     new_child_ids.erase(child_id);
-    serialized_node->child_ids.push_back(child_id);
+    actual_serialized_node_child_ids.push_back(child_id);
     if (client_child_id_map.find(child_id) != client_child_id_map.end()) {
       ClientTreeNode* reused_child = client_child_id_map[child_id];
       client_node->children.push_back(reused_child);
@@ -454,13 +462,14 @@ void AXTreeSerializer<AXSourceNode>::SerializeChangedNodes(
       new_child->parent = client_node;
       client_node->children.push_back(new_child);
       client_id_map_[child_id] = new_child;
-      children_to_serialize.push_back(child);
+      SerializeChangedNodes(child, out_update);
     }
   }
 
-  // Serialize all of the new children, recursively.
-  for (size_t i = 0; i < children_to_serialize.size(); ++i)
-    SerializeChangedNodes(children_to_serialize[i], out_update);
+  // Finally, update the child ids of this node to reflect the actual child
+  // ids that were valid during serialization.
+  out_update->nodes[serialized_node_index].child_ids.swap(
+      actual_serialized_node_child_ids);
 }
 
 }  // namespace ui