Bug 233888 - AX: Use RefPtr<AXCoreObject> instead of raw AXCoreObject* pointers in Accessibility::findMatchingObjects and downstream functions
Summary: AX: Use RefPtr<AXCoreObject> instead of raw AXCoreObject* pointers in Accessi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-06 11:44 PST by Tyler Wilcock
Modified: 2021-12-09 07:42 PST (History)
10 users (show)

See Also:


Attachments
Patch (8.44 KB, patch)
2021-12-06 11:54 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (8.66 KB, patch)
2021-12-07 15:20 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (8.80 KB, patch)
2021-12-07 15:24 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (11.21 KB, patch)
2021-12-08 10:32 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (10.13 KB, patch)
2021-12-08 15:18 PST, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2021-12-06 11:44:54 PST
Move some usages of raw pointers to RefPtr.
Comment 1 Radar WebKit Bug Importer 2021-12-06 11:45:09 PST
<rdar://problem/86115809>
Comment 2 Tyler Wilcock 2021-12-06 11:54:20 PST
Created attachment 446064 [details]
Patch
Comment 3 chris fleizach 2021-12-06 11:59:28 PST
Comment on attachment 446064 [details]
Patch

will this cause more RefPtr churn than its worth? what's the main benefit
Comment 4 Tyler Wilcock 2021-12-07 15:20:49 PST
Created attachment 446242 [details]
Patch
Comment 5 Tyler Wilcock 2021-12-07 15:24:07 PST
Created attachment 446243 [details]
Patch
Comment 6 Tyler Wilcock 2021-12-07 15:24:48 PST
(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).
Comment 7 Andres Gonzalez 2021-12-07 15:41:13 PST
(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.
Comment 8 Tyler Wilcock 2021-12-07 16:23:03 PST
(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.
Comment 9 Tyler Wilcock 2021-12-08 10:32:40 PST
Created attachment 446383 [details]
Patch
Comment 10 Tyler Wilcock 2021-12-08 15:18:16 PST
Created attachment 446436 [details]
Patch
Comment 11 EWS 2021-12-09 07:42:13 PST
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].