fast/encoding/parser-tests-*.html tests sometimes crash on WK2 EFL port with the following backtrace: crash log for WebProcess (pid <unknown>): STDOUT: <empty> STDERR: 1 0x7f9650b8bc5b STDERR: 2 0x7f96538fb4a0 STDERR: 3 0x7f964a6ae190 vtable for __cxxabiv1::__si_class_type_info STDERR: LEAK: 1 WebPage STDERR: LEAK: 2 WebFrame STDERR: LEAK: 1 JSLazyEventListener STDERR: LEAK: 73 RenderObject STDERR: LEAK: 3 Page STDERR: LEAK: 4 Frame STDERR: LEAK: 2035 CachedResource STDERR: LEAK: 146 WebCoreNode
Created attachment 184945 [details] Backtrace for all threads
Basically, WebCore::AccessibilityScrollView::addChildScrollbar() calls axObjectCache()->getOrCreate(scrollbar) and casts the returned object to AccessibilityScrollbar*. However, the returned object is a AccessibilityScrollView*. I don't know yet why AXObjectCache::getOrCreate(Widget*) would return a AccessibilityScrollView* when given a WebCore::Scrollbar*.
The issue is that FrameViews do not always remove themselves from the AXObjectCache on destruction. removeFromAXObjectCache() is called in FrameView destructor but it sometimes does not have any effect because frame()->document()->axObjectCacheExists() returns false. As a consequence, the FrameView object pointer stays in AXObjectCache::m_widgetObjectMapping and then a Scrollbar object gets created and uses the same pointer. As a consequence, AXObjectCache::getOrCreate(Widget*) will return the AccessibilityScrollView* that is still in the cache instead of creating an AccessibilityScrollbar.
The issue seems to be in Frame::setView(PassRefPtr<FrameView> view): 1. m_view->prepareForDetach(); is called which removes the FrameView from the AXObjectCache. 2. m_doc->prepareForDestruction(); is called and it causes the FrameView to be added to the AXObjectCache again. 3. The FrameView is destroyed but it is not properly removed from the cache because it is no longer correctly hooked in the document tree. I think one solution would be to call: m_view->prepareForDetach() *AFTER* m_doc->prepareForDestruction(). Note that I don't think we need to call m_view->detachCustomScrollbars() before m_doc->prepareForDestruction() because this is already done in Document::detach() (which is called in Document::prepareForDestruction()).
Calling m_view->prepareForDetach() *AFTER* m_doc->prepareForDestruction() does not help because axObjectCacheExists() will still return false in m_view->prepareForDetach(). I digged a bit deeper and it appears RenderObject::willBeDestroyed() is adding the FrameView to the axObjectCache again. In particular, the call to: document()->axObjectCache()->childrenChanged(parentObject);
The ScrollView is added again to the cache because of AXObjectCache::childrenChanged(). childrenChanged() is not supposed to create the parent object if it does not exist. To ensure this, it calls AccessibilityObject::parentObjectIfExists() instead of AccessibilityObject::parentObject(). Unfortunately, AXObjectCache::childrenChanged() also calls AccessibilityObject::accessibilityIsIgnored() which calls AccessibilityObject::parentObject() and ends up creating the parent. I'm not sure what the correct fix is yet. AccessibilityObject::accessibilityIsIgnored() is overridden is many places and making sure the implementation does not call parentObject() (directly or not) is not trivial. Can we somehow avoid the call to AccessibilityObject::accessibilityIsIgnored() in AXObjectCache::childrenChanged() instead?
(In reply to comment #6) > The ScrollView is added again to the cache because of AXObjectCache::childrenChanged(). childrenChanged() is not supposed to create the parent object if it does not exist. To ensure this, it calls AccessibilityObject::parentObjectIfExists() instead of AccessibilityObject::parentObject(). > > Unfortunately, AXObjectCache::childrenChanged() also calls AccessibilityObject::accessibilityIsIgnored() which calls AccessibilityObject::parentObject() and ends up creating the parent. > > I'm not sure what the correct fix is yet. AccessibilityObject::accessibilityIsIgnored() is overridden is many places and making sure the implementation does not call parentObject() (directly or not) is not trivial. Can we somehow avoid the call to AccessibilityObject::accessibilityIsIgnored() in AXObjectCache::childrenChanged() instead? It looks like the call to axIsIgnored() only happens if the parent object already exists if (obj->parentObjectIfExists() && obj->cachedIsIgnoredValue() != obj->accessibilityIsIgnored()) so is this happening higher up the chain, as in a grandparent renderer that is being recreated?
(In reply to comment #7) > (In reply to comment #6) > > The ScrollView is added again to the cache because of AXObjectCache::childrenChanged(). childrenChanged() is not supposed to create the parent object if it does not exist. To ensure this, it calls AccessibilityObject::parentObjectIfExists() instead of AccessibilityObject::parentObject(). > > > > Unfortunately, AXObjectCache::childrenChanged() also calls AccessibilityObject::accessibilityIsIgnored() which calls AccessibilityObject::parentObject() and ends up creating the parent. > > > > I'm not sure what the correct fix is yet. AccessibilityObject::accessibilityIsIgnored() is overridden is many places and making sure the implementation does not call parentObject() (directly or not) is not trivial. Can we somehow avoid the call to AccessibilityObject::accessibilityIsIgnored() in AXObjectCache::childrenChanged() instead? > > It looks like the call to axIsIgnored() only happens if the parent object already exists > > if (obj->parentObjectIfExists() && obj->cachedIsIgnoredValue() != obj->accessibilityIsIgnored()) > > so is this happening higher up the chain, as in a grandparent renderer that is being recreated? Right. In some cases, accessibilityIsIgnored() will creates all the ancestors. For example, AccessibilityRenderObject::accessibilityIsIgnored() calls AccessibilityNodeObject::isDescendantOfBarrenParent() which iterates through all the ancestors by calling parentObject(). Same applies to AccessibilityRenderObject::isAllowedChildOfTree() which is also called from AccessibilityRenderObject::accessibilityIsIgnored().
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > The ScrollView is added again to the cache because of AXObjectCache::childrenChanged(). childrenChanged() is not supposed to create the parent object if it does not exist. To ensure this, it calls AccessibilityObject::parentObjectIfExists() instead of AccessibilityObject::parentObject(). > > > > > > Unfortunately, AXObjectCache::childrenChanged() also calls AccessibilityObject::accessibilityIsIgnored() which calls AccessibilityObject::parentObject() and ends up creating the parent. > > > > > > I'm not sure what the correct fix is yet. AccessibilityObject::accessibilityIsIgnored() is overridden is many places and making sure the implementation does not call parentObject() (directly or not) is not trivial. Can we somehow avoid the call to AccessibilityObject::accessibilityIsIgnored() in AXObjectCache::childrenChanged() instead? > > > > It looks like the call to axIsIgnored() only happens if the parent object already exists > > > > if (obj->parentObjectIfExists() && obj->cachedIsIgnoredValue() != obj->accessibilityIsIgnored()) > > > > so is this happening higher up the chain, as in a grandparent renderer that is being recreated? > > Right. In some cases, accessibilityIsIgnored() will creates all the ancestors. > For example, AccessibilityRenderObject::accessibilityIsIgnored() calls AccessibilityNodeObject::isDescendantOfBarrenParent() which iterates through all the ancestors by calling parentObject(). Same applies to AccessibilityRenderObject::isAllowedChildOfTree() which is also called from AccessibilityRenderObject::accessibilityIsIgnored(). I've suggested this before, but I think the answer might be to call childrenChanged() after a timer callback on the next iteration of the run loop. That way the render tree will be in a valid state when we perform operations like checking parent chains and so on. I can give that a try.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #6) > > > > The ScrollView is added again to the cache because of AXObjectCache::childrenChanged(). childrenChanged() is not supposed to create the parent object if it does not exist. To ensure this, it calls AccessibilityObject::parentObjectIfExists() instead of AccessibilityObject::parentObject(). > > > > > > > > Unfortunately, AXObjectCache::childrenChanged() also calls AccessibilityObject::accessibilityIsIgnored() which calls AccessibilityObject::parentObject() and ends up creating the parent. > > > > > > > > I'm not sure what the correct fix is yet. AccessibilityObject::accessibilityIsIgnored() is overridden is many places and making sure the implementation does not call parentObject() (directly or not) is not trivial. Can we somehow avoid the call to AccessibilityObject::accessibilityIsIgnored() in AXObjectCache::childrenChanged() instead? > > > > > > It looks like the call to axIsIgnored() only happens if the parent object already exists > > > > > > if (obj->parentObjectIfExists() && obj->cachedIsIgnoredValue() != obj->accessibilityIsIgnored()) > > > > > > so is this happening higher up the chain, as in a grandparent renderer that is being recreated? > > > > Right. In some cases, accessibilityIsIgnored() will creates all the ancestors. > > For example, AccessibilityRenderObject::accessibilityIsIgnored() calls AccessibilityNodeObject::isDescendantOfBarrenParent() which iterates through all the ancestors by calling parentObject(). Same applies to AccessibilityRenderObject::isAllowedChildOfTree() which is also called from AccessibilityRenderObject::accessibilityIsIgnored(). > > I've suggested this before, but I think the answer might be to call childrenChanged() after a timer callback on the next iteration of the run loop. That way the render tree will be in a valid state when we perform operations like checking parent chains and so on. I can give that a try. Ok. I can easily reproduce the problem locally if needed.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #6) > > > > The ScrollView is added again to the cache because of AXObjectCache::childrenChanged(). childrenChanged() is not supposed to create the parent object if it does not exist. To ensure this, it calls AccessibilityObject::parentObjectIfExists() instead of AccessibilityObject::parentObject(). > > > > > > > > Unfortunately, AXObjectCache::childrenChanged() also calls AccessibilityObject::accessibilityIsIgnored() which calls AccessibilityObject::parentObject() and ends up creating the parent. > > > > > > > > I'm not sure what the correct fix is yet. AccessibilityObject::accessibilityIsIgnored() is overridden is many places and making sure the implementation does not call parentObject() (directly or not) is not trivial. Can we somehow avoid the call to AccessibilityObject::accessibilityIsIgnored() in AXObjectCache::childrenChanged() instead? > > > > > > It looks like the call to axIsIgnored() only happens if the parent object already exists > > > > > > if (obj->parentObjectIfExists() && obj->cachedIsIgnoredValue() != obj->accessibilityIsIgnored()) > > > > > > so is this happening higher up the chain, as in a grandparent renderer that is being recreated? > > > > Right. In some cases, accessibilityIsIgnored() will creates all the ancestors. > > For example, AccessibilityRenderObject::accessibilityIsIgnored() calls AccessibilityNodeObject::isDescendantOfBarrenParent() which iterates through all the ancestors by calling parentObject(). Same applies to AccessibilityRenderObject::isAllowedChildOfTree() which is also called from AccessibilityRenderObject::accessibilityIsIgnored(). > > I've suggested this before, but I think the answer might be to call childrenChanged() after a timer callback on the next iteration of the run loop. That way the render tree will be in a valid state when we perform operations like checking parent chains and so on. I can give that a try. Unfortunately that plan did not work out. A whole lot of crashes came out out of it. I'll keep thinking of a solution for this one
Created attachment 186262 [details] Patch I am not familiar with this part of the code but this patch fixes the issue for me and does not seem to cause any regression.
Comment on attachment 186262 [details] Patch Attachment 186262 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16355600
Comment on attachment 186262 [details] Patch Attachment 186262 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16347780
(In reply to comment #12) > Created an attachment (id=186262) [details] > Patch > > I am not familiar with this part of the code but this patch fixes the issue for me and does not seem to cause any regression. I too don't know the consequences of such a change. ... Taking another approach i wonder if we really need to check the axIgnored value() in childrenChanged(). Attaching a patch to that effect. Dominic, can you comment on whether you think we can get rid of this? Originally childrenChanged() was added to be a very lightweight setter to tell the axtree to update itself at the next opportunity. Doing work within that method is dangerous.
Created attachment 186511 [details] possibly fix
Comment on attachment 186511 [details] possibly fix Attachment 186511 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16370477 New failing tests: platform/chromium/accessibility/is-ignored-change-sends-notification.html
Comment on attachment 186511 [details] possibly fix Attachment 186511 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16371548
Comment on attachment 186511 [details] possibly fix View in context: https://bugs.webkit.org/attachment.cgi?id=186511&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:-620 > - if (obj->parentObjectIfExists() && obj->cachedIsIgnoredValue() != obj->accessibilityIsIgnored()) Yes, doing this also fixes the crashes, I checked before. However, I was not sure it was appropriate to remove it. If we can then fine, it is easier.
Comment on attachment 186511 [details] possibly fix Attachment 186511 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16375619 New failing tests: accessibility/loading-iframe-updates-axtree.html
Doing the isIgnored check is important, we can't just eliminate the code - but that doesn't mean we have to implement it exactly that way. Would deferring the isIgnored check using a one-shot timer fix the problem? Maybe we could just post a simple notification, but then in the async notification handler, add a special case for this one notification that checks isIgnored and fires another event as needed.
(In reply to comment #21) > Doing the isIgnored check is important, we can't just eliminate the code - but that doesn't mean we have to implement it exactly that way. Would deferring the isIgnored check using a one-shot timer fix the problem? Maybe we could just post a simple notification, but then in the async notification handler, add a special case for this one notification that checks isIgnored and fires another event as needed. If we deferred that check I think it would work out
(In reply to comment #21) > Doing the isIgnored check is important, we can't just eliminate the code - but that doesn't mean we have to implement it exactly that way. Would deferring the isIgnored check using a one-shot timer fix the problem? Maybe we could just post a simple notification, but then in the async notification handler, add a special case for this one notification that checks isIgnored and fires another event as needed. I'm not seeing a good way to delay that notification after a cursory look Why is the isIgnored() check important instead of just always marking childrenChanged() ?
Created attachment 187375 [details] Patch
A little more info: 1. Why is the isignored check needed? The best way to understand the motivation is to look at platform/chromium/accessibility/is-ignored-change-sends-notification.html, and look at the emptyDiv case in particular. Initially, emptyDiv is ignored. Adding text to emptyDiv causes it to be not ignored. However, if that doesn't actually fire a childrenChanged event on emptyDiv's parent, that parent will never know that it now has a child. (On Windows, the notification has to be fired - you can't just mark it as dirty.) I'd prefer not to call childrenChanged on all ancestors. On Mac, a childrenChanged just marks a node as dirty, but Windows screen readers maintain an offscreen virtual buffer, so childrenChanged means refreshing that entire subtree again. 2. Why did deferring the childrenChanged call cause other tests to crash? Most of the tests that crashed had iframes. This change was queuing childrenChanged notifications that didn't fire until after the iframe was deleted. The fix is simple - when an iframe is deleted, AXObjectCache is already removing all of the objects associated with that frame - but we were never checking in notificationPostTimerFired to see if an object was still valid. The easiest way to check this is with its id, since AXObjectCache sets the id to 0 when an object is deleted (but still has references to it). I suspect that obj->isDetached() would also work - is that always equivalent? I wonder if this is the same problem as https://bugs.webkit.org/show_bug.cgi?id=106106? That'd be a nice side effect if so.
Comment on attachment 187375 [details] Patch Attachment 187375 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16418910 New failing tests: accessibility/loading-iframe-updates-axtree.html
Created attachment 187447 [details] Patch
Comment on attachment 187447 [details] Patch looks like all good changes here.
Comment on attachment 187447 [details] Patch Clearing flags on attachment: 187447 Committed r142385: <http://trac.webkit.org/changeset/142385>
All reviewed patches have been landed. Closing bug.
I confirm that the fix is working on EFL port. I unskipped the test cases in http://trac.webkit.org/changeset/142396. Thanks for taking care of it!