Bug 106922

Summary: REGRESSION (r139444): Crashes in three accessibility tests on GTK
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKitGTKAssignee: Joanmarie Diggs <jdiggs>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, buildbot, cfleizach, dmazzoni, jdiggs, rniwa, svillar, webkit.review.bot
Priority: P2 Keywords: Gtk, LayoutTestFailure, Regression
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 98347    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Zan Dobersek 2013-01-15 10:33:24 PST
http://trac.webkit.org/changeset/139444

The affected tests:
accessibility/aria-tables.html
accessibility/aria-hidden-with-elements.html
platform/gtk/accessibility/aria-table-hierarchy.html

The backtrace of the crashing thread:
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)
Comment 1 Joanmarie Diggs 2013-01-15 17:21:04 PST
While r139444 is officially the crasher cause; this really seems like an infinite loop that was just waiting to happen.

  0. parentObjectUnignored() calls parentObject()
  1. parentObject() calls isWebArea()
  2. isWebArea() calls roleValue()

  In the case of a table row:

  3. roleValue() calls isTableRow()
  4. isTableRow() calls parentTable() 

  For non-aria table rows, it looks at the renderer and gets the
  renderer's table and then gets the corresponding accessible from
  AXObjectCache. BUT for aria table rows, it works it's way up the
  accessible tree:

  5. parentTable() calls parentObjectUnignored()

Proposed patch comin' up.
Comment 2 Joanmarie Diggs 2013-01-15 17:44:22 PST
Created attachment 182886 [details]
Patch
Comment 3 Joanmarie Diggs 2013-01-15 17:45:52 PST
Chris and Dominic: Mind taking a look? Thanks in advance!
Comment 4 chris fleizach 2013-01-15 17:48:11 PST
Comment on attachment 182886 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityARIAGridRow.cpp:127
> +    // Calling parentObjectUnignored() can result in an infinite loop when roleValue() is examined.

would parentObjectIfExists() work here?
Comment 5 Joanmarie Diggs 2013-01-15 17:58:42 PST
(In reply to comment #4)
> (From update of attachment 182886 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182886&action=review
> 
> > Source/WebCore/accessibility/AccessibilityARIAGridRow.cpp:127
> > +    // Calling parentObjectUnignored() can result in an infinite loop when roleValue() is examined.
> 
> would parentObjectIfExists() work here?

I didn't try it, but taking a look at the method:

{
    // WebArea's parent should be the scroll view containing it.
    if (isWebArea() || isSeamlessWebArea())
        return axObjectCache()->get(m_renderer->frame()->view());

    return axObjectCache()->get(renderParentObject());

It's the isWebArea() call in parentObject() that's getting us into trouble here. So my guess is that it wouldn't work. (Right?)
Comment 6 chris fleizach 2013-01-15 18:00:31 PST
(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 182886 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=182886&action=review
> > 
> > > Source/WebCore/accessibility/AccessibilityARIAGridRow.cpp:127
> > > +    // Calling parentObjectUnignored() can result in an infinite loop when roleValue() is examined.
> > 
> > would parentObjectIfExists() work here?
> 
> I didn't try it, but taking a look at the method:
> 
> {
>     // WebArea's parent should be the scroll view containing it.
>     if (isWebArea() || isSeamlessWebArea())
>         return axObjectCache()->get(m_renderer->frame()->view());
> 
>     return axObjectCache()->get(renderParentObject());
> 
> It's the isWebArea() call in parentObject() that's getting us into trouble here. So my guess is that it wouldn't work. (Right?)

I hope isWebArea() isn't causing a problem. all it does is

 bool isWebArea() const { return roleValue() == WebAreaRole; }
Comment 7 Joanmarie Diggs 2013-01-15 18:09:13 PST
(In reply to comment #6)

> I hope isWebArea() isn't causing a problem. all it does is
> 
>  bool isWebArea() const { return roleValue() == WebAreaRole; }

From the opening report:

#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

Having said that, I suppose my previous statement isn't quite right. isWebArea() itself is not to blame; it's what happens when we call roleValue() in a table row -- in particular an ARIA table row.

For table rows we determine the role value by first looking to the parent table. In non-ARIA tables, we get the parent table by looking to the renderer, getting its table, and turning that into an accessible. Something I assume we can count on working because non-ARIA tables are, well, tables. :)

In ARIA tables, we call parentObjectUnignored() which can result in our calling isWebArea() which results in our calling roleValue() which result in our looking to the parent table which we get by parentObjectUnignored().
Comment 8 chris fleizach 2013-01-15 19:39:12 PST
Ah I see

I don't have the code handy but it occurs to me that maybe roleValue() should be const and the role for this table row should be pre determined like other objects

(In reply to comment #7)
> (In reply to comment #6)
> 
> > I hope isWebArea() isn't causing a problem. all it does is
> > 
> >  bool isWebArea() const { return roleValue() == WebAreaRole; }
> 
> From the opening report:
> 
> #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
> 
> Having said that, I suppose my previous statement isn't quite right. isWebArea() itself is not to blame; it's what happens when we call roleValue() in a table row -- in particular an ARIA table row.
> 
> For table rows we determine the role value by first looking to the parent table. In non-ARIA tables, we get the parent table by looking to the renderer, getting its table, and turning that into an accessible. Something I assume we can count on working because non-ARIA tables are, well, tables. :)
> 
> In ARIA tables, we call parentObjectUnignored() which can result in our calling isWebArea() which results in our calling roleValue() which result in our looking to the parent table which we get by parentObjectUnignored().
Comment 9 Joanmarie Diggs 2013-01-15 19:52:06 PST
(In reply to comment #8)
> Ah I see
> 
> I don't have the code handy but it occurs to me that maybe roleValue() should be const and the role for this table row should be pre determined like other objects

I can give that a try. But I'm wondering if the way it was done as it is currently is due to differences in hierarchy, differences in how tables get exposed on different platforms, etc.

So do you think looking to the render objects as I did in my patch is a bad idea?
Comment 10 chris fleizach 2013-01-15 20:04:36 PST
I think your proposed solution gets the job done, I'm just trying to think if we can address the issue at a more fundamental level
Comment 11 Joanmarie Diggs 2013-01-16 08:15:43 PST
Created attachment 182986 [details]
Patch
Comment 12 Joanmarie Diggs 2013-01-16 08:17:22 PST
(In reply to comment #10)
> I think your proposed solution gets the job done, I'm just trying to think if we can address the issue at a more fundamental level

But a one-line fix takes all the fun out of it. ;) ;) ;) ;)

Ok, I tried what you suggested and it works on the Gtk port. Let's see if the EWS spits up.

And thanks for the suggestion!
Comment 13 Joanmarie Diggs 2013-01-16 10:38:23 PST
Comment on attachment 182986 [details]
Patch

Chris, the EWS didn't spit up. The fix is a one-liner which does (I think) what you suggested. :)

Please review. Thanks!
Comment 14 chris fleizach 2013-01-16 10:41:10 PST
Thanks. I'm only concerned with if there are cases where an ARIA grid row is not a RowRole.

I am thinking whether the spec states that a role="grid row" should not be a row if it does not have a parent that is a table

ie)

<div>
<div role="gridrow">row</div>
</div>

should the row show up as a RowRole out of the table context? 

Part of me thinks there was something in the spec that stated this should not happen
Comment 15 Joanmarie Diggs 2013-01-16 11:31:30 PST
(In reply to comment #14)
> Thanks. I'm only concerned with if there are cases where an ARIA grid row is not a RowRole.
> 
> I am thinking whether the spec states that a role="grid row" should not be a row if it does not have a parent that is a table
> 
> ie)
> 
> <div>
> <div role="gridrow">row</div>
> </div>
> 
> should the row show up as a RowRole out of the table context? 
> 
> Part of me thinks there was something in the spec that stated this should not happen

In which case, we would need to check the parentTable, would we not? Checking the parentTable is what was being done before (though with the infinite loop). My original patch continues to check the parentTable (though avoids the infinite loop).

Is there a third option which both addresses the concern you mention here whilst still addresses the problem at a more fundamental level as you suggested before?
Comment 16 chris fleizach 2013-01-16 11:41:28 PST
maybe we could check the parent table in initialization?
Comment 17 Joanmarie Diggs 2013-01-16 11:51:06 PST
(In reply to comment #16)
> maybe we could check the parent table in initialization?

You know of any code I can look at to see this being done?

Also what happens if a web app changes the tree? Does that matter?
Comment 18 chris fleizach 2013-01-16 12:20:07 PST
(In reply to comment #17)
> (In reply to comment #16)
> > maybe we could check the parent table in initialization?
> 
> You know of any code I can look at to see this being done?
> 
> Also what happens if a web app changes the tree? Does that matter?

You might be able to just override

AccessibilityRole AccessibilityRenderObject::determineAccessibilityRole()

in AccessibilityTableRow (I bet this problem happens to regular tables too, not just ARIA tables)

and put the roleValue() logic in there. 

if the web app changes the role of the object on the fly, i think we clear the object and it is remade a new
Comment 19 Joanmarie Diggs 2013-01-16 15:14:31 PST
Created attachment 183040 [details]
Patch
Comment 20 Joanmarie Diggs 2013-01-16 15:23:14 PST
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > maybe we could check the parent table in initialization?
> > 
> > You know of any code I can look at to see this being done?
> > 
> > Also what happens if a web app changes the tree? Does that matter?
> 
> You might be able to just override
> 
> AccessibilityRole AccessibilityRenderObject::determineAccessibilityRole()

Awesome. Thank you!! Works on Gtk. Let's see what the EWS thinks.
Comment 21 chris fleizach 2013-01-16 15:25:07 PST
(In reply to comment #20)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > (In reply to comment #16)
> > > > maybe we could check the parent table in initialization?
> > > 
> > > You know of any code I can look at to see this being done?
> > > 
> > > Also what happens if a web app changes the tree? Does that matter?
> > 
> > You might be able to just override
> > 
> > AccessibilityRole AccessibilityRenderObject::determineAccessibilityRole()
> 
> Awesome. Thank you!! Works on Gtk. Let's see what the EWS thinks.

I like this version
Comment 22 Build Bot 2013-01-16 17:50:44 PST
Comment on attachment 183040 [details]
Patch

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

New failing tests:
platform/mac/accessibility/search-when-element-starts-in-table.html
Comment 23 Build Bot 2013-01-17 04:01:53 PST
Comment on attachment 183040 [details]
Patch

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

New failing tests:
platform/mac/accessibility/search-when-element-starts-in-table.html
Comment 24 Joanmarie Diggs 2013-01-17 05:31:59 PST
(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > (In reply to comment #17)
> > > > (In reply to comment #16)
> > > > > maybe we could check the parent table in initialization?
> > > > 
> > > > You know of any code I can look at to see this being done?
> > > > 
> > > > Also what happens if a web app changes the tree? Does that matter?
> > > 
> > > You might be able to just override
> > > 
> > > AccessibilityRole AccessibilityRenderObject::determineAccessibilityRole()
> > 
> > Awesome. Thank you!! Works on Gtk. Let's see what the EWS thinks.
> 
> I like this version

One of the mac tests didn't. Any idea why?
Comment 25 Joanmarie Diggs 2013-01-17 12:34:57 PST
(In reply to comment #24)
 
> > I like this version
> 
> One of the mac tests didn't. Any idea why?

Having looked at the test that failed, we examine the cell and the row, but not the table. So I'm guessing an accessible object hasn't been created for it. Which brings me to the following:

The new AccessibilityTableRow::determineAccessibilityRole() does this:

    if (!isTableRow())
        return AccessibilityRenderObject::determineAccessibilityRole();

isTableRow() is true only if parentTable() returns an AccessibilityTable.

parentTable() has this code in it:

    // Do not use getOrCreate. parentTable() can be called while the render tree is being modified.
    return axObjectCache()->get(toRenderTableRow(m_renderer)->table());

And get() ain't returning a table.

As an experiment, I ignored the comment and called getOrCreate() and the failing test now passes. Assuming the comment is not lying, time to find a clever solution....
Comment 26 chris fleizach 2013-01-17 13:01:18 PST
(In reply to comment #25)
> (In reply to comment #24)
> 
> > > I like this version
> > 
> > One of the mac tests didn't. Any idea why?
> 
> Having looked at the test that failed, we examine the cell and the row, but not the table. So I'm guessing an accessible object hasn't been created for it. Which brings me to the following:
> 
> The new AccessibilityTableRow::determineAccessibilityRole() does this:
> 
>     if (!isTableRow())
>         return AccessibilityRenderObject::determineAccessibilityRole();
> 
> isTableRow() is true only if parentTable() returns an AccessibilityTable.
> 
> parentTable() has this code in it:
> 
>     // Do not use getOrCreate. parentTable() can be called while the render tree is being modified.
>     return axObjectCache()->get(toRenderTableRow(m_renderer)->table());
> 
> And get() ain't returning a table.
> 
> As an experiment, I ignored the comment and called getOrCreate() and the failing test now passes. Assuming the comment is not lying, time to find a clever solution....

You could make a parentTable() method that gets and one that getsOrCreates(), then use the later in the determineRole field. 

The only question is whether there's a problem creating a parent object from within a child object
Comment 27 Joanmarie Diggs 2013-01-17 13:35:52 PST
> You could make a parentTable() method that gets and one that getsOrCreates(), then use the later in the determineRole field. 
> 
> The only question is whether there's a problem creating a parent object from within a child object

I may have an even better idea. It works on Gtk+, it works on my Mac and involves removing code rather than adding it. :) Patch coming soon.

Remember the original problem was that for ARIA we had an endless loop because of the call to parentObjectUnignored(). And that problem was solved thanks to your suggestion of moving that logic into determineAccessibilityRole(). Given any accessible table row, the unignored parent should be an accessible table. And that parent should not be far away element-wize. So why not use that approach for all table rows?
Comment 28 Joanmarie Diggs 2013-01-17 13:43:57 PST
Created attachment 183264 [details]
Patch
Comment 29 Joanmarie Diggs 2013-01-17 14:17:47 PST
Comment on attachment 183264 [details]
Patch

To review my own patch, I just moved this bit of code and parent should be non-null. But my paranoia wants to add a null-check just in case. So I think I'll do that -- after code review on the rest.
Comment 30 chris fleizach 2013-01-17 14:19:59 PST
Comment on attachment 183264 [details]
Patch

i think this is good, but something happened with the bots. we should try to get the bots to process this just in case
Comment 31 Joanmarie Diggs 2013-01-17 14:45:38 PST
Created attachment 183285 [details]
Patch
Comment 32 Joanmarie Diggs 2013-01-17 14:47:25 PST
(In reply to comment #30)
> (From update of attachment 183264 [details])
> i think this is good, but something happened with the bots. we should try to get the bots to process this just in case

Agreed. Martin Robinson suggested I resubmit the patch and since I wanted to add the null check, I did that as well. So if you're happy and EWS is happy, then I am happy with what I just attached.
Comment 33 Joanmarie Diggs 2013-01-17 19:00:18 PST
Comment on attachment 183285 [details]
Patch

All bots are green. The Gtk bot has non-patch related issues at the moment. Given that this bug is causing crashes (not just in tests, not just with people using ATs) and is in the current unstable release of WebKitGtk, if there are no other issues it would be great to get this resolved.

Thanks again Chris for all your help on this! :)
Comment 34 WebKit Review Bot 2013-01-17 19:48:04 PST
Comment on attachment 183285 [details]
Patch

Clearing flags on attachment: 183285

Committed r140095: <http://trac.webkit.org/changeset/140095>
Comment 35 WebKit Review Bot 2013-01-17 19:48:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 36 Sergio Villar Senin 2013-01-18 03:54:00 PST
Unfortunately I keep experiencing a11y related crashes quite frequently:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff510a14b in WebCore::AccessibilityTableCell::parentTable() const () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
(gdb) bt
#0  0x00007ffff510a14b in WebCore::AccessibilityTableCell::parentTable() const () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#1  0x00007ffff510a09d in WebCore::AccessibilityTableCell::isTableCell() const () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#2  0x00007ffff510a0dd in WebCore::AccessibilityTableCell::roleValue() const () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#3  0x00007ffff5f8d106 in webkitAccessibleDetach () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#4  0x00007ffff510de3c in WebCore::AXObjectCache::remove(unsigned int) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#5  0x00007ffff510e05e in WebCore::AXObjectCache::remove(WebCore::RenderObject*) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#6  0x00007ffff58bb4ac in WebCore::RenderObject::willBeDestroyed() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#7  0x00007ffff57dbd5d in WebCore::RenderBlock::willBeDestroyed() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#9  0x00007ffff58b1b04 in WebCore::RenderObjectChildList::destroyLeftoverChildren() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#10 0x00007ffff58bb1fa in WebCore::RenderObject::willBeDestroyed() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#11 0x00007ffff58b9e5d in WebCore::RenderObject::destroy() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#12 0x00007ffff58b1b04 in WebCore::RenderObjectChildList::destroyLeftoverChildren() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#13 0x00007ffff58bb1fa in WebCore::RenderObject::willBeDestroyed() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#14 0x00007ffff58b9e5d in WebCore::RenderObject::destroy() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#15 0x00007ffff58b1b04 in WebCore::RenderObjectChildList::destroyLeftoverChildren() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#16 0x00007ffff57dbc7c in WebCore::RenderBlock::willBeDestroyed() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#17 0x00007ffff58b9e5d in WebCore::RenderObject::destroy() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#18 0x00007ffff58b1b04 in WebCore::RenderObjectChildList::destroyLeftoverChildren() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#19 0x00007ffff57dbc7c in WebCore::RenderBlock::willBeDestroyed() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#20 0x00007ffff58b9e5d in WebCore::RenderObject::destroy() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#21 0x00007ffff58b1b04 in WebCore::RenderObjectChildList::destroyLeftoverChildren() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#22 0x00007ffff57dbc7c in WebCore::RenderBlock::willBeDestroyed() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#23 0x00007ffff58b9e5d in WebCore::RenderObject::destroy() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#24 0x00007ffff5346ac2 in WebCore::Node::detach() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#25 0x00007ffff52d6b6e in WebCore::ContainerNode::detach() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#26 0x00007ffff5322734 in WebCore::Element::detach() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#27 0x00007ffff53232cc in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#28 0x00007ffff5322fe5 in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#29 0x00007ffff5322fe5 in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#30 0x00007ffff5322fe5 in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#31 0x00007ffff5322fe5 in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#32 0x00007ffff5322fe5 in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#33 0x00007ffff5322fe5 in WebCore::Element::recalcStyle(WebCore::Node::StyleChange) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#34 0x00007ffff52f72db in WebCore::Document::recalcStyle(WebCore::Node::StyleChange) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#35 0x00007ffff52f76ce in WebCore::Document::updateStyleIfNeeded() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#36 0x00007ffff52f8212 in WebCore::Document::updateLayout() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#37 0x00007ffff52fa939 in WebCore::Document::updateLayoutIgnorePendingStylesheets() () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#38 0x00007ffff572590b in WebCore::DOMWindow::scrollTo(int, int) const () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
#39 0x00007ffff5af5848 in WebCore::jsDOMWindowPrototypeFunctionScrollTo(JSC::ExecState*) () from ~/opt/gnome3/lib64/libwebkitgtk-3.0.so.0
Comment 37 Joanmarie Diggs 2013-01-18 04:28:01 PST
Different bug.