Invalidate client subtrees in AXTreeSerializer

In two recent changes, we added some calls to
AXTreeSerializer::DeleteClientSubtree. This is often a great way to
ensure that portions of the tree are re-serialized.

However, what can happen is that there can be reparenting within a subtree,
and the client doesn't clear out the subtree first before unserializing it,
leading to an unserialization error.

This change introduces the concept of invalidating a subtree
instead of deleting it. That ensures the correct nodes are
re-serialized, while also allowing the code that checks for
reparenting.

Includes a regression test.

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

chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.cc

@@ -204,7 +204,7 @@ void AXTreeSourceArc::NotifyAccessibilityEvent(AXEventData* event_data) {
 
   event_bundle.updates.emplace_back();
   if (event_data->event_type == AXEventType::WINDOW_CONTENT_CHANGED) {
-    current_tree_serializer_->DeleteClientSubtree(
+    current_tree_serializer_->InvalidateSubtree(
         GetFromId(event_data->source_id));
   }
   current_tree_serializer_->SerializeChanges(GetFromId(event_data->source_id),

content/browser/accessibility/dump_accessibility_tree_browsertest.cc

@@ -1639,6 +1639,10 @@ IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest, AccessibilityQ) {
   RunHtmlTest(FILE_PATH_LITERAL("q.html"));
 }
 
+IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest, AccessibilityReparentCrash) {
+  RunHtmlTest(FILE_PATH_LITERAL("reparent-crash.html"));
+}
+
 IN_PROC_BROWSER_TEST_F(DumpAccessibilityTreeTest, AccessibilityReplaceData) {
   RunHtmlTest(FILE_PATH_LITERAL("replace-data.html"));
 }

content/renderer/accessibility/render_accessibility_impl.cc

@@ -274,7 +274,7 @@ void RenderAccessibilityImpl::HandleAXEvent(const blink::WebAXObject& obj,
   // Force the newly focused node to be re-serialized so we include its
   // inline text boxes.
   if (event == ax::mojom::Event::kFocus)
-    serializer_.DeleteClientSubtree(obj);
+    serializer_.InvalidateSubtree(obj);
 #endif
 
   // If some cell IDs have been added or removed, we need to update the whole
@@ -283,7 +283,7 @@ void RenderAccessibilityImpl::HandleAXEvent(const blink::WebAXObject& obj,
       event == ax::mojom::Event::kChildrenChanged) {
     WebAXObject table_like_object = obj.ParentObject();
     if (!table_like_object.IsDetached()) {
-      serializer_.DeleteClientSubtree(table_like_object);
+      serializer_.InvalidateSubtree(table_like_object);
       HandleAXEvent(table_like_object, ax::mojom::Event::kChildrenChanged);
     }
   }
@@ -294,7 +294,7 @@ void RenderAccessibilityImpl::HandleAXEvent(const blink::WebAXObject& obj,
       event == ax::mojom::Event::kChildrenChanged) {
     WebAXObject popup_like_object = obj.ParentObject();
     if (!popup_like_object.IsDetached()) {
-      serializer_.DeleteClientSubtree(popup_like_object);
+      serializer_.InvalidateSubtree(popup_like_object);
       HandleAXEvent(popup_like_object, ax::mojom::Event::kChildrenChanged);
     }
   }
@@ -459,7 +459,7 @@ void RenderAccessibilityImpl::SendPendingAccessibilityEvents() {
       block = block.ParentObject();
     }
     if (!block.IsDetached() && !block.Equals(obj))
-      serializer_.DeleteClientSubtree(block);
+      serializer_.InvalidateSubtree(block);
 
     // Whenever there's a change within a table, invalidate the
     // whole table so that row and cell indexes are recomputed.
@@ -471,7 +471,7 @@ void RenderAccessibilityImpl::SendPendingAccessibilityEvents() {
              !ui::IsTableLikeRole(AXRoleFromBlink(table.Role())))
         table = table.ParentObject();
       if (!table.IsDetached())
-        serializer_.DeleteClientSubtree(table);
+        serializer_.InvalidateSubtree(table);
     }
 
     VLOG(1) << "Accessibility event: " << ui::ToString(event.event_type)
@@ -744,7 +744,7 @@ void RenderAccessibilityImpl::OnLoadInlineTextBoxes(
 
   // This object may not be a leaf node. Force the whole subtree to be
   // re-serialized.
-  serializer_.DeleteClientSubtree(obj);
+  serializer_.InvalidateSubtree(obj);
 
   // Explicitly send a tree change update event now.
   HandleAXEvent(obj, ax::mojom::Event::kTreeChanged);
@@ -763,7 +763,7 @@ void RenderAccessibilityImpl::OnGetImageData(
   if (document.IsNull())
     return;
 
-  serializer_.DeleteClientSubtree(obj);
+  serializer_.InvalidateSubtree(obj);
   HandleAXEvent(obj, ax::mojom::Event::kImageFrameUpdated);
 }
 

content/test/data/accessibility/html/reparent-crash-expected-blink.txt

+rootWebArea
+++listItem name='@NO_CHILDREN_DUMP'
+++genericContainer
+++++staticText name='Done'
+++++++inlineTextBox name='Done'

content/test/data/accessibility/html/reparent-crash.html

+<!--
+@WAIT-FOR:Done
+
+This is a regression test for a bug that caused a crash when an
+inline node was reparented. The actual test output is not
+interesting, all that matters is that it correctly waits for
+"Done" to appear in the accessibility tree and doesn't generate
+an invalid tree update in the process.
+-->
+
+<img id="cl"></img>
+<li aria-label="@NO_CHILDREN_DUMP">
+  <div id="aw"><pre id="d3"><object id="b" aria-hidden=true></object></pre></div>
+</li>
+<object id="z"><div id="b5"></div> <option></option></object>
+<div id="s">Status</div>
+
+<script>
+  $ = document.querySelector.bind(document);
+  $('#d3').appendChild($('#z'));
+  $('#b').offsetTop;
+  setInterval(function() {
+    $('#z').appendChild($('#cl'));
+  }, 1);
+  $('#z').removeChild($('#b5'));
+  setTimeout(function() {
+    $('#s').innerText = "Done";
+  }, 10);
+</script>

ui/accessibility/ax_tree_serializer.h

@@ -83,9 +83,10 @@ class AXTreeSerializer {
   bool SerializeChanges(AXSourceNode node,
                         AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update);
 
-  // Delete the client subtree for this node, ensuring that the subtree
-  // is re-serialized.
-  void DeleteClientSubtree(AXSourceNode node);
+  // Invalidate the subtree rooted at this node, ensuring that the whole
+  // subtree is re-serialized the next time any of those nodes end up
+  // being serialized.
+  void InvalidateSubtree(AXSourceNode node);
 
   // Only for unit testing. Normally this class relies on getting a call
   // to SerializeChanges() every time the source tree changes. For unit
@@ -134,8 +135,8 @@ class AXTreeSerializer {
 
   // Return the least common ancestor of |node| that's in the client tree.
   // This just walks up the ancestors of |node| until it finds a node that's
-  // also in the client tree, and then calls LeastCommonAncestor on the
-  // source node and client node.
+  // also in the client tree and not inside an invalid subtree, and then calls
+  // LeastCommonAncestor on the source node and client node.
   AXSourceNode LeastCommonAncestor(AXSourceNode node);
 
   // Walk the subtree rooted at |node| and return true if any nodes that
@@ -147,8 +148,10 @@ class AXTreeSerializer {
 
   ClientTreeNode* ClientTreeNodeById(int32_t id);
 
-  // Delete the given client tree node and recursively delete all of its
-  // descendants.
+  // Invalidate the subtree rooted at this node.
+  void InvalidateClientSubtree(ClientTreeNode* client_node);
+
+  // Delete the client subtree rooted at this node.
   void DeleteClientSubtree(ClientTreeNode* client_node);
 
   // Helper function, called recursively with each new node to serialize.
@@ -178,13 +181,14 @@ class AXTreeSerializer {
 
 // In order to keep track of what nodes the client knows about, we keep a
 // representation of the client tree - just IDs and parent/child
-// relationships.
+// relationships, and a marker indicating whether it's been invalidated.
 struct AX_EXPORT ClientTreeNode {
   ClientTreeNode();
   virtual ~ClientTreeNode();
   int32_t id;
   ClientTreeNode* parent;
   std::vector<ClientTreeNode*> children;
+  bool invalid;
 };
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
@@ -261,9 +265,18 @@ AXSourceNode
 AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::LeastCommonAncestor(
     AXSourceNode node) {
   // Walk up the tree until the source node's id also exists in the
-  // client tree, then call LeastCommonAncestor on those two nodes.
+  // client tree, whose parent is not invalid, then call LeastCommonAncestor
+  // on those two nodes.
+  //
+  // Note that it's okay if |client_node| is invalid - the LCA can be the
+  // root of an invalid subtree, since we're going to serialize the
+  // LCA. But it's not okay if |client_node->parent| is invalid - that means
+  // that we're inside of an invalid subtree that all needs to be
+  // re-serialized, so the LCA should be higher.
   ClientTreeNode* client_node = ClientTreeNodeById(tree_->GetId(node));
-  while (tree_->IsValid(node) && !client_node) {
+  while (
+      tree_->IsValid(node) &&
+      (!client_node || (client_node->parent && client_node->parent->invalid))) {
     node = tree_->GetParent(node);
     if (tree_->IsValid(node))
       client_node = ClientTreeNodeById(tree_->GetId(node));
@@ -385,12 +398,19 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges(
 }
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
-void AXTreeSerializer<AXSourceNode,
-                      AXNodeData,
-                      AXTreeData>::DeleteClientSubtree(AXSourceNode node) {
+void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::InvalidateSubtree(
+    AXSourceNode node) {
   ClientTreeNode* client_node = ClientTreeNodeById(tree_->GetId(node));
   if (client_node)
-    DeleteClientSubtree(client_node);
+    InvalidateClientSubtree(client_node);
+}
+
+template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
+void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
+    InvalidateClientSubtree(ClientTreeNode* client_node) {
+  client_node->invalid = true;
+  for (size_t i = 0; i < client_node->children.size(); ++i)
+    InvalidateClientSubtree(client_node->children[i]);
 }
 
 template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
@@ -432,6 +452,9 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
     client_id_map_[client_node->id] = client_node;
   }
 
+  // We're about to serialize it, so mark it as valid.
+  client_node->invalid = false;
+
   // Iterate over the ids of the children of |node|.
   // Create a set of the child ids so we can quickly look
   // up which children are new and which ones were there before.
@@ -521,10 +544,17 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
     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);
+      // Re-serialize it if the child is marked as invalid, otherwise
+      // we don't have to because the client already has it.
+      if (reused_child->invalid) {
+        if (!SerializeChangedNodes(child, out_update))
+          return false;
+      }
     } else {
       ClientTreeNode* new_child = new ClientTreeNode();
       new_child->id = child_id;
       new_child->parent = client_node;
+      new_child->invalid = false;
       client_node->children.push_back(new_child);
       client_id_map_[child_id] = new_child;
       if (!SerializeChangedNodes(child, out_update))