Avoid illegal serializations with aria-owns and unincluded nodes

When adding children, make sure we only add aria-owned children to the
owning nodes. This avoids a common crash in chrome://bookmarks that
has a confusing crash stack. In order to do this, when
ComputeAccessibilityIsIgnored() is called, the relation cache
must already be updated with the knowledge that the object is owned.

Do not ever allow owned objects to ever have a "not included in tree"
parent. Therefore, they can never be added as second line children
via the recursion in InsertChild().

Also re-remove WalkAllDescendants(), which was it was recently
re-added as a band-aid for the chrome://bookmarks crashes.
Without this change, removing WalkAllDescendants() causes assertions
in AXTreeSerializer, when aria-owns points to an unincluded node,
that the same object is being added twice.

Finally, add better logging and DCHECKs that will help diagnose issues
more quickly.

Bug: 1100968, 1131848
Change-Id: I3bb5d069814e144d426b5c3de0991bc41123206f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2446200
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813955}

content/browser/accessibility/dump_accessibility_tree_browsertest.cc

@@ -947,6 +947,16 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityAriaOwns) {
   RunAriaTest(FILE_PATH_LITERAL("aria-owns.html"));
 }
 
+IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
+                       AccessibilityAriaOwnsIgnored) {
+  RunAriaTest(FILE_PATH_LITERAL("aria-owns-ignored.html"));
+}
+
+IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
+                       AccessibilityAriaOwnsIncludedInTree) {
+  RunAriaTest(FILE_PATH_LITERAL("aria-owns-included-in-tree.html"));
+}
+
 IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityAriaOwnsList) {
   RunAriaTest(FILE_PATH_LITERAL("aria-owns-list.html"));
 }
@@ -1478,6 +1488,10 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
   RunHtmlTest(FILE_PATH_LITERAL("contenteditable-with-no-descendants.html"));
 }
 
+IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityCustomElement) {
+  RunHtmlTest(FILE_PATH_LITERAL("custom-element.html"));
+}
+
 IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, AccessibilityEm) {
   RunHtmlTest(FILE_PATH_LITERAL("em.html"));
 }

content/test/data/accessibility/aria/aria-owns-ignored-expected-auralinux.txt

@@ -0,0 +1,14 @@
+[document web]
+++[separator] name='Ordinary presentation element with id' horizontal
+++[separator] name='An aria-owned element is never ignored' horizontal
+++[panel]
+++++[push button] name='button-in-owned-tree'
+++[separator] name='Element with aria-owns is never ignored' horizontal
+++[section]
+++++[entry] selectable-text
+++[separator] name='Owning an element with an ignored parent serializes cleanly' horizontal
+++[panel]
+++++[check box] checkable checkable:true
+++[separator] name='All the above in one' horizontal
+++[section]
+++++[slider] horizontal current=50.000000 minimum=0.000000 maximum=100.000000

content/test/data/accessibility/aria/aria-owns-ignored-expected-blink.txt

@@ -0,0 +1,26 @@
+rootWebArea
+++genericContainer ignored
+++++genericContainer ignored
+++++++splitter horizontal name='Ordinary presentation element with id'
+++++++presentational ignored
+++++++splitter horizontal name='An aria-owned element is never ignored'
+++++++group
+++++++++presentational ignored
+++++++++++button name='button-in-owned-tree'
+++++++++++++staticText name='button-in-owned-tree'
+++++++++++++++inlineTextBox name='button-in-owned-tree'
+++++++splitter horizontal name='Element with aria-owns is never ignored'
+++++++genericContainer
+++++++++textField
+++++++++++genericContainer
+++++++splitter horizontal name='Owning an element with an ignored parent serializes cleanly'
+++++++presentational ignored
+++++++++group
+++++++++++checkBox checkedState=false
+++++++++presentational ignored
+++++++splitter horizontal name='All the above in one'
+++++++presentational ignored
+++++++++genericContainer
+++++++++++presentational ignored
+++++++++++++slider horizontal value='50' valueForRange=50.00 minValueForRange=0.00 maxValueForRange=100.00
+++++++++++++++sliderThumb

content/test/data/accessibility/aria/aria-owns-ignored.html

@@ -0,0 +1,30 @@
+<!-- Make sure that the serialization/deserialization process works without
+  crashing when aria-owns is used with ignored nodes -->
+<hr aria-label="Ordinary presentation element with id">
+<div role="presentation" id="presentational0"></div>
+
+<hr aria-label="An aria-owned element is never ignored">
+<div role="group" aria-owns="presentational1"></div>
+<div role="presentation" id="presentational1">
+  <input type="button" value="button-in-owned-tree">
+</div>
+
+<hr aria-label="Element with aria-owns is never ignored">
+<input type="text" id="textbox">
+<div role="presentation" aria-owns="textbox"></div>
+
+<hr aria-label="Owning an element with an ignored parent serializes cleanly">
+<div role="presentation">
+  <div role="group" aria-owns="checkbox"></div>
+  <div role="presentation">
+    <input type="checkbox" id="checkbox">
+  </div>
+</div>
+
+<hr aria-label="All the above in one">
+<div role="presentation" id="presentational3">
+  <input type="range" id="range">
+</div>
+<div role="presentation">
+  <div role="presentation" aria-owns="presentational3"></div>
+</div>

content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-auralinux.txt

@@ -0,0 +1,21 @@
+[document web]
+++[separator] name='Control: elements that are not in tree' horizontal
+++[section]
+++++[section]
+++++++[section]
+++++++++[entry] selectable-text
+++[separator] name='An aria-owned element is always in tree' horizontal
+++[panel]
+++++[push button] name='button-in-owned-tree'
+++[separator] name='Element with aria-owns is always in tree' horizontal
+++[section]
+++++[entry] selectable-text
+++[separator] name='Owning an element with unincluded ancestors serializes cleanly' horizontal
+++[panel]
+++++[check box] checkable checkable:true
+++[separator] name='All the above in one' horizontal
+++[section]
+++++[section]
+++++++[static] name='xyz'
+++++[section]
+++++++[slider] horizontal current=50.000000 minimum=0.000000 maximum=100.000000

content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-blink.txt

@@ -0,0 +1,33 @@
+rootWebArea
+++genericContainer ignored
+++++genericContainer ignored
+++++++splitter horizontal name='Control: elements that are not in tree'
+++++++layoutTable
+++++++++layoutTableRow
+++++++++++layoutTableCell
+++++++++++++textField
+++++++++++++++genericContainer
+++++++splitter horizontal name='An aria-owned element is always in tree'
+++++++group
+++++++++genericContainer ignored
+++++++++++button name='button-in-owned-tree'
+++++++++++++staticText name='button-in-owned-tree'
+++++++++++++++inlineTextBox name='button-in-owned-tree'
+++++++splitter horizontal name='Element with aria-owns is always in tree'
+++++++genericContainer
+++++++++textField
+++++++++++genericContainer
+++++++splitter horizontal name='Owning an element with unincluded ancestors serializes cleanly'
+++++++group
+++++++++checkBox checkedState=false
+++++++splitter horizontal name='All the above in one'
+++++++none ignored
+++++++++genericContainer
+++++++++++genericContainer
+++++++++++++staticText name='xyz'
+++++++++++++++inlineTextBox name='xyz'
+++++++++++genericContainer
+++++++++++++slider horizontal value='50' valueForRange=50.00 minValueForRange=0.00 maxValueForRange=100.00
+++++++++++++++sliderThumb
+++++++none ignored
+++++++genericContainer ignored invisible

content/test/data/accessibility/aria/aria-owns-included-in-tree.html

@@ -0,0 +1,61 @@
+<!-- Make sure that the serialization/deserialization process works without
+  crashing when aria-owns is used with nodes that normally wouldn't be in the
+  tree, and that any target of an aria-owns is included in the tree -->
+<test-element></test-element>
+<template id="test-contents">
+  <hr aria-label="Control: elements that are not in tree">
+  <span id="presentational0"></span>  <!-- Should not be in tree -->
+  <table>
+    <tbody>  <!-- Should not be in tree -->
+      <td><input></td>
+    </tbody>
+  </table>
+
+  <hr aria-label="An aria-owned element is always in tree">
+  <span role="group" aria-owns="presentational1"></span>
+  <span id="presentational1">  <!-- Should be in tree -->
+    <input type="button" value="button-in-owned-tree">
+  </span>
+
+  <hr aria-label="Element with aria-owns is always in tree">
+  <input type="text" id="textbox">
+  <span aria-owns="textbox"></span>  <!-- Should be in tree -->
+
+  <hr aria-label="Owning an element with unincluded ancestors serializes cleanly">
+  <span>
+    <span role="group" aria-owns="checkbox"></span>
+    <span>
+      <span>
+        <input type="checkbox" id="checkbox">
+      </span>
+    </span>
+  </span>
+
+  <hr aria-label="All the above in one">
+  <table role="none">
+    <tbody aria-owns="td">
+      <td>xyz</td>
+    </tbody>
+  </table>
+  <table role="none">
+    <tbody>  <!-- Not in tree -->
+      <td id="td">
+        <span>
+          <input type="range">
+        </span>
+      </td>
+    </tbody>
+  </table>
+</template>
+<script>
+  class TestElement extends HTMLElement {
+    constructor() {
+      super();
+
+      const template = document.getElementById('test-contents');
+      const instance = template.content.cloneNode(true);
+      this.attachShadow({mode: 'open'}).appendChild(instance);
+    }
+  }
+  customElements.define('test-element', TestElement);
+</script>

content/test/data/accessibility/html/custom-element-expected-blink.txt

@@ -0,0 +1,13 @@
+rootWebArea
+++genericContainer ignored
+++++genericContainer ignored
+++++++splitter horizontal name='Outside custom element'
+++++++genericContainer
+++++++++presentational ignored
+++++++++++staticText name='a'
+++++++++++++inlineTextBox name='a'
+++++++splitter horizontal name='Inside custom element'
+++++++genericContainer ignored
+++++++++presentational ignored
+++++++++++staticText name='a'
+++++++++++++inlineTextBox name='a'

content/test/data/accessibility/html/custom-element.html

@@ -0,0 +1,21 @@
+<hr aria-label="Outside custom element">
+<div id="test-contents">
+  <div role="presentation">
+    <span>a</span>
+    <span style="display:none">b</span>
+    <span style="visibility:hidden">c</span>
+  </div>
+</div>
+<hr aria-label="Inside custom element">
+<test-element></test-element>
+<script>
+  class TestElement extends HTMLElement {
+    constructor() {
+      super();
+
+      const testContents = document.getElementById('test-contents');
+      this.attachShadow({mode: 'open'}).innerHTML = testContents.outerHTML;
+    }
+  }
+  customElements.define('test-element', TestElement);
+</script>

third_party/blink/renderer/modules/accessibility/ax_node_object.cc

@@ -3396,14 +3396,11 @@ void AXNodeObject::AddChildren() {
          child = LayoutTreeBuilderTraversal::NextSibling(*child)) {
       if (child->IsMarkerPseudoElement() && AccessibilityIsIgnored())
         continue;
-      AXObject* child_obj = AXObjectCache().GetOrCreate(child);
-      if (child_obj && !AXObjectCache().IsAriaOwned(child_obj))
-        AddChild(child_obj);
+      AddChild(AXObjectCache().GetOrCreate(child));
     }
   } else {
     for (AXObject* obj = RawFirstChild(); obj; obj = obj->RawNextSibling()) {
-      if (!AXObjectCache().IsAriaOwned(obj))
-        AddChild(obj);
+      AddChild(obj);
     }
   }
 
@@ -3417,7 +3414,7 @@ void AXNodeObject::AddChildren() {
   AddAccessibleNodeChildren();
 
   for (const auto& owned_child : owned_children)
-    AddChild(owned_child);
+    AddChild(owned_child, true);
 
   for (const auto& child : children_) {
     if (!child->CachedParentObject())
@@ -3425,16 +3422,29 @@ void AXNodeObject::AddChildren() {
   }
 }
 
-void AXNodeObject::AddChild(AXObject* child) {
+void AXNodeObject::AddChild(AXObject* child, bool is_from_aria_owns) {
   unsigned int index = children_.size();
-  InsertChild(child, index);
+  if (child)
+    InsertChild(child, index, is_from_aria_owns);
 }
 
-void AXNodeObject::InsertChild(AXObject* child, unsigned index) {
+void AXNodeObject::InsertChild(AXObject* child,
+                               unsigned index,
+                               bool is_from_aria_owns) {
   if (!child || !CanHaveChildren())
     return;
 
+  if (is_from_aria_owns) {
+    DCHECK(AXObjectCache().IsAriaOwned(child));
+  } else {
+    // Don't add an aria-owned child to its natural parent, because it will
+    // already be the child of the element with aria-owns.
+    if (AXObjectCache().IsAriaOwned(child))
+      return;
+  }
+
   if (!child->AccessibilityIsIncludedInTree()) {
+    DCHECK(!is_from_aria_owns) << "Owned elements smust be in tree";
     // Child is ignored and not in the tree.
     // Recompute the child's children now as we skip over the ignored object.
     child->SetNeedsToUpdateChildren();
@@ -3443,8 +3453,13 @@ void AXNodeObject::InsertChild(AXObject* child, unsigned index) {
     // unignored descendants as it goes.
     const auto& children = child->ChildrenIncludingIgnored();
     wtf_size_t length = children.size();
-    for (wtf_size_t i = 0; i < length; ++i)
-      children_.insert(index + i, children[i]);
+    for (wtf_size_t i = 0; i < length; ++i) {
+      // If the child was owned, it will be added elsewhere as a direct
+      // child of the object owning it, and not as an indirect child under
+      // an object not included in the tree.
+      if (!AXObjectCache().IsAriaOwned(children[i]))
+        children_.insert(index + i, children[i]);
+    }
   } else if (!child->IsMenuListOption()) {
     // MenuListOptions must only be added in AXMenuListPopup::AddChildren.
     children_.insert(index, child);

third_party/blink/renderer/modules/accessibility/ax_node_object.h

@@ -221,8 +221,12 @@ class MODULES_EXPORT AXNodeObject : public AXObject {
   void AddChildren() override;
 
   bool CanHaveChildren() const override;
-  void AddChild(AXObject*);
-  void InsertChild(AXObject*, unsigned index);
+  // Set is_from_aria_owns to true if the child is being added because it was
+  // pointed to from aria-owns.
+  void AddChild(AXObject*, bool is_from_aria_owns = false);
+  // Set is_from_aria_owns to true if the child is being insert because it was
+  // pointed to from aria-owns.
+  void InsertChild(AXObject*, unsigned index, bool is_from_aria_owns = false);
   void ClearChildren() override;
   bool NeedsToUpdateChildren() const override { return children_dirty_; }
   void SetNeedsToUpdateChildren() override { children_dirty_ = true; }

third_party/blink/renderer/modules/accessibility/ax_object.cc

@@ -1643,6 +1643,12 @@ const AXObject* AXObject::DisabledAncestor() const {
 }
 
 bool AXObject::ComputeAccessibilityIsIgnoredButIncludedInTree() const {
+  if (AXObjectCache().IsAriaOwned(this)) {
+    // Always include an aria-owned object. It must be a child of the
+    // element with aria-owns.
+    return true;
+  }
+
   if (!GetNode())
     return false;
 

third_party/blink/renderer/modules/accessibility/ax_relation_cache.cc

@@ -152,6 +152,7 @@ void AXRelationCache::UpdateAriaOwnsFromAttrAssociatedElements(
   // attr-associated elements have already had their scope validated, but they
   // need to be further validated to determine if they introduce a cycle or are
   // already owned by another element.
+
   Vector<String> owned_id_vector;
   for (const auto& element : attr_associated_elements) {
     AXObject* child = GetOrCreate(element);
@@ -225,6 +226,13 @@ void AXRelationCache::UpdateAriaOwnerToChildrenMapping(
   // Finally, update the mapping from the owner to the list of child IDs.
   aria_owner_to_children_mapping_.Set(owner->AXObjectID(),
                                       validated_owned_child_axids);
+
+#if DCHECK_IS_ON()
+  // Owned children must be in tree to avoid serialization issues.
+  for (AXObject* child : validated_owned_children_result) {
+    DCHECK(child->AccessibilityIsIncludedInTree());
+  }
+#endif
 }
 
 bool AXRelationCache::MayHaveHTMLLabelViaForAttribute(

third_party/blink/web_tests/accessibility/aria-owns-presentational-node-edge-cases.html

@@ -0,0 +1,30 @@
+<!DOCTYPE HTML>
+<script src="../resources/testharness.js"></script>
+<script src="../resources/testharnessreport.js"></script>
+
+<div role="presentation">
+    <div id="parent"> <!-- Will get aria-owns="2" -->
+    </div>
+    <div id="2" role="presentation">
+        <div id="3" role="button"></div>
+        <div id="4" role="button"></div>
+    </div>
+</div>
+
+<script>
+test(function(t)
+{
+    const axParent = accessibilityController.accessibleElementById("parent");
+    const axRoot = axParent.parentElement();
+    assert_equals(axRoot.role, "AXRole: AXPresentational");
+    assert_equals(axRoot.childrenCount, 2);
+
+    document.getElementById('parent').setAttribute('aria-owns', '2');
+    assert_equals(axParent.childrenCount, 1);
+    assert_equals(axRoot.childrenCount, 1);  // Root no longer owns #2.
+
+    const axChild = axParent.childAtIndex(0);
+    assert_equals(axChild.role, "AXRole: AXPresentational");
+}, "Aria-owns cannot be fooled by pointing it at a presentational node.");
+</script>
+

ui/accessibility/ax_tree_serializer.h

@@ -178,9 +178,6 @@ class AXTreeSerializer {
       AXSourceNode node,
       AXTreeUpdateBase<AXNodeData, AXTreeData>* out_update);
 
-  // Visit all of the descendants of |node| once.
-  void WalkAllDescendants(AXSourceNode node);
-
   // Delete the entire client subtree but don't set the did_reset_ flag
   // like when Reset() is called.
   void InternalReset();
@@ -459,12 +456,6 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::SerializeChanges(
   if (!tree_->IsValid(lca))
     lca = tree_->GetRoot();
 
-  // Work around flaky source trees where nodes don't figure out their
-  // correct parent/child relationships until you walk the whole tree once.
-  // Covered by this test in the content_browsertests suite:
-  //     DumpAccessibilityTreeTest.AccessibilityAriaOwns.
-  WalkAllDescendants(lca);
-
   if (!SerializeChangedNodes(lca, out_update))
     return false;
 
@@ -588,12 +579,23 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
 
     ClientTreeNode* client_child = ClientTreeNodeById(new_child_id);
     if (client_child && GetClientTreeNodeParent(client_child) != client_node) {
-      DVLOG(1) << "Illegal reparenting detected";
-#if defined(ADDRESS_SANITIZER)
+#if defined(ADDRESS_SANITIZER) || !defined(NDEBUG)
       // Wrapping this in ADDRESS_SANITIZER will cause it to run on
-      // clusterfuzz, which should help us narrow down the issue.
-      NOTREACHED() << "Illegal reparenting detected";
+      // clusterfuzz, which should help us narrow down remaining issues.
+      // TODO(accessibility) Remove all cases where this occurs and re-add
+      // NOTREACHED(), or add a histogram.
+      // This condition leads to performance problems. It will
+      // also reset virtual buffers, causing users to lose their place.
+      NOTREACHED()
+#else
+      LOG(ERROR)
 #endif
+          << "Illegal reparenting detected: "
+          << "\nPassed-in parent: "
+          << tree_->GetDebugString(tree_->GetFromId(client_node->id))
+          << "\nChild: " << tree_->GetDebugString(child) << "\nChild's parent: "
+          << tree_->GetDebugString(tree_->GetFromId(client_child->parent->id))
+          << "\n-----------------------------------------\n\n\n";
       Reset();
       return false;
     }
@@ -672,6 +674,28 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
       new_child->ignored = tree_->IsIgnored(child);
       new_child->invalid = false;
       client_node->children.push_back(new_child);
+      if (ClientTreeNodeById(child_id)) {
+#if defined(ADDRESS_SANITIZER) || !defined(NDEBUG)
+        // Wrapping this in ADDRESS_SANITIZER will cause it to run on
+        // clusterfuzz, which should help us narrow down remaining issues.
+        // TODO(accessibility) Remove all cases where this occurs and re-add
+        // NOTREACHED(), or add a histogram.
+        // This condition leads to performance problems. It will
+        // also reset virtual buffers, causing users to lose their place.
+        NOTREACHED()
+#else
+        LOG(ERROR)
+#endif
+            << "Child id " << child_id << " already exists in map."
+            << "\nChild is "
+            << tree_->GetDebugString(tree_->GetFromId(child_id))
+            << "\nWanted as child of parent " << tree_->GetDebugString(node)
+            << "\nAlready had parent "
+            << tree_->GetDebugString(
+                   tree_->GetFromId(ClientTreeNodeById(child_id)->parent->id));
+        Reset();
+        return false;
+      }
       client_id_map_[child_id] = new_child;
       if (!SerializeChangedNodes(child, out_update))
         return false;
@@ -686,15 +710,6 @@ bool AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::
   return true;
 }
 
-template <typename AXSourceNode, typename AXNodeData, typename AXTreeData>
-void AXTreeSerializer<AXSourceNode, AXNodeData, AXTreeData>::WalkAllDescendants(
-    AXSourceNode node) {
-  std::vector<AXSourceNode> children;
-  tree_->GetChildren(node, &children);
-  for (size_t i = 0; i < children.size(); ++i)
-    WalkAllDescendants(children[i]);
-}
-
 }  // namespace ui
 
 #endif  // UI_ACCESSIBILITY_AX_TREE_SERIALIZER_H_