Bug 30123 – [GTK] Segfault while testing accessibility/iframe-bastardization.html

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Philippe Normand]

** (DumpRenderTree:3701): CRITICAL **: AtkObject* webkit_accessible_ref_child(AtkObject*, gint): assertion `static_cast<size_t>(index) < coreObject->children().size()' failed

** (DumpRenderTree:3701): CRITICAL **: atk_object_ref_accessible_child: assertion `ATK_IS_OBJECT (accessible)' failed
ASSERTION FAILED: m_element
(../../WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:131 AccessibilityUIElement AccessibilityUIElement::parentElement())

Program received signal SIGSEGV, Segmentation fault.
0x08053a6a in AccessibilityUIElement::parentElement (this=0x8151318) at ../../WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:131
131	    ASSERT(m_element);
(gdb) t a a bt

Thread 2 (Thread 0xf4120b90 (LWP 4407)):
#0  0xf7fdf430 in __kernel_vsyscall ()
#1  0xf55f4292 in pthread_cond_timedwait@@GLIBC_2.3.2 () at ../nptl/sysdeps/unix/sysv/linux/i386/i686/../i486/pthread_cond_timedwait.S:179
#2  0xf4fcb06d in g_cond_timed_wait_posix_impl (cond=0x80fbc00, entered_mutex=0x80, abs_time=0x7)
    at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/gthread/gthread-posix.c:242
#3  0xf4e00b19 in g_async_queue_pop_intern_unlocked (queue=0x80977e0, try=<value optimized out>, end_time=0xf41202e4)
    at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/glib/gasyncqueue.c:365
#4  0xf4e537a8 in g_thread_pool_wait_for_new_task (data=0x80fa450) at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/glib/gthreadpool.c:220
#5  g_thread_pool_thread_proxy (data=0x80fa450) at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/glib/gthreadpool.c:254
#6  0xf4e5211f in g_thread_create_proxy (data=0x80fa4a8) at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/glib/gthread.c:635
#7  0xf55f04b5 in start_thread (arg=0xf4120b90) at pthread_create.c:300
#8  0xf4c38a5e in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:130

Thread 1 (Thread 0xf42e4760 (LWP 3701)):
#0  0x08053a6a in AccessibilityUIElement::parentElement (this=0x8151318) at ../../WebKitTools/DumpRenderTree/gtk/AccessibilityUIElementGtk.cpp:131
#1  0x0804d79b in parentElementCallback (context=0xf351c0a0, function=0xf34c32c0, thisObject=0xf34c3280, argumentCount=0, arguments=0xffffc02c, exception=0xffffc074)
    at ../../WebKitTools/DumpRenderTree/AccessibilityUIElement.cpp:230
#2  0xf699219c in JSC::JSCallbackFunction::call (exec=0xf351c0a0, functionObject=0xf34c32c0, thisValue=..., args=...) at ../../JavaScriptCore/API/JSCallbackFunction.cpp:65
#3  0xf69d8531 in cti_op_call_NotJSFunction (args=0x8139a88) at ../../JavaScriptCore/jit/JITStubs.cpp:1607
#4  0xf69cf4fa in doubleHash (key=4086416376) at ../../JavaScriptCore/wtf/HashTable.h:437
#5  0xf6a08787 in JSC::JITCode::execute (this=0x80e6b08, registerFile=0x8106f4c, callFrame=0xf351c048, globalData=0x81045b8, exception=0xffffc2b8)
    at ../../JavaScriptCore/jit/JITCode.h:79
#6  0xf69f6eeb in JSC::Interpreter::execute (this=0x8106f40, program=0x80e6af8, callFrame=0x8107324, scopeChain=0x8107490, thisObj=0xf34c0000, exception=0xffffc2b8)
    at ../../JavaScriptCore/interpreter/Interpreter.cpp:658
#7  0xf6abd2df in JSC::evaluate (exec=0x8107324, scopeChain=..., source=..., thisValue=...) at ../../JavaScriptCore/runtime/Completion.cpp:60
#8  0xf6b83722 in WebCore::ScriptController::evaluate (this=0x80c9290, sourceCode=...) at ../../WebCore/bindings/js/ScriptController.cpp:110
#9  0xf6e8e922 in WebCore::FrameLoader::executeScript (this=0x80c8ff4, sourceCode=...) at ../../WebCore/loader/FrameLoader.cpp:701
#10 0xf6e00113 in WebCore::HTMLTokenizer::scriptExecution (this=0x80e60e0, sourceCode=..., state=...) at ../../WebCore/html/HTMLTokenizer.cpp:563
#11 0xf6e00e3e in WebCore::HTMLTokenizer::scriptHandler (this=0x80e60e0, state=...) at ../../WebCore/html/HTMLTokenizer.cpp:505
#12 0xf6e01518 in WebCore::HTMLTokenizer::parseNonHTMLText (this=0x80e60e0, src=..., state=...) at ../../WebCore/html/HTMLTokenizer.cpp:352
#13 0xf6e03a76 in WebCore::HTMLTokenizer::parseTag (this=0x80e60e0, src=..., state=...) at ../../WebCore/html/HTMLTokenizer.cpp:1522
#14 0xf6e04670 in WebCore::HTMLTokenizer::write (this=0x80e60e0, str=..., appendData=true) at ../../WebCore/html/HTMLTokenizer.cpp:1756
#15 0xf6e8d5ee in WebCore::FrameLoader::write (this=0x80c8ff4, 
    str=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., len=1468, flush=false) at ../../WebCore/loader/FrameLoader.cpp:950
#16 0xf6e8d750 in WebCore::FrameLoader::addData (this=0x80c8ff4, 
    bytes=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., length=1468) at ../../WebCore/loader/FrameLoader.cpp:1587
#17 0xf6965385 in WebKit::FrameLoaderClient::committedLoad (this=0x80c8a10, loader=0x80f5868, 
    data=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., length=1468) at ../../WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:150
#18 0xf6e84afc in WebCore::FrameLoader::committedLoad (this=0x80c8ff4, loader=0x80f5868, 
    data=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., length=1468) at ../../WebCore/loader/FrameLoader.cpp:3406
#19 0xf6e6f66f in WebCore::DocumentLoader::commitLoad (this=0x80f5868, 
    data=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test ma---Type <return> to continue, or q <return> to quit---
kes sure that the AX parent chain hierarchy with iframe"..., length=1468) at ../../WebCore/loader/DocumentLoader.cpp:342
#20 0xf6e6f6d8 in WebCore::DocumentLoader::receivedData (this=0x80f5868, 
    data=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., length=1468) at ../../WebCore/loader/DocumentLoader.cpp:354
#21 0xf6e89393 in WebCore::FrameLoader::receivedData (this=0x80c8ff4, 
    data=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., length=1468) at ../../WebCore/loader/FrameLoader.cpp:2210
#22 0xf6e9d9d0 in WebCore::MainResourceLoader::addData (this=0x80f8e00, 
    data=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., length=1468, allAtOnce=false) at ../../WebCore/loader/MainResourceLoader.cpp:143
#23 0xf6ea772b in WebCore::ResourceLoader::didReceiveData (this=0x80f8e00, 
    data=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., length=1468, lengthReceived=1468, allAtOnce=false) at ../../WebCore/loader/ResourceLoader.cpp:248
#24 0xf6e9ce54 in WebCore::MainResourceLoader::didReceiveData (this=0x80f8e00, 
    data=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., length=1468, lengthReceived=1468, allAtOnce=false) at ../../WebCore/loader/MainResourceLoader.cpp:356
#25 0xf6ea6a98 in WebCore::ResourceLoader::didReceiveData (this=0x80f8e00, 
    data=0x80fe400 "<html>\n<script>\n    if (window.layoutTestController)\n        layoutTestController.dumpAsText();\n</script>\n<body id=\"body\">\n\n    <!-- This test makes sure that the AX parent chain hierarchy with iframe"..., length=1468, lengthReceived=1468) at ../../WebCore/loader/ResourceLoader.cpp:398
#26 0xf72cdb99 in readCallback (source=0x80dfac0, res=0x80f8720) at ../../WebCore/platform/network/soup/ResourceHandleSoup.cpp:729
#27 0xf4f225cf in async_ready_callback_wrapper (source_object=0x80dfac0, res=0x80f8720, user_data=0x0)
    at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/gio/ginputstream.c:471
#28 0xf4f30cd9 in IA__g_simple_async_result_complete (simple=0x80f8720) at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/gio/gsimpleasyncresult.c:588
#29 0xf4f3100e in complete_in_idle_cb_for_thread (_data=0x8097778) at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/gio/gsimpleasyncresult.c:650
#30 0xf4e260b1 in g_idle_dispatch (source=0x80fb1a0, callback=0xbbadbeef, user_data=0x8097778) at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/glib/gmain.c:4065
#31 0xf4e27e98 in g_main_dispatch (context=0x8095da0) at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/glib/gmain.c:1960
#32 IA__g_main_context_dispatch (context=0x8095da0) at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/glib/gmain.c:2513
#33 0xf4e2b623 in g_main_context_iterate (context=0x8095da0, block=1, dispatch=1, self=0x8073060)
    at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/glib/gmain.c:2591
#34 0xf4e2b7a8 in IA__g_main_context_iteration (context=0x8095da0, may_block=1) at /build/buildd-glib2.0_2.22.1-1-i386-tx7y62/glib2.0-2.22.1/glib/gmain.c:2654
#35 0x08055f17 in runTest (testPathOrURL=...) at ../../WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:484
#36 0x08056424 in main (argc=2, argv=0xffffd7b4) at ../../WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:807
(gdb)

[Alejandro G. Castro]

*** Bug 30565 has been marked as a duplicate of this bug. ***

[Mario Sanchez Prada]

Created attachment 71443 [details]
Patch proposal

The attached patch fixed the crash by properly checking that m_element is not a null value before actually using it. Current code was failing because the ASSERT(m_element) statement will not work out of debug builds, so it's needed (as it's done in other functions in the same file) to add that extra check. On top of that, now that it's 100% sure m_element is not a null value if it passes that check, we can ASSERT(ATK_IS_OBJECT(m_element)) instead of just doing ASSERT(m_element)

Please notice that this patch just fixes the crash (which is what this bug is about), but it doesn't fix anything else so the Layout test can't be removed yet from the Skipped file. That would be another bug, IMHO.

Asking for review.

[Martin Robinson]

How might an AccessibilityUIElement be created with a m_element member that is null?

[Mario Sanchez Prada]

(In reply to comment #3)
> How might an AccessibilityUIElement be created with a m_element member that is null?

I can't give a categoric answer to this, but in the past I've observed that sometimes (probably depending on how the layout test was written) the platform dependant wrapper for the a11y object had not been created (normally because it was not rendered yet) at the moment of creating the AccessibilityUIElement. 

In those cases, the fix I found was normally to modify the layout test to actually perform all the  checks in a function called in the body.onLoad event (to make sure at least the document was loaded before doing anything).

In other situations the problem was that calling to a function like rootElement or focusedElement was returning NULL (see bug 38648), and perhaps there are other scenarios were you could get a null m_element.

Hence, because of this "experience" I'd say the extra check doesn't hurt and it's generally a good practice to do in AccessibilityUIElement.

But, as always, I might be wrong... :-)

[chris fleizach]

(In reply to comment #4)
> (In reply to comment #3)
> > How might an AccessibilityUIElement be created with a m_element member that is null?
> 
> I can't give a categoric answer to this, but in the past I've observed that sometimes (probably depending on how the layout test was written) the platform dependant wrapper for the a11y object had not been created (normally because it was not rendered yet) at the moment of creating the AccessibilityUIElement. 
> 
> In those cases, the fix I found was normally to modify the layout test to actually perform all the  checks in a function called in the body.onLoad event (to make sure at least the document was loaded before doing anything).
> 
> In other situations the problem was that calling to a function like rootElement or focusedElement was returning NULL (see bug 38648), and perhaps there are other scenarios were you could get a null m_element.
> 
> Hence, because of this "experience" I'd say the extra check doesn't hurt and it's generally a good practice to do in AccessibilityUIElement.
> 
> But, as always, I might be wrong... :-)

A m_renderer can be detached and set to NULL, but the AX object might still be around (waiting to be cleaned up). it doesn't happen frequently, but it does happen

[chris fleizach]

Comment on attachment 71443 [details]
Patch proposal

r=me

[Mario Sanchez Prada]

Comment on attachment 71443 [details]
Patch proposal

(In reply to comment #5)
> [...] 
> A m_renderer can be detached and set to NULL, but the AX object might still be around (waiting to be cleaned up). it doesn't happen frequently, but it does happen

Thanks for the clarification and for the review Chris.

Would you mind setting the cq+ flag? My WebKit commit permission is on its way but not granted yet... you know, the day I'll stop annoying you about this cq flag is nigh, but for the time being I still need this from you :-)

[chris fleizach]

cq flag was not set to ? (In reply to comment #7)
> (From update of attachment 71443 [details])
> (In reply to comment #5)
> > [...] 
> > A m_renderer can be detached and set to NULL, but the AX object might still be around (waiting to be cleaned up). it doesn't happen frequently, but it does happen
> 
> Thanks for the clarification and for the review Chris.
> 
> Would you mind setting the cq+ flag? My WebKit commit permission is on its way but not granted yet... you know, the day I'll stop annoying you about this cq flag is nigh, but for the time being I still need this from you :-)

cq flag was not set to ? (if i recall correctly). make sure that's set if you want that

[Mario Sanchez Prada]

(In reply to comment #8)
> [...]
> cq flag was not set to ? (if i recall correctly). make sure that's set if you want that

You're right, it was not set (so, my fault), but that's because normally (unless I think there many chances to get the r+ rightaway) I prefer to ask first for r? and then for cq? if I get the r+.

Anyway, thanks for setting cq+!

[WebKit Commit Bot]

Comment on attachment 71443 [details]
Patch proposal

Clearing flags on attachment: 71443

Committed r70270: <http://trac.webkit.org/changeset/70270>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.