From 6a69beb22f124e520918f34b84d28514f1bae797 Mon Sep 17 00:00:00 2001
From: Aaron Leventhal <aleventhal@google.com>
Date: Mon, 5 Oct 2020 22:11:35 +0000
Subject: [PATCH] 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}
---
.../dump_accessibility_tree_browsertest.cc | 14 +++++
.../aria-owns-ignored-expected-auralinux.txt | 14 +++++
.../aria/aria-owns-ignored-expected-blink.txt | 26 ++++++++
.../accessibility/aria/aria-owns-ignored.html | 30 +++++++++
...ns-included-in-tree-expected-auralinux.txt | 21 +++++++
...a-owns-included-in-tree-expected-blink.txt | 33 ++++++++++
.../aria/aria-owns-included-in-tree.html | 61 +++++++++++++++++++
.../html/custom-element-expected-blink.txt | 13 ++++
.../accessibility/html/custom-element.html | 21 +++++++
.../modules/accessibility/ax_node_object.cc | 37 +++++++----
.../modules/accessibility/ax_node_object.h | 8 ++-
.../modules/accessibility/ax_object.cc | 6 ++
.../accessibility/ax_relation_cache.cc | 8 +++
...a-owns-presentational-node-edge-cases.html | 30 +++++++++
ui/accessibility/ax_tree_serializer.h | 59 +++++++++++-------
15 files changed, 346 insertions(+), 35 deletions(-)
create mode 100644 content/test/data/accessibility/aria/aria-owns-ignored-expected-auralinux.txt
create mode 100644 content/test/data/accessibility/aria/aria-owns-ignored-expected-blink.txt
create mode 100644 content/test/data/accessibility/aria/aria-owns-ignored.html
create mode 100644 content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-auralinux.txt
create mode 100644 content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-blink.txt
create mode 100644 content/test/data/accessibility/aria/aria-owns-included-in-tree.html
create mode 100644 content/test/data/accessibility/html/custom-element-expected-blink.txt
create mode 100644 content/test/data/accessibility/html/custom-element.html
create mode 100644 third_party/blink/web_tests/accessibility/aria-owns-presentational-node-edge-cases.html
diff --git a/content/browser/accessibility/dump_accessibility_tree_browsertest.cc b/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
index 036ebd59355a2..1f07747678b8d 100644
--- a/content/browser/accessibility/dump_accessibility_tree_browsertest.cc
+++ b/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"));
}
diff --git a/content/test/data/accessibility/aria/aria-owns-ignored-expected-auralinux.txt b/content/test/data/accessibility/aria/aria-owns-ignored-expected-auralinux.txt
new file mode 100644
index 0000000000000..8068ae388c064
--- /dev/null
+++ b/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
diff --git a/content/test/data/accessibility/aria/aria-owns-ignored-expected-blink.txt b/content/test/data/accessibility/aria/aria-owns-ignored-expected-blink.txt
new file mode 100644
index 0000000000000..1ce7785e4de7d
--- /dev/null
+++ b/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
diff --git a/content/test/data/accessibility/aria/aria-owns-ignored.html b/content/test/data/accessibility/aria/aria-owns-ignored.html
new file mode 100644
index 0000000000000..217a66da5ba1d
--- /dev/null
+++ b/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>
diff --git a/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-auralinux.txt b/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-auralinux.txt
new file mode 100644
index 0000000000000..cb22e5f98fcf2
--- /dev/null
+++ b/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
diff --git a/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-blink.txt b/content/test/data/accessibility/aria/aria-owns-included-in-tree-expected-blink.txt
new file mode 100644
index 0000000000000..4176f605f93cb
--- /dev/null
+++ b/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
diff --git a/content/test/data/accessibility/aria/aria-owns-included-in-tree.html b/content/test/data/accessibility/aria/aria-owns-included-in-tree.html
new file mode 100644
index 0000000000000..c2d1c18ecd10e
--- /dev/null
+++ b/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>
diff --git a/content/test/data/accessibility/html/custom-element-expected-blink.txt b/content/test/data/accessibility/html/custom-element-expected-blink.txt
new file mode 100644
index 0000000000000..849b12aa37135
--- /dev/null
+++ b/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'
diff --git a/content/test/data/accessibility/html/custom-element.html b/content/test/data/accessibility/html/custom-element.html
new file mode 100644
index 0000000000000..4526dc6f79cda
--- /dev/null
+++ b/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>
diff --git a/third_party/blink/renderer/modules/accessibility/ax_node_object.cc b/third_party/blink/renderer/modules/accessibility/ax_node_object.cc
index cc644926e0b0b..17fdb1690d9ce 100644
--- a/third_party/blink/renderer/modules/accessibility/ax_node_object.cc
+++ b/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);
diff --git a/third_party/blink/renderer/modules/accessibility/ax_node_object.h b/third_party/blink/renderer/modules/accessibility/ax_node_object.h
index 0538e45266caa..3220246b4f47f 100644
--- a/third_party/blink/renderer/modules/accessibility/ax_node_object.h
+++ b/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; }
diff --git a/third_party/blink/renderer/modules/accessibility/ax_object.cc b/third_party/blink/renderer/modules/accessibility/ax_object.cc
index daae2412f4d2c..1e6f995ddc193 100644
--- a/third_party/blink/renderer/modules/accessibility/ax_object.cc
+++ b/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;
diff --git a/third_party/blink/renderer/modules/accessibility/ax_relation_cache.cc b/third_party/blink/renderer/modules/accessibility/ax_relation_cache.cc
index cb5f72fe40fbd..36480b95ef2eb 100644
--- a/third_party/blink/renderer/modules/accessibility/ax_relation_cache.cc
+++ b/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(
diff --git a/third_party/blink/web_tests/accessibility/aria-owns-presentational-node-edge-cases.html b/third_party/blink/web_tests/accessibility/aria-owns-presentational-node-edge-cases.html
new file mode 100644
index 0000000000000..d521f7aeaa79c
--- /dev/null
+++ b/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>
+
diff --git a/ui/accessibility/ax_tree_serializer.h b/ui/accessibility/ax_tree_serializer.h
index a8ea1e8dcff4c..f05530b49724f 100644
--- a/ui/accessibility/ax_tree_serializer.h
+++ b/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_