Bug 175340 – AX: crash at WebCore::AccessibilityObject::supportsARIALiveRegion() const + 24

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Nan Wang]

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff6474c628 WebCore::AccessibilityObject::supportsARIALiveRegion() const + 24
1   com.apple.WebCore             	0x00007fff64754210 WebCore::AccessibilityObject::isInsideARIALiveRegion() const + 96
2   com.apple.WebCore             	0x00007fff6573c86c -[WebAccessibilityObjectWrapper additionalAccessibilityAttributeNames] + 636
3   com.apple.WebCore             	0x00007fff6573e395 -[WebAccessibilityObjectWrapper accessibilityAttributeNames] + 6261
4   com.apple.AppKit              	0x00007fff54dd2030 __NSAccessibilityEntryPointAttributeNames_block_invoke.llvm.9569B7B1 + 85
5   com.apple.AppKit              	0x00007fff54dd18bb NSAccessibilityPerformEntryPointObject.llvm.9569B7B1 + 16
6   com.apple.AppKit              	0x00007fff54936d16 NSAccessibilityEntryPointAttributeNames + 87
7   com.apple.AppKit              	0x00007fff54b7ba29 -[NSObject(NSAccessibilityInternal) _accessibilityAttributeNamesClientError:] + 56
8   com.apple.AppKit              	0x00007fff54b804a7 CopyAttributeNames + 169
9   com.apple.HIServices          	0x00007fff55a74d39 _AXXMIGCopyAttributeNames + 216
10  com.apple.HIServices          	0x00007fff55a7efe3 _XCopyAttributeNames + 362
11  com.apple.HIServices          	0x00007fff55a57989 mshMIGPerform + 212
12  com.apple.CoreFoundation      	0x00007fff5716c679 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE1_PERFORM_FUNCTION__ + 41
13  com.apple.CoreFoundation      	0x00007fff5716c5c5 __CFRunLoopDoSource1 + 533
14  com.apple.CoreFoundation      	0x00007fff571641b0 __CFRunLoopRun + 2848
15  com.apple.CoreFoundation      	0x00007fff57163403 CFRunLoopRunSpecific + 483

Related crash:
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff7ea52fce __pthread_kill + 10
1   libsystem_pthread.dylib       	0x00007fff7eb90384 pthread_kill + 333
2   libsystem_c.dylib             	0x00007fff7e9aed56 abort + 127
3   libc++abi.dylib               	0x00007fff7c9a3f8f abort_message + 245
4   libc++abi.dylib               	0x00007fff7c9c0952 __cxa_pure_virtual + 18
5   com.apple.WebCore             	0x00007fff6475712c WebCore::AccessibilityObject::defaultObjectInclusion() const + 92
6   com.apple.WebCore             	0x00007fff6476a496 WebCore::AccessibilitySVGElement::computeAccessibilityIsIgnored() const + 22
7   com.apple.WebCore             	0x00007fff647571ec WebCore::AccessibilityObject::accessibilityIsIgnored() const + 76
8   com.apple.WebCore             	0x00007fff647d6114 WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*) + 1716
9   com.apple.WebCore             	0x00007fff64764b20 WebCore::AccessibilityRenderObject::addChildren() + 48
10  com.apple.WebCore             	0x00007fff64752892 WebCore::AccessibilityObject::updateChildrenIfNecessary() + 66
11  com.apple.WebCore             	0x00007fff6474d22d WebCore::AccessibilityObject::updateBackingStore() + 77
12  com.apple.WebCore             	0x00007fff6573614d -[WebAccessibilityObjectWrapperBase updateObjectBackingStore] + 61
13  com.apple.WebCore             	0x00007fff657424c7 -[WebAccessibilityObjectWrapper accessibilityAttributeValue:] + 39
14  com.apple.AppKit              	0x00007fff5482300f NSAccessibilityGetObjectForAttributeUsingLegacyAPI + 371
15  com.apple.AppKit              	0x00007fff54dd5d2d ___NSAccessibilityEntryPointValueForAttribute_block_invoke.816.llvm.9569B7B1 + 1674
16  com.apple.AppKit              	0x00007fff54dd18bb NSAccessibilityPerformEntryPointObject.llvm.9569B7B1 + 16
17  com.apple.AppKit              	0x00007fff54dd1a87 _NSAccessibilityEntryPointValueForAttribute.llvm.9569B7B1 + 182
18  com.apple.AppKit              	0x00007fff54b7bd68 -[NSObject(NSAccessibilityInternal) _accessibilityValueForAttribute:clientError:] + 351

[Radar WebKit Bug Importer]

<rdar://problem/33782159>

[Nan Wang]

Created attachment 317606 [details]
patch

[Nan Wang]

Will keep looking for a better way to notify ax that the node has been detached.

[Nan Wang]

Created attachment 317713 [details]
patch

update

[chris fleizach]

Comment on attachment 317713 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317713&action=review

> Source/WebCore/ChangeLog:10
> +        the element has already been relayout but we are still holding onto its stale children. Fixed it

has already been layed out

[Nan Wang]

(In reply to chris fleizach from comment #5)
> Comment on attachment 317713 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317713&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        the element has already been relayout but we are still holding onto its stale children. Fixed it
> 
> has already been layed out

Ok, will update and commit

[Nan Wang]

Committed r220463: <http://trac.webkit.org/changeset/220463>

[alan]

I am not sure if this patch is correct. Just because some element becomes floating (or non-floating) it does not mean it's stale. They become stale when we detached them from the tree/destroy them. 

What exactly do you mean by "When adding a psuedo element child to a RenderBlockFlow element, there might be a chance where the element has already been relayout but we are still holding onto its stale children."
What is the stale child in this context? The pseudo element's RenderInline?

"by notifying AX correctly when inserting/removing children during layout"
the floating object map is an internal helper container; Not sure why AX needs to know whether we add a renderer to a internal container.
r-

[alan]

See my previous comment.

[Nan Wang]

(In reply to zalan from comment #9)
> See my previous comment.

Ok, I'll revisit on this.

[Nan Wang]

(In reply to zalan from comment #8)
> I am not sure if this patch is correct. Just because some element becomes
> floating (or non-floating) it does not mean it's stale. They become stale
> when we detached them from the tree/destroy them. 
> 
> What exactly do you mean by "When adding a psuedo element child to a
> RenderBlockFlow element, there might be a chance where the element has
> already been relayout but we are still holding onto its stale children."
> What is the stale child in this context? The pseudo element's RenderInline?
> 
> "by notifying AX correctly when inserting/removing children during layout"
> the floating object map is an internal helper container; Not sure why AX
> needs to know whether we add a renderer to a internal container.
> r-

I think there's something going on during layout process that invalidates some of the RenderBlockFlow object's children. Do you think it would be suitable to notify AX in layoutBlock? I think we have something similar in RenderElement::styleWillChange to notify AX on visibility change.

[alan]

(In reply to Nan Wang from comment #11)
> (In reply to zalan from comment #8)
> > I am not sure if this patch is correct. Just because some element becomes
> > floating (or non-floating) it does not mean it's stale. They become stale
> > when we detached them from the tree/destroy them. 
> > 
> > What exactly do you mean by "When adding a psuedo element child to a
> > RenderBlockFlow element, there might be a chance where the element has
> > already been relayout but we are still holding onto its stale children."
> > What is the stale child in this context? The pseudo element's RenderInline?
> > 
> > "by notifying AX correctly when inserting/removing children during layout"
> > the floating object map is an internal helper container; Not sure why AX
> > needs to know whether we add a renderer to a internal container.
> > r-
> 
> I think there's something going on during layout process that invalidates
> some of the RenderBlockFlow object's children. Do you think it would be
> suitable to notify AX in layoutBlock?
By invalidating you mean destroying the child renderer? In such cases, AX is already notified through ::willBedDestroyed.

[Nan Wang]

(In reply to zalan from comment #12)
> (In reply to Nan Wang from comment #11)
> > (In reply to zalan from comment #8)
> > > I am not sure if this patch is correct. Just because some element becomes
> > > floating (or non-floating) it does not mean it's stale. They become stale
> > > when we detached them from the tree/destroy them. 
> > > 
> > > What exactly do you mean by "When adding a psuedo element child to a
> > > RenderBlockFlow element, there might be a chance where the element has
> > > already been relayout but we are still holding onto its stale children."
> > > What is the stale child in this context? The pseudo element's RenderInline?
> > > 
> > > "by notifying AX correctly when inserting/removing children during layout"
> > > the floating object map is an internal helper container; Not sure why AX
> > > needs to know whether we add a renderer to a internal container.
> > > r-
> > 
> > I think there's something going on during layout process that invalidates
> > some of the RenderBlockFlow object's children. Do you think it would be
> > suitable to notify AX in layoutBlock?
> By invalidating you mean destroying the child renderer? In such cases, AX is
> already notified through ::willBedDestroyed.

The case here seems to be the child renderer is not attached to the same parent, so that AX is holding a stale tree. When we ask for the ax parent object from the child renderer it will then lead to accessing an invalid memory.

[Nan Wang]

Created attachment 317763 [details]
patch

updated patch

[alan]

(In reply to Nan Wang from comment #13)
> (In reply to zalan from comment #12)
> > (In reply to Nan Wang from comment #11)
> > > (In reply to zalan from comment #8)
> > > > I am not sure if this patch is correct. Just because some element becomes
> > > > floating (or non-floating) it does not mean it's stale. They become stale
> > > > when we detached them from the tree/destroy them. 
> > > > 
> > > > What exactly do you mean by "When adding a psuedo element child to a
> > > > RenderBlockFlow element, there might be a chance where the element has
> > > > already been relayout but we are still holding onto its stale children."
> > > > What is the stale child in this context? The pseudo element's RenderInline?
> > > > 
> > > > "by notifying AX correctly when inserting/removing children during layout"
> > > > the floating object map is an internal helper container; Not sure why AX
> > > > needs to know whether we add a renderer to a internal container.
> > > > r-
> > > 
> > > I think there's something going on during layout process that invalidates
> > > some of the RenderBlockFlow object's children. Do you think it would be
> > > suitable to notify AX in layoutBlock?
> > By invalidating you mean destroying the child renderer? In such cases, AX is
> > already notified through ::willBedDestroyed.
> 
> The case here seems to be the child renderer is not attached to the same
> parent, so that AX is holding a stale tree. When we ask for the ax parent
> object from the child renderer it will then lead to accessing an invalid
> memory.
Reparenting is not very uncommon in the rendering code. How do you handle first letter or column changes? They are very similar to what you just described here.

[alan]

Comment on attachment 317763 [details]
patch

If it's about repareting, then this change is incorrect (it's incorrect for other reasons as well)

[Nan Wang]

(In reply to zalan from comment #16)
> Comment on attachment 317763 [details]
> patch
> 
> If it's about repareting, then this change is incorrect (it's incorrect for
> other reasons as well)

Thanks for the comment. 
I have figured out the issue. So like you said, we are notifying accessibility on childrenChanged() correctly when objects being inserted/removed. I think this particular case is that when a svg image being destroyed we didn't remove the corresponding ax object correctly, and accessing the stale object then leads to a crash.

[Nan Wang]

Created attachment 317793 [details]
patch

fix the bug

[chris fleizach]

(In reply to Nan Wang from comment #17)
> (In reply to zalan from comment #16)
> > Comment on attachment 317763 [details]
> > patch
> > 
> > If it's about repareting, then this change is incorrect (it's incorrect for
> > other reasons as well)
> 
> Thanks for the comment. 
> I have figured out the issue. So like you said, we are notifying
> accessibility on childrenChanged() correctly when objects being
> inserted/removed. I think this particular case is that when a svg image
> being destroyed we didn't remove the corresponding ax object correctly, and
> accessing the stale object then leads to a crash.

That's promising. It may finally resolve

<rdar://problem/32188382> com.apple.WebKit.WebContent at WebCore::AccessibilityRenderObject::remoteSVGRootElement const + 32

[alan]

Comment on attachment 317793 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317793&action=review

> Source/WebCore/rendering/RenderImage.cpp:156
> +    // Remove this from accessibility first, since the below shutdown() function
> +    // will clear m_cachedImage and lead to a stale ax render object.
> +    if (UNLIKELY(AXObjectCache::accessibilityEnabled())) {
> +        if (AXObjectCache* cache = document().existingAXObjectCache())
> +            cache->remove(this);
> +    }

Do you mind explaining what you mean by "clear m_cachedImage and lead to a stale ax render object". What's the connection between the m_cachedImage and the AX render object? What exactly prevents us from going through the normal RenderObject::willBeDestroyed path? Also, though Chris might know more about this, but this changeset makes a call to AXObjectCache::remove() before notifying the parent (please see the related comments in RenderObject::willBeDestoryed()).
If the problem here is that the AccessibilitySVGRoot outlives the AX RenderImage (which is correct, since svg root is a resource, while the RenderImage is a renderer), then it needs to be addressed in another way.

I tried to debug this myself but the attached test case (in second patch) passes fine even with guard malloc. -is it ASan only?

[Nan Wang]

Created attachment 317814 [details]
patch

Ok, I think this is problematic due to the accessibility set up. We rely on the CachedImage object to retrieve the corresponding AccessibilitySVGRoot when detaching. But since that's been cleared, the detaching was fail so that the AccessibilitySVGRoot object is still pointing to the RenderImage's ax render object as parent.

[Nan Wang]

Sorry wrong patch

[Nan Wang]

Created attachment 317815 [details]
patch

[chris fleizach]

Comment on attachment 317815 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317815&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3096
> +    m_remoteSVGRoot = root;

do we need to set this back to nullptr if we clear our children?

[Nan Wang]

Comment on attachment 317815 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317815&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3096
>> +    m_remoteSVGRoot = root;
> 
> do we need to set this back to nullptr if we clear our children?

You are right

[Nan Wang]

Created attachment 317817 [details]
patch

updated

[alan]

Comment on attachment 317817 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317817&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:112
> +    , m_remoteSVGRoot(nullptr)

Remove this please and switch over to "AccessibilitySVGRoot* m_remoteSVGRoot { nullptr };".

[alan]

This is core accessibility, I'd rather have Chris r+ it.

[Nan Wang]

Rolled out previous patch in r220535: <http://trac.webkit.org/changeset/220535>

[Nan Wang]

Created attachment 317834 [details]
patch

Introducing weak pointer to solve the issue, based on Chris' suggestion.

[Nan Wang]

(In reply to Nan Wang from comment #30)
> Created attachment 317834 [details]
> patch
> 
> Introducing weak pointer to solve the issue, based on Chris' suggestion.

wrong patch again..

[Nan Wang]

Created attachment 317835 [details]
patch

[chris fleizach]

Comment on attachment 317835 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317835&action=review

> LayoutTests/ChangeLog:16
> +        Layout test accessibility/press-target-uses-text-descendant-node.html is flaky.

need to remove this entry

[chris fleizach]

(In reply to zalan from comment #28)
> This is core accessibility, I'd rather have Chris r+ it.

Zalan can you also take a look at this and see if we've implemented weak pointer correctly

[Nan Wang]

Created attachment 317838 [details]
patch

corrected change log

[Build Bot]

Attachment 317838 [details] did not pass style-queue:


ERROR: /Volumes/Data/StyleQueue/WebKit/LayoutTests/platform/gtk/TestExpectations:3406:  Duplicate or ambiguous entry lines LayoutTests/platform/gtk/TestExpectations:2661 and LayoutTests/platform/gtk/TestExpectations:3406.  [test/expectations] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nan Wang]

Created attachment 317839 [details]
patch

Removed bad TestExpectation change

[alan]

IIRC WeakPtr is not cheap. Did you measure the impact on memory consumption?

[Nan Wang]

(In reply to zalan from comment #38)
> IIRC WeakPtr is not cheap. Did you measure the impact on memory consumption?

This should only impact the page with svg root available. 
I've tested with the page with lots of svg elements being added/removed constantly. The memory consumptions are similar before/after this change.

[chris fleizach]

Comment on attachment 317839 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=317839&action=review

> LayoutTests/ChangeLog:12
> +        * platform/gtk/TestExpectations:

not seeing this file in the diff

[Nan Wang]

(In reply to chris fleizach from comment #40)
> Comment on attachment 317839 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=317839&action=review
> 
> > LayoutTests/ChangeLog:12
> > +        * platform/gtk/TestExpectations:
> 
> not seeing this file in the diff

Oh I removed it but didn't update the changeLog

[alan]

(In reply to Nan Wang from comment #39)
> (In reply to zalan from comment #38)
> > IIRC WeakPtr is not cheap. Did you measure the impact on memory consumption?
> 
> This should only impact the page with svg root available. 
> I've tested with the page with lots of svg elements being added/removed
> constantly. The memory consumptions are similar before/after this change.
It increases the size of every AccessibilityRenderObject. Anyway, it should be fine.

[chris fleizach]

Comment on attachment 317839 [details]
patch

looks ok to me from AX perspective. glad we got to the actual problem

[Nan Wang]

Committed r220551: <http://trac.webkit.org/changeset/220551>