Re-land: Avoid serializing the same accessibility node twice in the same message

Originally landed: r560165, crrev.com/c/1063007
Reverted: r561246, crrev.com/c/1069891

TBR=dtseng@chromium.org
Bug: 651614, 845778

Change-Id: I77b54de28cb0c4a231b5b0758ba4708100d27fff
Reviewed-on: https://chromium-review.googlesource.com/1072875Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561913}

chrome/browser/ui/aura/accessibility/automation_manager_aura.cc

@@ -196,7 +196,8 @@ void AutomationManagerAura::SendEvent(BrowserContext* context,
   std::vector<ExtensionMsg_AccessibilityEventParams> events;
   events.emplace_back(ExtensionMsg_AccessibilityEventParams());
   ExtensionMsg_AccessibilityEventParams& params = events.back();
-  if (!current_tree_serializer_->SerializeChanges(aura_obj, &params.update)) {
+  current_tree_serializer_->BeginSerializingChanges(&params.update);
+  if (!current_tree_serializer_->SerializeOneChange(aura_obj)) {
     LOG(ERROR) << "Unable to serialize one accessibility event.";
     return;
   }
@@ -205,7 +206,9 @@ void AutomationManagerAura::SendEvent(BrowserContext* context,
   views::AXAuraObjWrapper* focus =
       views::AXAuraObjCache::GetInstance()->GetFocus();
   if (focus)
-    current_tree_serializer_->SerializeChanges(focus, &params.update);
+    current_tree_serializer_->SerializeOneChange(focus);
+
+  current_tree_serializer_->FinishSerializingChanges();
 
   params.tree_id = 0;
   params.id = aura_obj->GetUniqueId().Get();

chromecast/browser/ui/aura/accessibility/automation_manager_aura.cc

@@ -141,7 +141,8 @@ void AutomationManagerAura::SendEvent(BrowserContext* context,
   std::vector<ExtensionMsg_AccessibilityEventParams> events;
   events.emplace_back(ExtensionMsg_AccessibilityEventParams());
   ExtensionMsg_AccessibilityEventParams& params = events.back();
-  if (!current_tree_serializer_->SerializeChanges(aura_obj, &params.update)) {
+  current_tree_serializer_->BeginSerializingChanges(&params.update);
+  if (!current_tree_serializer_->SerializeOneChange(aura_obj)) {
     LOG(ERROR) << "Unable to serialize one accessibility event.";
     return;
   }
@@ -150,7 +151,9 @@ void AutomationManagerAura::SendEvent(BrowserContext* context,
   views::AXAuraObjWrapper* focus =
       views::AXAuraObjCache::GetInstance()->GetFocus();
   if (focus)
-    current_tree_serializer_->SerializeChanges(focus, &params.update);
+    current_tree_serializer_->SerializeOneChange(focus);
+
+  current_tree_serializer_->FinishSerializingChanges();
 
   params.tree_id = 0;
   params.id = aura_obj->GetUniqueId().Get();

content/renderer/accessibility/render_accessibility_impl.cc

@@ -454,8 +454,15 @@ void RenderAccessibilityImpl::SendPendingAccessibilityEvents() {
     if (plugin_tree_source_)
       event_msg.update.has_tree_data = true;
 
-    if (!serializer_.SerializeChanges(obj, &event_msg.update)) {
+    if (event_msgs.size() == 0)
+      serializer_.BeginSerializingChanges(&event_msg.update);
+
+    if (!serializer_.SerializeOneChange(obj)) {
       VLOG(1) << "Failed to serialize one accessibility event.";
+
+      if (event_msgs.size() == 0)
+        serializer_.FinishSerializingChanges();
+
       continue;
     }
 
@@ -481,6 +488,9 @@ void RenderAccessibilityImpl::SendPendingAccessibilityEvents() {
             << "\n" << event_msg.update.ToString();
   }
 
+  if (event_msgs.size())
+    serializer_.FinishSerializingChanges();
+
   Send(new AccessibilityHostMsg_Events(routing_id(), event_msgs, reset_token_,
                                        ack_token_));
   reset_token_ = 0;

ui/accessibility/ax_tree_serializer.h

@@ -76,10 +76,23 @@ class AXTreeSerializer {
     max_node_count_ = max_node_count;
   }
 
+  // Serialize a sequence of one or more changes. This is more efficient
+  // than calling SerializeChanges more than once with the same output
+  // AXTreeUpdate; it will automatically avoid serializing the same
+  // node more than once when there are overlapping changes.
+  // It's an error to call DeleteClientSubtree in the middle of a sequence.
+  void BeginSerializingChanges(
+      AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update);
+  bool SerializeOneChange(AXSourceNode node);
+  void FinishSerializingChanges();
+
   // Serialize all changes to |node| and append them to |out_update|.
   // Returns true on success. On failure, returns false and calls Reset();
   // this only happens when the source tree has a problem like duplicate
-  // ids or changing during serialization.
+  // ids or changing during serialization. Note that it's more efficient
+  // to use BeginSerializingChanges, SerializeOneChange, and
+  // FinishSerializingChanges if you're going to include more than one
+  // change in the same output AXTreeUpdate.
   bool SerializeChanges(AXSourceNode node,
                         AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update);
 
@@ -152,9 +165,7 @@ class AXTreeSerializer {
   void DeleteClientSubtree(ClientTreeNode* client_node);
 
   // Helper function, called recursively with each new node to serialize.
-  bool SerializeChangedNodes(
-      AXSourceNode node,
-      AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update);
+  bool SerializeChangedNodes(AXSourceNode node);
 
   // Visit all of the descendants of |node| once.
   void WalkAllDescendants(AXSourceNode node);
@@ -171,6 +182,14 @@ class AXTreeSerializer {
   // A map from IDs to nodes in the client tree.
   base::hash_map<int32_t, ClientTreeNode*> client_id_map_;
 
+  // The current update, valid in-between calls to BeginSerializingChanges
+  // and FinishSerializingChanges.
+  AXTreeUpdateBase<AXNodeData, AXTreeData>* current_update_ = nullptr;
+
+  // The set of IDs already serialized, valid in-between calls to
+  // BeginSerializingChanges and FinishSerializingChanges.
+  std::set<int32_t> already_serialized_ids_;
+
   // 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;
@@ -320,16 +339,25 @@ AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::ClientTreeNodeById(
 }
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
-bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges(
-    AXSourceNode node,
-    AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update) {
+void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
+    BeginSerializingChanges(
+        AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update) {
+  CHECK(!current_update_);
+  current_update_ = out_update;
+}
+
+template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
+bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeOneChange(
+    AXSourceNode node) {
+  CHECK(current_update_);
+
   // Send the tree data if it's changed since the last update, or if
-  // out_update->has_tree_data is already set to true.
+  // current_update_->has_tree_data is already set to true.
   AXTreeData new_tree_data;
   if (tree_->GetTreeData(&new_tree_data) &&
-      (out_update->has_tree_data || new_tree_data != client_tree_data_)) {
-    out_update->has_tree_data = true;
-    out_update->tree_data = new_tree_data;
+      (current_update_->has_tree_data || new_tree_data != client_tree_data_)) {
+    current_update_->has_tree_data = true;
+    current_update_->tree_data = new_tree_data;
     client_tree_data_ = new_tree_data;
   }
 
@@ -357,13 +385,13 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges(
       if (!tree_->IsValid(lca)) {
         // 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;
+        current_update_->node_id_to_clear = client_root_->id;
         Reset();
       } 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
         // SerializeChangedNodes() will be sure to send them again.
-        out_update->node_id_to_clear = tree_->GetId(lca);
+        current_update_->node_id_to_clear = tree_->GetId(lca);
         ClientTreeNode* client_lca = ClientTreeNodeById(tree_->GetId(lca));
         CHECK(client_lca);
         DeleteClientSubtree(client_lca);
@@ -381,13 +409,34 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges(
   //     DumpAccessibilityTreeTest.AccessibilityAriaOwns.
   WalkAllDescendants(lca);
 
-  return SerializeChangedNodes(lca, out_update);
+  return SerializeChangedNodes(lca);
+}
+
+template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
+void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
+    FinishSerializingChanges() {
+  CHECK(current_update_);
+  current_update_ = nullptr;
+  already_serialized_ids_.clear();
+}
+
+template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
+bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges(
+    AXSourceNode node,
+    AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update) {
+  CHECK(!current_update_);
+  BeginSerializingChanges(out_update);
+  bool success = SerializeOneChange(node);
+  FinishSerializingChanges();
+  return success;
 }
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
 void AXTreeSerializer<AXSourceNode,
                       AXNodeData,
                       AXTreeData>::DeleteClientSubtree(AXSourceNode node) {
+  CHECK(!current_update_);
+
   ClientTreeNode* client_node = ClientTreeNodeById(tree_->GetId(node));
   if (client_node)
     DeleteClientSubtree(client_node);
@@ -406,9 +455,7 @@ void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
 bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
-    SerializeChangedNodes(
-        AXSourceNode node,
-        AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update) {
+    SerializeChangedNodes(AXSourceNode node) {
   // This method has three responsibilities:
   // 1. Serialize |node| into an AXNodeData, and append it to
   //    the AXTreeUpdate to be sent to the client.
@@ -422,6 +469,10 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
   // about. If we don't find it, then this must be the new root of the
   // accessibility tree.
   int id = tree_->GetId(node);
+  if (already_serialized_ids_.find(id) != already_serialized_ids_.end())
+    return true;
+  already_serialized_ids_.insert(id);
+
   ClientTreeNode* client_node = ClientTreeNodeById(id);
   if (!client_node) {
     Reset();
@@ -440,7 +491,7 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
   // consistent results.
   base::hash_set<int32_t> new_child_ids;
   std::vector<AXSourceNode> children;
-  if (max_node_count_ == 0 || out_update->nodes.size() < max_node_count_) {
+  if (max_node_count_ == 0 || current_update_->nodes.size() < max_node_count_) {
     tree_->GetChildren(node, &children);
   } else if (max_node_count_ > 0) {
     static bool logged_once = false;
@@ -487,17 +538,18 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
 
   // 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());
+  size_t serialized_node_index = current_update_->nodes.size();
+  current_update_->nodes.push_back(AXNodeData());
   {
     // 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];
+    AXNodeData* serialized_node =
+        &current_update_->nodes[serialized_node_index];
 
     tree_->SerializeNode(node, serialized_node);
     if (serialized_node->id == client_root_->id)
-      out_update->root_id = serialized_node->id;
+      current_update_->root_id = serialized_node->id;
   }
 
   // Iterate over the children, serialize them, and update the ClientTreeNode
@@ -527,14 +579,14 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
       new_child->parent = client_node;
       client_node->children.push_back(new_child);
       client_id_map_[child_id] = new_child;
-      if (!SerializeChangedNodes(child, out_update))
+      if (!SerializeChangedNodes(child))
         return false;
     }
   }
 
   // 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(
+  current_update_->nodes[serialized_node_index].child_ids.swap(
       actual_serialized_node_child_ids);
 
   return true;

ui/accessibility/ax_tree_serializer_unittest.cc

@@ -11,6 +11,7 @@
 
 #include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
+#include "testing/gtest/include/gtest/gtest-death-test.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/accessibility/ax_node.h"
 #include "ui/accessibility/ax_serializable_tree.h"
@@ -343,4 +344,96 @@ TEST_F(AXTreeSerializerTest, DuplicateIdsReturnsErrorAndFlushes) {
   ASSERT_EQ(static_cast<size_t>(5), update.nodes.size());
 }
 
+TEST_F(AXTreeSerializerTest, SerializeOneChangeDeathTest) {
+  treedata0_.root_id = 1;
+  treedata0_.nodes.resize(1);
+  treedata0_.nodes[0].id = 1;
+
+  treedata1_.root_id = 1;
+  treedata1_.nodes.resize(1);
+  treedata1_.nodes[0].id = 1;
+
+  CreateTreeSerializer();
+
+  // It's not legal to call SerializeOneChange without calling
+  // BeginSerializingChanges first.
+  ASSERT_DEATH_IF_SUPPORTED(
+      { serializer_->SerializeOneChange(tree1_->GetFromId(1)); }, ".*");
+}
+
+TEST_F(AXTreeSerializerTest, DeleteClientSubtreeDeathTest) {
+  treedata0_.root_id = 1;
+  treedata0_.nodes.resize(1);
+  treedata0_.nodes[0].id = 1;
+
+  treedata1_.root_id = 1;
+  treedata1_.nodes.resize(1);
+  treedata1_.nodes[0].id = 1;
+
+  CreateTreeSerializer();
+
+  // It's not legal to call DeleteClientSubtree in the middle of a
+  // serialization sequence.
+  AXTreeUpdate update;
+  serializer_->BeginSerializingChanges(&update);
+  ASSERT_DEATH_IF_SUPPORTED(
+      { serializer_->DeleteClientSubtree(tree1_->GetFromId(1)); }, ".*");
+}
+
+TEST_F(AXTreeSerializerTest, BeginSerializingChangesTwiceDeathTest) {
+  treedata0_.root_id = 1;
+  treedata0_.nodes.resize(1);
+  treedata0_.nodes[0].id = 1;
+
+  treedata1_.root_id = 1;
+  treedata1_.nodes.resize(1);
+  treedata1_.nodes[0].id = 1;
+
+  CreateTreeSerializer();
+
+  AXTreeUpdate update;
+  serializer_->BeginSerializingChanges(&update);
+  ASSERT_DEATH_IF_SUPPORTED({ serializer_->BeginSerializingChanges(&update); },
+                            ".*");
+}
+
+TEST_F(AXTreeSerializerTest, SerializationSequence) {
+  // (1 (2 3))
+  treedata0_.root_id = 1;
+  treedata0_.nodes.resize(3);
+  treedata0_.nodes[0].id = 1;
+  treedata0_.nodes[0].child_ids.push_back(2);
+  treedata0_.nodes[0].child_ids.push_back(3);
+  treedata0_.nodes[1].id = 2;
+  treedata0_.nodes[2].id = 3;
+
+  // (1 (4 2 3))
+  treedata1_.root_id = 1;
+  treedata1_.nodes.resize(4);
+  treedata1_.nodes[0].id = 1;
+  treedata1_.nodes[0].child_ids.push_back(4);
+  treedata1_.nodes[0].child_ids.push_back(2);
+  treedata1_.nodes[0].child_ids.push_back(3);
+  treedata1_.nodes[1].id = 2;
+  treedata1_.nodes[2].id = 3;
+  treedata1_.nodes[3].id = 4;
+
+  CreateTreeSerializer();
+  AXTreeUpdate update;
+  serializer_->BeginSerializingChanges(&update);
+  EXPECT_EQ(0U, update.nodes.size());
+  ASSERT_TRUE(serializer_->SerializeOneChange(tree1_->GetFromId(1)));
+  EXPECT_EQ(2U, update.nodes.size());
+
+  // If we serialize the same node again, nothing should happen.
+  ASSERT_TRUE(serializer_->SerializeOneChange(tree1_->GetFromId(1)));
+  EXPECT_EQ(2U, update.nodes.size());
+
+  // If we serialize a different node, we should get it serialized.
+  ASSERT_TRUE(serializer_->SerializeOneChange(tree1_->GetFromId(2)));
+  EXPECT_EQ(3U, update.nodes.size());
+
+  serializer_->FinishSerializingChanges();
+}
+
 }  // namespace ui