From 8dc07f045c6287da3404fa8e0fd8b1b3960afbfa Mon Sep 17 00:00:00 2001 From: dmazzoni Date: Wed, 3 Jun 2015 17:24:59 -0700 Subject: [PATCH] When serializing accessibility tree, skip invalid children. See bug for specific repro in the wild, but essentially we need to check if the child is valid just before serializing, and not trust the list of children of a node. BUG=479743 Review URL: https://codereview.chromium.org/1144363004 Cr-Commit-Position: refs/heads/master@{#332748} --- ui/accessibility/ax_tree_serializer.h | 47 ++++++++++-------- ui/accessibility/ax_tree_serializer_unittest.cc | 65 +++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 19 deletions(-) diff --git a/ui/accessibility/ax_tree_serializer.h b/ui/accessibility/ax_tree_serializer.h index 53e515aaa484..5435bbe2bae7 100644 --- a/ui/accessibility/ax_tree_serializer.h +++ b/ui/accessibility/ax_tree_serializer.h @@ -417,34 +417,42 @@ void AXTreeSerializer::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(); 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. + 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 children_to_serialize; + std::vector 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. 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::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 diff --git a/ui/accessibility/ax_tree_serializer_unittest.cc b/ui/accessibility/ax_tree_serializer_unittest.cc index 733e289f0f56..8db1937450dc 100644 --- a/ui/accessibility/ax_tree_serializer_unittest.cc +++ b/ui/accessibility/ax_tree_serializer_unittest.cc @@ -179,4 +179,69 @@ TEST_F(AXTreeSerializerTest, ReparentingUpdatesSubtree) { EXPECT_EQ(5, update.nodes[3].id); } +// A variant of AXTreeSource that returns true for IsValid() for one +// particular id. +class AXTreeSourceWithInvalidId : public AXTreeSource { + public: + AXTreeSourceWithInvalidId(AXTree* tree, int invalid_id) + : tree_(tree), + invalid_id_(invalid_id) {} + ~AXTreeSourceWithInvalidId() override {} + + // AXTreeSource implementation. + AXNode* GetRoot() const override { return tree_->root(); } + AXNode* GetFromId(int32 id) const override { return tree_->GetFromId(id); } + int32 GetId(const AXNode* node) const override { return node->id(); } + void GetChildren(const AXNode* node, + std::vector* out_children) const override { + for (int i = 0; i < node->child_count(); ++i) + out_children->push_back(node->ChildAtIndex(i)); + } + AXNode* GetParent(const AXNode* node) const override { + return node->parent(); + } + bool IsValid(const AXNode* node) const override { + return node != NULL && node->id() != invalid_id_; + } + bool IsEqual(const AXNode* node1, const AXNode* node2) const override { + return node1 == node2; + } + const AXNode* GetNull() const override { return NULL; } + void SerializeNode(const AXNode* node, AXNodeData* out_data) const override { + *out_data = node->data(); + if (node->id() == invalid_id_) + out_data->id = -1; + } + + private: + AXTree* tree_; + int invalid_id_; + + DISALLOW_COPY_AND_ASSIGN(AXTreeSourceWithInvalidId); +}; + +// Test that the serializer skips invalid children. +TEST(AXTreeSerializerInvalidTest, InvalidChild) { + // (1 (2 3)) + AXTreeUpdate treedata; + treedata.nodes.resize(3); + treedata.nodes[0].id = 1; + treedata.nodes[0].role = AX_ROLE_ROOT_WEB_AREA; + treedata.nodes[0].child_ids.push_back(2); + treedata.nodes[0].child_ids.push_back(3); + treedata.nodes[1].id = 2; + treedata.nodes[2].id = 3; + + AXTree tree(treedata); + AXTreeSourceWithInvalidId source(&tree, 3); + + AXTreeSerializer serializer(&source); + AXTreeUpdate update; + serializer.SerializeChanges(tree.root(), &update); + + ASSERT_EQ(2U, update.nodes.size()); + EXPECT_EQ(1, update.nodes[0].id); + EXPECT_EQ(2, update.nodes[1].id); +} + } // namespace ui -- 2.11.4.GIT