RESOLVED DUPLICATE of bug 124381 62289
AX: ARIA live regions don't trigger notifications for elements that aren't in the AX tree
https://bugs.webkit.org/show_bug.cgi?id=62289
Summary AX: ARIA live regions don't trigger notifications for elements that aren't in...
chris fleizach
Reported 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
Attachments
patch (14.83 KB, patch)
2011-06-08 09:59 PDT, chris fleizach
darin: review-
patch (14.72 KB, patch)
2011-06-21 11:29 PDT, chris fleizach
darin: review+
webkit.review.bot: commit-queue-
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
test patch (16.04 KB, patch)
2011-09-28 18:32 PDT, chris fleizach
no flags
chris fleizach
Comment 1 2011-06-08 09:59:04 PDT
Darin Adler
Comment 2 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”.
chris fleizach
Comment 3 2011-06-21 11:29:21 PDT
Created attachment 98019 [details] patch Addressed all of Darin's comments.
WebKit Review Bot
Comment 4 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
WebKit Review Bot
Comment 5 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
Darin Adler
Comment 6 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.
chris fleizach
Comment 7 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
Darin Adler
Comment 8 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.
chris fleizach
Comment 9 2011-06-23 11:01:39 PDT
chris fleizach
Comment 10 2011-06-23 11:05:25 PDT
and layout tests (which didn't make the first commit) http://trac.webkit.org/changeset/89593
Dmitry Titov
Comment 11 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]
chris fleizach
Comment 12 2011-06-23 12:19:46 PDT
Ok I'll roll out
chris fleizach
Comment 13 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
Ryosuke Niwa
Comment 14 2011-06-23 12:38:04 PDT
Any update on the fix?
chris fleizach
Comment 15 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
Dmitry Titov
Comment 16 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?
chris fleizach
Comment 17 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
chris fleizach
Comment 18 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
chris fleizach
Comment 19 2011-09-28 18:32:30 PDT
Created attachment 109112 [details] test patch
chris fleizach
Comment 20 2011-09-29 09:37:19 PDT
chris fleizach
Comment 21 2011-09-29 10:45:43 PDT
Adam Barth
Comment 22 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
chris fleizach
Comment 23 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
Adam Barth
Comment 24 2011-09-29 11:55:53 PDT
> Gah... i will roll it out I can take care of that for you.
chris fleizach
Comment 25 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.
chris fleizach
Comment 26 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.
Adam Barth
Comment 27 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
Adam Barth
Comment 28 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.
James Craig
Comment 30 2013-10-23 19:27:22 PDT
Misread. Reopening.
James Craig
Comment 31 2013-11-18 15:33:17 PST
Duping out to bug 124381 *** This bug has been marked as a duplicate of bug 124381 ***
Note You need to log in before you can comment on or make changes to this bug.