[webkit-changes] [WebKit/WebKit] b095a6: AX: AXIsolatedObject::m_childrenIDs is cleared too...

[Tyler Wilcock]

  Branch: refs/heads/main
  Home:   https://github.com/WebKit/WebKit
  Commit: b095a6aaca2631aecf11ff0bc5b1c371ca698d2f
      https://github.com/WebKit/WebKit/commit/b095a6aaca2631aecf11ff0bc5b1c371ca698d2f
  Author: Tyler Wilcock <tyler_w at apple.com>
  Date:   2022-11-30 (Wed, 30 Nov 2022)

  Changed paths:
    M Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp
    M Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h

  Log Message:
  -----------
  AX: AXIsolatedObject::m_childrenIDs is cleared too early when processing AXIsolatedTree::m_pendingSubtreeRemovals
https://bugs.webkit.org/show_bug.cgi?id=248451
rdar://problem/102743478

Reviewed by Andres Gonzalez.

In `AXIsolatedTree::applyPendingChanges()`, when processing `m_pendingSubtreeRemovals`, we start
by taking the last axID in the Vector and using it to get the associated `nodeForID`. Then we call
`AXCoreObject::detach(AccessibilityDetachmentType::ElementDestroyed)`, which in turn calls
`AXIsolatedObject::detachRemoteParts`. Currently, the implementation of this function loops over
`m_childrenIDs` and detaches those children from `this`, and then clears `m_childrenIDs`.

Then, we jump back to `AXIsolatedTree::applyPendingChanges()` and run this line of code:

`m_pendingSubtreeRemovals.appendVector(object->m_childrenIDs);`

Which does nothing because we cleared `m_childrenIDs` as part of `detachRemoteParts`.

With this patch, the call to `detach` is replaced by `detachWrapper`, eschewing the call to `detachRemoteParts`.

There is no test in this patch because there is technically no behavior change. The children missed
by this early `m_childrenIDs` clear are added to `m_pendingSubtreeRemovals` as part of their own removal
from the DOM / render tree. This patch _would_ fix a bug for AX objects not cleaned up this way
(i.e. `AccessibilityMockObjects`, like the `AccessibilitySpinButton` created in `AccessibilityRenderObject::addTextFieldChildren()`),
but unfortunately we can't use that for a test either since we leak these types of objects in live tree
mode too[1] (which should be fixed in a future patch).

This patch also changes `queueRemovals`, `queueRemovalsLocked`, and `queueRemovalsAndUnresolvedChanges`
to take an r-value reference to their parameter Vectors as an optimization.

[1]: By this, I mean that we create them like so:

`auto& axSpinButton = downcast<AccessibilitySpinButton>(*axObjectCache()->create(AccessibilityRole::SpinButton));`

And never call `AXObjectCache::remove(AXID)` for this object, so its refcount never hits zero. We just constantly
create new ones each time `clearChildren` and `addChildren` is called.

* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:
(WebCore::AXIsolatedTree::queueRemovals):
(WebCore::AXIsolatedTree::queueRemovalsLocked):
(WebCore::AXIsolatedTree::queueRemovalsAndUnresolvedChanges):
(WebCore::AXIsolatedTree::updateChildren):
(WebCore::AXIsolatedTree::applyPendingChanges):
* Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.h:

Canonical link: https://commits.webkit.org/257217@main