Bug 107261 – Crash in AccessibilityTableCell::parentTable()

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Sergio Villar Senin]

It's quite easy to get a crash when performing browsing actions on pages including tables. I always get one when doing things like:

1- go to www.amazon.com
2- input some text in the search box and click on "Go"
3- sroll down to the end of the list and click on "Next Page"

you'd get a crash like this one (release webkitgtk build):

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

[Sergio Villar Senin]

Created attachment 183426 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 183426 [details]
Patch

Actually I'm no longer setting it for review, because it looked like an ad hoc fix. The crashes are still there in some other cases even if the patch is applied.

[chris fleizach]

I think the isTableCell call can rely solely on roleValue() and we could
Do the same thing we just did for table rows, which is determine the role up front and cache it

[Joanmarie Diggs]

(In reply to comment #3)
> I think the isTableCell call can rely solely on roleValue() and we could
> Do the same thing we just did for table rows, which is determine the role up front and cache it

Cool. I'll give that a try. Thanks!

I'm also working on creating a layout test for this bug.

[Joanmarie Diggs]

Created attachment 183461 [details]
Patch

[Joanmarie Diggs]

It's taking longer than I thought to create this layout test. :( So tossing the proposed patch at EWS in the meantime.

[chris fleizach]

I think the concept looks good.

[Joanmarie Diggs]

(In reply to comment #7)
> I think the concept looks good.

And while the EWS takes its time and spits up, I ran the mac tests. Looks like we have a similar issue with search-when-element-starts-in-table.html.

I'll fix that and submit a new patch.

[Build Bot]

Comment on attachment 183461 [details]
Patch

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

New failing tests:
platform/mac/accessibility/search-when-element-starts-in-table.html

[Joanmarie Diggs]

Created attachment 183493 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 183493 [details]
Patch

Chris, this addresses the regression on the mac.

I'm having problems coming up with a reliable crasher Layout Test. Given that we have a bunch of table tests to ensure this doesn't introduce a regression, and given that non-AT users have been complaining quite a bit about this one.... How essential is it that I create a new layout test in order to get this fix committed?

[chris fleizach]

(In reply to comment #11)
> (From update of attachment 183493 [details])
> Chris, this addresses the regression on the mac.
> 
> I'm having problems coming up with a reliable crasher Layout Test. Given that we have a bunch of table tests to ensure this doesn't introduce a regression, and given that non-AT users have been complaining quite a bit about this one.... How essential is it that I create a new layout test in order to get this fix committed?

looking at the BT, it looks like if you listen for a scroll to event, then delete an entire table, after having accessed the whole table through accessibility, it should occur

it seems like as soon as webkitAccessibleDetach calls roleValue() on a tableCell who's parent is already gone, you should hit it.

perhaps having a reference to the table cell and then deleting the parent...

it's also possible this one fixed it? or have you been reproducing on a build after this?

http://trac.webkit.org/changeset/139444

[Joanmarie Diggs]

(In reply to comment #12)

> it's also possible this one fixed it? or have you been reproducing on a build after this?
> 
> http://trac.webkit.org/changeset/139444

I can reproduce with git master. :(

[Joanmarie Diggs]

(In reply to comment #12)
 
> looking at the BT, it looks like if you listen for a scroll to event, then delete an entire table, after having accessed the whole table through accessibility, it should occur

Brilliant! I don't even have to listen for (or even have) that event. Thanks!!

[Joanmarie Diggs]

Created attachment 183509 [details]
Same fix; now with layout test.

[Joanmarie Diggs]

(In reply to comment #7)
> I think the concept looks good.

How about the code and layout test? :) I'm not sure what's holding up the EFL bot but all the other EWS bots are green.

[Sergio Villar Senin]

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

[chris fleizach]

Comment on attachment 183509 [details]
Same fix; now with layout test.

r=me

[WebKit Review Bot]

Comment on attachment 183509 [details]
Same fix; now with layout test.

Clearing flags on attachment: 183509

Committed r140340: <http://trac.webkit.org/changeset/140340>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.