Summary: | [GTK] [Stable] Infinite recursion in WebCore::AXObjectCache::getOrCreate | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Critical | CC: | cgarcia, jdiggs, mario, svillar, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Martin Robinson
2012-09-17 10:43:03 PDT
Oops. Here's the bottom of the stack: #98817 0x00007ffff5f1c61c in WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*) () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98818 0x00007ffff5f02cbf in WebCore::AccessibilityObject::firstAnonymousBlockChild() const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98819 0x00007ffff6e4f326 in WebCore::AccessibilityObject::accessibilityPlatformIncludesObject() const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98820 0x00007ffff5f10f43 in WebCore::AccessibilityRenderObject::accessibilityIsIgnoredBase() const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98821 0x00007ffff5f14dc0 in WebCore::AccessibilityRenderObject::accessibilityIsIgnored() const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98822 0x00007ffff5efe022 in WebCore::AccessibilityNodeObject::remapAriaRoleDueToParent(WebCore::AccessibilityRole) const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98823 0x00007ffff5efe0ac in WebCore::AccessibilityNodeObject::determineAriaRoleAttribute() const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98824 0x00007ffff5f11dbc in WebCore::AccessibilityRenderObject::determineAccessibilityRole() () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98825 0x00007ffff5efd98d in WebCore::AccessibilityNodeObject::init() () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98826 0x00007ffff5f0eb6e in WebCore::AccessibilityRenderObject::create(WebCore::RenderObject*) () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98827 0x00007ffff5f1c61c in WebCore::AXObjectCache::getOrCreate(WebCore::RenderObject*) () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98828 0x00007ffff5f02cbf in WebCore::AccessibilityObject::firstAnonymousBlockChild() const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98829 0x00007ffff6e4f326 in WebCore::AccessibilityObject::accessibilityPlatformIncludesObject() const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98830 0x00007ffff5f10f43 in WebCore::AccessibilityRenderObject::accessibilityIsIgnoredBase() const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98831 0x00007ffff5f14dc0 in WebCore::AccessibilityRenderObject::accessibilityIsIgnored() const () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98832 0x00007ffff5f13d0b in WebCore::AccessibilityRenderObject::addChildren() () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98833 0x00007ffff5f023cd in WebCore::AccessibilityObject::children() () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98834 0x00007ffff5f13dba in WebCore::AccessibilityRenderObject::addChildren() () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98835 0x00007ffff5f023cd in WebCore::AccessibilityObject::children() () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98836 0x00007ffff6e5b02c in webkitAccessibleGetNChildren(_AtkObject*) () from /home/martin/work/gnome-shell/install/lib/libwebkitgtk-3.0.so.0 #98837 0x00007fffeef0c44c in append_children (traversal=0x6dd2c0, accessible=0x2b75770) at accessible-cache.c:235 #98838 add_pending_items (data=<optimized out>) at accessible-cache.c:291 #98839 0x00007ffff3c893e3 in g_main_dispatch (context=0x704d40) at gmain.c:2715 #98840 g_main_context_dispatch (context=0x704d40) at gmain.c:3219 #98841 0x00007ffff3c89730 in g_main_context_iterate (dispatch=1, block=<optimized out>, context=0x704d40, self=<optimized out>) at gmain.c:3290 ---Type <return> to continue, or q <return> to quit--- #98842 g_main_context_iterate (context=0x704d40, block=<optimized out>, dispatch=1, self=<optimized out>) at gmain.c:3227 #98843 0x00007ffff3c897f4 in g_main_context_iteration (context=0x704d40, may_block=1) at gmain.c:3351 #98844 0x00007ffff42538b4 in g_application_run (application=0x808000, argc=<optimized out>, argv=0x7fffffffd668) at gapplication.c:1607 This seems to be the crux of the problem: WebCore::AccessibilityObject::firstAnonymousBlockChild calls WebCore::AXObjectCache::getOrCreate calls WebCore::AccessibilityRenderObject::create calls WebCore::AccessibilityNodeObject::init calls WebCore::AccessibilityRenderObject::determineAccessibilityRole calls WebCore::AccessibilityNodeObject::determineAriaRoleAttribute calls WebCore::AccessibilityNodeObject::remapAriaRoleDueToParent calls WebCore::AccessibilityRenderObject::accessibilityIsIgnored calls WebCore::AccessibilityRenderObject::accessibilityIsIgnoredBase calls WebCore::AccessibilityObject::accessibilityPlatformIncludesObject calls WebCore::AccessibilityObject::firstAnonymousBlockChild I'm working on this. Rather than merely back out the associated commit (which would result in needed objects getting ignored by the core a11y code), worst case scenario is that we always include objects of ParagraphRole and DivRole for now. That would eliminate the recursion and the price would not be too high: the occasional extra paragraph/div. In the meantime, if you can find the original test case for the purpose of a new Layout Test, that would be great as I have not encountered this crash and am not yet familiar with the ARIA code that is getting called enough to guess the exact scenario. Created attachment 164839 [details]
Patch
Martin, sorry for the delay on this. The F18-related crashers and inability to run any WebKitGtk test wound up requiring downgrades, re-installations, etc. to F17 :( Anyhoo, the attached patch solves the problem and includes a new test based on the Google+ crash. Thanks for that! Please review. It's always good to make tests as simple and minimal as possible, so please consider this alternate test: <html> </body> <div> The presence of remapped ARIA objects should not crash the test harness. <a role="option"></a> </div> <script> if (window.testRunner) window.testRunner.dumpAsText(); document.body.focus(); </script> </body> </html> By the way, thanks for fixing this! (In reply to comment #6) > It's always good to make tests as simple and minimal as possible, so please consider this alternate test: > > <html> > </body> > > <div> > The presence of remapped ARIA objects should not crash the test harness. > <a role="option"></a> > </div> > > <script> > if (window.testRunner) > window.testRunner.dumpAsText(); > document.body.focus(); > </script> > > </body> > </html> Does that reliably crash for you? I tried all sorts of simple ways to make the crash occur 100% of the time, but could not: * Just loading the test and giving the body focus * Synthesizing a click on the various elements * Directly accessing a specific accessible (and I tried them all) The reason I went with a full hierarchy dump is that the one way I can reliably repro the crash outside of the test harness is to view the page and drill down the accessible tree in Accerciser. So that is what I did. For me the attached test is the ONLY thing which will crash 100% of the time. Comment on attachment 164839 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164839&action=review > Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:95 > + for (RenderObject* r = renderer()->firstChild(); r; r = r->nextSibling()) { > + if (r->isAnonymousBlock()) > + return IncludeObject; I notice here that the check for child->isLink() is gone now. Was it unimportant before or was it replaced by another check? I'm also a bit confused by the change in logic. I'm probably reading it incorrectly. My understanding: Before: If the child is a link or the first child isn't an anonymous block, include the object. Now: If any child is an anonymous block, include the object. Not sure I understand that fully. (In reply to comment #9) > (From update of attachment 164839 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164839&action=review > > > Source/WebCore/accessibility/gtk/AccessibilityObjectAtk.cpp:95 > > + for (RenderObject* r = renderer()->firstChild(); r; r = r->nextSibling()) { > > + if (r->isAnonymousBlock()) > > + return IncludeObject; > > I notice here that the check for child->isLink() is gone now. Was it unimportant before or was it replaced by another check? I'm also a bit confused by the change in logic. I'm probably reading it incorrectly. > > My understanding: > Before: If the child is a link or the first child isn't an anonymous block, include the object. > Now: If any child is an anonymous block, include the object. > > Not sure I understand that fully. The whole Accessible Anonymous Block (is that an oxymoron?) situation makes my eyes cross. So I am sure I'm doing a poor job of explaining. The change in logic is admittedly trading a bit of certainty for probability in order to achieve stability. FWIW there are already regression tests which I added back when I was initially sorting all of this out to ensure the critical problems don't creep back in. I think we'll find a new edge case here and there, but we can deal with those when they arise. Besides I'll take hypothetical edge case over a very real crasher any day of the week. ;) Anyhoo.... Given a Para/Div which has at least one anonymous block child, we want to include the Para/Div. Exception 1: Nested anonymous blocks are a mess. Defer to WebCore. Exception 2: The first child of the anonymous block is a link. In that case we also want to include the Para/Div because that Para/Div is the HTML element which contains the link. The old/current/crashy logic was: * If none of Para/Div's children is an anonymous block: Defer to WebCore. * If the first child of the first anonymous block found is a link: include Para/Div. * If the first child of the first anonymous block has no anonymous block children, we are not in a nested anonymous block situation: Include Para/Div. * Otherwise fall back on the default WebCore behavior to sort out the nested anonymous block mess. The new/proposed logic is: * If we're pretty confident [1] none of Para/Div's children is an anonymous block: Defer to WebCore. * If Para/Div's parent is an anonymous block we may [2] have a mess of nested anonymous blocks: Defer to WebCore. * We are not in a nested anonymous block situation. Thus if Para/Div has any anonymous block children: include Para/Div. [3] [1] What really matters / has been problematic is Anonymous Blocks *which contain text* getting included at the expense of ignoring the parent Para/Div. If there is no text under the Para/Div element at all, there cannot be Anonymous Blocks with text as immediate children. [2] Then again, we may not. There's a chance for an unaddressed edge case here. Haven't found it yet. And perhaps WebCore is just doing the RightThing(tm) for us in these cases. [3] The Para/Div->Anon Block->Link case gets addressed here because we include all Para/Div->Anon Block cases as long as the parent of the Para/Div is not also an Anon Block. And perhaps we are missing another edge case here. But falling back to WebCore to sort out this mess is not, to me, a huge risk. Created attachment 165043 [details]
Patch
Comment on attachment 165043 [details] Patch Clearing flags on attachment: 165043 Committed r129246: <http://trac.webkit.org/changeset/129246> All reviewed patches have been landed. Closing bug. |