Summary: | AX: Use RefPtr<AXCoreObject> instead of raw AXCoreObject* pointers in Accessibility::findMatchingObjects and downstream functions | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tyler Wilcock <tyler_w> | ||||||||||||
Component: | Accessibility | Assignee: | 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: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Tyler Wilcock
2021-12-06 11:44:54 PST
Created attachment 446064 [details]
Patch
Comment on attachment 446064 [details]
Patch
will this cause more RefPtr churn than its worth? what's the main benefit
Created attachment 446242 [details]
Patch
Created attachment 446243 [details]
Patch
(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). (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. (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. Created attachment 446383 [details]
Patch
Created attachment 446436 [details]
Patch
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]. |