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

This reverts commit 714e3f7d.

Reason for revert: reproducible crash: http://crbug.com/847276

Original change's description:
> Reland "Re-land: Avoid serializing the same accessibility node twice in the same message"
>
> Reverted second time speculatively to see if it was related to
> failures on Win10 Tests x64 (dbg), but it didn't seem related.
>
> This reverts commit 50da791e.
>
> Original change's description:
> > Revert "Re-land: Avoid serializing the same accessibility node twice in the same message"
> >
> > Speculative revert, see build failures on Win10 Tests x64 (dbg):
> > https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win10%20Tests%20x64%20%28dbg%29/803
> >
> > Bug: http://crbug.com/846837
> >
> > This reverts commit 983e926b.
> >
> > Reason for revert: <INSERT REASONING HERE>
> >
> > Original change's description:
> > > 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/1072875
> > > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
> > > Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#561913}
> >
> > TBR=dmazzoni@chromium.org,dtseng@chromium.org
> >
> > Change-Id: I5cbf8006e4917a9c008143ae4b746543d2b038c2
> > No-Presubmit: true
> > No-Tree-Checks: true
> > No-Try: true
> > Bug: 651614, 845778, 846837
> > Reviewed-on: https://chromium-review.googlesource.com/1074050
> > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
> > Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#562043}
>
> TBR=dmazzoni@chromium.org,dtseng@chromium.org
>
> Change-Id: I3dc8009836eb084271a3ac5239311008dc17105b
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: http://crbug.com/846837, 651614, 845778, 846837
> Reviewed-on: https://chromium-review.googlesource.com/1074587
> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
> Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#562118}

TBR=dmazzoni@chromium.org,dtseng@chromium.org

NOTRY=true

Bug: http://crbug.com/846837, 651614, 845778, 846837, 847276
Change-Id: I342db1180d09bc9eaa1a9c8fc7054fa2ca366eb6
Reviewed-on: https://chromium-review.googlesource.com/1079090
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563061}

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

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

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

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

content/renderer/accessibility/render_accessibility_impl.cc

@@ -454,15 +454,8 @@ void RenderAccessibilityImpl::SendPendingAccessibilityEvents() {
     if (plugin_tree_source_)
       event_msg.update.has_tree_data = true;
 
-    if (event_msgs.size() == 0)
-      serializer_.BeginSerializingChanges(&event_msg.update);
-
-    if (!serializer_.SerializeOneChange(obj)) {
+    if (!serializer_.SerializeChanges(obj, &event_msg.update)) {
       VLOG(1) << "Failed to serialize one accessibility event.";
-
-      if (event_msgs.size() == 0)
-        serializer_.FinishSerializingChanges();
-
       continue;
     }
 
@@ -488,9 +481,6 @@ 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,23 +76,10 @@ 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. 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.
+  // ids or changing during serialization.
   bool SerializeChanges(AXSourceNode node,
                         AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update);
 
@@ -165,7 +152,9 @@ class AXTreeSerializer {
   void DeleteClientSubtree(ClientTreeNode* client_node);
 
   // Helper function, called recursively with each new node to serialize.
-  bool SerializeChangedNodes(AXSourceNode node);
+  bool SerializeChangedNodes(
+      AXSourceNode node,
+      AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update);
 
   // Visit all of the descendants of |node| once.
   void WalkAllDescendants(AXSourceNode node);
@@ -182,14 +171,6 @@ 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;
@@ -339,25 +320,16 @@ AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::ClientTreeNodeById(
 }
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
-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_);
-
+bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges(
+    AXSourceNode node,
+    AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update) {
   // Send the tree data if it's changed since the last update, or if
-  // current_update_->has_tree_data is already set to true.
+  // out_update->has_tree_data is already set to true.
   AXTreeData new_tree_data;
   if (tree_->GetTreeData(&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;
+      (out_update->has_tree_data || new_tree_data != client_tree_data_)) {
+    out_update->has_tree_data = true;
+    out_update->tree_data = new_tree_data;
     client_tree_data_ = new_tree_data;
   }
 
@@ -385,13 +357,13 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeOneChange(
       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.
-        current_update_->node_id_to_clear = client_root_->id;
+        out_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.
-        current_update_->node_id_to_clear = tree_->GetId(lca);
+        out_update->node_id_to_clear = tree_->GetId(lca);
         ClientTreeNode* client_lca = ClientTreeNodeById(tree_->GetId(lca));
         CHECK(client_lca);
         DeleteClientSubtree(client_lca);
@@ -409,34 +381,13 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeOneChange(
   //     DumpAccessibilityTreeTest.AccessibilityAriaOwns.
   WalkAllDescendants(lca);
 
-  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;
+  return SerializeChangedNodes(lca, out_update);
 }
 
 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);
@@ -455,7 +406,9 @@ void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
 bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
-    SerializeChangedNodes(AXSourceNode node) {
+    SerializeChangedNodes(
+        AXSourceNode node,
+        AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update) {
   // This method has three responsibilities:
   // 1. Serialize |node| into an AXNodeData, and append it to
   //    the AXTreeUpdate to be sent to the client.
@@ -469,10 +422,6 @@ 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();
@@ -491,7 +440,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 || current_update_->nodes.size() < max_node_count_) {
+  if (max_node_count_ == 0 || out_update->nodes.size() < max_node_count_) {
     tree_->GetChildren(node, &children);
   } else if (max_node_count_ > 0) {
     static bool logged_once = false;
@@ -538,18 +487,17 @@ 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 = current_update_->nodes.size();
-  current_update_->nodes.push_back(AXNodeData());
+  size_t serialized_node_index = out_update->nodes.size();
+  out_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 =
-        &current_update_->nodes[serialized_node_index];
+    AXNodeData* serialized_node = &out_update->nodes[serialized_node_index];
 
     tree_->SerializeNode(node, serialized_node);
     if (serialized_node->id == client_root_->id)
-      current_update_->root_id = serialized_node->id;
+      out_update->root_id = serialized_node->id;
   }
 
   // Iterate over the children, serialize them, and update the ClientTreeNode
@@ -579,14 +527,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))
+      if (!SerializeChangedNodes(child, out_update))
         return false;
     }
   }
 
   // Finally, update the child ids of this node to reflect the actual child
   // ids that were valid during serialization.
-  current_update_->nodes[serialized_node_index].child_ids.swap(
+  out_update->nodes[serialized_node_index].child_ids.swap(
       actual_serialized_node_child_ids);
 
   return true;

ui/accessibility/ax_tree_serializer_unittest.cc

@@ -11,7 +11,6 @@
 
 #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"
@@ -344,96 +343,4 @@ 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