Bug 256761 – AX: Avoid hitting the main thread in NSApplicationAccessibilityFocusedUIElement in isolated tree mode.

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Andres Gonzalez]

NSApplicationAccessibilityFocusedUIElement is frequently called by AT to track the focus object. Currently it has to hit the main thread every time.

[Radar WebKit Bug Importer]

<rdar://problem/109307200>

[Andres Gonzalez]

Created attachment 466345 [details]
Patch

[chris fleizach]

Do we need those methods that set the focused tree when we add or update? Seems like maybe this assumption won't always be true

[chris fleizach]

Can we assert that callers to focusedTree() will not be on the main thread?

[Tyler Wilcock]

Comment on attachment 466345 [details]
Patch

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

NSApplicationAccessibilityFocusedUIElement currently loops through the web process' web pages to get the focused one, and then calls AXObjectCache::focusedObjectForPage, which does a few transformations that I think your patch doesn't cover now (e.g. active descendant handling, image map focus handling).

Also, I don't think we have layout tests covering NSApplicationAccessibilityFocusedUIElement, so there's no way to know our behavior is the same before and after this patch. It would be nice to have tests simulating the tricky bits here -- e.g. removing a focused tree and expecting some result, focusing different webpages within the process, etc.

> Source/WebCore/accessibility/AXTreeStore.h:111
> +    // Assume that if a new tree is being added to the store, this must be the focused tree.
> +    setFocusedTreeID(axID);

What if we're adding a tree that has no focused element? It probably shouldn't overwrite the previously focused tree in that case.

> Source/WebCore/accessibility/AXTreeStore.h:143
> +    // If the tree being removed is the focused tree, make s_focusedTreeID invalid.
> +    {
> +        Locker locker { s_storeLock };
> +        if (axID == s_focusedTreeID)
> +            s_focusedTreeID = { };
> +    }

In this case, shouldn't the focused tree become the second most recently focused tree and not empty? E.g., given this sequence:

1. Tree A is added
2. Element in Tree A is focused, so A is now focused tree
3. Tree B is added with a focused element, tree B is now focused tree
4. Tree B is removed, but A still has a focused element, so it should be the focused tree. But in this patch, we will have no focused tree, which is not accurate, right?

[Andres Gonzalez]

Created attachment 466370 [details]
Patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Do we need those methods that set the focused tree when we add or update?
> Seems like maybe this assumption won't always be true

Reworked the mechanism to keep track of the focused Page based on the ActivityState which is what it is used behind the scene by the Page and WebPage.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #5)
> Comment on attachment 466345 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466345&action=review
> 
> NSApplicationAccessibilityFocusedUIElement currently loops through the web
> process' web pages to get the focused one, and then calls
> AXObjectCache::focusedObjectForPage, which does a few transformations that I
> think your patch doesn't cover now (e.g. active descendant handling, image
> map focus handling).

We need to handle those special cases as a result of a notification, not here. That way the AXIsolatedTree would know the focused object at any time. That should be a separate fix if we are not doing that already.
> 
> Also, I don't think we have layout tests covering
> NSApplicationAccessibilityFocusedUIElement, so there's no way to know our
> behavior is the same before and after this patch. It would be nice to have
> tests simulating the tricky bits here -- e.g. removing a focused tree and
> expecting some result, focusing different webpages within the process, etc.
> 
> > Source/WebCore/accessibility/AXTreeStore.h:111
> > +    // Assume that if a new tree is being added to the store, this must be the focused tree.
> > +    setFocusedTreeID(axID);
> 
> What if we're adding a tree that has no focused element? It probably
> shouldn't overwrite the previously focused tree in that case.
> 
> > Source/WebCore/accessibility/AXTreeStore.h:143
> > +    // If the tree being removed is the focused tree, make s_focusedTreeID invalid.
> > +    {
> > +        Locker locker { s_storeLock };
> > +        if (axID == s_focusedTreeID)
> > +            s_focusedTreeID = { };
> > +    }
> 
> In this case, shouldn't the focused tree become the second most recently
> focused tree and not empty? E.g., given this sequence:
> 
> 1. Tree A is added
> 2. Element in Tree A is focused, so A is now focused tree
> 3. Tree B is added with a focused element, tree B is now focused tree
> 4. Tree B is removed, but A still has a focused element, so it should be the
> focused tree. But in this patch, we will have no focused tree, which is not
> accurate, right?

Reworked all this to track the focused tree using the Page ActivityState.

[Tyler Wilcock]

Comment on attachment 466370 [details]
Patch

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

Nice, this approach matches the existing one nicely! Is it possible to add a test for this?

> Source/WebCore/accessibility/AXObjectCache.cpp:254
> +        m_pageActivityState = m_document.page()->activityState();

Is it safe to unconditionally dereference page() here? Some code above null-checks page().

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:683
> +void AXIsolatedTree::setPageActivityState(OptionSet<ActivityState> state)
> +{
> +    ASSERT(isMainThread());
> +
> +    Locker locker { s_storeLock };
> +    m_pageActivityState = state;
> +}

Nit, feel free to not address, but seems a little strange to use the s_storeLock for this purpose? Modifying this data doesn't feel related to the AXTreeStore. Maybe it makes sense to have a dedicated lock -- perhaps there's some reason that wouldn't work, though, since I think we rely on holding the lock when querying this state in the new findTree() method.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:320
> +    // Use only if the s_storeLock is already held.
> +    OptionSet<ActivityState> lockedPageActivityState() const { return m_pageActivityState; }

Would it be useful to ASSERT(s_storeLock.isLocked())?

[Andres Gonzalez]

Created attachment 466566 [details]
Patch

[Andres Gonzalez]

Created attachment 466568 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #9)
> Comment on attachment 466370 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466370&action=review
> 
> Nice, this approach matches the existing one nicely! Is it possible to add a
> test for this?

Now all tests that call accessibilityController::focusedElement are using this, more than 500 calls in LayoutTests/accessibility.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:254
> > +        m_pageActivityState = m_document.page()->activityState();
> 
> Is it safe to unconditionally dereference page() here? Some code above
> null-checks page().

The only way there is an m_pageID is if document.page() is not null
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:683
> > +void AXIsolatedTree::setPageActivityState(OptionSet<ActivityState> state)
> > +{
> > +    ASSERT(isMainThread());
> > +
> > +    Locker locker { s_storeLock };
> > +    m_pageActivityState = state;
> > +}
> 
> Nit, feel free to not address, but seems a little strange to use the
> s_storeLock for this purpose? Modifying this data doesn't feel related to
> the AXTreeStore. Maybe it makes sense to have a dedicated lock -- perhaps
> there's some reason that wouldn't work, though, since I think we rely on
> holding the lock when querying this state in the new findTree() method.

Yes, we need to hold the store Lock because of finding the focused tree with findAXTree.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:320
> > +    // Use only if the s_storeLock is already held.
> > +    OptionSet<ActivityState> lockedPageActivityState() const { return m_pageActivityState; }
> 
> Would it be useful to ASSERT(s_storeLock.isLocked())?

Done. Thanks.

[Andres Gonzalez]

Created attachment 466571 [details]
Patch

[Tyler Wilcock]

Comment on attachment 466571 [details]
Patch

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

> Source/WebCore/accessibility/AXTreeStore.h:87
> -    static HashMap<AXID, ThreadSafeWeakPtr<AXIsolatedTree>>& isolatedTreeMap() WTF_REQUIRES_LOCK(s_storeLock);
> +    static HashMap<AXID, ThreadSafeWeakPtr<AXIsolatedTree>>& isolatedTreeMap();

Why remove WTF_REQUIRES_LOCK(s_storeLock)? Seems like we still acquire the lock before calling this, right?

> Source/WebCore/page/Page.cpp:2683
> +    auto* localMainFrame = dynamicDowncast<LocalFrame>(m_mainFrame.get());

Should this be a WeakPtr / RefPtr? Seems like non-trivial work happens after we create this pointer, potentially destroying it.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:242
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> +    if (!isMainRunLoop()) {
> +        // Avoid hitting the main thread by getting the focused object from the focused isolated tree.
> +        auto tree = findAXTree([] (AXTreePtr tree) -> bool {
> +            OptionSet<ActivityState> state;
> +            switchOn(tree,
> +                [&state] (RefPtr<AXIsolatedTree>& typedTree) {
> +                    if (typedTree)
> +                        state = typedTree->lockedPageActivityState();
> +                }
> +                , [] (auto&) { }
> +            );
> +            return state.containsAll({ ActivityState::IsVisible, ActivityState::IsFocused, ActivityState::WindowIsActive });
> +        });
> +
> +        RefPtr object = switchOn(tree,
> +            [] (RefPtr<AXIsolatedTree>& typedTree) -> RefPtr<AXIsolatedObject> {
> +                return typedTree ? typedTree->focusedNode() : nullptr;
> +            }
> +            , [] (auto&) -> RefPtr<AXIsolatedObject> {
> +                return nullptr;
> +            }
> +        );
> +        return object ? object->wrapper() : nil;
> +    }
> +#endif
> +
> +    WebPage* page = WebProcess::singleton().focusedWebPage();
> +    if (!page || !page->accessibilityRemoteObject())
> +        return nil;
> +    return [page->accessibilityRemoteObject() accessibilityFocusedUIElement];

This new off main-thread codepath will result in different behavior because AXObjectCache::focusedObjectForPage handled two special cases:

    Element* focusedElement = document->focusedElement();
    if (is<HTMLAreaElement>(focusedElement))
        return focusedImageMapUIElement(downcast<HTMLAreaElement>(focusedElement));

and 

    if (focus->shouldFocusActiveDescendant()) {
        if (auto* descendant = focus->activeDescendant())
            focus = descendant;
    }

The former special case has a layout test (fast/events/tab-imagemap.html), but unfortunately said layout test doesn't test any accessibility state. It was added in https://github.com/WebKit/WebKit/commit/a5d019bcb572b5b60605910ee6ed3238f88848c5.

The latter special case (aria-activedescendant) does not have a layout test at all (added in 2008 with https://github.com/WebKit/WebKit/commit/eb24533a042e5e82dfad152998ac6dced1f8be39).

There's also this case at the bottom:

    // the HTML element, for example, is focusable but has an AX object that is ignored
    if (focus->accessibilityIsIgnored())
        focus = focus->parentObjectUnignored();

We'll probably need to do something here too when we compute the focused ITM node (if we don't already).

I understand these probably make sense as separate changes. But it makes me nervous to land this patch and have things be broken for an unknown amount of time until we land those follow-up fixes. What do you think is the best way forward?

[Andres Gonzalez]

Created attachment 466593 [details]
Patch

[Andres Gonzalez]

Created attachment 466606 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #14)
> Comment on attachment 466571 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466571&action=review
> 
> > Source/WebCore/accessibility/AXTreeStore.h:87
> > -    static HashMap<AXID, ThreadSafeWeakPtr<AXIsolatedTree>>& isolatedTreeMap() WTF_REQUIRES_LOCK(s_storeLock);
> > +    static HashMap<AXID, ThreadSafeWeakPtr<AXIsolatedTree>>& isolatedTreeMap();
> 
> Why remove WTF_REQUIRES_LOCK(s_storeLock)? Seems like we still acquire the
> lock before calling this, right?

Put it back, at some point it was giving me an error because it was used inside a lambda, but seems ok now.
> 
> > Source/WebCore/page/Page.cpp:2683
> > +    auto* localMainFrame = dynamicDowncast<LocalFrame>(m_mainFrame.get());
> 
> Should this be a WeakPtr / RefPtr? Seems like non-trivial work happens after
> we create this pointer, potentially destroying it.

Fixed.
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:242
> > +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> > +    if (!isMainRunLoop()) {
> > +        // Avoid hitting the main thread by getting the focused object from the focused isolated tree.
> > +        auto tree = findAXTree([] (AXTreePtr tree) -> bool {
> > +            OptionSet<ActivityState> state;
> > +            switchOn(tree,
> > +                [&state] (RefPtr<AXIsolatedTree>& typedTree) {
> > +                    if (typedTree)
> > +                        state = typedTree->lockedPageActivityState();
> > +                }
> > +                , [] (auto&) { }
> > +            );
> > +            return state.containsAll({ ActivityState::IsVisible, ActivityState::IsFocused, ActivityState::WindowIsActive });
> > +        });
> > +
> > +        RefPtr object = switchOn(tree,
> > +            [] (RefPtr<AXIsolatedTree>& typedTree) -> RefPtr<AXIsolatedObject> {
> > +                return typedTree ? typedTree->focusedNode() : nullptr;
> > +            }
> > +            , [] (auto&) -> RefPtr<AXIsolatedObject> {
> > +                return nullptr;
> > +            }
> > +        );
> > +        return object ? object->wrapper() : nil;
> > +    }
> > +#endif
> > +
> > +    WebPage* page = WebProcess::singleton().focusedWebPage();
> > +    if (!page || !page->accessibilityRemoteObject())
> > +        return nil;
> > +    return [page->accessibilityRemoteObject() accessibilityFocusedUIElement];
> 
> This new off main-thread codepath will result in different behavior because
> AXObjectCache::focusedObjectForPage handled two special cases:
> 
>     Element* focusedElement = document->focusedElement();
>     if (is<HTMLAreaElement>(focusedElement))
>         return
> focusedImageMapUIElement(downcast<HTMLAreaElement>(focusedElement));
> 
> and 
> 
>     if (focus->shouldFocusActiveDescendant()) {
>         if (auto* descendant = focus->activeDescendant())
>             focus = descendant;
>     }
> 
> The former special case has a layout test (fast/events/tab-imagemap.html),
> but unfortunately said layout test doesn't test any accessibility state. It
> was added in
> https://github.com/WebKit/WebKit/commit/
> a5d019bcb572b5b60605910ee6ed3238f88848c5.
> 
> The latter special case (aria-activedescendant) does not have a layout test
> at all (added in 2008 with
> https://github.com/WebKit/WebKit/commit/
> eb24533a042e5e82dfad152998ac6dced1f8be39).
> 
> There's also this case at the bottom:
> 
>     // the HTML element, for example, is focusable but has an AX object that
> is ignored
>     if (focus->accessibilityIsIgnored())
>         focus = focus->parentObjectUnignored();
> 
> We'll probably need to do something here too when we compute the focused ITM
> node (if we don't already).
> 
> I understand these probably make sense as separate changes. But it makes me
> nervous to land this patch and have things be broken for an unknown amount
> of time until we land those follow-up fixes. What do you think is the best
> way forward?

See https://bugs.webkit.org/show_bug.cgi?id=257739. Thanks.

[Andres Gonzalez]

Created attachment 466607 [details]
Patch

[Andres Gonzalez]

Created attachment 466627 [details]
Patch

[Andres Gonzalez]

Created attachment 466631 [details]
Patch

[Andres Gonzalez]

Created attachment 466635 [details]
Patch

[EWS]

Committed 264985@main (4ca184ab5339): <https://commits.webkit.org/264985@main>

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