AX: Crash at WebCore::Document::updateLayout

https://bugs.webkit.org/show_bug.cgi?id=225677 Reviewed by Alan Bujtas. Source/WebCore: Handling notifications while during layout is risky because we can call back into an update method. We have handled many other cases like this by deferring the notification, as we do here. Special note: this test can't be made to crash because actual crash requires the accessibility runtime to be initialized and posting real notifications to the system. But, we can verify that the notification is still sent correctly under conditions that could lead to a crash. Test: accessibility/mac/menu-selection-notification-crash.html * accessibility/AXObjectCache.cpp: (WebCore::AXObjectCache::deferMenuListValueChange): (WebCore::AXObjectCache::performDeferredCacheUpdate): * accessibility/AXObjectCache.h: * accessibility/AccessibilityMenuList.cpp: (WebCore::AccessibilityMenuList::didUpdateActiveOption): LayoutTests: * accessibility/mac/menu-selection-notification-crash-expected.txt: Added. * accessibility/mac/menu-selection-notification-crash.html: Added. Canonical link: https://commits.webkit.org/237682@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@277434 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · May 13, 2021 · 5dc7c6114c2ab0266ea7885b22e289884a538155 · 5dc7c61
1 parent 2ef790f
commit 5dc7c6114c2ab0266ea7885b22e289884a538155
Showing 7 changed files with 107 additions and 1 deletion.
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,13 @@
+2021-05-13  Chris Fleizach  <cfleizach@apple.com>
+
+        AX: Crash at WebCore::Document::updateLayout
+        https://bugs.webkit.org/show_bug.cgi?id=225677
+
+        Reviewed by Alan Bujtas.
+
+        * accessibility/mac/menu-selection-notification-crash-expected.txt: Added.
+        * accessibility/mac/menu-selection-notification-crash.html: Added.
+
 2021-05-13  Enrique Ocaña González  <eocanha@igalia.com>
 
         [GStreamer] media/track/in-band/track-in-band-srt-mkv-kind.html is a flaky crash

diff --git a/LayoutTests/accessibility/mac/menu-selection-notification-crash-expected.txt b/LayoutTests/accessibility/mac/menu-selection-notification-crash-expected.txt
@@ -0,0 +1,13 @@
+
+This tests that inserting a node into a select menu won't cause a crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS addedNotification is true
+menu role: AXRole: AXPopUpButton
+Notification received successfully.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/accessibility/mac/menu-selection-notification-crash.html b/LayoutTests/accessibility/mac/menu-selection-notification-crash.html
@@ -0,0 +1,48 @@
+<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
+<html>
+<head>
+<script src="../../resources/js-test-pre.js"></script>
+<script src="../../resources/accessibility-helper.js"></script>
+</head>
+<body id="body">
+
+<div id="content">
+<select id="menu">
+</select>
+</div>
+
+<p id="description"></p>
+<div id="console"></div>
+
+<script>
+
+    description("This tests that inserting a node into a select menu won't cause a crash.");
+
+    var axNotificationElement = null;
+    function notificationCallback(element, notification, state) {
+        if (notification == "AXMenuItemSelected") {
+            debug("Notification received successfully.");
+            finishJSTest();
+        }
+    }
+
+    if (window.accessibilityController) {
+        jsTestIsAsync = true;
+        var addedNotification = accessibilityController.addNotificationListener(notificationCallback);
+        shouldBeTrue("addedNotification");
+
+        var menu = accessibilityController.accessibleElementById("menu");
+        debug("menu role: " + menu.role);
+
+        setTimeout(function() {
+            var opt = document.createElement('option');
+            opt.value = "c";
+            opt.innerHTML = "c";
+            document.getElementById("menu").appendChild(opt);
+        }, 0);
+    }
+
+</script>
+<script src="../../resources/js-test-post.js"></script>
+</body>
+</html>
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,25 @@
+2021-05-13  Chris Fleizach  <cfleizach@apple.com>
+
+        AX: Crash at WebCore::Document::updateLayout
+        https://bugs.webkit.org/show_bug.cgi?id=225677
+
+        Reviewed by Alan Bujtas.
+
+        Handling notifications while during layout is risky because we can call back into an update method.
+        We have handled many other cases like this by deferring the notification, as we do here.
+        Special note: this test can't be made to crash because actual crash requires the accessibility
+        runtime to be initialized and posting real notifications to the system. But, we can verify that the
+        notification is still sent correctly under conditions that could lead to a crash.
+
+        Test: accessibility/mac/menu-selection-notification-crash.html
+
+        * accessibility/AXObjectCache.cpp:
+        (WebCore::AXObjectCache::deferMenuListValueChange):
+        (WebCore::AXObjectCache::performDeferredCacheUpdate):
+        * accessibility/AXObjectCache.h:
+        * accessibility/AccessibilityMenuList.cpp:
+        (WebCore::AccessibilityMenuList::didUpdateActiveOption):
+
 2021-05-13  Enrique Ocaña González  <eocanha@igalia.com>
 
         [GStreamer] media/track/in-band/track-in-band-srt-mkv-kind.html is a flaky crash

diff --git a/Source/WebCore/accessibility/AXObjectCache.cpp b/Source/WebCore/accessibility/AXObjectCache.cpp
@@ -1230,6 +1230,13 @@ void AXObjectCache::deferFocusedUIElementChangeIfNeeded(Node* oldNode, Node* new
         handleFocusedUIElementChanged(oldNode, newNode);
 }
 
+void AXObjectCache::deferMenuListValueChange(Element* element)
+{
+    m_deferredMenuListChange.add(element);
+    if (!m_performCacheUpdateTimer.isActive())
+        m_performCacheUpdateTimer.startOneShot(0_s);
+}
+
 void AXObjectCache::deferModalChange(Element* element)
 {
     m_deferredModalChangedList.add(element);
@@ -3196,6 +3203,10 @@ void AXObjectCache::performDeferredCacheUpdate()
         handleModalChange(deferredModalChangedElement);
     m_deferredModalChangedList.clear();
 
+    for (auto& deferredMenuListChangeElement : m_deferredMenuListChange)
+        postNotification(&deferredMenuListChangeElement, AXObjectCache::AXMenuListValueChanged);
+    m_deferredMenuListChange.clear();
+
     platformPerformDeferredCacheUpdate();
 }
 

diff --git a/Source/WebCore/accessibility/AXObjectCache.h b/Source/WebCore/accessibility/AXObjectCache.h
@@ -187,6 +187,7 @@ class AXObjectCache {
 
     void deferFocusedUIElementChangeIfNeeded(Node* oldFocusedNode, Node* newFocusedNode);
     void deferModalChange(Element*);
+    void deferMenuListValueChange(Element*);
     void handleScrolledToAnchor(const Node* anchorNode);
     void handleScrollbarUpdate(ScrollView*);
 
@@ -513,6 +514,7 @@ class AXObjectCache {
     ListHashSet<RefPtr<AXCoreObject>> m_deferredChildrenChangedList;
     ListHashSet<Node*> m_deferredChildrenChangedNodeList;
     WeakHashSet<Element> m_deferredModalChangedList;
+    WeakHashSet<Element> m_deferredMenuListChange;
     HashMap<Element*, String> m_deferredTextFormControlValue;
     HashMap<Element*, QualifiedName> m_deferredAttributeChange;
     Vector<std::pair<Node*, Node*>> m_deferredFocusedNodeChange;

diff --git a/Source/WebCore/accessibility/AccessibilityMenuList.cpp b/Source/WebCore/accessibility/AccessibilityMenuList.cpp
@@ -136,7 +136,7 @@ void AccessibilityMenuList::didUpdateActiveOption(int optionIndex)
     }
 
     if (auto* cache = document->axObjectCache())
-        cache->postNotification(this, document.ptr(), AXObjectCache::AXMenuListValueChanged);
+        cache->deferMenuListValueChange(element());
 }
 
 } // namespace WebCore