| Summary: | AX: crash at WebCore::AccessibilityObject::supportsARIALiveRegion() const + 24 | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nan Wang <n_wang> | ||||||||||||||||||||||||
| Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cfleizach, dmazzoni, jcraig, jdiggs, n_wang, samuel_white, simon.fraser, webkit-bug-importer, zalan | ||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||
| Bug Depends on: | 175435 | ||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
Nan Wang
2017-08-08 13:18:39 PDT
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> |