| Summary: | AX: Fix for crash in AXIsolatedTree::removeNode. | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Andres Gonzalez <andresg_22> | ||||||||||||
| Component: | Accessibility | Assignee: | Andres Gonzalez <andresg_22> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, changseok, dmazzoni, esprehn+autocc, ews-watchlist, glenn, jcraig, jdiggs, kondapallykalyan, pdr, samuel_white, tyler_w, webkit-bug-importer | ||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||
| Hardware: | All | ||||||||||||||
| OS: | All | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Andres Gonzalez
2023-01-23 15:10:06 PST
Created attachment 464613 [details]
Patch
Comment on attachment 464613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464613&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1052 > + // The removal needs to be async because this is called during a RenderTree > + // update and remove(AXID) updates the isolated tree that in turn calls > + // parentObjectUnignored() on the object being removed. I feel like this comment explanation doesn't go far enough — why is it bad to call parentObjectUnignored() on an object that is being removed? I know the answer from reading your commit message, but it could be useful to put here too. > Source/WebCore/accessibility/AXObjectCache.cpp:3469 > + AXLOG(makeString("RemovedObjects size ", m_deferredRemovedObjects.size())); Not something to worry about for this patch, but in my opinion it would be nice if these m_deferredFoo log statements in performDeferredCacheUpdate didn't print anything unless their size was > 0. Right now they're very spammy and make reading the logs quite a bit harder. Created attachment 464631 [details]
Patch
(In reply to Tyler Wilcock from comment #3) > Comment on attachment 464613 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=464613&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:1052 > > + // The removal needs to be async because this is called during a RenderTree > > + // update and remove(AXID) updates the isolated tree that in turn calls > > + // parentObjectUnignored() on the object being removed. > > I feel like this comment explanation doesn't go far enough — why is it bad > to call parentObjectUnignored() on an object that is being removed? I know > the answer from reading your commit message, but it could be useful to put > here too. Expanded comment. > > > Source/WebCore/accessibility/AXObjectCache.cpp:3469 > > + AXLOG(makeString("RemovedObjects size ", m_deferredRemovedObjects.size())); > > Not something to worry about for this patch, but in my opinion it would be > nice if these m_deferredFoo log statements in performDeferredCacheUpdate > didn't print anything unless their size was > 0. Right now they're very > spammy and make reading the logs quite a bit harder. Separate bug/patch. (In reply to Tyler Wilcock from comment #3) > Comment on attachment 464613 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=464613&action=review > > > Source/WebCore/accessibility/AXObjectCache.cpp:1052 > > + // The removal needs to be async because this is called during a RenderTree > > + // update and remove(AXID) updates the isolated tree that in turn calls > > + // parentObjectUnignored() on the object being removed. > > I feel like this comment explanation doesn't go far enough — why is it bad > to call parentObjectUnignored() on an object that is being removed? I know > the answer from reading your commit message, but it could be useful to put > here too. > > > Source/WebCore/accessibility/AXObjectCache.cpp:3469 > > + AXLOG(makeString("RemovedObjects size ", m_deferredRemovedObjects.size())); > > Not something to worry about for this patch, but in my opinion it would be > nice if these m_deferredFoo log statements in performDeferredCacheUpdate > didn't print anything unless their size was > 0. Right now they're very > spammy and make reading the logs quite a bit harder. See https://bugs.webkit.org/show_bug.cgi?id=251121. Comment on attachment 464631 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=464631&action=review > COMMIT_MESSAGE:9 > +The crash happens in ITM because AXObjectCache::remove updates the isolated tree by calling AXIsolatedTree::removeNode, that calls parentObjectUnignored(), which results in a call to textUnderElement which cannot be called during a layout. The solution in this patch is to make the removal of the object in question asynchroniously. spelling: asynchroniously Created attachment 464652 [details]
Patch
Created attachment 464686 [details]
Patch
Created attachment 464687 [details]
Patch
Committed 259484@main (66bfe7c6900e): <https://commits.webkit.org/259484@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 464687 [details]. |