| Summary: | AX: ARIA live regions don't trigger notifications for elements that aren't in the AX tree | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||||
| Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||
| Status: | RESOLVED DUPLICATE | ||||||||||||
| Severity: | Normal | CC: | abarth, bdakin, darin, ddkilzer, dglazkov, dimich, jcraig, rniwa, webkit-bug-importer, webkit.review.bot | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | 63279, 69098 | ||||||||||||
| Bug Blocks: | |||||||||||||
| Attachments: |
|
||||||||||||
|
Description
chris fleizach
2011-06-08 09:16:50 PDT
Created attachment 96437 [details]
patch
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”. Created attachment 98019 [details]
patch
Addressed all of Darin's comments.
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 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 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. (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 (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. and layout tests (which didn't make the first commit) http://trac.webkit.org/changeset/89593 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] Ok I'll roll out 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 Any update on the fix? (In reply to comment #14) > Any update on the fix? working on it right now. was stuck in a meeting 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? (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 (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 Created attachment 109112 [details]
test patch
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 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 > Gah... i will roll it out
I can take care of that for you.
(In reply to comment #24) > > Gah... i will roll it out > > I can take care of that for you. Ok thanks. 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. 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 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. Misread. Reopening. Duping out to bug 124381 *** This bug has been marked as a duplicate of bug 124381 *** |