Skip to content

Commit c635f35

Browse files
twilcofilipe-norte-red
twilco
authored and
filipe-norte-red
committed
AX: AXObjectCache::remove(Node&) can be called while performing a cache update causing m_deferred* vectors to be modified as we iterate over them
https://bugs.webkit.org/show_bug.cgi?id=244421 rdar://98729717 Reviewed by Chris Fleizach. Handling certain attribute changes in AXObjectCache::handleAttributeChange can cause AXObjectCache::remove(Node&) to be called as a side effect. For example, this can happen as a result of a `role` attribute change. When this happens, the Node is removed from m_deferredAttributeChanges while we are iterating over this vector. This patch prevents this by making AXObjectCache::remove(Node&) bail early before modifying any m_deferred* vector if we detect that we're in the middle of a cache update. If this is true, these vectors will be cleared anyways upon completion of their processing. * LayoutTests/accessibility/multiple-attribute-change-crash-expected.txt: Added. * LayoutTests/accessibility/multiple-attribute-change-crash.html: Added. * LayoutTests/platform/ios/TestExpectations: Enable new test. * LayoutTests/platform/glib/accessibility/multiple-attribute-change-crash-expected.txt: Added * LayoutTests/platform/ios/accessibility/multiple-attribute-change-crash-expected.txt: Added. * LayoutTests/platform/win/TestExpectations: Disable new test. * Source/WebCore/accessibility/AXObjectCache.cpp: (WebCore::AXObjectCache::remove): (WebCore::AXObjectCache::handleAttributeChange): Fix unchecked null dereference of the input Element* in an AXLOG. Canonical link: https://commits.webkit.org/253871@main
1 parent c9e2f0d commit c635f35

7 files changed

Lines changed: 105 additions & 6 deletions

File tree

‎LayoutTests/accessibility/multiple-attribute-change-crash-expected.txt‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
This test ensures that we don't crash when processing attribute changes after a deferred role attribute change.
2+
3+
#grid element's initial role: AXRole: AXTable
4+
Changing #grid's role attribute to 'presentation', and its class to 'foo'.
5+
#grid element's new role after DOM changes: AXRole:
6+
7+
No crash, so test passed.
8+
9+
PASS successfullyParsed is true
10+
11+
TEST COMPLETE
12+

‎LayoutTests/accessibility/multiple-attribute-change-crash.html‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
2+
<html>
3+
<head>
4+
<script src="../resources/accessibility-helper.js"></script>
5+
<script src="../resources/js-test.js"></script>
6+
</head>
7+
<body>
8+
9+
<div id="grid" role="grid">
10+
<div role="row">
11+
<div role="columnheader">header 1</div>
12+
<div role="columnheader">header 2</div>
13+
</div>
14+
<div role="row">
15+
<div role="gridcell">cell</div>
16+
<div role="gridcell">cell</div>
17+
</div>
18+
</div>
19+
20+
<script>
21+
var testOutput = "This test ensures that we don't crash when processing attribute changes after a deferred role attribute change.\n\n";
22+
23+
if (window.accessibilityController) {
24+
window.jsTestIsAsync = true;
25+
26+
const axGrid = accessibilityController.accessibleElementById("grid");
27+
const initialRole = axGrid.role;
28+
testOutput += `#grid element's initial role: ${initialRole}\n`;
29+
30+
testOutput += "Changing #grid's role attribute to 'presentation', and its class to 'foo'.\n";
31+
// Force dirty layout and style by moving this element and changing its color. This will cause
32+
// AXObjectCache::rendererNeedsDeferredUpdate to be true for this element, deferring any
33+
// subsequent attribute changes to be processed after a timer elapses.
34+
document.getElementById("grid").setAttribute("style", "margin-left: 300px; background-color: red;");
35+
// Change the element's role such that we attempt to remove and re-create the object.
36+
document.getElementById("grid").setAttribute("role", "presentation");
37+
// If we behave correctly, processing this 'class' attribute change should not cause a crash.
38+
document.getElementById("grid").setAttribute("class", "foo");
39+
setTimeout(async function() {
40+
await waitFor(() => axGrid.role !== initialRole);
41+
testOutput += `#grid element's new role after DOM changes: ${axGrid.role}\n`;
42+
testOutput += `\nNo crash, so test passed.\n`;
43+
44+
debug(testOutput);
45+
document.getElementById("grid").style.visibility = "hidden";
46+
finishJSTest();
47+
}, 0);
48+
}
49+
</script>
50+
</body>
51+
</html>
52+

‎LayoutTests/platform/glib/accessibility/multiple-attribute-change-crash-expected.txt‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
This test ensures that we don't crash when processing attribute changes after a deferred role attribute change.
2+
3+
#grid element's initial role: AXRole: AXTable
4+
Changing #grid's role attribute to 'presentation', and its class to 'foo'.
5+
#grid element's new role after DOM changes: AXRole: AXInvalid
6+
7+
No crash, so test passed.
8+
9+
PASS successfullyParsed is true
10+
11+
TEST COMPLETE
12+

‎LayoutTests/platform/ios/TestExpectations‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2141,6 +2141,7 @@ accessibility/insert-text-into-password-field.html [ Pass ]
21412141
accessibility/list-with-dynamically-changing-content.html [ Pass ]
21422142
accessibility/live-region-attributes-update-after-dynamic-change.html [ Pass ]
21432143
accessibility/menu-list-crash2.html [ Pass ]
2144+
accessibility/multiple-attribute-change-crash.html [ Pass ]
21442145
accessibility/node-only-inert-object.html [ Pass ]
21452146
accessibility/node-only-object-element-rect.html [ Pass ]
21462147
accessibility/placeholder.html [ Pass ]

‎LayoutTests/platform/ios/accessibility/multiple-attribute-change-crash-expected.txt‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
This test ensures that we don't crash when processing attribute changes after a deferred role attribute change.
2+
3+
#grid element's initial role: Grid
4+
Changing #grid's role attribute to 'presentation', and its class to 'foo'.
5+
#grid element's new role after DOM changes: null
6+
7+
No crash, so test passed.
8+
9+
PASS successfullyParsed is true
10+
11+
TEST COMPLETE
12+

‎LayoutTests/platform/win/TestExpectations‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,6 +484,9 @@ accessibility/aria-multiline.html [ Skip ]
484484
accessibility/list-with-dynamically-changing-content.html [ Skip ]
485485
accessibility/ignored-aria-role-description.html [ Skip ]
486486

487+
# Failing since added in https://bugs.webkit.org/show_bug.cgi?id=244421.
488+
accessibility/multiple-attribute-change-crash.html [ Skip ]
489+
487490
# Need to implement AccessibilityUIElement::clearSelectedChildren()
488491
accessibility/aria-listbox-clear-selection-crash.html [ Skip ]
489492
accessibility/listbox-clear-selection.html [ Skip ]

‎Source/WebCore/accessibility/AXObjectCache.cpp‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -924,6 +924,18 @@ void AXObjectCache::remove(RenderObject* renderer)
924924
void AXObjectCache::remove(Node& node)
925925
{
926926
AXTRACE(makeString("AXObjectCache::remove 0x"_s, hex(reinterpret_cast<uintptr_t>(this))));
927+
928+
removeNodeForUse(node);
929+
remove(m_nodeObjectMapping.take(&node));
930+
remove(node.renderer());
931+
932+
// If we're in the middle of a cache update, don't modify any of these vectors because we are currently
933+
// iterating over them. They will be cleared at the end of the cache update, so not removing them here is fine.
934+
if (m_performingDeferredCacheUpdate) {
935+
AXLOG("Bailing out before removing node from m_deferred* vectors as we are in the middle of a cache update.");
936+
return;
937+
}
938+
927939
if (is<Element>(node)) {
928940
m_deferredTextFormControlValue.remove(downcast<Element>(&node));
929941
m_deferredAttributeChange.removeAllMatching([&node] (const auto& entry) {
@@ -947,11 +959,6 @@ void AXObjectCache::remove(Node& node)
947959
if (entry.first == &node)
948960
entry.first = nullptr;
949961
});
950-
951-
removeNodeForUse(node);
952-
953-
remove(m_nodeObjectMapping.take(&node));
954-
remove(node.renderer());
955962
}
956963

957964
void AXObjectCache::remove(Widget* view)
@@ -1954,7 +1961,7 @@ bool AXObjectCache::shouldProcessAttributeChange(Element* element, const Qualifi
19541961
void AXObjectCache::handleAttributeChange(Element* element, const QualifiedName& attrName)
19551962
{
19561963
AXTRACE(makeString("AXObjectCache::handleAttributeChange 0x"_s, hex(reinterpret_cast<uintptr_t>(this))));
1957-
AXLOG(makeString("attribute ", attrName.localName().string(), " for element ", element->debugDescription()));
1964+
AXLOG(makeString("attribute ", attrName.localName().string(), " for element ", element ? element->debugDescription() : String("nullptr"_s)));
19581965

19591966
if (!shouldProcessAttributeChange(element, attrName))
19601967
return;

0 commit comments

Comments
 (0)