[GTK] Missing call to g_object_ref while retrieving accessible table cells

[Joanmarie Diggs]

Created attachment 182758 [details]
test case

Steps to reproduce:

1. Load the test case in Epiphany
2. Start the event listener in a terminal
3. Give focus back to Epiphany

Expected results:

1. "Success" would be printed in the terminal window
2. Epiphany would not crash

Actual results:

1. "Failure after x calls" is printed in the terminal window
2. Epiphany might crash

Other details:

* The number of calls before failure is, for me, at least 3
* This problem does not occur if I prevent the unref of table cells in at-spi2-atk
* This problem does not occur in Firefox or gtk-demo

So it seems that we might be unrefing a table cell somewhere where we shouldn't be. But I so far have been unable to find it. And the fact that the number of times varies makes me wonder if there's not a race condition or at-spi2 registry timeout coming into play. :-/

[Joanmarie Diggs]

Created attachment 182759 [details]
event listener

[Joanmarie Diggs]

CCing Piñeiro as I have spent an entire day GDBing this and am still clueless. :(

[Priit Laes (IRC: plaes)]

Example traceback from my case:

[snip]
Program received signal SIGSEGV, Segmentation fault.
0x00007ffff54088db in WebCore::AccessibilityTableCell::parentTable () from /usr/lib64/libwebkitgtk-3.0.so.0
(gdb) t
[Current thread is 1 (Thread 0x7ffff7f8c940 (LWP 31706))]
(gdb) bt
#0  0x00007ffff54088db in WebCore::AccessibilityTableCell::parentTable () from /usr/lib64/libwebkitgtk-3.0.so.0
#1  0x00007ffff54086dd in WebCore::AccessibilityTableCell::isTableCell () from /usr/lib64/libwebkitgtk-3.0.so.0
#2  0x00007ffff540870d in WebCore::AccessibilityTableCell::roleValue () from /usr/lib64/libwebkitgtk-3.0.so.0
#3  0x00007ffff62480e8 in webkitAccessibleDetach () from /usr/lib64/libwebkitgtk-3.0.so.0
#4  0x00007ffff540c5b0 in WebCore::AXObjectCache::remove () from /usr/lib64/libwebkitgtk-3.0.so.0
#5  0x00007ffff540c7c9 in WebCore::AXObjectCache::remove () from /usr/lib64/libwebkitgtk-3.0.so.0
#6  0x00007ffff5bb08bc in WebCore::RenderObject::willBeDestroyed () from /usr/lib64/libwebkitgtk-3.0.so.0
#7  0x00007ffff5acbbfc in WebCore::RenderBlock::willBeDestroyed () from /usr/lib64/libwebkitgtk-3.0.so.0
#8  0x00007ffff5bad2cd in WebCore::RenderObject::destroy () from /usr/lib64/libwebkitgtk-3.0.so.0
#9  0x00007ffff5ba4af9 in WebCore::RenderObjectChildList::destroyLeftoverChildren () from /usr/lib64/libwebkitgtk-3.0.so.0
...
[/snip]

[Joanmarie Diggs]

And also:

#0  0x00007ffff711607d in webkitAccessibleDetach () from /opt/gnome/lib64/libwebkitgtk-3.0.so.0
#1  0x00007ffff630631f in WebCore::AXObjectCache::~AXObjectCache() () from /opt/gnome/lib64/libwebkitgtk-3.0.so.0
#2  0x00007ffff64d84a5 in WebCore::Document::clearAXObjectCache() () from /opt/gnome/lib64/libwebkitgtk-3.0.so.0
#3  0x00007ffff64eb888 in WebCore::Document::detach() () from /opt/gnome/lib64/libwebkitgtk-3.0.so.0
#4  0x00007ffff6907a30 in WebCore::Frame::setView(WTF::PassRefPtr<WebCore::FrameView>) ()
   from /opt/gnome/lib64/libwebkitgtk-3.0.so.0
#5  0x00007ffff686b1b3 in WebCore::FrameLoader::detachFromParent() () from /opt/gnome/lib64/libwebkitgtk-3.0.so.0
#6  0x00007ffff62d5288 in webkit_web_view_dispose(_GObject*) () from /opt/gnome/lib64/libwebkitgtk-3.0.so.0
#7  0x00007fffef9903b1 in g_object_run_dispose (object=0x161e530) at gobject.c:1062
#8  0x00007ffff135205f in gtk_scrolled_window_forall (container=0x717620, include_internals=0, 
    callback=0x7ffff141df30 <gtk_widget_destroy>, callback_data=0x0) at gtkscrolledwindow.c:1581
#9  0x00007ffff1236cb5 in gtk_container_destroy (widget=0x717620) at gtkcontainer.c:1378
#10 0x00007fffef989f9e in g_closure_invoke (closure=closure@entry=0x6d7c10, return_value=return_value@entry=0x0, 
    n_param_values=1, param_values=param_values@entry=0x7fffffffb180, 
    invocation_hint=invocation_hint@entry=0x7fffffffb120) at gclosure.c:777
#11 0x00007fffef99c585 in signal_emit_unlocked_R (node=node@entry=0x6d80b0, detail=detail@entry=0, 
    instance=instance@entry=0x717620, emission_return=emission_return@entry=0x0, 
    instance_and_params=instance_and_params@entry=0x7fffffffb180) at gsignal.c:3683
#12 0x00007fffef9a44ce in g_signal_emit_valist (instance=0x717620, signal_id=<optimized out>, detail=0, 
    var_args=var_args@entry=0x7fffffffb3b8) at gsignal.c:3315
#13 0x00007fffef9a46c2 in g_signal_emit (instance=instance@entry=0x717620, signal_id=<optimized out>, 
    detail=detail@entry=0) at gsignal.c:3371
#14 0x00007ffff142a498 in gtk_widget_dispose (object=0x717620) at gtkwidget.c:10354
#15 0x00007fffef9903b1 in g_object_run_dispose (object=0x717620) at gobject.c:1062

[snip]

[Joanmarie Diggs]

And one more:

#0  0x00007fffebdf66fc in g_type_check_instance_is_a (type_instance=type_instance@entry=0x1dfa8f0, 
    iface_type=<optimized out>) at gtype.c:3986
#1  0x00007fffece488dc in atk_object_set_parent (accessible=0x1dfa8f0, parent=0x1dec770) at atkobject.c:1072
#2  0x00007ffff39307b0 in webkitAccessibleRefChild (object=0x1dec770, index=8)
    at Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:364
#3  0x00007ffff19a93b7 in append_cache_item (obj=0x1dec770, data=0x7fffffffd3e0) at cache-adaptor.c:156
#4  0x00007fffebad68b0 in g_hash_table_foreach (hash_table=0x939a40, 
    func=func@entry=0x7ffff19a9770 <append_accessible_hf>, user_data=user_data@entry=0x7fffffffd3e0) at ghash.c:1488
#5  0x00007ffff19a0259 in spi_cache_foreach (cache=<optimized out>, 
    func=func@entry=0x7ffff19a9770 <append_accessible_hf>, data=data@entry=0x7fffffffd3e0) at accessible-cache.c:418
#6  0x00007ffff19a90a5 in impl_GetItems (bus=<optimized out>, message=<optimized out>, user_data=<optimized out>)
    at cache-adaptor.c:315
#7  0x00007ffff19a6c71 in handle_other (pathstr=0x1f67588 "/org/a11y/atspi/cache", member=<optimized out>, 
    iface=0x1f675b8 "org.a11y.atspi.Cache", path=0x1236800, message=0x1cd8770, bus=0x906040) at droute.c:539
#8  handle_message (bus=0x906040, message=message@entry=0x1cd8770, user_data=user_data@entry=0x1236800)
    at droute.c:586
#9  0x000000367881d9c5 in _dbus_object_tree_dispatch_and_unlock (tree=0x932b30, message=message@entry=0x1cd8770, 
    found_object=found_object@entry=0x7fffffffd5b4) at dbus-object-tree.c:862

[snip]

[Joanmarie Diggs]

I think that this bug is now reflective of multiple bugs. But until we get to the bottom of things, this can be the "tables are f*cked badly" meta bug. ;)

In that spirit, several accessibility-table-related Layout Tests are now crashing. :( Through the magic of git bisect, the winner seems to be the commit associated with: https://bugs.webkit.org/show_bug.cgi?id=104171

My gut is telling me this commit is not so much the cause of the bugs here, but rather made more obvious the fact that we have some serious brokenness elsewhere related to tables. For instance, I've seen flakiness similar to what I state in my opening report for months now. I just hadn't found a reliable way to reproduce it.

[Zan Dobersek]

Here's another backtrace from the debug builder, showing the crashes in the aforementioned a11y layout tests.
The tests that are crashing:
accessibility/aria-tables.html
accessibility/aria-hidden-with-elements.html
platform/gtk/accessibility/aria-table-hierarchy.html

Thread 1 (Thread 0x7f97216e0900 (LWP 11963)):
#0  0x00007f972cbf127c in WTF::HashTable<WebCore::RenderBoxModelObject const*, WTF::KeyValuePair<WebCore::RenderBoxModelObject const*, WebCore::RenderBoxModelObject*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::RenderBoxModelObject const*, WebCore::RenderBoxModelObject*> >, WTF::PtrHash<WebCore::RenderBoxModelObject const*>, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::RenderBoxModelObject const*>, WTF::HashTraits<WebCore::RenderBoxModelObject*> >, WTF::HashTraits<WebCore::RenderBoxModelObject const*> >::checkKey<WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::RenderBoxModelObject const*> >, WebCore::RenderBoxModelObject const*> (this=<error reading variable: Cannot access memory at address 0x7fff02651fe8>, key=<error reading variable: Cannot access memory at address 0x7fff02651fe0>) at ../../Source/WTF/wtf/HashTable.h:584
#1  0x00007f972cbef6a7 in WTF::HashTable<WebCore::RenderBoxModelObject const*, WTF::KeyValuePair<WebCore::RenderBoxModelObject const*, WebCore::RenderBoxModelObject*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::RenderBoxModelObject const*, WebCore::RenderBoxModelObject*> >, WTF::PtrHash<WebCore::RenderBoxModelObject const*>, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::RenderBoxModelObject const*>, WTF::HashTraits<WebCore::RenderBoxModelObject*> >, WTF::HashTraits<WebCore::RenderBoxModelObject const*> >::lookup<WTF::IdentityHashTranslator<WTF::PtrHash<WebCore::RenderBoxModelObject const*> >, WebCore::RenderBoxModelObject const*> (this=0x2c40220, key=@0x7fff026520e8: 0x2cdf978) at ../../Source/WTF/wtf/HashTable.h:602
#2  0x00007f972cbedfaf in WTF::HashTable<WebCore::RenderBoxModelObject const*, WTF::KeyValuePair<WebCore::RenderBoxModelObject const*, WebCore::RenderBoxModelObject*>, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<WebCore::RenderBoxModelObject const*, WebCore::RenderBoxModelObject*> >, WTF::PtrHash<WebCore::RenderBoxModelObject const*>, WTF::HashMapValueTraits<WTF::HashTraits<WebCore::RenderBoxModelObject const*>, WTF::HashTraits<WebCore::RenderBoxModelObject*> >, WTF::HashTraits<WebCore::RenderBoxModelObject const*> >::lookup (this=0x2c40220, key=@0x7fff026520e8: 0x2cdf978) at ../../Source/WTF/wtf/HashTable.h:419
#3  0x00007f972cbecb8b in WTF::HashMap<WebCore::RenderBoxModelObject const*, WebCore::RenderBoxModelObject*, WTF::PtrHash<WebCore::RenderBoxModelObject const*>, WTF::HashTraits<WebCore::RenderBoxModelObject const*>, WTF::HashTraits<WebCore::RenderBoxModelObject*> >::get (this=0x2c40220, key=@0x7fff026520e8: 0x2cdf978) at ../../Source/WTF/wtf/HashMap.h:368
#4  0x00007f972cbe9bb7 in WebCore::RenderBoxModelObject::continuation (this=0x2cdf978) at ../../Source/WebCore/rendering/RenderBoxModelObject.cpp:2683
#5  0x00007f972cb543de in WebCore::RenderBlock::inlineElementContinuation (this=0x2cdf978) at ../../Source/WebCore/rendering/RenderBlock.cpp:3113
#6  0x00007f972c170d57 in WebCore::startOfContinuations (r=0x2cdf978) at ../../Source/WebCore/accessibility/AccessibilityRenderObject.cpp:261
#7  0x00007f972c171676 in WebCore::AccessibilityRenderObject::renderParentObject (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityRenderObject.cpp:436
#8  0x00007f972c171996 in WebCore::AccessibilityRenderObject::parentObject (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityRenderObject.cpp:491
#9  0x00007f972c165c2d in WebCore::AccessibilityObject::parentObjectUnignored (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityObject.cpp:345
#10 0x00007f972c1547d3 in WebCore::AccessibilityARIAGridRow::parentTable (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityARIAGridRow.cpp:123
#11 0x00007f972c189421 in WebCore::AccessibilityTableRow::isTableRow (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityTableRow.cpp:72
#12 0x00007f972c1893df in WebCore::AccessibilityTableRow::roleValue (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityTableRow.cpp:64
#13 0x00007f972c1630b7 in WebCore::AccessibilityObject::isWebArea (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityObject.h:381
#14 0x00007f972c1719d7 in WebCore::AccessibilityRenderObject::parentObject (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityRenderObject.cpp:496
#15 0x00007f972c165c2d in WebCore::AccessibilityObject::parentObjectUnignored (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityObject.cpp:345
#16 0x00007f972c1547d3 in WebCore::AccessibilityARIAGridRow::parentTable (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityARIAGridRow.cpp:123
#17 0x00007f972c189421 in WebCore::AccessibilityTableRow::isTableRow (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityTableRow.cpp:72
#18 0x00007f972c1893df in WebCore::AccessibilityTableRow::roleValue (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityTableRow.cpp:64
#19 0x00007f972c1630b7 in WebCore::AccessibilityObject::isWebArea (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityObject.h:381
#20 0x00007f972c1719d7 in WebCore::AccessibilityRenderObject::parentObject (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityRenderObject.cpp:496
#21 0x00007f972c165c2d in WebCore::AccessibilityObject::parentObjectUnignored (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityObject.cpp:345
#22 0x00007f972c1547d3 in WebCore::AccessibilityARIAGridRow::parentTable (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityARIAGridRow.cpp:123
#23 0x00007f972c189421 in WebCore::AccessibilityTableRow::isTableRow (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityTableRow.cpp:72
#24 0x00007f972c1893df in WebCore::AccessibilityTableRow::roleValue (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityTableRow.cpp:64
#25 0x00007f972c1630b7 in WebCore::AccessibilityObject::isWebArea (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityObject.h:381
#26 0x00007f972c1719d7 in WebCore::AccessibilityRenderObject::parentObject (this=0x2bb4c00) at ../../Source/WebCore/accessibility/AccessibilityRenderObject.cpp:496
... (Note that the backtrace shows a loop through the last 6 frames)

I've created bug #106922 to cover the crashing layout tests, but that bug essentially depends on this one.

[Joanmarie Diggs]

(In reply to comment #7)
 
> I've created bug #106922 to cover the crashing layout tests, but that bug essentially depends on this one.

Thanks Zan! :)

Having looked into it, the crashing layout tests had absolutely nothing to do with my original report or my comment 4 and comment 5 crashers. It looks similar to Priit's comment 3 crasher, but I'm not positive it is. Like I said, we have all sorts of table-related issues. :-/ So I wound up attaching my proposed fix for the crashing layout tests on the bug you filed.

Priit do you have a reliable crasher that you could use to determine if your problem is bug 106922 and/or give us more to debug with?

[Priit Laes (IRC: plaes)]

(In reply to comment #8)
> (In reply to comment #7)
> 
> Priit do you have a reliable crasher that you could use to determine if your problem is bug 106922 and/or give us more to debug with?

Two quite reliable crashers:
  1) epiphany - close tab
  2) evolution - switch emails (I guess this was html-email containing tables)

[Mario Sanchez Prada]

Created attachment 186349 [details]
Add missing extra ref to implementation of atk_table_ref_at()

(In reply to comment #0)
> Created an attachment (id=182758) [details]
> test case
> 
> Steps to reproduce:
> 
> 1. Load the test case in Epiphany
> 2. Start the event listener in a terminal
> 3. Give focus back to Epiphany
> 
> Expected results:
> 
> 1. "Success" would be printed in the terminal window
> 2. Epiphany would not crash
> 
> Actual results:
> 
> 1. "Failure after x calls" is printed in the terminal window
> 2. Epiphany might crash
> 
> Other details:
> 
> * The number of calls before failure is, for me, at least 3
> * This problem does not occur if I prevent the unref of table cells in at-spi2-atk
> * This problem does not occur in Firefox or gtk-demo
> 
> So it seems that we might be unrefing a table cell somewhere where we shouldn't be.
> But I so far have been unable to find it. And the fact that the number of times
> varies makes me wonder if there's not a race condition or at-spi2 registry timeout
> coming into play. :-/

I was checking this for a while now and I think the problem might be other than an extra and uneeded unref. More specifically, I think the problem is that there's a "missing and needed ref" in the implementation of the atk_table_ref_at() method, which is meant to return a transfer-full reference to the AtkObject representing the cell, but that it's now returning the internal pointer held in WebCore's AccessibilityObject::m_wrapper

   In accessibility/atk/WebKitAccessibleInterfaceTable.cpp:
   ========================================================

   static AtkObject* webkitAccessibleTableRefAt(AtkTable* table, gint row, gint column)
   {
        AccessibilityTableCell* axCell = cell(table, row, column);
        if (!axCell)
            return 0;

       return axCell->wrapper();
   }

   [...]

   In accessibility/atk/AccessibilityObjectAtk.cpp:
   ================================================
   AccessibilityObjectWrapper* AccessibilityObject::wrapper() const
   {
       return m_wrapper;
   }


Thus, I believe this is what was causing the issue when getting the cell from the event listener example (I could reproduce the issue in Accerciser too), since the implementation of "GetAccessibleAt" in the side of the ATK bridge is as follows:

   static DBusMessage *
   impl_GetAccessibleAt (DBusConnection * bus, DBusMessage * message,
                         void *user_data)
   {
     AtkTable *table = (AtkTable *) user_data;
     [...]

     obj = atk_table_ref_at (table, row, column);
     reply = spi_object_return_reference (message, obj);
     if (obj)
       g_object_unref (obj);

     return reply;
   }


I think that g_object_unref() is the one you (Joanie) commented to check if it fixed the issue, but that unref is Ok since atk_table_ref_at() is a transfer-full method, which reinforces my belief that the problem here was a missing call to g_object_ref(). You can actually check with gdb that ref_count for "obj" is zero here, too.

Last, regarding to the undeterministic nature of the failure, I do also think that the at-spi2 registry might have something to do with that. Probably the cached version of the AT-SPI object representing the still valid version of the cell (before reaching ref_count zero in WebKit) is still being returned to pyatspi for some time.

So, attaching a one-liner patch :)

[Joanmarie Diggs]

> So, attaching a one-liner patch :)

Suhweet!! :) That fixes the problem I was seeing with Orca (but could not come up with a reliably-reproducible test case for). Thank YOU!!!

Any suggestions regarding a layout test to go with this patch? (Or could we go layout-testless on the grounds that it's a one-liner patch that fixes a hard-to-repro, but pain in the arse bug?)

I am now going to build webkit with this patch in my GNOME jhbuild environment to see if it makes the Orca + Evolution crasher go away. /me crosses her fingers.

Thanks again!!

[Mario Sanchez Prada]

(In reply to comment #11)
> > So, attaching a one-liner patch :)
> 
> Suhweet!! :) That fixes the problem I was seeing with Orca (but could not come 
> up with a reliably-reproducible test case for). Thank YOU!!!

Glad to hear that. Thanks!

> Any suggestions regarding a layout test to go with this patch? (Or could we go 
> layout-testless on the grounds that it's a one-liner patch that fixes a 
> hard-to-repro, but pain in the arse bug?)

Yes, I think I could write a layout test that calls repeatedly to cellForColumnAndRow() for the accessibilityUIElement representing the table, which should crash without the patch and not with it.

Or we could just write an unit tests to call atk_table_ref_at() directly, too.

I'll work on that now.

> I am now going to build webkit with this patch in my GNOME jhbuild 
> environment to see if it makes the Orca + Evolution crasher go away. 
> /me crosses her fingers.

/me crosses fingers too!

[Build Bot]

Comment on attachment 186349 [details]
Add missing extra ref to implementation of atk_table_ref_at()

Attachment 186349 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16366314

[Dominik Röttsches (drott)]

(In reply to comment #11)
> > So, attaching a one-liner patch :)
> 
> Suhweet!! :) [...] Thank YOU!!!

+1! Thanks, Mario. - this one makes our bots really sad. Hope we can land this soon, ideally with a test.

[Joanmarie Diggs]

(In reply to comment #14)
> (In reply to comment #11)
> > > So, attaching a one-liner patch :)
> > 
> > Suhweet!! :) [...] Thank YOU!!!
> 
> +1! Thanks, Mario. - this one makes our bots really sad.

This comment troubles me slightly. Because all the a11y-related crashers related to tables **without an AT like a screen reader running** have AFAIK been fixed already. Which bots are having this problem and with which tests?

[Mario Sanchez Prada]

(In reply to comment #14)
> (In reply to comment #11)
> > > So, attaching a one-liner patch :)
> > 
> > Suhweet!! :) [...] Thank YOU!!!
> 
> +1! Thanks, Mario. - this one makes our bots really sad. Hope we can land this soon, ideally with a test.

You're welcome. And yes, there will be a patch for it, I'm actually about to upload it now.

Besides, I did some further tests and wrote a layout test to check this patch, confirming that was actually the issue here. Thus I'm renaming the bug to avoid confusion.

[Mario Sanchez Prada]

Created attachment 186387 [details]
Patch proposal plus new Layout test

This patch seems to fix the issue as described with the bug report. The test I'm adding is cross platform, so I'm adding Dominic too (Apple and EFL guys already present). Now actually asking for review

[chris fleizach]

Comment on attachment 186387 [details]
Patch proposal plus new Layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=186387&action=review

> Source/WebCore/ChangeLog:10
> +        Test: platform/gtk/accessibility/table-cell-for-column-and-row-crash.html

Looks like the test is wrong in this changelog

[Mario Sanchez Prada]

(In reply to comment #18)
> (From update of attachment 186387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186387&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Test: platform/gtk/accessibility/table-cell-for-column-and-row-crash.html
> 
> Looks like the test is wrong in this changelog

Ah, right... this comes from a previous version of the patch. 

Thanks for catching this. I'll change that.

[Mario Sanchez Prada]

Created attachment 186392 [details]
Patch proposal plus new Layout test

New version addressing issue from comment #18

[Build Bot]

Comment on attachment 186392 [details]
Patch proposal plus new Layout test

Attachment 186392 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16372296

New failing tests:
accessibility/table-cell-for-column-and-row-crash.html

[Martin Robinson]

Comment on attachment 186387 [details]
Patch proposal plus new Layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=186387&action=review

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:874
>      AtkObject* foundCell = atk_table_ref_at(ATK_TABLE(m_element), row, col);

Could you use GRefPtr here?

> LayoutTests/accessibility/table-cell-for-column-and-row-crash.html:14
> +   axCell = axTable.cellForColumnAndRow(row, column);
> +   shouldBe("axCell.role", "'AXRole: AXCell'");
> +   axCell = null;
> +
> +   // We need to force a call to the Garbage Collector here so we are
> +   // sure that axCell will get actually destroyed after using it.
> +   window.gc();

It's good practice to use "var" to make sure your variable is scoped to the function instead of a property on window.

> LayoutTests/accessibility/table-cell-for-column-and-row-crash.html:35
> +      axBody = accessibilityController.focusedElement;
> +
> +      axTable = axBody.childAtIndex(0);
> +      shouldBe("axTable.role", "'AXRole: AXTable'");
> +
> +      // Trying to reference the same cell for the table
> +      // multiple times shouldn't result in a crash.
> +      for (i = 0; i < 10; i++)
> +        getCellForTableAndRelease(axTable, 0, 0);
> +  }

Same here, I'd say.

[WebKit Review Bot]

Comment on attachment 186392 [details]
Patch proposal plus new Layout test

Attachment 186392 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16373275

New failing tests:
accessibility/table-cell-for-column-and-row-crash.html

[Build Bot]

Comment on attachment 186392 [details]
Patch proposal plus new Layout test

Attachment 186392 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16368319

New failing tests:
accessibility/table-cell-for-column-and-row-crash.html

[Build Bot]

Comment on attachment 186392 [details]
Patch proposal plus new Layout test

Attachment 186392 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16377241

[Dominik Röttsches (drott)]

(In reply to comment #15)
> This comment troubles me slightly. Because all the a11y-related crashers related to tables **without an AT like a screen reader running** have AFAIK been fixed already. Which bots are having this problem and with which tests?

I discussed about this with Mario on IRC - might be that my analysis was slightly incomplete. I was observing a11n related assertion failures on the EFL Wk2 bots. But you might be right and these had already gone - sorry if this was confusing.

[Mario Sanchez Prada]

Created attachment 186582 [details]
Patch proposal plus new Layout test

(In reply to comment #22)
> (From update of attachment 186387 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186387&action=review
> 
> > Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:874
> >      AtkObject* foundCell = atk_table_ref_at(ATK_TABLE(m_element), row, col);
> 
> Could you use GRefPtr here?

Oh! I missed that, sorry. Will change it here and in DRT as well.

> > LayoutTests/accessibility/table-cell-for-column-and-row-crash.html:14
> > +   axCell = axTable.cellForColumnAndRow(row, column);
> > +   shouldBe("axCell.role", "'AXRole: AXCell'");
> > +   axCell = null;
> > +
> > +   // We need to force a call to the Garbage Collector here so we are
> > +   // sure that axCell will get actually destroyed after using it.
> > +   window.gc();
> 
> It's good practice to use "var" to make sure your variable is scoped to the function instead of a property on window.

Yes, but the problem here is that I wrote all my javascript code inside functions (e.g. test(), to be run when body.onload) and if I use "var" to declare those variables I will obviously give them the scope of those functions, resulting on the shouldBe() statements not working, as they expect strings with the name of globally scoped variables.

Anyway, I agree with you it's good practice, so I'll change the test not to use functions and just to use a plain javascript block at the end of the body, as many other tests work (and in this case I don't think the body.onload thing is crucial anyway). Also, I will make the most of this change to replace the call to window.gc() with a simple call to gc(), which seems to be a safer implementation defined in js-test-pre.js.

> > LayoutTests/accessibility/table-cell-for-column-and-row-crash.html:35
> > +      axBody = accessibilityController.focusedElement;
> > +
> > +      axTable = axBody.childAtIndex(0);
> > +      shouldBe("axTable.role", "'AXRole: AXTable'");
> > +
> > +      // Trying to reference the same cell for the table
> > +      // multiple times shouldn't result in a crash.
> > +      for (i = 0; i < 10; i++)
> > +        getCellForTableAndRelease(axTable, 0, 0);
> > +  }
> 
> Same here, I'd say.

Ok. Additionally, as I moved everything to a JS block at the end of body, I also merged the content of getCellForTableAndRelease right in the for loop.

(In reply to comment #23)
> (From update of attachment 186392 [details])
> Attachment 186392 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16373275
> 
> New failing tests:
> accessibility/table-cell-for-column-and-row-crash.html
>
(In reply to comment #24)
> (From update of attachment 186392 [details])
> Attachment 186392 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://queues.webkit.org/results/16368319
> 
> New failing tests:
> accessibility/table-cell-for-column-and-row-crash.html

I wonder if these two failures might be related to the (mis)usage of window.gc() instead of js-test-pre.js's gc() function. Let's see...

(In reply to comment #25)
> (From update of attachment 186392 [details])
> Attachment 186392 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/16377241

No idea why I get this while the EWS is green at the same time. Btw, any idea how to check the actual output in these EWS bots? It's quite hard to just wild guess what might be going on without any other input.

[WebKit Review Bot]

Comment on attachment 186582 [details]
Patch proposal plus new Layout test

Attachment 186582 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16366728

New failing tests:
accessibility/table-cell-for-column-and-row-crash.html

[Build Bot]

Comment on attachment 186582 [details]
Patch proposal plus new Layout test

Attachment 186582 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16372703

New failing tests:
accessibility/table-cell-for-column-and-row-crash.html

[Build Bot]

Comment on attachment 186582 [details]
Patch proposal plus new Layout test

Attachment 186582 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16365753

New failing tests:
accessibility/table-cell-for-column-and-row-crash.html

[WebKit Review Bot]

Comment on attachment 186582 [details]
Patch proposal plus new Layout test

Attachment 186582 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16366740

New failing tests:
accessibility/table-cell-for-column-and-row-crash.html

[Mario Sanchez Prada]

(In reply to comment #31)
> (From update of attachment 186582 [details])
> Attachment 186582 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16366740
> 
> New failing tests:
> accessibility/table-cell-for-column-and-row-crash.html

[...]

(In reply to comment #30)
> (From update of attachment 186582 [details])
> Attachment 186582 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/16365753
> 
> New failing tests:
> accessibility/table-cell-for-column-and-row-crash.html

Dominic, Chris, do you have any idea about why this test might be failing in Mac and Chromium? I reviewed other cross platform tests involving tables as direct descendants of <body> and I guess this should be working now that rolenames are consistent.

Also, if there's a way to check the actual output of Mac/Chromium's EWS, that would be fantastic too :)

[chris fleizach]

Comment on attachment 186582 [details]
Patch proposal plus new Layout test

my guess is that this <Table> doesn't qualify as an accessibility table, so the roles are different. There are a number of things you can do to "make" it a table. maybe the easiest is to add a row of <th> or add a <summary>

[Build Bot]

Comment on attachment 186582 [details]
Patch proposal plus new Layout test

Attachment 186582 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16367827

New failing tests:
accessibility/table-cell-for-column-and-row-crash.html

[Mario Sanchez Prada]

Created attachment 186641 [details]
Patch proposal plus new Layout test

(In reply to comment #33)
> (From update of attachment 186582 [details])
> my guess is that this <Table> doesn't qualify as an accessibility table,
> so the roles are different. There are a number of things you can do to
> "make" it a table. maybe the easiest is to add a row of <th> or add a <summary>

Thanks for the suggestion, Chris. Let's give it a shot then to the idea of adding a summary, I guess that should be enough according to http://www.w3.org/TR/html401/struct/tables.html#adef-summary

[WebKit Review Bot]

Comment on attachment 186641 [details]
Patch proposal plus new Layout test

Attachment 186641 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16366865

New failing tests:
accessibility/table-cell-for-column-and-row-crash.html

[chris fleizach]

FYI This passed on my mac with these results

---------------

foo
bar
This tests that retrieving a cell for a table multiple times doesn't crash.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS axTable.role is 'AXRole: AXTable'
PASS axCell.role is 'AXRole: AXCell'
PASS axCell.role is 'AXRole: AXCell'
PASS axCell.role is 'AXRole: AXCell'
PASS axCell.role is 'AXRole: AXCell'
PASS axCell.role is 'AXRole: AXCell'
PASS axCell.role is 'AXRole: AXCell'
PASS axCell.role is 'AXRole: AXCell'
PASS axCell.role is 'AXRole: AXCell'
PASS axCell.role is 'AXRole: AXCell'
PASS axCell.role is 'AXRole: AXCell'
PASS successfullyParsed is true

TEST COMPLETE
---------------------

[Mario Sanchez Prada]

Created attachment 186814 [details]
Patch proposal plus new Layout test

(In reply to comment #37)
> FYI This passed on my mac with these results

Thanks for running it. It also passed in the EWS so I guess that was it.

Still failing in chromium, though. And after reviewing more carefully other expected results for chromium, I think I was perhaps too fast assuming it would work there out of the box, as the output is not going to be exactly the same for now, since the AccessibilityUIElement::cellForColumnAndRowCallback() is not implemented in DRT.

Thus, I'm now skipping the test in chromium too.

[Mario Sanchez Prada]

(In reply to comment #38)
> [...]
> Still failing in chromium, though. And after reviewing more carefully other expected results for chromium, I think I was perhaps too fast assuming it would work there out of the box, as the output is not going to be exactly the same for now, since the AccessibilityUIElement::cellForColumnAndRowCallback() is not implemented in DRT.
> 
> Thus, I'm now skipping the test in chromium too.

Just a ping to make sure whether Dominic is ok with this, so we can get it fixed soon.

[Mario Sanchez Prada]

Created attachment 188332 [details]
Patch proposal plus new Layout test

> [...]
> Just a ping to make sure whether Dominic is ok with this, so we can get it fixed soon.

Actually, I also needed to update this patch after bug 105007 got fixed a few days ago.

[Mario Sanchez Prada]

Thanks Martin for the r+. I guess I will push it now already because in any case there's not much we can do to make it pass in chromium other than implementing the missing functionality in DRT, and that would be another bug already.

So, I guess Dominic will be Ok anyway, so I'll be committing this right now :)

[Mario Sanchez Prada]

Committed r142883: <http://trac.webkit.org/changeset/142883>

[Dominik Röttsches (drott)]

(In reply to comment #39)

> Just a ping to make sure whether Dominic is ok with this, so we can get it fixed soon.

Sorry for the late reply - I am not too familiar with this code - so certainly no objections.

[Dominic Mazzoni]

Yes, this is fine with me. I haven't decided what I want to do about Chromium layout tests for table-specific functions. In Chromium we reimplement those methods much higher up in the abstraction layer, so I haven't really decided what it makes sense to test here.