Bug 256244 – AX: AXIsolatedObject should perform actions and setters asynchronously

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

Currently, most AXIsolatedObjects that need to set a value or perform an action (i.e scroll to make visible) on the main-thread do so synchronously. This is not necessary in most cases, and affects WebKit's responsiveness to other AX requests.

[Radar WebKit Bug Importer]

<rdar://problem/108825366>

[Tyler Wilcock]

Created attachment 466188 [details]
Patch

[Tyler Wilcock]

Created attachment 466189 [details]
Patch

[chris fleizach]

Comment on attachment 466189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=466189&action=review

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2753
> +            backingObject->setSelectedRows(WTFMove(selectedRows));

do we want to keep having these methods that take a by-ref values instead of just returning the data?

[Tyler Wilcock]

(In reply to chris fleizach from comment #4)
> Comment on attachment 466189 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466189&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2753
> > +            backingObject->setSelectedRows(WTFMove(selectedRows));
> 
> do we want to keep having these methods that take a by-ref values instead of
> just returning the data?
We are trying to move away from those style methods, but I think this one is different in that it doesn't return anything, just takes the rows as input. The change I made here was to move the rows into the method (because we can) rather than copying them in.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Created attachment 466189 [details]
> Patch

--- a/Source/WebCore/accessibility/AccessibilityObject.h
+++ b/Source/WebCore/accessibility/AccessibilityObject.h

     bool setValue(const String&) override { return false; }
+    void setValueIgnoringResult(const String& value) final { setValue(value); }

We may be able to just make setValue to return void since in neither Mac or iOS the returned bool is used. ATSPI may be using it but it is quite irrelevant because in the cases where setValue actually does something we don't know if it succeeded or not, we just return true.


 void AXIsolatedObject::scrollToMakeVisibleWithSubFocus(const IntRect& rect) const
 {
-    performFunctionOnMainThread([&rect] (AccessibilityObject* axObject) {
+    performFunctionOnMainThread([rect] (AccessibilityObject* axObject) {

Should move rect instead of copying.

-    performFunctionOnMainThread([&point] (AccessibilityObject* axObject) {
+    performFunctionOnMainThread([point] (AccessibilityObject* axObject) {


Move instead of copying.

 void AXIsolatedObject::setSelectedTextRange(const PlainTextRange& value)
 {
-    performFunctionOnMainThread([&value] (AccessibilityObject* object) {
+    performFunctionOnMainThread([value] (AccessibilityObject* object) {

Dito.

-    performFunctionOnMainThread([&axRange] (AccessibilityObject* axObject) {
+    performFunctionOnMainThread([axRange] (AccessibilityObject* axObject) {

Dito.

 bool AXIsolatedObject::press()
 {
+    ASSERT(isMainThread());
+
     if (auto* object = associatedAXObject())

associatedAXObject() already asserts isMainThread().


-    performFunctionOnMainThread([&value, this](AXCoreObject* object) {
+    performFunctionOnMainThreadAndWait([&value, this](AXCoreObject* object) {
         object->setPreventKeyboardDOMEventDispatch(value);
         setProperty(AXPropertyName::PreventKeyboardDOMEventDispatch, value);
     });


Do we need to wait in this one?

-    performFunctionOnMainThread([&value, this](AXCoreObject* object) {
+    performFunctionOnMainThreadAndWait([&value, this](AXCoreObject* object) {
         object->setCaretBrowsingEnabled(value);
         setProperty(AXPropertyName::CaretBrowsingEnabled, value);
     });


and this one?

-        Accessibility::performFunctionOnMainThread([textMarkerRange = retainPtr(textMarkerRange), protectedSelf = retainPtr(self)] {
+        // FIXME: If we were to make this call the main-thread asynchronously, is passing RetainPtr<TextMarkerRange> to the lamdba safe?
+        Accessibility::performFunctionOnMainThreadAndWait([textMarkerRange = retainPtr(textMarkerRange), protectedSelf = retainPtr(self)] {


I believe it will be ok not to wait.

[Tyler Wilcock]

Created attachment 466220 [details]
Patch

[Tyler Wilcock]

I was able to use WTFMove in all the places you suggested, but it required making the lambdas in AXIsolatedObject::performFunctionOnMainThread and Accessibility::performFunctionOnMainThread mutable so we could call move inside the lambda function body.

>      bool setValue(const String&) override { return false; }
> +    void setValueIgnoringResult(const String& value) final {
> setValue(value); }
> 
> We may be able to just make setValue to return void since in neither Mac or
> iOS the returned bool is used. ATSPI may be using it but it is quite
> irrelevant because in the cases where setValue actually does something we
> don't know if it succeeded or not, we just return true.
We do use it in AccessibilityNodeObject::setNodeValue to determine whether to send a notification. I'm also hesitant to change the behavior for ATSPI, since it looks like the result is used for their platform somehow.

> -    performFunctionOnMainThread([&value, this](AXCoreObject* object) {
> +    performFunctionOnMainThreadAndWait([&value, this](AXCoreObject* object)
> {
>          object->setPreventKeyboardDOMEventDispatch(value);
>          setProperty(AXPropertyName::PreventKeyboardDOMEventDispatch, value);
>      });
> 
> 
> Do we need to wait in this one?
> 
> -    performFunctionOnMainThread([&value, this](AXCoreObject* object) {
> +    performFunctionOnMainThreadAndWait([&value, this](AXCoreObject* object)
> {
>          object->setCaretBrowsingEnabled(value);
>          setProperty(AXPropertyName::CaretBrowsingEnabled, value);
>      });
> 
> 
> and this one?
I did originally make these ones async, but then made them sync again because of the call to setProperty. But reviewing this again, it seems like outside of initial AXIsolatedObject::create, we can only read / write properties on the secondary thread, and because this function runs on the secondary thread we aren't in danger of violating thread-safety of the map.

Does this sound correct to you? If so, these can be async.

(AXIsolatedObject::getOrRetrievePropertyValue does write to AXIsolatedObject::m_propertyMap from the main-thread, but does so synchronously blocking the AX thread at the same time, so not a concern for this case).

In addressing your review, I discovered that AccessibilityObject::makeRangeVisible is a no-op, meaning it was a wasted main-thread hit to do nothing. I elected to remove it rather than try to figure out how to implement it. We can bring it back later if need be.

Addressed all other non-quoted comments.

[Tyler Wilcock]

Created attachment 466221 [details]
Patch

[Tyler Wilcock]

Created attachment 466225 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #10)
> Created attachment 466225 [details]
> Patch

+template<typename U> inline void performFunctionOnMainThread(U&& lambda)
+{
+    callOnMainThread([lambda = WTFMove(lambda)] () mutable {
+        lambda();
+    });
+}

don't think you need mutable here because nothing is modified inside the lamda.

+    template<typename U> void performFunctionOnMainThreadAndWait(U&& lambda) const
+    {
+        Accessibility::performFunctionOnMainThreadAndWait([&lambda, this]() {
+            if (RefPtr object = associatedAXObject())
+                lambda(object.get());
+        });
+    }

Missing space between the capture clause and (), even though it wasn't there before I think it is kind of standard, surprised that the style checker doesn't catch this.


     template<typename U> void performFunctionOnMainThread(U&& lambda) const
     {
-        Accessibility::performFunctionOnMainThread([&lambda, this]() {
-            if (auto* object = associatedAXObject())
-                lambda(object);
+        Accessibility::performFunctionOnMainThread([lambda = WTFMove(lambda), protectedThis = Ref { *this }]() mutable {
+            if (RefPtr object = protectedThis->associatedAXObject())
+                lambda(object.get());

Missing space between the capture clause and (). Also,I don't think you need mutable here either for the same reason as before.


-    performFunctionOnMainThread([&value, this](AXCoreObject* object) {
+    setProperty(AXPropertyName::PreventKeyboardDOMEventDispatch, value);
+    performFunctionOnMainThread([value](AccessibilityObject* object) {

Missing space. Should we use auto* instead of AccessibilityObject* in all these lambda params?

+    setProperty(AXPropertyName::CaretBrowsingEnabled, value);
+    performFunctionOnMainThread([value](AccessibilityObject* object) {


Dito.


     if ([attributeName isEqualToString: NSAccessibilitySelectedTextAttribute]
-        || [attributeName isEqualToString: NSAccessibilitySelectedTextRangeAttribute]
-        || [attributeName isEqualToString: NSAccessibilityVisibleCharacterRangeAttribute])
+        || [attributeName isEqualToString: NSAccessibilitySelectedTextRangeAttribute])


While touching this, pleae remove space before params, per style.

-        if ([attributeName isEqualToString: NSAccessibilitySelectedTextAttribute]) {
+        if ([attributeName isEqualToString: NSAccessibilitySelectedTextAttribute])


Dito.

-        } else if ([attributeName isEqualToString: NSAccessibilitySelectedTextRangeAttribute]) {
+        else if ([attributeName isEqualToString: NSAccessibilitySelectedTextRangeAttribute])

Dito.

[Tyler Wilcock]

Created attachment 466226 [details]
Patch

[EWS]

Committed 263759@main (b7af35598b4e): <https://commits.webkit.org/263759@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 466226 [details].