WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
chris fleizach
Comment 1
2011-06-08 09:59:04 PDT
Created
attachment 96437
[details]
patch
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
http://trac.webkit.org/changeset/89591
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
rdar://9565898
chris fleizach
Comment 21
2011-09-29 10:45:43 PDT
http://trac.webkit.org/changeset/96340
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 29
2013-10-23 19:26:43 PDT
<
rdar://problem/9565898
>
http://trac.webkit.org/changeset/89591
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.
Top of Page
Format For Printing
XML
Clone This Bug