Bug 234985 – Make [WKAccessibilityWebPageObjectBase accessibilityRootObjectWrapper]handle calls on and off the main thread properly.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Accessibility

[Andres Gonzalez]

Make [WKAccessibilityWebPageObjectBase accessibilityRootObjectWrapper]handle calls on and off the main thread properly.

[Radar WebKit Bug Importer]

<rdar://problem/87272381>

[Andres Gonzalez]

Created attachment 448632 [details]
Patch

[chris fleizach]

Comment on attachment 448632 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:84
> +    if (!WebCore::AXObjectCache::accessibilityEnabled())

this code looks pretty identical to whats inside the lambda from accessibilityRootObjectWrapper

1) accessibilityRootObjectWrapper already retrieves this value from the main thread. do we need to have a separate condition to do it here?
2) can we consolidate the between these two methods?

[Andres Gonzalez]

Created attachment 448742 [details]
Patch

[chris fleizach]

Comment on attachment 448742 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:92
> +            WebCore::AXCoreObject* root;

should we make this a ternary?

auto* root = mainThread ? cache->rootAXObject() : cache->rootObject();

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:93
> +            if (mainThread)

do we need this mainThread check at all because the mainThread cases will be handled with the if (isMainRunLoop())
case at line 105?

[Andres Gonzalez]

Stack trace that shows the scenario in which this condition occurs:

(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
  * frame #0: 0x00000007275b732e JavaScriptCore`::WTFCrash() at Assertions.cpp:322:35
    frame #1: 0x000000073ab7c34b WebCore`WTFCrashWithInfo((null)=447, (null)="./accessibility/isolatedtree/AXIsolatedTree.cpp", (null)="void WebCore::AXIsolatedTree::applyPendingChanges()", (null)=1006) at Assertions.h:732:5
    frame #2: 0x000000073d4b45f6 WebCore`WebCore::AXIsolatedTree::applyPendingChanges(this=0x00000007051ad960) at AXIsolatedTree.cpp:447:5
    frame #3: 0x000000073d4b211d WebCore`WebCore::AXIsolatedObject::updateBackingStore(this=0x00000007050a6f80) at AXIsolatedObject.cpp:1001:15
    frame #4: 0x000000073fd2558c WebCore`-[WebAccessibilityObjectWrapperBase updateObjectBackingStore](self=0x00007f84b6c128f0, _cmd="updateObjectBackingStore") at WebAccessibilityObjectWrapperBase.mm:335:20
    frame #5: 0x000000073fd44c31 WebCore`-[WebAccessibilityObjectWrapper accessibilityFocusedUIElement](self=0x00007f84b6c128f0, _cmd="accessibilityFocusedUIElement") at WebAccessibilityObjectWrapperMac.mm:2938:32
    frame #6: 0x000000070d3c9471 WebKit`-[WKAccessibilityWebPageObjectBase accessibilityFocusedUIElement](self=0x00007f84b6c1c110, _cmd="accessibilityFocusedUIElement") at WKAccessibilityWebPageObjectBase.mm:138:12
    frame #7: 0x000000070d3e0205 WebKit`WebKit::NSApplicationAccessibilityFocusedUIElement((null)=0x00007f84b5f0ff80, (null)="accessibilityFocusedUIElement") at WebProcessCocoa.mm:202:12
...

I.e., [WKAccessibilityWebPageObjectBase accessibilityFocusedUIElement] calls the same method on the returned value of accessibilityRootObjectWrapper that would be an IsolatedObject if Isolated tree mode has already been enabled, resulting in doing an AXIsolatedObject::updateBackingStore which should only happen off the main thread.

[Darin Adler]

Comment on attachment 448742 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.h:154
> +    WEBCORE_EXPORT AccessibilityObject* rootAXObject();

I think we need easier to understand terminology.

Nothing in the name AXObjectCache::rootAXObject makes me understand that "AXObject" here means a non-isolated object that is only suitable for the main thread and that is distinct from AXObjectCache::rootObject; the comments are explicit but the functions names themselves aren’t clear enough to make the code using them understandable.

Even the facts "AXIsolatedObejct is not an AXObject" and "AccessibilityObject can be either AXObject or AXIsolatedObject" are hard to understand given our names. And shows some flaws in our terminology. It’s worth a little thinking if we want this code to be maintainable and understandable in the future. A little renaming might solve this problem. Even the idea that this class is called AXObjectCache, but is not for AXObjects.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:110
> +    return ax::retrieveAutoreleasedValueFromMainThread<id>([&rootObjectWrapper, protectedSelf = retainPtr(self)] () -> RetainPtr<id> {
> +        return rootObjectWrapper(protectedSelf.get(), false);
>      });

I don’t understand the reason for the retainPtr(self). Given this is synchronous and we won’t return until the thread hopping is done. I don’t think it’s needed.

[Andres Gonzalez]

This change is no longer necessary since bug 235046 solves the underlying issue.

*** This bug has been marked as a duplicate of bug 235046 ***