Bug 234841 – AX: Make more tests async to pass in isolated tree mode

• Component: Accessibility
• Status: NEW

[Carlos Garcia Campos]

.

[Radar WebKit Bug Importer]

<rdar://problem/87087797>

[Carlos Garcia Campos]

Created attachment 448287 [details]
Patch

[Carlos Garcia Campos]

Created attachment 448290 [details]
Patch

[Tyler Wilcock]

In general, we're trying to only make tests async when necessary. If a test has only static content, it *should* pass in isolated tree mode without the test being made async. The only time a test should need to be async is if the test changes elements dynamically, i.e. via JavaScript. Otherwise we may be masking real isolated tree bugs.

It seems like a lot of the changes to tests in this patch only add:

window.jsTestIsAsync = true;

setTimeout(function() {

    ...test logic moved inside here...

    finishJSTest();
}, 0);

It surprises me that this makes the tests pass in isolated tree mode where they didn't before. Do you know why this could be?

[Carlos Garcia Campos]

(In reply to Tyler Wilcock from comment #4)
> In general, we're trying to only make tests async when necessary.

I agree, I'm converting only the ones that are failing.

> If a test
> has only static content, it *should* pass in isolated tree mode without the
> test being made async. The only time a test should need to be async is if
> the test changes elements dynamically, i.e. via JavaScript.

This is no longer the ase since r287533, see my comment in https://bugs.webkit.org/show_bug.cgi?id=234739#c5. It was because of a side effect of triggering a layout while populating the isolated tree.

> Otherwise we may
> be masking real isolated tree bugs.
> 
> It seems like a lot of the changes to tests in this patch only add:
> 
> window.jsTestIsAsync = true;
> 
> setTimeout(function() {
> 
>     ...test logic moved inside here...
> 
>     finishJSTest();
> }, 0);
> 
> It surprises me that this makes the tests pass in isolated tree mode where
> they didn't before. Do you know why this could be?

Yes, in many cases the ax wrappers haven't been attached to the isolated object yet.

[Carlos Garcia Campos]

I've analyzed in more detail what happens. Let's use a simple test as an example, like accessibility/language-attribute.html.

1- AccessibilityController::focusedElement() is called, when getting the root element the axObjectCache creates the isolated tree:
2- AXIsolatedTree::generateSubtree() is called for the root object, that already has one child, the web area.
 2.1-  AXIsolatedTree::createSubtree is called for the root object.
 2.2- AXIsolatedTree::createSubtree is called for the web area, but web area childrenCount is 0 a this point.
 2.3- AXIsolatedTree::generateSubtree() finished with 2 pending children updates
3- AccessibilityController::focusedElement() returns the web area element
4- AccessibilityUIElement::childAtIndex(0) is called on web area element
 4.1- executeOnAXThreadAndWait() is called to get the element first child
 4.2- On the main thread update backing store is called, so AXIsolatedTree::applyPendingChanges() is called and the 2 pending children updates are processed
 4.3- AXIsolatedObject::children() is called on web area element, and returns an empty array, so nullptr is returned as child.
 4.4- AXObjectCache::notificationPostTimerFired is called on the main thread (executeOnAXThreadAndWait iterates the main thread while waiting for the ax thread to complete the task), and that calls AXObjectCache::updateIsolatedTree() that ends up calling AXIsolatedTree::updateChildren() and now is when the rest of the isolated tree is populated, but it's too late because the ax thread already returned nullptr for the child.

The test fails with:

CONSOLE MESSAGE: TypeError: null is not an object (evaluating 'accessibilityObject.childrenCount')

accessibilityObject is webArea.childAtIndex(0) for which we returned null. 

This is what I tried to fix in bug #232141 by notifying about children changed earlier.

I can try other solutions like processing main thread loop also before going to the ax thread.

[Carlos Garcia Campos]

So, processing any pending events on main thread loop before going to the ax thread in AccessibilityController::executeOnAXThreadAndWait() worked for me. If you don't want to make the tests async and it's not needed for mac we can just close this (and bug #234822) and I'll workaround it in WTR for atspi.

[Tyler Wilcock]

Thanks Carlos, your investigation here and in https://bugs.webkit.org/show_bug.cgi?id=234739 has been really helpful.

This is definitely a big problem for us in the Mac isolated tree layout tests, both before and after https://commits.webkit.org/r287533 (but especially after :)). It's good that you caught that.

Andres thinks that this is symptomatic of a deeper issue in the core isolated tree logic, so he has some experiments he's going to try. We have done some limited testing with isolated tree + Safari internally and found that VoiceOver misses large swaths of content only in isolated tree mode, which may or may not be related to this problem.

For now, we aren't ready to move forward with this async test patch, or changing AccessibilityController::executeOnAXThreadAndWait. It would be helpful if you could:

1. Post another patch (without obsoleting the current one) with your changes to AccessibilityController::executeOnAXThreadAndWait as mentioned in comment 7
2. Leave this bug open for now while we investigate further

But in the meantime, feel free to workaround in WTR for atspi. If we find a core issue, and a fix for that issue, we can see if these changes aren't necessary. Otherwise we'll revisit after some more investigation.

[Carlos Garcia Campos]

(In reply to Tyler Wilcock from comment #8)
> Thanks Carlos, your investigation here and in
> https://bugs.webkit.org/show_bug.cgi?id=234739 has been really helpful.
> 
> This is definitely a big problem for us in the Mac isolated tree layout
> tests, both before and after https://commits.webkit.org/r287533 (but
> especially after :)). It's good that you caught that.
> 
> Andres thinks that this is symptomatic of a deeper issue in the core
> isolated tree logic, so he has some experiments he's going to try. We have
> done some limited testing with isolated tree + Safari internally and found
> that VoiceOver misses large swaths of content only in isolated tree mode,
> which may or may not be related to this problem.

Ok, let me know if I can help somehow.

> For now, we aren't ready to move forward with this async test patch, or
> changing AccessibilityController::executeOnAXThreadAndWait. It would be
> helpful if you could:
> 
> 1. Post another patch (without obsoleting the current one) with your changes
> to AccessibilityController::executeOnAXThreadAndWait as mentioned in comment
> 7
> 2. Leave this bug open for now while we investigate further

Ok, I'll do it, but opening a new bug to attach the atspi workaround patch.

> But in the meantime, feel free to workaround in WTR for atspi. If we find a
> core issue, and a fix for that issue, we can see if these changes aren't
> necessary. Otherwise we'll revisit after some more investigation.

I'll leave bug #234822 in stand by too.

[Carlos Garcia Campos]

I've investigated the issue a bit more. I realized that the initial problem of not getting the child after focusedElement, is because when AXIsolatedTree::create() is called the renderers haven't been attached yet, that's why the web area reports 0 children. But then I noticed that renderers were attached right after the tree is initially populated. It ended up being the call to focusedObjectForPage() in AXIsolatedTree::create() that calls Document::updateStyleIfNeeded(). If I move the call to focusedObjectForPage() before the generateSubtree() or explicitly call Document::updateStyleIfNeeded() then when the tree is initially populated the web area has children and the problem of getting the children after focusedElement is fixed.

But unfortunately that didn't fix the tests. For example with accessibility/image-map1.html I got a valid role for the first link, but null title because the returned child is detached between the call tom role() and title(). This happens because the first layout after the tree is populated causes the children to be recomputed, due to changes in ignored flag. So, I tried by doing Document::updateLayout() instead of Document::updateStyleIfNeeded() in AXIsolatedTree::create(), and it somehow worked, now we get the right child after the ignore flag is recomputed, but then I got the same problem with the second link, this time another childrenChanged caused the returned child to be detached before role() is called. And this is caused the main thread run loop iterations in AccessibilityController::executeOnAXThreadAndWait() while waiting for the ax thread to complete (not the workaround I tried, the ones after calling axRunLoop().dispatch()). So, removing those and using a semaphore as we used to do finally fixed accessibility/image-map1.html, but caused other tests to timeout (I guess the cases in which we call the main thread from the ax thread).

So, I still don't have a solution...

[Carlos Garcia Campos]

In the end I managed to make most of the test pass by doing the layout in AXIsolatedTree::create and only processing main thread events while waiting for the ax thread when the ax thread is probably blocked waiting for the main thread. See bug #234950. I don't like the solution of only processing main thread events after a timeout, but at least it seems to work for now. I think the other tests still failing really need to be made async. I'll check them.