Commit 59975492 authored by Kurt Catti-Schmidt's avatar Kurt Catti-Schmidt Committed by Commit Bot

Null-dereference READ in blink::AXPosition::AXPosition

This crash is caused by the following DOM structure (achieved via)
DOM manipupation:

blink::HTMLBRElement
    blink::HTMLMapElement
        blink::Text
        blink::Text
        blink::HTMLAreaElement

Note that the <br> tag has children, which is not possible via markup,
but can be generated easily with JavaScript.

In AXPosition::CreatePositionBeforeObject, the crash occurs here:

  const AXObject* parent = child.ParentObjectIncludedInTree();
  DCHECK(parent); // parent is nullptr

While there is technically a parent node in the DOM, there is no layout
node, since the tree is corrupted. The AXObject implementation for
this depends on either a valid parent node or a layout node (see
AXImageMapLink::ComputeParent). Creating an AXPosition in this state
results in the nullptr deference listed above.

An alternative would be to generate a parent specifically for
AXImageMapLink in this scenario, however I dropped that approach when
it became clear that the layout and accessibility engines hit lots of
DCHECKS when the DOM tree is invalid (as they should).

Instead, the approach taken here is to add additional resilience in
AXPosition to handle invalid parent/child relationships.

Bug: 996525
Change-Id: I598e4ac930d4caeb10ce74f6328d3a75eb9580fe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1830012
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: default avatarNektarios Paisios <nektar@chromium.org>
Reviewed-by: default avatarAaron Leventhal <aleventhal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#703459}
parent 04ab2226
......@@ -26,7 +26,7 @@ namespace blink {
const AXPosition AXPosition::CreatePositionBeforeObject(
const AXObject& child,
const AXPositionAdjustmentBehavior adjustment_behavior) {
if (child.IsDetached())
if (child.IsDetached() || !child.AccessibilityIsIncludedInTree())
return {};
// If |child| is a text object, but not a text control, make behavior the same
......@@ -37,6 +37,10 @@ const AXPosition AXPosition::CreatePositionBeforeObject(
return CreateFirstPositionInObject(child, adjustment_behavior);
const AXObject* parent = child.ParentObjectIncludedInTree();
if (!parent || parent->IsDetached())
return {};
DCHECK(parent);
AXPosition position(*parent);
position.text_offset_or_child_index_ = child.IndexInParent();
......@@ -51,7 +55,7 @@ const AXPosition AXPosition::CreatePositionBeforeObject(
const AXPosition AXPosition::CreatePositionAfterObject(
const AXObject& child,
const AXPositionAdjustmentBehavior adjustment_behavior) {
if (child.IsDetached())
if (child.IsDetached() || !child.AccessibilityIsIncludedInTree())
return {};
// If |child| is a text object, but not a text control, make behavior the same
......@@ -62,6 +66,10 @@ const AXPosition AXPosition::CreatePositionAfterObject(
return CreateLastPositionInObject(child, adjustment_behavior);
const AXObject* parent = child.ParentObjectIncludedInTree();
if (!parent || parent->IsDetached())
return {};
DCHECK(parent);
AXPosition position(*parent);
position.text_offset_or_child_index_ = child.IndexInParent() + 1;
......
......@@ -77,6 +77,12 @@ constexpr char kAOM[] = R"HTML(
</script>
)HTML";
constexpr char kMap[] = R"HTML(
<br id="br">
<map id="map">
<area shape="rect" coords="0,0,10,10" href="about:blank">
</map>
)HTML";
} // namespace
//
......@@ -1620,5 +1626,33 @@ TEST_F(AccessibilityTest, DISABLED_PositionInVirtualAOMNode) {
EXPECT_EQ(ax_after, ax_position_after_from_dom.ChildAfterTreePosition());
}
TEST_F(AccessibilityTest, PositionInInvalidMapLayout) {
SetBodyInnerHTML(kMap);
Node* br = GetElementById("br");
ASSERT_NE(nullptr, br);
Node* map = GetElementById("map");
ASSERT_NE(nullptr, map);
// Create an invalid layout by appending a child to the <br>
br->appendChild(map);
GetDocument().UpdateStyleAndLayoutTree();
const AXObject* ax_map = GetAXObjectByElementId("map");
ASSERT_NE(nullptr, ax_map);
ASSERT_EQ(ax::mojom::Role::kGenericContainer, ax_map->RoleValue());
const auto ax_position_before =
AXPosition::CreatePositionBeforeObject(*ax_map);
const auto position_before = ax_position_before.ToPositionWithAffinity();
EXPECT_EQ(nullptr, position_before.AnchorNode());
EXPECT_EQ(0, position_before.GetPosition().OffsetInContainerNode());
const auto ax_position_after = AXPosition::CreatePositionAfterObject(*ax_map);
const auto position_after = ax_position_after.ToPositionWithAffinity();
EXPECT_EQ(nullptr, position_after.AnchorNode());
EXPECT_EQ(0, position_after.GetPosition().OffsetInContainerNode());
}
} // namespace test
} // namespace blink
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