Bug 239398

Summary: Node changes created by AXIsolatedTree::updateNode are overwritten when performed during AXIsolatedTree::collectNodeChangesForSubtree
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch ews-feeder: commit-queue-

Description Tyler Wilcock 2022-04-15 11:56:02 PDT 
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
Comment 1 Radar WebKit Bug Importer 2022-04-15 11:56:11 PDT 
<rdar://problem/91820790>
Comment 2 Tyler Wilcock 2022-04-15 12:23:54 PDT 
Created attachment 457715 [details]
Patch
Comment 3 Andres Gonzalez 2022-04-15 13:44:27 PDT 
(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.
Comment 4 Andres Gonzalez 2022-04-15 14:15:34 PDT 
See https://bugs.webkit.org/show_bug.cgi?id=239402.
Comment 5 Tyler Wilcock 2022-04-15 18:01:02 PDT 
Created attachment 457738 [details]
Patch
Comment 6 Andres Gonzalez 2022-04-16 10:16:07 PDT 
(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 };
Comment 7 Tyler Wilcock 2022-04-16 10:50:27 PDT 
Created attachment 457750 [details]
Patch
Comment 8 Andres Gonzalez 2022-04-16 11:45:36 PDT 
(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?
Comment 9 Tyler Wilcock 2022-04-16 13:12:37 PDT 
Created attachment 457752 [details]
Patch
Comment 10 Tyler Wilcock 2022-04-16 13:41:48 PDT 
Created attachment 457753 [details]
Patch
Comment 11 Tyler Wilcock 2022-04-16 13:55:36 PDT 
Created attachment 457754 [details]
Patch
Comment 12 Tyler Wilcock 2022-04-17 08:07:11 PDT 
Created attachment 457770 [details]
Patch
Comment 13 EWS 2022-04-18 13:30:53 PDT 
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].