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
<rdar://problem/33782159>
Created attachment 317606 [details] patch
Will keep looking for a better way to notify ax that the node has been detached.
Created attachment 317713 [details] patch update
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
(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
Committed r220463: <http://trac.webkit.org/changeset/220463>
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-
See my previous comment.
(In reply to zalan from comment #9) > See my previous comment. Ok, I'll revisit on this.
(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.
(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.
(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.
Created attachment 317763 [details] patch updated patch
(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.
Comment on attachment 317763 [details] patch If it's about repareting, then this change is incorrect (it's incorrect for other reasons as well)
(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.
Created attachment 317793 [details] patch fix the bug
(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
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?
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.
Sorry wrong patch
Created attachment 317815 [details] patch
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?
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
Created attachment 317817 [details] patch updated
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 };".
This is core accessibility, I'd rather have Chris r+ it.
Rolled out previous patch in r220535: <http://trac.webkit.org/changeset/220535>
Created attachment 317834 [details] patch Introducing weak pointer to solve the issue, based on Chris' suggestion.
(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..
Created attachment 317835 [details] patch
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
(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
Created attachment 317838 [details] patch corrected change log
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.
Created attachment 317839 [details] patch Removed bad TestExpectation change
IIRC WeakPtr is not cheap. Did you measure the impact on memory consumption?
(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.
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
(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
(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.
Comment on attachment 317839 [details] patch looks ok to me from AX perspective. glad we got to the actual problem
Committed r220551: <http://trac.webkit.org/changeset/220551>