Title: [285859] trunk/Tools
Revision
285859
Author
commit-qu...@webkit.org
Date
2021-11-16 06:38:18 -0800 (Tue, 16 Nov 2021)

Log Message

AX: Stop returning AccessibilityUIElements backed by a null pointer
https://bugs.webkit.org/show_bug.cgi?id=233138

Patch by Tyler Wilcock <tyle...@apple.com> on 2021-11-16
Reviewed by Andres Gonzalez.

Prior to this patch, some callers of AccessibilityUIElement::create
passed null `id`s, resulting in a seemingly valid AccessibilityUIElement
that is actually backed by a null pointer. This makes writing tests harder,
since you can't rely on common operations like `!myElement` to do the right thing.

With this patch, we add a RELEASE_ASSERT to
AccessibilityUIElement::create requiring the passed pointer to be
valid. This patch also fixes up some callsites where we could (and did) pass null pointers.

* WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:
(WTR::AccessibilityController::focusedElement):
* WebKitTestRunner/InjectedBundle/win/AccessibilityControllerWin.cpp:
(WTR::AccessibilityController::focusedElement):
* WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:
(WTR::AccessibilityController::focusedElement):
Return RefPtr instead of Ref, since it's possible that no element is
focused.

* WebKitTestRunner/InjectedBundle/AccessibilityController.h:
* WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:
(WTR::AccessibilityUIElement::create):
Add RELEASE_ASSERT() to verify passed pointer is valid.

* WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:
(WTR::AccessibilityUIElement::focusedElement const):
(WTR::AccessibilityUIElement::uiElementForSearchPredicate):
* WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:
(-[AccessibilityNotificationHandler _notificationReceived:]):
* WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:
(WTR::AccessibilityUIElement::uiElementAttributeValue const):
(WTR::AccessibilityUIElement::uiElementForSearchPredicate):
(WTR::AccessibilityUIElement::cellForColumnAndRow):
(WTR::AccessibilityUIElement::horizontalScrollbar const):
(WTR::AccessibilityUIElement::verticalScrollbar const):
Don't call AccessibilityUIElement::create with null pointers.

Modified Paths

Diff

Modified: trunk/Tools/ChangeLog (285858 => 285859)


--- trunk/Tools/ChangeLog	2021-11-16 13:40:51 UTC (rev 285858)
+++ trunk/Tools/ChangeLog	2021-11-16 14:38:18 UTC (rev 285859)
@@ -1,3 +1,46 @@
+2021-11-16  Tyler Wilcock  <tyle...@apple.com>
+
+        AX: Stop returning AccessibilityUIElements backed by a null pointer
+        https://bugs.webkit.org/show_bug.cgi?id=233138
+
+        Reviewed by Andres Gonzalez.
+
+        Prior to this patch, some callers of AccessibilityUIElement::create
+        passed null `id`s, resulting in a seemingly valid AccessibilityUIElement
+        that is actually backed by a null pointer. This makes writing tests harder,
+        since you can't rely on common operations like `!myElement` to do the right thing.
+
+        With this patch, we add a RELEASE_ASSERT to
+        AccessibilityUIElement::create requiring the passed pointer to be
+        valid. This patch also fixes up some callsites where we could (and did) pass null pointers.
+
+        * WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp:
+        (WTR::AccessibilityController::focusedElement):
+        * WebKitTestRunner/InjectedBundle/win/AccessibilityControllerWin.cpp:
+        (WTR::AccessibilityController::focusedElement):
+        * WebKitTestRunner/InjectedBundle/AccessibilityController.cpp:
+        (WTR::AccessibilityController::focusedElement):
+        Return RefPtr instead of Ref, since it's possible that no element is
+        focused.
+
+        * WebKitTestRunner/InjectedBundle/AccessibilityController.h:
+        * WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:
+        (WTR::AccessibilityUIElement::create):
+        Add RELEASE_ASSERT() to verify passed pointer is valid.
+
+        * WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:
+        (WTR::AccessibilityUIElement::focusedElement const):
+        (WTR::AccessibilityUIElement::uiElementForSearchPredicate):
+        * WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:
+        (-[AccessibilityNotificationHandler _notificationReceived:]):
+        * WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:
+        (WTR::AccessibilityUIElement::uiElementAttributeValue const):
+        (WTR::AccessibilityUIElement::uiElementForSearchPredicate):
+        (WTR::AccessibilityUIElement::cellForColumnAndRow):
+        (WTR::AccessibilityUIElement::horizontalScrollbar const):
+        (WTR::AccessibilityUIElement::verticalScrollbar const):
+        Don't call AccessibilityUIElement::create with null pointers.
+
 2021-11-15  Michael Catanzaro  <mcatanz...@gnome.org>
 
         Make valgrind work properly without extra environment variables

Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp (285858 => 285859)


--- trunk/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp	2021-11-16 13:40:51 UTC (rev 285858)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.cpp	2021-11-16 14:38:18 UTC (rev 285859)
@@ -90,14 +90,12 @@
     return AccessibilityUIElement::create(root);
 }
 
-Ref<AccessibilityUIElement> AccessibilityController::focusedElement()
+RefPtr<AccessibilityUIElement> AccessibilityController::focusedElement()
 {
     auto page = InjectedBundle::singleton().page()->page();
     auto root = static_cast<PlatformUIElement>(WKAccessibilityRootObject(page));
     auto rootElement = AccessibilityUIElement::create(root);
-    if (auto focusedElement = rootElement->focusedElement())
-        return *focusedElement;
-    return AccessibilityUIElement::create(nullptr);
+    return rootElement->focusedElement();
 }
 
 void AccessibilityController::executeOnAXThreadAndWait(Function<void()>&& function)

Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h (285858 => 285859)


--- trunk/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h	2021-11-16 13:40:51 UTC (rev 285858)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h	2021-11-16 14:38:18 UTC (rev 285859)
@@ -63,7 +63,7 @@
     // Controller Methods - platform-independent implementations.
 #if HAVE(ACCESSIBILITY)
     Ref<AccessibilityUIElement> rootElement();
-    Ref<AccessibilityUIElement> focusedElement();
+    RefPtr<AccessibilityUIElement> focusedElement();
 #endif
     RefPtr<AccessibilityUIElement> elementAtPoint(int x, int y);
     RefPtr<AccessibilityUIElement> accessibleElementById(JSStringRef idAttribute);

Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp (285858 => 285859)


--- trunk/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp	2021-11-16 13:40:51 UTC (rev 285858)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp	2021-11-16 14:38:18 UTC (rev 285859)
@@ -35,6 +35,7 @@
 
 Ref<AccessibilityUIElement> AccessibilityUIElement::create(PlatformUIElement uiElement)
 {
+    RELEASE_ASSERT(uiElement);
     return adoptRef(*new AccessibilityUIElement(uiElement));
 }
     

Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp (285858 => 285859)


--- trunk/Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp	2021-11-16 13:40:51 UTC (rev 285858)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityControllerAtk.cpp	2021-11-16 14:38:18 UTC (rev 285859)
@@ -112,7 +112,7 @@
     return AccessibilityUIElement::create(static_cast<AtkObject*>(root));
 }
 
-Ref<AccessibilityUIElement> AccessibilityController::focusedElement()
+RefPtr<AccessibilityUIElement> AccessibilityController::focusedElement()
 {
     WKBundlePageRef page = InjectedBundle::singleton().page()->page();
     void* root = WKAccessibilityFocusedObject(page);

Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm (285858 => 285859)


--- trunk/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm	2021-11-16 13:40:51 UTC (rev 285858)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm	2021-11-16 14:38:18 UTC (rev 285859)
@@ -635,7 +635,9 @@
 
 RefPtr<AccessibilityUIElement> AccessibilityUIElement::focusedElement() const
 {
-    return AccessibilityUIElement::create([m_element accessibilityFocusedUIElement]);
+    if (id focusedUIElement = [m_element accessibilityFocusedUIElement])
+        return AccessibilityUIElement::create(focusedUIElement);
+    return nullptr;
 }
 
 bool AccessibilityUIElement::isFocused() const
@@ -765,14 +767,18 @@
 RefPtr<AccessibilityUIElement> AccessibilityUIElement::uiElementForSearchPredicate(JSContextRef context, AccessibilityUIElement *startElement, bool isDirectionNext, JSValueRef searchKey, JSStringRef searchText, bool visibleOnly, bool immediateDescendantsOnly)
 {
     NSDictionary *parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 5, searchKey, searchText, visibleOnly, immediateDescendantsOnly);
-    id value = [m_element accessibilityFindMatchingObjects:parameterizedAttribute];
-    if (![value isKindOfClass:[NSArray class]])
+    id results = [m_element accessibilityFindMatchingObjects:parameterizedAttribute];
+    if (![results isKindOfClass:[NSArray class]])
         return nullptr;
-    for (id element in value) {
+
+    for (id element in results) {
         if ([element isAccessibilityElement])
             return AccessibilityUIElement::create(element);
     }
-    return AccessibilityUIElement::create([value firstObject]);
+
+    if (id firstResult = [results firstObject])
+        return AccessibilityUIElement::create(firstResult);
+    return nullptr;
 }
 
 JSRetainPtr<JSStringRef> AccessibilityUIElement::selectTextWithCriteria(JSContextRef, JSStringRef ambiguityResolution, JSValueRef searchStrings, JSStringRef replacementString, JSStringRef activity)

Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm (285858 => 285859)


--- trunk/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm	2021-11-16 13:40:51 UTC (rev 285858)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm	2021-11-16 14:38:18 UTC (rev 285859)
@@ -127,7 +127,8 @@
     } else {
         // A global listener gets the element, notification name and userInfo.
         JSValueRef arguments[3];
-        arguments[0] = toJS(context, WTR::AccessibilityUIElement::create([notification object]).ptr());
+        id notificationObject = [notification object];
+        arguments[0] = toJS(context, notificationObject ? WTR::AccessibilityUIElement::create(notificationObject).ptr() : nullptr);
         arguments[1] = notificationNameArgument;
         arguments[2] = userInfoArgument;
         JSObjectCallAsFunction(context, const_cast<JSObjectRef>(m_notificationFunctionCallback), 0, 3, arguments, 0);

Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm (285858 => 285859)


--- trunk/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm	2021-11-16 13:40:51 UTC (rev 285858)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm	2021-11-16 14:38:18 UTC (rev 285859)
@@ -617,8 +617,9 @@
 
 RefPtr<AccessibilityUIElement> AccessibilityUIElement::uiElementAttributeValue(JSStringRef attribute) const
 {
-    auto value = attributeValue([NSString stringWithJSStringRef:attribute]);
-    return AccessibilityUIElement::create(value.get());
+    if (auto value = attributeValue([NSString stringWithJSStringRef:attribute]))
+        return AccessibilityUIElement::create(value.get());
+    return nullptr;
 }
 
 bool AccessibilityUIElement::boolAttributeValue(JSStringRef attribute)
@@ -1241,9 +1242,11 @@
 {
     BEGIN_AX_OBJC_EXCEPTIONS
     NSDictionary *parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly, immediateDescendantsOnly);
-    id value = [m_element accessibilityAttributeValue:@"AXUIElementsForSearchPredicate" forParameter:parameterizedAttribute];
-    if ([value isKindOfClass:[NSArray class]])
-        return AccessibilityUIElement::create([value lastObject]);
+    id searchResults = [m_element accessibilityAttributeValue:@"AXUIElementsForSearchPredicate" forParameter:parameterizedAttribute];
+    if ([searchResults isKindOfClass:[NSArray class]]) {
+        if (id lastResult = [searchResults lastObject])
+            return AccessibilityUIElement::create(lastResult);
+    }
     END_AX_OBJC_EXCEPTIONS
     
     return nullptr;
@@ -1410,7 +1413,8 @@
 {
     NSArray *colRowArray = @[@(col), @(row)];
     BEGIN_AX_OBJC_EXCEPTIONS
-    return AccessibilityUIElement::create([m_element accessibilityAttributeValue:@"AXCellForColumnAndRow" forParameter:colRowArray]);
+    if (id cell = [m_element accessibilityAttributeValue:@"AXCellForColumnAndRow" forParameter:colRowArray])
+        return AccessibilityUIElement::create(cell);
     END_AX_OBJC_EXCEPTIONS    
 
     return nullptr;
@@ -1422,7 +1426,8 @@
         return nullptr;
 
     BEGIN_AX_OBJC_EXCEPTIONS
-    return AccessibilityUIElement::create(attributeValue(NSAccessibilityHorizontalScrollBarAttribute).get());
+    if (id scrollbar = attributeValue(NSAccessibilityHorizontalScrollBarAttribute).get())
+        return AccessibilityUIElement::create(scrollbar);
     END_AX_OBJC_EXCEPTIONS    
 
     return nullptr;
@@ -1434,7 +1439,8 @@
         return nullptr;
 
     BEGIN_AX_OBJC_EXCEPTIONS
-    return AccessibilityUIElement::create(attributeValue(NSAccessibilityVerticalScrollBarAttribute).get());
+    if (id scrollbar = attributeValue(NSAccessibilityVerticalScrollBarAttribute).get())
+        return AccessibilityUIElement::create(scrollbar);
     END_AX_OBJC_EXCEPTIONS        
 
     return nullptr;

Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/win/AccessibilityControllerWin.cpp (285858 => 285859)


--- trunk/Tools/WebKitTestRunner/InjectedBundle/win/AccessibilityControllerWin.cpp	2021-11-16 13:40:51 UTC (rev 285858)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/win/AccessibilityControllerWin.cpp	2021-11-16 14:38:18 UTC (rev 285859)
@@ -61,10 +61,10 @@
     return AccessibilityUIElement::create(nullptr);
 }
 
-Ref<AccessibilityUIElement> AccessibilityController::focusedElement()
+RefPtr<AccessibilityUIElement> AccessibilityController::focusedElement()
 {
     notImplemented();
-    return AccessibilityUIElement::create(nullptr);
+    return nullptr;
 }
 
 bool AccessibilityController::addNotificationListener(JSValueRef)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to