Bug 251045 - AX: Fix for crash in AXIsolatedTree::removeNode.
Summary: AX: Fix for crash in AXIsolatedTree::removeNode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-01-23 15:10 PST by Andres Gonzalez
Modified: 2023-01-27 09:16 PST (History)
16 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 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,
Comment 1 Radar WebKit Bug Importer 2023-01-23 15:10:20 PST
<rdar://problem/104575681>
Comment 2 Andres Gonzalez 2023-01-23 15:25:02 PST
Created attachment 464613 [details]
Patch
Comment 3 Tyler Wilcock 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.
Comment 4 Andres Gonzalez 2023-01-24 07:04:56 PST
Created attachment 464631 [details]
Patch
Comment 5 Andres Gonzalez 2023-01-24 08:16:56 PST
rdar://103361530
Comment 6 Radar WebKit Bug Importer 2023-01-24 08:17:08 PST
<rdar://problem/104602406>
Comment 7 Andres Gonzalez 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.
Comment 8 Andres Gonzalez 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.
Comment 9 chris fleizach 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
Comment 10 Andres Gonzalez 2023-01-25 12:50:52 PST
Created attachment 464652 [details]
Patch
Comment 11 Andres Gonzalez 2023-01-27 04:25:32 PST
Created attachment 464686 [details]
Patch
Comment 12 Andres Gonzalez 2023-01-27 06:04:16 PST
Created attachment 464687 [details]
Patch
Comment 13 EWS 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].