Bug 239398 – Node changes created by AXIsolatedTree::updateNode are overwritten when performed during AXIsolatedTree::collectNodeChangesForSubtree

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

[Tyler Wilcock]

We currently do the wrong thing in this scenario:

  1. A dynamic page change causes an element to be included on the page, so we create a node change for it and its subtree by calling collectNodeChangesForSubtree. This causes a call to children() on the live object

  2. In the process of (or after) updating the children of the live object, we perform some operation that triggers AXIsolatedTree::updateNode on said object. AccessibilityRenderObject::updateRoleAfterChildrenCreation would be an example of this if it were properly coded to update the isolated tree if the object's role changes (I'll address that in a separate patch). updateNode results in a node change with the correct properties being added to m_pendingAppends

  3. collectNodeChangesForSubtree (started in step 1) finishes, and queues a node change for the same object, but with the wrong properties (because it was created before step 2). Because it comes after the node change added in step 2 to m_pendingAppends, it wins, and we add an object with the wrong properties to the tree

[Radar WebKit Bug Importer]

<rdar://problem/91820790>

[Tyler Wilcock]

Created attachment 457715 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 457715 [details]
> Patch

This change seems right for the most part. However, some of the cleanup made me realize a bug in the existing code that was the reason the parameter attachWrapper was added. In 

void AXIsolatedTree::updateNode(AXCoreObject& axObject)
{
...
    auto change = nodeChangeForObject(axObject, parentID, true);
...

we have to pass false instead. The reason is that you cannot attach a wrapper on the main thread when that wrapper is already being used on the AX thread.

[Andres Gonzalez]

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

[Tyler Wilcock]

Created attachment 457738 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #5)
> Created attachment 457738 [details]
> Patch

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h

+    enum class AttachWrapperImmediately : bool { No, Yes };

I think it would be more accurate:

+    enum class AttachWrapper : bool { OnMainThread, OnAXThread };

[Tyler Wilcock]

Created attachment 457750 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #7)
> Created attachment 457750 [details]
> Patch

I see a potential problem with splitting the queuingg of removals and appends:

-    queueChangesAndRemovals(changes, oldChildrenIDs);
+    resolveAndQueueChanges();
+    queueRemovals(oldChildrenIDs);

The appends and the removals may be applied in two separate calls of applyPendingChanges, since now we are releasing the lock in between the queuing. Would that have some unintended side effect?

[Tyler Wilcock]

Created attachment 457752 [details]
Patch

[Tyler Wilcock]

Created attachment 457753 [details]
Patch

[Tyler Wilcock]

Created attachment 457754 [details]
Patch

[Tyler Wilcock]

Created attachment 457770 [details]
Patch

[EWS]

Committed r292967 (249732@main): <https://commits.webkit.org/249732@main>

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