Bug 108058 - fast/encoding/parser-tests-*.html tests sometimes crash
Summary: fast/encoding/parser-tests-*.html tests sometimes crash
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominic Mazzoni
URL:
Keywords:
Depends on:
Blocks: 106216
  Show dependency treegraph
 
Reported: 2013-01-28 00:20 PST by Chris Dumez
Modified: 2013-02-10 04:44 PST (History)
12 users (show)

See Also:


Attachments
Backtrace for all threads (8.07 KB, text/plain)
2013-01-28 00:24 PST, Chris Dumez
no flags Details
Patch (5.30 KB, patch)
2013-02-03 07:44 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
possibly fix (506 bytes, patch)
2013-02-04 17:56 PST, chris fleizach
no flags Details | Formatted Diff | Diff
Patch (2.91 KB, patch)
2013-02-08 15:48 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff
Patch (5.39 KB, patch)
2013-02-09 14:56 PST, Dominic Mazzoni
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2013-01-28 00:20:58 PST
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
Comment 1 Chris Dumez 2013-01-28 00:24:10 PST
Created attachment 184945 [details]
Backtrace for all threads
Comment 2 Chris Dumez 2013-01-28 04:56:41 PST
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*.
Comment 3 Chris Dumez 2013-01-28 06:14:52 PST
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.
Comment 4 Chris Dumez 2013-01-29 04:14:25 PST
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()).
Comment 5 Chris Dumez 2013-01-29 05:33:15 PST
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);
Comment 6 Chris Dumez 2013-01-29 07:46:26 PST
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?
Comment 7 chris fleizach 2013-01-29 09:01:44 PST
(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?
Comment 8 Chris Dumez 2013-01-29 09:14:50 PST
(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().
Comment 9 chris fleizach 2013-01-29 09:18:16 PST
(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.
Comment 10 Chris Dumez 2013-01-29 09:20:03 PST
(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.
Comment 11 chris fleizach 2013-01-29 16:16:04 PST
(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
Comment 12 Chris Dumez 2013-02-03 07:44:13 PST
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 13 Build Bot 2013-02-03 10:05:04 PST
Comment on attachment 186262 [details]
Patch

Attachment 186262 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16355600
Comment 14 Build Bot 2013-02-03 10:47:34 PST
Comment on attachment 186262 [details]
Patch

Attachment 186262 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16347780
Comment 15 chris fleizach 2013-02-04 17:55:41 PST
(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.
Comment 16 chris fleizach 2013-02-04 17:56:01 PST
Created attachment 186511 [details]
possibly fix
Comment 17 WebKit Review Bot 2013-02-04 19:25:02 PST
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 18 Build Bot 2013-02-04 20:55:43 PST
Comment on attachment 186511 [details]
possibly fix

Attachment 186511 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16371548
Comment 19 Chris Dumez 2013-02-04 22:29:36 PST
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 20 Build Bot 2013-02-05 00:47:33 PST
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
Comment 21 Dominic Mazzoni 2013-02-05 19:17:26 PST
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.
Comment 22 chris fleizach 2013-02-05 20:03:55 PST
(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
Comment 23 chris fleizach 2013-02-05 23:18:51 PST
(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() ?
Comment 24 Dominic Mazzoni 2013-02-08 15:48:58 PST
Created attachment 187375 [details]
Patch
Comment 25 Dominic Mazzoni 2013-02-08 16:00:54 PST
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 26 Build Bot 2013-02-08 17:06:12 PST
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
Comment 27 Dominic Mazzoni 2013-02-09 14:56:47 PST
Created attachment 187447 [details]
Patch
Comment 28 chris fleizach 2013-02-09 21:15:58 PST
Comment on attachment 187447 [details]
Patch

looks like all good changes here.
Comment 29 WebKit Review Bot 2013-02-09 22:12:52 PST
Comment on attachment 187447 [details]
Patch

Clearing flags on attachment: 187447

Committed r142385: <http://trac.webkit.org/changeset/142385>
Comment 30 WebKit Review Bot 2013-02-09 22:12:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 31 Chris Dumez 2013-02-10 04:44:33 PST
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!