Bug 243456

Summary: AX ITM: Avoid dispatching AXIsolatedObject methods to the main thread when it is busy updating the isolated tree.
Product: WebKit Reporter: Andres Gonzalez <andresg_22>
Component: AccessibilityAssignee: Andres Gonzalez <andresg_22>
Status: NEW ---    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, darin, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch andresg_22: review?, ews-feeder: commit-queue-

Description Andres Gonzalez 2022-08-02 08:29:43 PDT 
This causes VoiceOver to become irresponsive.
Comment 1 Radar WebKit Bug Importer 2022-08-02 08:29:55 PDT 
<rdar://problem/97982075>
Comment 2 Andres Gonzalez 2022-08-02 08:38:56 PDT 
Created attachment 461361 [details]
Patch
Comment 3 Darin Adler 2022-08-03 12:39:59 PDT 
Comment on attachment 461361 [details]
Patch

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

I didn’t get a chance to review the whole thing, but a couple comments.

> Source/WebCore/accessibility/AccessibilityObjectInterface.h:1695
> +T retrieveValueFromMainThreadWithTimeout(U&& function, Seconds&& timeout)

Since Seconds is a class wrapping a double, we can just use Seconds, we don’t need to use Seconds&&.

> Source/WebCore/accessibility/AccessibilityObjectInterface.h:1708
> +    while (!done && !hasElapsed(elapseTime)) { }

This kind of loop is not an efficient way to have one thread wait on another. Surely there is a better idiom for such things using locks, mutexes, or semaphores of some kind.
Comment 4 Andres Gonzalez 2022-08-12 06:53:25 PDT 
Created attachment 461559 [details]
Patch

Extended the check for whether to dispatch to the main thread to all AXIsolatedObject methods that were using Accessibility::retrieveValueFromMainThread.
Comment 5 Andres Gonzalez 2022-08-12 06:59:48 PDT 
(In reply to Darin Adler from comment #3)
> Comment on attachment 461361 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=461361&action=review
> 
> I didn’t get a chance to review the whole thing, but a couple comments.
> 
> > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1695
> > +T retrieveValueFromMainThreadWithTimeout(U&& function, Seconds&& timeout)
> 
> Since Seconds is a class wrapping a double, we can just use Seconds, we
> don’t need to use Seconds&&.

Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityObjectInterface.h:1708
> > +    while (!done && !hasElapsed(elapseTime)) { }
> 
> This kind of loop is not an efficient way to have one thread wait on
> another. Surely there is a better idiom for such things using locks,
> mutexes, or semaphores of some kind.

Yes, still working on that. I'm getting deadlocks using BinarySemaphore that I'm still debugging. I'm keeping this for now so that it is testable but not the final version yet. Thanks!