Bug 251045

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:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	ews-feeder: commit-queue-

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.