Bug 62289 - AX: ARIA live regions don't trigger notifications for elements that aren't in the AX tree
Summary: AX: ARIA live regions don't trigger notifications for elements that aren't in...
Status: RESOLVED DUPLICATE of bug 124381
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on: 63279 69098
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-08 09:16 PDT by chris fleizach
Modified: 2013-11-18 15:33 PST (History)
10 users (show)

See Also:


Attachments
patch (14.83 KB, patch)
2011-06-08 09:59 PDT, chris fleizach
darin: review-
Details | Formatted Diff | Diff
patch (14.72 KB, patch)
2011-06-21 11:29 PDT, chris fleizach
darin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.28 MB, application/zip)
2011-06-21 11:52 PDT, WebKit Review Bot
no flags Details
test patch (16.04 KB, patch)
2011-09-28 18:32 PDT, chris fleizach
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 fleizach 2011-06-08 09:16:50 PDT
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
Comment 1 chris fleizach 2011-06-08 09:59:04 PDT
Created attachment 96437 [details]
patch
Comment 2 Darin Adler 2011-06-20 12:37:33 PDT
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”.
Comment 3 chris fleizach 2011-06-21 11:29:21 PDT
Created attachment 98019 [details]
patch

Addressed all of Darin's comments.
Comment 4 WebKit Review Bot 2011-06-21 11:52:07 PDT
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
Comment 5 WebKit Review Bot 2011-06-21 11:52:11 PDT
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
Comment 6 Darin Adler 2011-06-22 11:45:51 PDT
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.
Comment 7 chris fleizach 2011-06-22 12:40:11 PDT
(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
Comment 8 Darin Adler 2011-06-22 13:55:52 PDT
(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.
Comment 9 chris fleizach 2011-06-23 11:01:39 PDT
http://trac.webkit.org/changeset/89591
Comment 10 chris fleizach 2011-06-23 11:05:25 PDT
and layout tests (which didn't make the first commit)
http://trac.webkit.org/changeset/89593
Comment 11 Dmitry Titov 2011-06-23 12:14:21 PDT
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]
Comment 12 chris fleizach 2011-06-23 12:19:46 PDT
Ok
I'll roll out
Comment 13 chris fleizach 2011-06-23 12:23:31 PDT
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
Comment 14 Ryosuke Niwa 2011-06-23 12:38:04 PDT
Any update on the fix?
Comment 15 chris fleizach 2011-06-23 12:41:30 PDT
(In reply to comment #14)
> Any update on the fix?

working on it right now. was stuck in a meeting
Comment 16 Dmitry Titov 2011-06-23 13:09:32 PDT
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?
Comment 17 chris fleizach 2011-06-23 13:14:06 PDT
(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
Comment 18 chris fleizach 2011-09-28 14:00:30 PDT
(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
Comment 19 chris fleizach 2011-09-28 18:32:30 PDT
Created attachment 109112 [details]
test patch
Comment 20 chris fleizach 2011-09-29 09:37:19 PDT
rdar://9565898
Comment 21 chris fleizach 2011-09-29 10:45:43 PDT
http://trac.webkit.org/changeset/96340
Comment 22 Adam Barth 2011-09-29 11:49:02 PDT
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
Comment 23 chris fleizach 2011-09-29 11:50:39 PDT
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
Comment 24 Adam Barth 2011-09-29 11:55:53 PDT
> Gah... i will roll it out 

I can take care of that for you.
Comment 25 chris fleizach 2011-09-29 11:56:49 PDT
(In reply to comment #24)
> > Gah... i will roll it out 
> 
> I can take care of that for you.

Ok thanks.
Comment 26 chris fleizach 2011-09-29 11:59:47 PDT
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.
Comment 27 Adam Barth 2011-09-29 12:11:00 PDT
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
Comment 28 Adam Barth 2011-09-29 12:12:56 PDT
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.
Comment 30 James Craig 2013-10-23 19:27:22 PDT
Misread. Reopening.
Comment 31 James Craig 2013-11-18 15:33:17 PST
Duping out to bug 124381

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