Skip to content

Commit

Permalink
Speculative fixes for crashes in AXPosition
Browse files Browse the repository at this point in the history
Crash stack traces are pointing to very occasional errors with AXPosition.
They all seem to be cases where a DCHECK would have failed. I think the
best approach is to not only DCHECK but also gracefully handle the failure
case if it does occur. Not setting the selection doesn't have to be a
fatal error.

Bug: 1116340
Change-Id: I038dc793118214905aa46daf587b9c7a2f9018e2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2404386
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806687}
  • Loading branch information
minorninth authored and Commit Bot committed Sep 14, 2020
1 parent 0505b6f commit e645bb4
Showing 1 changed file with 20 additions and 0 deletions.
20 changes: 20 additions & 0 deletions third_party/blink/renderer/modules/accessibility/ax_position.cc
Original file line number Diff line number Diff line change
Expand Up @@ -762,10 +762,16 @@ const AXPosition AXPosition::AsValidDOMPosition(
DCHECK(container_node) << "All anonymous layout objects and list markers "
"should have a containing block element.";
DCHECK(!container->IsDetached());
if (!container_node || container->IsDetached())
return {};

auto& ax_object_cache_impl = container->AXObjectCache();
const AXObject* new_container =
ax_object_cache_impl.GetOrCreate(container_node);
DCHECK(new_container);
if (!new_container)
return {};

AXPosition position(*new_container);
if (new_container == container->ParentObjectIncludedInTree()) {
position.text_offset_or_child_index_ = container->IndexInParent();
Expand Down Expand Up @@ -796,6 +802,9 @@ const PositionWithAffinity AXPosition::ToPositionWithAffinity(
const Node* container_node = adjusted_position.container_object_->GetNode();
DCHECK(container_node) << "AX positions that are valid DOM positions should "
"always be connected to their DOM nodes.";
if (!container_node)
return {};

if (!adjusted_position.IsTextPosition()) {
// AX positions that are unumbiguously at the start or end of a container,
// should convert to the corresponding DOM positions at the start or end of
Expand All @@ -810,10 +819,16 @@ const PositionWithAffinity AXPosition::ToPositionWithAffinity(
DCHECK(child_node) << "AX objects used in AX positions that are valid "
"DOM positions should always be connected to their "
"DOM nodes.";
if (!child_node)
return {};

if (!child_node->previousSibling()) {
// Creates a |PositionAnchorType::kBeforeChildren| position.
container_node = child_node->parentNode();
DCHECK(container_node);
if (!container_node)
return {};

return PositionWithAffinity(
Position::FirstPositionInNode(*container_node), affinity_);
}
Expand All @@ -830,12 +845,17 @@ const PositionWithAffinity AXPosition::ToPositionWithAffinity(
DCHECK(last_child_node) << "AX objects used in AX positions that are "
"valid DOM positions should always be "
"connected to their DOM nodes.";
if (!last_child_node)
return {};

// Check if this is an "after children" position in the DOM as well.
if (!last_child_node->nextSibling()) {
// Creates a |PositionAnchorType::kAfterChildren| position.
container_node = last_child_node->parentNode();
DCHECK(container_node);
if (!container_node)
return {};

return PositionWithAffinity(
Position::LastPositionInNode(*container_node), affinity_);
}
Expand Down

0 comments on commit e645bb4

Please sign in to comment.