Bug 233888 – AX: Use RefPtr<AXCoreObject> instead of raw AXCoreObject* pointers in Accessibility::findMatchingObjects and downstream functions

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

[Tyler Wilcock]

Move some usages of raw pointers to RefPtr.

[Radar WebKit Bug Importer]

<rdar://problem/86115809>

[Tyler Wilcock]

Created attachment 446064 [details]
Patch

[chris fleizach]

Comment on attachment 446064 [details]
Patch

will this cause more RefPtr churn than its worth? what's the main benefit

[Tyler Wilcock]

Created attachment 446242 [details]
Patch

[Tyler Wilcock]

Created attachment 446243 [details]
Patch

[Tyler Wilcock]

(In reply to chris fleizach from comment #3)
> Comment on attachment 446064 [details]
> Patch
> 
> will this cause more RefPtr churn than its worth? what's the main benefit
This fixes two isolated tree mode only crashes (updated the changelog to say this, too).

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #6)
> (In reply to chris fleizach from comment #3)
> > Comment on attachment 446064 [details]
> > Patch
> > 
> > will this cause more RefPtr churn than its worth? what's the main benefit
> This fixes two isolated tree mode only crashes (updated the changelog to say
> this, too).

I think we need to get to the bottom of why the pointers become dangling in ITM. Otherwise, this may be masking the actual bug.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #7)
> (In reply to Tyler Wilcock from comment #6)
> > (In reply to chris fleizach from comment #3)
> > > Comment on attachment 446064 [details]
> > > Patch
> > > 
> > > will this cause more RefPtr churn than its worth? what's the main benefit
> > This fixes two isolated tree mode only crashes (updated the changelog to say
> > this, too).
> 
> I think we need to get to the bottom of why the pointers become dangling in
> ITM. Otherwise, this may be masking the actual bug.
They dangle in ITM because this sequence of events can happen:

1. Search begins on secondary thread. We store potential candidate object pointers on the stack.

2. Simultaneously, one or more searches begin on the main thread (or any other operation that could change the tree). Searches call the update version of children(). This queues up changes to children for the isolated tree that can result in any object losing refcounts, and therefore potentially being destroyed (including the ones we stored above as a raw pointer rather than RefPtrs)

3. In the secondary thread, AXIsolatedObject::children can then be called as the search continues, which calls AXIsolatedTree::applyPendingChanges.

4. The changes from step 2 are applied, and the objects we only hold raw pointers to could have been destroyed.

By holding RefPtrs during search, we delay dropping objects until the search is over.

As a follow-up to this patch, Chris and I discussed introducing a mechanism to defer applying any pending changes to the isolated tree while a search is running. This patch may prevent dangling pointers in ITM, but doesn't prevent unexpected search results due to tree changes mid-search.

[Tyler Wilcock]

Created attachment 446383 [details]
Patch

[Tyler Wilcock]

Created attachment 446436 [details]
Patch

[EWS]

Committed r286780 (245021@main): <https://commits.webkit.org/245021@main>

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