251045 – AX: Fix for crash in AXIsolatedTree::removeNode.

RESOLVED FIXED251045

AX: Fix for crash in AXIsolatedTree::removeNode.

https://bugs.webkit.org/show_bug.cgi?id=251045

Summary AX: Fix for crash in AXIsolatedTree::removeNode.

Andres Gonzalez

Reported 2023-01-23 15:10:06 PST

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x7ff81f56ff22 WTFCrashWithInfo(int, char const*, char const*, int) + 18 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.0.Internal.sdk/usr/local/include/wtf/Assertions.h:754) 1 com.apple.WebCore 0x7ff81f3f8202 WebCore::Document::updateLayout() + 610 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./dom/Document.cpp:2244) 2 com.apple.WebCore 0x7ff8208ed2a0 WebCore::TextIterator::TextIterator(WebCore::SimpleRange const&, WTF::OptionSet<WebCore::TextIteratorBehavior>) + 240 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./dom/Document.cpp:2274) 3 com.apple.WebCore 0x7ff8208e8a9c WebCore::plainText(WebCore::SimpleRange const&, WTF::OptionSet<WebCore::TextIteratorBehavior>, bool) + 108 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./editing/TextIterator.cpp:354) 4 com.apple.WebCore 0x7ff8203aa7b6 WebCore::AccessibilityRenderObject::textUnderElement(WebCore::AccessibilityTextUnderElementMode) const + 998 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/AccessibilityRenderObject.cpp:705) 5 com.apple.WebCore 0x7ff8203ab044 WebCore::AccessibilityRenderObject::stringValue() const + 372 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/AccessibilityRenderObject.cpp:779) 6 com.apple.WebCore 0x7ff81f811208 WebCore::AccessibilityObject::accessibilityPlatformIncludesObject() const + 456 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/mac/AccessibilityObjectMac.mm:128) 7 com.apple.WebCore 0x7ff8203ade50 WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const + 48 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/AccessibilityRenderObject.cpp:1298) 8 com.apple.WebCore 0x7ff8203a4316 WebCore::AccessibilityObject::accessibilityIsIgnored() const + 310 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/AccessibilityObject.cpp:3710) 9 com.apple.WebCore 0x7ff8203a9b52 WebCore::AccessibilityRenderObject::parentObjectUnignored() const + 34 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/AccessibilityObject.cpp:447) 10 com.apple.WebCore 0x7ff8203ae0a8 WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const + 648 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/AccessibilityRenderObject.cpp:1345) 11 com.apple.WebCore 0x7ff820368ba8 WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*) + 568 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/AccessibilityObject.cpp:3710) 12 com.apple.WebCore 0x7ff81f8111ba WebCore::AccessibilityObject::accessibilityPlatformIncludesObject() const + 378 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/mac/AccessibilityObjectMac.mm:126) 13 com.apple.WebCore 0x7ff8203ade50 WebCore::AccessibilityRenderObject::computeAccessibilityIsIgnored() const + 48 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32217d8374/Library/Caches/com.apple.xbs/Sources/WebCore/Source/WebCore/./accessibility/AccessibilityRenderObject.cpp:1298) 14 com.apple.WebCore 0x7ff820368ba8 WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*) + 568 (/AppleInternal/Library/BuildRoots/52864998-4bb1-11ed-960e-2e32,

Attachments
Patch (9.47 KB, patch) 2023-01-23 15:25 PST, Andres Gonzalez	no flags	Details Formatted Diff Diff
Patch (9.54 KB, patch) 2023-01-24 07:04 PST, Andres Gonzalez	no flags	Details Formatted Diff Diff
Patch (9.54 KB, patch) 2023-01-25 12:50 PST, Andres Gonzalez	no flags	Details Formatted Diff Diff
Patch (9.52 KB, patch) 2023-01-27 04:25 PST, Andres Gonzalez	no flags	Details Formatted Diff Diff
Patch (10.75 KB, patch) 2023-01-27 06:04 PST, Andres Gonzalez	ews-feeder: commit-queue-	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2023-01-23 15:10:20 PST

<rdar://problem/104575681>

Andres Gonzalez

Comment 2 2023-01-23 15:25:02 PST

Created attachment 464613 [details] Patch

Tyler Wilcock

Comment 3 2023-01-23 16:02:24 PST

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.

Andres Gonzalez

Comment 4 2023-01-24 07:04:56 PST

Created attachment 464631 [details] Patch

Andres Gonzalez

Comment 5 2023-01-24 08:16:56 PST

rdar://103361530

Radar WebKit Bug Importer

Comment 6 2023-01-24 08:17:08 PST

<rdar://problem/104602406>

Andres Gonzalez

Comment 7 2023-01-24 08:19:16 PST

(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.

Andres Gonzalez

Comment 8 2023-01-24 16:48:02 PST

(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.

chris fleizach

Comment 9 2023-01-24 23:19:50 PST

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

Andres Gonzalez

Comment 10 2023-01-25 12:50:52 PST

Created attachment 464652 [details] Patch

Andres Gonzalez

Comment 11 2023-01-27 04:25:32 PST

Created attachment 464686 [details] Patch

Andres Gonzalez

Comment 12 2023-01-27 06:04:16 PST

Created attachment 464687 [details] Patch

EWS

Comment 13 2023-01-27 07:28:01 PST

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].

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Accessibility

Assignee

Andres Gonzalez

Reported

2023-01-23 15:10 PST

Modified

2023-01-27 09:16 PST History

CC List

16 users Show

URL

Keywords InRadar

Depends on

Blocks