Skip to content

GitLab

T
tangled
Commit 66559848 authored by Dominic Mazzoni's avatar Dominic Mazzoni Committed by Commit Bot
Browse files
Options

Fix issue with accessibility events on hidden nodes 

This is an issue with AXObjects that are ignored and not
included in the accessibility tree. When we fire an event
on such a node, the serializer owned by
RenderAccessibilityImpl first walks up to find a common
ancestor that is in the tree, and then serializes that
node, even if it was already serialized. It breaks the
mechanism that avoids re-serializing nodes that were
already just serialized.

In extreme circumstances, this led to a bug where the
size of the AXTreeUpdate for a large table grew to 200MB
for one particular website, even though the resulting
AXTree had only around 2MB worth of data.

The fix is simple: when a node changes, only fire a
notification on the nearest ancestor that's in the tree.
That way the serializer can avoid redundantly processing
the same node more than once.

Covered by a new test and a DCHECK that asserts that we
should never get an AXTreeUpdate with more nodes than the
size of the resulting tree. Without the new fix, the test
fails with an AXTreeUpdate containing 1000 nodes with a
tree of size 10.

Bug: 1066880

Change-Id: I247ad6aecaaf45b02371d2dad430b50489a86fa2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2134848
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: default avatarAdam Ettenberger <Adam.Ettenberger@microsoft.com>
Reviewed-by: default avatarAaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756970}
parent 5c5e175b
Hide whitespace changes
Inline Side-by-side
Showing with 85 additions and 0 deletions
content/browser/accessibility/browser_accessibility_manager.cc
View file @ 66559848
...@@ -405,6 +405,13 @@ bool BrowserAccessibilityManager::OnAccessibilityEvents( ...@@ -405,6 +405,13 @@ bool BrowserAccessibilityManager::OnAccessibilityEvents(
} }
return false; return false;
} }
// It's a bug if we got an update containing more nodes than
// the size of the resulting tree. If Unserialize succeeded that
// means a node just got repeated or something harmless like that,
// but it should still be investigated and could be the sign of a
// performance issue.
DCHECK_LE(int{tree_update.nodes.size()}, ax_tree()->size());
} }
// If this page is hidden by an interstitial, suppress all events. // If this page is hidden by an interstitial, suppress all events.
......
content/browser/accessibility/dump_accessibility_tree_browsertest.cc
View file @ 66559848
...@@ -2385,6 +2385,10 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, DISABLED_XmlInIframeCrash) { ...@@ -2385,6 +2385,10 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, DISABLED_XmlInIframeCrash) {
RunRegressionTest(FILE_PATH_LITERAL("xml-in-iframe-crash.html")); RunRegressionTest(FILE_PATH_LITERAL("xml-in-iframe-crash.html"));
} }
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, HiddenTable) {
RunRegressionTest(FILE_PATH_LITERAL("hidden-table.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
LanguageDetectionLangAttribute) { LanguageDetectionLangAttribute) {
RunLanguageDetectionTest(FILE_PATH_LITERAL("lang-attribute.html")); RunLanguageDetectionTest(FILE_PATH_LITERAL("lang-attribute.html"));
......
content/renderer/accessibility/render_accessibility_impl.cc
View file @ 66559848
...@@ -802,6 +802,14 @@ void RenderAccessibilityImpl::SendPendingAccessibilityEvents() { ...@@ -802,6 +802,14 @@ void RenderAccessibilityImpl::SendPendingAccessibilityEvents() {
// parts of the code as well, so we need to ensure the object still exists. // parts of the code as well, so we need to ensure the object still exists.
if (!obj.UpdateLayoutAndCheckValidity()) if (!obj.UpdateLayoutAndCheckValidity())
continue; continue;
// If the object in question is not included in the tree, get the
// nearest ancestor that is (ParentObject() will do this for us).
// Otherwise this can lead to the serializer doing extra work because
// the object won't be in |already_serialized_ids|.
if (!obj.AccessibilityIsIncludedInTree())
obj = obj.ParentObject();
if (already_serialized_ids.find(obj.AxID()) != already_serialized_ids.end()) if (already_serialized_ids.find(obj.AxID()) != already_serialized_ids.end())
continue; continue;
......
content/test/data/accessibility/regression/hidden-table-expected-blink.txt 0 → 100644
View file @ 66559848
rootWebArea
++genericContainer ignored
++++table
++++++row
++++++++columnHeader name='Header 1'
++++++++++staticText name='Header 1'
++++++++++++inlineTextBox name='Header 1'
++++++++columnHeader name='Header 2'
++++++++++staticText name='Header 2'
++++++++++++inlineTextBox name='Header 2'
++++genericContainer
++++++staticText name='Done'
++++++++inlineTextBox name='Done'
content/test/data/accessibility/regression/hidden-table.html 0 → 100644
View file @ 66559848
<!doctype html>
<!--
@WAIT-FOR:Done
-->
<body>
<table>
<thead>
<tr>
<th>Header 1</th>
<th>Header 2</th>
</tr>
</thead>
<tbody hidden>
</tbody>
<tfoot>
</tfoot>
</table>
<script>
function addRow(container) {
let row = document.createElement('tr');
container.appendChild(row);
for (let j = 0; j < 100; j++) {
let cell = document.createElement('td');
cell.innerHTML = Math.random();
row.appendChild(cell);
cell.tabIndex = -1;
}
}
setTimeout(() => {
// Now add a bunch of rows to the tbody, which is hidden.
// Every one of those added cells changes its tabIndex, which
// triggers a call to MarkAXObjectDirty on a node that's not
// included in the accessibility tree. That caused a bug where
// all of the real nodes of the table keep getting re-serialized.
// This is now caught by a DCHECK in BrowserAccessibilityManager
// that an AXTreeUpdate shouldn't be larger than the resulting tree.
for (let i = 0; i < 10; i++) {
addRow(document.querySelector('tbody'));
}
// Finally, clear out all of those extra rows and finish
// the test.
setTimeout(() => {
document.querySelector('tbody').innerHTML = '';
let done = document.createElement('div');
done.innerHTML = 'Done';
document.body.appendChild(done);
}, 10);
}, 10);
</script>
</body>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment