Skip to content

Commit

Permalink
M118: [A11y] When marking entire document dirty, don't repair parents
Browse files Browse the repository at this point in the history
This fixes a crash that can occur when marking a document dirty.

Description of crash:
MarkDocumentDirtyWithCleanLayout() iterates all nodes and
calls SetNeedsToUpdateChildren(). This performs additional work
to ensure that each node, all the way to the root, has a
"has dirty descendants" flag set, and walks the parent chain,
repairing missing parents if necessary. This extra loop is unnecessary
because each node in the tree will already be visited by
MarkDocumentDirtyWithCleanLayout(). The code path gets out of
control because the calls to RepairMissingParent() end up creating
objects as we are marking them all dirty, as well as updating
cached values on objects (this was not expected to occur until
UpdateTreeIfNeeded()). Finally, children changed events are fired
because of the newly created objects. All of these unexpected
occurrences result in methods being called on detached objects where
that is not expected.

Description of fix:
Avoid walking parent chains while marking the document dirty.
This fix may also be ported to later branches, however, these specific crashes have not been seen there yet. Either way, it is an improvement
to avoid the unnecessary parent chain loops while visiting each node.

Bug: 1480675, b/305236511, b/305967411, 1493459
Change-Id: Ifc1c803962bfcb98e49a986793d50470754a3580
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4951974
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Benjamin Beaudry <benjamin.beaudry@microsoft.com>
Cr-Commit-Position: refs/branch-heads/5993@{#1342}
Cr-Branched-From: 5113507-refs/heads/main@{#1192594}
  • Loading branch information
aleventhal authored and Chromium LUCI CQ committed Oct 18, 2023
1 parent 0614079 commit 5190268
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 1 deletion.
12 changes: 12 additions & 0 deletions third_party/blink/renderer/modules/accessibility/ax_object.cc
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,18 @@ void AXObject::SetAncestorsHaveDirtyDescendants() const {
return;
}

if (AXObjectCache().EntireDocumentIsDirty()) {
// No need to walk parent chain when marking the entire document dirty,
// as every node will have the bit set. In addition, attempting to repair
// the parent chain while marking everything dirty is actually against
// the point, because all child-parent relationships will be rebuilt
// from the top down.
if (LastKnownIsIncludedInTreeValue()) {
SetHasDirtyDescendants(true);
}
return;
}

const AXObject* ancestor = this;
bool can_repair_parents = AXObjectCache().IsProcessingDeferredEvents();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,10 @@ void AXObjectCacheImpl::Invalidate(Document& document, AXID ax_id) {
}

AXID AXObjectCacheImpl::GetAXID(Node* node) {
AXID existing_axid = GetExistingAXID(node);
if (existing_axid != ui::AXNodeData::kInvalidAXID) {
return existing_axid;
}
UpdateAXForAllDocuments();
return GetExistingAXID(node);
}
Expand Down Expand Up @@ -2681,6 +2685,8 @@ void AXObjectCacheImpl::ProcessDeferredAccessibilityEvents(Document& document) {
}
#endif

mark_all_dirty_ = false;

// Build out tree, such that each node has computed its children.
if (RuntimeEnabledFeatures::AccessibilityEagerAXTreeUpdateEnabled()) {
UpdateTreeIfNeeded();
Expand Down Expand Up @@ -4225,7 +4231,6 @@ void AXObjectCacheImpl::MarkDocumentDirtyWithCleanLayout() {
// but will not create new AXObjects, which avoids resetting the user's
// position in the content.
DCHECK(mark_all_dirty_);
mark_all_dirty_ = false;

// Assume all nodes in the tree need to recompute their properties.
// Note that objects can remain in the tree without being re-created.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,7 @@ class MODULES_EXPORT AXObjectCacheImpl
bool IsProcessingDeferredEvents() const {
return processing_deferred_events_;
}
bool EntireDocumentIsDirty() const { return mark_all_dirty_; }
// Returns true if UpdateTreeIfNeeded has been called and has not finished.
bool UpdatingTree() { return updating_tree_; }
// The document/cache are in the tear-down phase.
Expand Down

0 comments on commit 5190268

Please sign in to comment.