Bug 256288 – AX: Build the isolated tree asynchroniously.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

Currently the isolated tree is built on first request blocking the response to a client.

[Radar WebKit Bug Importer]

<rdar://problem/108871141>

[Andres Gonzalez]

Created attachment 466203 [details]
Patch

[Tyler Wilcock]

Comment on attachment 466203 [details]
Patch

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

> Source/WebCore/accessibility/AXObjectCache.cpp:233
> +    , m_buildIsolatedTreeTimer(*this, &AXObjectCache::buildIsolatedTree)

I think we need to add an equivalent to the !ENABLE(ACCESSIBLITY) AXObjectCache constructor to avoid breaking their build. See how other timers are handled (e.g. m_performCacheUpdateTimer + performCacheUpdateTimerFired)

> Source/WebCore/accessibility/AXObjectCache.cpp:843
> +    Accessibility::performFunctionOnMainThread([&tree, this] () {
> +        tree = AXIsolatedTree::createEmpty(this);
> +        setIsolatedTreeRoot(tree->rootNode().get());
> +    });

It might be helpful to have a comment here explaining why we are creating an empty tree.

> Source/WebCore/accessibility/AXObjectCache.cpp:859
> +    m_buildIsolatedTreeTimer.stop();

Would it be more robust to stop the timer first, even before the m_pageID check? That way AXObjectCaches without pageIDs don't have runaway timers (although that's not a concern because you use startOneShot...but might be more robust anyways?)

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:90
> +    Locker locker { s_storeLock };
> +    treePageCache().set(*pageID, tree.copyRef());

Maybe overkill so feel free to ignore, but rather than holding the lock until the end of the function, including the return, what if we scoped it to just the statement we need it for:

{
    Locker locker { s_storeLock };
    treePageCache().set(*pageID, tree.copyRef());
}

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:91
> +    tree->updateLoadingProgress(axObjectCache->loadingProgress());

Is it necessary to hold the s_storeLock when calling updateLoadingProgress? Or can we move it before the lock acquisition?

[Andres Gonzalez]

Created attachment 466214 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 466203 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=466203&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:233
> > +    , m_buildIsolatedTreeTimer(*this, &AXObjectCache::buildIsolatedTree)
> 
> I think we need to add an equivalent to the !ENABLE(ACCESSIBLITY)
> AXObjectCache constructor to avoid breaking their build. See how other
> timers are handled (e.g. m_performCacheUpdateTimer +
> performCacheUpdateTimerFired)

I think it is not needed because this line in the cmake takes care of it:

    WEBKIT_OPTION_DEPEND(ENABLE_ACCESSIBILITY_ISOLATED_TREE ENABLE_ACCESSIBILITY)

but if the build fails for other platforms, will add it.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:843
> > +    Accessibility::performFunctionOnMainThread([&tree, this] () {
> > +        tree = AXIsolatedTree::createEmpty(this);
> > +        setIsolatedTreeRoot(tree->rootNode().get());
> > +    });
> 
> It might be helpful to have a comment here explaining why we are creating an
> empty tree.

Added the comment and in deeper examination, this function cannot be called off the main thread, so removed the dispatch to the main thread altogether. The reason why that was there in the first place is because of the TestRunner was calling this on the AX thread.

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:859
> > +    m_buildIsolatedTreeTimer.stop();
> 
> Would it be more robust to stop the timer first, even before the m_pageID
> check? That way AXObjectCaches without pageIDs don't have runaway timers
> (although that's not a concern because you use startOneShot...but might be
> more robust anyways?)

Moved it to the top of the function, but was wondering if we need to stop at all a one shot timer. The reason I put it at the end before was to avoid another build of the iso tree to start while one was already in progress, but since this runs all on the main thread, that is not possible.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:90
> > +    Locker locker { s_storeLock };
> > +    treePageCache().set(*pageID, tree.copyRef());
> 
> Maybe overkill so feel free to ignore, but rather than holding the lock
> until the end of the function, including the return, what if we scoped it to
> just the statement we need it for:
> 
> {
>     Locker locker { s_storeLock };
>     treePageCache().set(*pageID, tree.copyRef());
> }

Yes, we ought to go all the way here... Added a a storeTree method to do this. Eventually I would like to get rid off the PageID map, it is actually not needed, we could use the TreeStore for everything.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:91
> > +    tree->updateLoadingProgress(axObjectCache->loadingProgress());
> 
> Is it necessary to hold the s_storeLock when calling updateLoadingProgress?
> Or can we move it before the lock acquisition?

I thought so, because trusted my memory and we were talking about using the lock or an std::atomic and going back and forth, and thought that ended up using the lock. Fixed now.

[Andres Gonzalez]

Created attachment 466217 [details]
Patch

[Tyler Wilcock]

Comment on attachment 466217 [details]
Patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:130
> +    treePageCache().set(*axObjectCache->pageID(), tree.copyRef());

Ref::copyRef was deprecated here:

https://bugs.webkit.org/show_bug.cgi?id=211705

You might just be able to use the implicit copy-constructor here?

[EWS]

Found 1 new test failure: imported/w3c/web-platform-tests/screen-orientation/fullscreen-interactions.html

[EWS]

Committed 263752@main (290519ffb9d7): <https://commits.webkit.org/263752@main>

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