Bug 108058 – fast/encoding/parser-tests-*.html tests sometimes crash

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Chris Dumez]

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

[Chris Dumez]

Created attachment 184945 [details]
Backtrace for all threads

[Chris Dumez]

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*.

[Chris Dumez]

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.

[Chris Dumez]

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()).

[Chris Dumez]

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);

[Chris Dumez]

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?

[chris fleizach]

(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?

[Chris Dumez]

(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().

[chris fleizach]

(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.

[Chris Dumez]

(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.

[chris fleizach]

(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

[Chris Dumez]

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.

[Build Bot]

Comment on attachment 186262 [details]
Patch

Attachment 186262 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16355600

[Build Bot]

Comment on attachment 186262 [details]
Patch

Attachment 186262 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16347780

[chris fleizach]

(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.

[chris fleizach]

Created attachment 186511 [details]
possibly fix

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 186511 [details]
possibly fix

Attachment 186511 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16371548

[Chris Dumez]

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.

[Build Bot]

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

[Dominic Mazzoni]

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.

[chris fleizach]

(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

[chris fleizach]

(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() ?

[Dominic Mazzoni]

Created attachment 187375 [details]
Patch

[Dominic Mazzoni]

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.

[Build Bot]

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

[Dominic Mazzoni]

Created attachment 187447 [details]
Patch

[chris fleizach]

Comment on attachment 187447 [details]
Patch

looks like all good changes here.

[WebKit Review Bot]

Comment on attachment 187447 [details]
Patch

Clearing flags on attachment: 187447

Committed r142385: <http://trac.webkit.org/changeset/142385>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

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!