Bug 62289 – AX: ARIA live regions don't trigger notifications for elements that aren't in the AX tree

• Component: Accessibility
• Status: RESOLVED DUPLICATE

[chris fleizach]

ARIA live regions that have elements that are not yet in the AX tree don't send update notifications. This means a lot of live region examples don't work.

The problem is that we don't create AX elements in the childrenChanged() method because the render tree is in a state of updating. Creating elements can ask questions of the render tree that can lead to crashes

I think the fix is to postpone the children update mechanism with a one shot timer

[chris fleizach]

Created attachment 96437 [details]
patch

[Darin Adler]

Comment on attachment 96437 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96437&action=review

Looks almost right, but I had a few concerns. review- because we don’t want a vector that might give us unwanted O(n^2) performance.

> Source/WebCore/accessibility/AXObjectCache.cpp:373
> +    size_t updateChildrenPos = m_childrenToUpdate.find(renderer);
> +    if (updateChildrenPos != WTF::notFound)
> +        m_childrenToUpdate.remove(updateChildrenPos);

If we need to support efficient removal, this should be a HashSet or ListHashSet. It’s not good to search in a vector.

> Source/WebCore/accessibility/AXObjectCache.cpp:446
> +    m_childrenUpdateTimer.stop();

It’s usually not necessary to stop the timer. Why was this needed?

> Source/WebCore/accessibility/AXObjectCache.cpp:452
> +    unsigned i = 0, count = m_childrenToUpdate.size();
> +    for (i = 0; i < count; ++i) {
> +        if (AccessibilityObject* axObj = getOrCreate(m_childrenToUpdate[i]))
> +            axObj->childrenChanged(AccessibilityObject::CreateParentObjects);
> +    }

This initializes i twice. It should use size_t. And normally for a local variable name we’d use “size” rather than mixing “count” and “size”.

The name axObj is not so good. I would just name the local “object”.

> Source/WebCore/accessibility/AXObjectCache.cpp:454
> +    m_childrenToUpdate.clear();

It’s safer to make a copy of the vector to iterate it. If there is any chance that childrenChanged could add items to the m_childrenToUpdate collection during the iteration, then we need protection.

> Source/WebCore/accessibility/AXObjectCache.cpp:467
> +        size_t updateChildrenPos = m_childrenToUpdate.find(renderer);

Again, if we are doing “find” we should probably be using a set rather than a vector. If ordering matters, it can be a ListHashSet.

> Source/WebCore/accessibility/AXObjectCache.cpp:475
> +        if (AccessibilityObject* obj = m_objects.get(axID).get())
> +            obj->childrenChanged(AccessibilityObject::DoNotCreateParentObjects);

You should name this “object” instead of “obj”.

[chris fleizach]

Created attachment 98019 [details]
patch

Addressed all of Darin's comments.

[WebKit Review Bot]

Comment on attachment 98019 [details]
patch

Attachment 98019 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8919044

New failing tests:
fast/css-generated-content/text-before-table-col-crash.html
scrollbars/scrollable-iframe-remove-crash.html

[WebKit Review Bot]

Created attachment 98029 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Darin Adler]

Comment on attachment 98019 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98019&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:444
> +    // Make a local copy in case childrenChanged() alters m_childrenToUpdate (it shouldn't).

Just wondering about the "it shouldn't" comment. Doesn’t childrenChanged end up calling out to the client? Couldn’t the client do any arbitrary work including calls that would change the AX tree?

> Source/WebCore/accessibility/AXObjectCache.cpp:446
> +    HashSet<RenderObject*> updateChildren = m_childrenToUpdate;
> +    m_childrenToUpdate.clear();

A much more efficient, although less clear, way to do this is:

    HashSet<RenderObject*> updateChildren;
    m_childrenToUpdate.swap(updateChildren);

The code in the patch has to build a whole new hash table.

Another way to do it is to just copy the values of the set into a vector. That's more efficient than making a new set, but not as efficient as swapping to grab the existing set.

[chris fleizach]

(In reply to comment #6)
> (From update of attachment 98019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98019&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:444
> > +    // Make a local copy in case childrenChanged() alters m_childrenToUpdate (it shouldn't).
> 
> Just wondering about the "it shouldn't" comment. Doesn’t childrenChanged end up calling out to the client? Couldn’t the client do any arbitrary work including calls that would change the AX tree?
> 

It seems like it might be possible this could happen, but when I looked at the code, it looked like we don't directly call the client from childrenChanged

instead, we call postNotification, which makes a timer to callback postPlatformNotification. The postPlatformNotification can then call request arbitrary work. 

But I think you're right it's a case that needs to be handled

[Darin Adler]

(In reply to comment #6)
> Another way to do it is to just copy the values of the set into a vector.

Just to be clear, I mean to do that with the copyToVector function, not with a handwritten loop.

[chris fleizach]

http://trac.webkit.org/changeset/89591

[chris fleizach]

and layout tests (which didn't make the first commit)
http://trac.webkit.org/changeset/89593

[Dmitry Titov]

This causes crashes in scrollbars/scrollable-iframe-remove-crash.html with callstack on SL: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r89591%20(30691)/scrollbars/scrollable-iframe-remove-crash-crash-log.txt

Also, in fast/css-generated-content/text-before-table-col-crash.html on Chromium bots:
[2716:2716:689688690682:ERROR:process_util_posix.cc(113)] Received signal 11

	WebCore::AXObjectCache::childrenUpdateTimerFired() [0x86fb64b]
	WebCore::Timer<>::fired() [0x86f81db]
	WebCore::ThreadTimers::sharedTimerFiredInternal() [0x8663e35]
	WebCore::ThreadTimers::sharedTimerFired() [0x8663ec1]
	(anonymous namespace)::TaskClosureAdapter::Run() [0x83b4bad]
	MessageLoop::RunTask() [0x83b9590]

[chris fleizach]

Ok
I'll roll out

[chris fleizach]

Actually looks like it might just be a nil pointer access. Might be straightforward fix

(In reply to comment #12)
> Ok
> I'll roll out

[Ryosuke Niwa]

Any update on the fix?

[chris fleizach]

(In reply to comment #14)
> Any update on the fix?

working on it right now. was stuck in a meeting

[Dmitry Titov]

It's better to rollout to keep the tree green if it takes more then a few minutes for a fix... The re-land later is simple and cheap. 

I need to decide whether to disable those tests for Chromium right now (I am Chromium WebKit sheriff today, I need either no crash or disabled tests to continue to roll WebKit into Chromium).

Do you want me to rollout it?

[chris fleizach]

(In reply to comment #16)
> It's better to rollout to keep the tree green if it takes more then a few minutes for a fix... The re-land later is simple and cheap. 
> 
> I need to decide whether to disable those tests for Chromium right now (I am Chromium WebKit sheriff today, I need either no crash or disabled tests to continue to roll WebKit into Chromium).
> 
> Do you want me to rollout it?

Yes roll it out. It's a bad memory access and gdb takes minutes to attach to DumpRenderTree. Thanks

[chris fleizach]

(In reply to comment #17)
> (In reply to comment #16)
> > It's better to rollout to keep the tree green if it takes more then a few minutes for a fix... The re-land later is simple and cheap. 
> > 
> > I need to decide whether to disable those tests for Chromium right now (I am Chromium WebKit sheriff today, I need either no crash or disabled tests to continue to roll WebKit into Chromium).
> > 
> > Do you want me to rollout it?
> 
> Yes roll it out. It's a bad memory access and gdb takes minutes to attach to DumpRenderTree. Thanks

Figured out the problem here. 

AX is expecting that when a render object is destroyed that all its children have been destroyed first (otherwise it's possible that a child will say, childrenChanged on the parent). That is what happens actually, but the hook in RenderObject::destroy tells AX the render object is gone before it gets to actually removing its children.

we need to move the ax updating mechanism to the end of destroy so that we are guaranteed all children are gone

[chris fleizach]

Created attachment 109112 [details]
test patch

[chris fleizach]

rdar://9565898

[chris fleizach]

http://trac.webkit.org/changeset/96340

[Adam Barth]

This patch seems to have caused a bunch of crashes of this flavor:

ASSERTION FAILED: m_wheelEventHandlerCount > 0
third_party/WebKit/Source/WebCore/dom/Document.cpp(5154) : void WebCore::Document::didRemoveWheelEventHandler()
[10683:10683:14161717355907:ERROR:process_util_posix.cc(134)] Received signal 11
	base::debug::StackTrace::StackTrace() [0x6f87fe]
	base::(anonymous namespace)::StackDumpSignalHandler() [0x6b5cf5]
	0x2ae788979af0
	WebCore::Document::didRemoveWheelEventHandler() [0xc2b787]
	WebCore::FrameView::willRemoveHorizontalScrollbar() [0x1248200]
	WebCore::ScrollView::setHasHorizontalScrollbar() [0xe07424]
	WebCore::FrameView::~FrameView() [0x12477c9]
	WTF::RefCounted<>::deref() [0x496b2c]
	WTF::derefIfNotNull<>() [0xf5ff2d]
	WTF::RefPtr<>::~RefPtr() [0xf5fc71]
	WebCore::AccessibilityScrollView::~AccessibilityScrollView() [0xf600c2]
	WTF::RefCounted<>::deref() [0x4768a4]
	WTF::derefIfNotNull<>() [0x4763ba]
	WTF::RefPtr<>::~RefPtr() [0x475a03]
	std::pair<>::~pair() [0x481606]
	WTF::HashTable<>::deallocateTable() [0xf34a39]
	WTF::HashTable<>::~HashTable() [0xf331e4]
	WTF::HashMap<>::~HashMap() [0xf32cac]
	WebCore::AXObjectCache::~AXObjectCache() [0xf303b2]
	WebCore::Document::clearAXObjectCache() [0xc1e6b8]
	WebCore::Document::detach() [0xc1e428]
	WebCore::Frame::setView() [0x123b98e]
	WebKit::WebFrameImpl::createFrameView() [0x48d5a6]
	WebKit::FrameLoaderClientImpl::makeDocumentView() [0x4dd3ac]
	WebKit::FrameLoaderClientImpl::transitionToCommittedForNewPage() [0x4e0cf2]
	WebCore::FrameLoader::transitionToCommitted() [0x11a838b]
	WebCore::FrameLoader::commitProvisionalLoad() [0x11a7818]
	WebCore::DocumentLoader::commitIfReady() [0x1189204]
	WebCore::DocumentLoader::commitLoad() [0x11892b2]
	WebCore::DocumentLoader::receivedData() [0x1189518]
	WebCore::MainResourceLoader::addData() [0x11bc393]
	WebCore::ResourceLoader::didReceiveData() [0x11cfc4d]
	WebCore::MainResourceLoader::didReceiveData() [0x11bd7dc]
	WebCore::ResourceLoader::didReceiveData() [0x11d055c]
	WebCore::ResourceHandleInternal::didReceiveData() [0x4f3f08]
	webkit_glue::WebURLLoaderImpl::Context::OnReceivedData() [0x1a46e7a]
	(anonymous namespace)::RequestProxy::NotifyReceivedData() [0x1b02df3]
	DispatchToMethod<>() [0x1b096bb]
	RunnableMethod<>::Run() [0x1b08b82]
	base::subtle::TaskClosureAdapter::Run() [0x6d54fb]
	base::internal::Invoker1<>::DoInvoke() [0x694246]
	base::Callback<>::Run() [0x6764f9]
	MessageLoop::RunTask() [0x690de9]
	MessageLoop::DeferOrRunPendingTask() [0x690ef1]
	MessageLoop::DoWork() [0x691707]
	base::MessagePumpGlib::HandleDispatch() [0x6ea091]
	(anonymous namespace)::WorkSourceDispatch() [0x6e95e3]
	0x2ae782dc38c2
	0x2ae782dc7748
	0x2ae782dc78fc
	base::MessagePumpGtk::RunOnce() [0x6eb8f9]
	base::MessagePumpGlib::RunWithDispatcher() [0x6e9d44]
	base::MessagePumpGlib::Run() [0x6ea16e]
	MessageLoop::RunInternal() [0x690bdd]
	MessageLoop::RunHandler() [0x690a90]
	MessageLoop::Run() [0x6904ab]
	webkit_support::RunMessageLoop() [0x60c3e0]
	TestShell::waitTestFinished() [0x461b95]
	TestShell::runFileTest() [0x45a8b8]
	runTest() [0x42ee78]
	main [0x42f88b]
	0x2ae788964c4d

[chris fleizach]

Gah... i will roll it out 

(In reply to comment #22)
> This patch seems to have caused a bunch of crashes of this flavor:
> 
> ASSERTION FAILED: m_wheelEventHandlerCount > 0
> third_party/WebKit/Source/WebCore/dom/Document.cpp(5154) : void WebCore::Document::didRemoveWheelEventHandler()
> [10683:10683:14161717355907:ERROR:process_util_posix.cc(134)] Received signal 11
>     base::debug::StackTrace::StackTrace() [0x6f87fe]
>     base::(anonymous namespace)::StackDumpSignalHandler() [0x6b5cf5]
>     0x2ae788979af0
>     WebCore::Document::didRemoveWheelEventHandler() [0xc2b787]
>     WebCore::FrameView::willRemoveHorizontalScrollbar() [0x1248200]
>     WebCore::ScrollView::setHasHorizontalScrollbar() [0xe07424]
>     WebCore::FrameView::~FrameView() [0x12477c9]
>     WTF::RefCounted<>::deref() [0x496b2c]
>     WTF::derefIfNotNull<>() [0xf5ff2d]
>     WTF::RefPtr<>::~RefPtr() [0xf5fc71]
>     WebCore::AccessibilityScrollView::~AccessibilityScrollView() [0xf600c2]
>     WTF::RefCounted<>::deref() [0x4768a4]
>     WTF::derefIfNotNull<>() [0x4763ba]
>     WTF::RefPtr<>::~RefPtr() [0x475a03]
>     std::pair<>::~pair() [0x481606]
>     WTF::HashTable<>::deallocateTable() [0xf34a39]
>     WTF::HashTable<>::~HashTable() [0xf331e4]
>     WTF::HashMap<>::~HashMap() [0xf32cac]
>     WebCore::AXObjectCache::~AXObjectCache() [0xf303b2]
>     WebCore::Document::clearAXObjectCache() [0xc1e6b8]
>     WebCore::Document::detach() [0xc1e428]
>     WebCore::Frame::setView() [0x123b98e]
>     WebKit::WebFrameImpl::createFrameView() [0x48d5a6]
>     WebKit::FrameLoaderClientImpl::makeDocumentView() [0x4dd3ac]
>     WebKit::FrameLoaderClientImpl::transitionToCommittedForNewPage() [0x4e0cf2]
>     WebCore::FrameLoader::transitionToCommitted() [0x11a838b]
>     WebCore::FrameLoader::commitProvisionalLoad() [0x11a7818]
>     WebCore::DocumentLoader::commitIfReady() [0x1189204]
>     WebCore::DocumentLoader::commitLoad() [0x11892b2]
>     WebCore::DocumentLoader::receivedData() [0x1189518]
>     WebCore::MainResourceLoader::addData() [0x11bc393]
>     WebCore::ResourceLoader::didReceiveData() [0x11cfc4d]
>     WebCore::MainResourceLoader::didReceiveData() [0x11bd7dc]
>     WebCore::ResourceLoader::didReceiveData() [0x11d055c]
>     WebCore::ResourceHandleInternal::didReceiveData() [0x4f3f08]
>     webkit_glue::WebURLLoaderImpl::Context::OnReceivedData() [0x1a46e7a]
>     (anonymous namespace)::RequestProxy::NotifyReceivedData() [0x1b02df3]
>     DispatchToMethod<>() [0x1b096bb]
>     RunnableMethod<>::Run() [0x1b08b82]
>     base::subtle::TaskClosureAdapter::Run() [0x6d54fb]
>     base::internal::Invoker1<>::DoInvoke() [0x694246]
>     base::Callback<>::Run() [0x6764f9]
>     MessageLoop::RunTask() [0x690de9]
>     MessageLoop::DeferOrRunPendingTask() [0x690ef1]
>     MessageLoop::DoWork() [0x691707]
>     base::MessagePumpGlib::HandleDispatch() [0x6ea091]
>     (anonymous namespace)::WorkSourceDispatch() [0x6e95e3]
>     0x2ae782dc38c2
>     0x2ae782dc7748
>     0x2ae782dc78fc
>     base::MessagePumpGtk::RunOnce() [0x6eb8f9]
>     base::MessagePumpGlib::RunWithDispatcher() [0x6e9d44]
>     base::MessagePumpGlib::Run() [0x6ea16e]
>     MessageLoop::RunInternal() [0x690bdd]
>     MessageLoop::RunHandler() [0x690a90]
>     MessageLoop::Run() [0x6904ab]
>     webkit_support::RunMessageLoop() [0x60c3e0]
>     TestShell::waitTestFinished() [0x461b95]
>     TestShell::runFileTest() [0x45a8b8]
>     runTest() [0x42ee78]
>     main [0x42f88b]
>     0x2ae788964c4d

[Adam Barth]

> Gah... i will roll it out 

I can take care of that for you.

[chris fleizach]

(In reply to comment #24)
> > Gah... i will roll it out 
> 
> I can take care of that for you.

Ok thanks.

[chris fleizach]

Are you aware which tests are failing on which platform? 

I had run new-webkit-tests and old-webkit-tests on the whole set of tests before checking in on Lion and did not have issues

(In reply to comment #24)
> > Gah... i will roll it out 
> 
> I can take care of that for you.

[Adam Barth]

fast/encoding/japanese-encoding-mix.html
fast/encoding/parser-tests-100.html
fast/encoding/parser-tests-120.html
fast/encoding/parser-tests-20.html
fast/encoding/parser-tests-60.html
fast/encoding/parser-tests-80.html
fast/events/blur-focus-window-should-blur-focus-element.html
fast/events/panScroll-click-hyperlink.html
fast/frames/frame-with-noresize-can-be-resized-after-removal-of-noresize.html
fast/frames/sandboxed-iframe-navigation-targetlink.html
editing/selection/home-end.html
http/tests/appcache/deferred-events-delete-while-raising.html
http/tests/misc/async-and-defer-script.html
http/tests/security/xssAuditor/full-block-iframe-no-inherit.php
http/tests/security/xssAuditor/full-block-post-from-iframe.html

[Adam Barth]

Those are from chromium-linux running on 64 bit Lucid Debug.  That likely just the fastest debug bot to cycle, which is why we see the problems there first.

[James Craig]

<rdar://problem/9565898>

http://trac.webkit.org/changeset/89591

[James Craig]

Misread. Reopening.

[James Craig]

Duping out to bug 124381

*** This bug has been marked as a duplicate of bug 124381 ***