Bug 107261 - Crash in AccessibilityTableCell::parentTable()
Summary: Crash in AccessibilityTableCell::parentTable()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Major
Assignee: Joanmarie Diggs (irc: joanie)
URL:
Keywords:
: 106431 (view as bug list)
Depends on:
Blocks: 106903
  Show dependency treegraph
 
Reported: 2013-01-18 04:53 PST by Sergio Villar Senin
Modified: 2013-01-21 08:59 PST (History)
11 users (show)

See Also:


Attachments
Patch (1.70 KB, patch)
2013-01-18 04:55 PST, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch (3.20 KB, patch)
2013-01-18 08:05 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Patch (3.83 KB, patch)
2013-01-18 10:03 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Same fix; now with layout test. (6.45 KB, patch)
2013-01-18 10:59 PST, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2013-01-18 04:53:33 PST
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
Comment 1 Sergio Villar Senin 2013-01-18 04:55:58 PST
Created attachment 183426 [details]
Patch
Comment 2 Sergio Villar Senin 2013-01-18 05:27:39 PST
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.
Comment 3 chris fleizach 2013-01-18 06:24:54 PST
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
Comment 4 Joanmarie Diggs (irc: joanie) 2013-01-18 06:30:40 PST
(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.
Comment 5 Joanmarie Diggs (irc: joanie) 2013-01-18 08:05:06 PST
Created attachment 183461 [details]
Patch
Comment 6 Joanmarie Diggs (irc: joanie) 2013-01-18 08:05:59 PST
It's taking longer than I thought to create this layout test. :( So tossing the proposed patch at EWS in the meantime.
Comment 7 chris fleizach 2013-01-18 08:09:22 PST
I think the concept looks good.
Comment 8 Joanmarie Diggs (irc: joanie) 2013-01-18 09:33:40 PST
(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.
Comment 9 Build Bot 2013-01-18 09:41:56 PST
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
Comment 10 Joanmarie Diggs (irc: joanie) 2013-01-18 10:03:52 PST
Created attachment 183493 [details]
Patch
Comment 11 Joanmarie Diggs (irc: joanie) 2013-01-18 10:09:07 PST
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?
Comment 12 chris fleizach 2013-01-18 10:13:36 PST
(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
Comment 13 Joanmarie Diggs (irc: joanie) 2013-01-18 10:16:17 PST
(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. :(
Comment 14 Joanmarie Diggs (irc: joanie) 2013-01-18 10:39:04 PST
(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!!
Comment 15 Joanmarie Diggs (irc: joanie) 2013-01-18 10:59:21 PST
Created attachment 183509 [details]
Same fix; now with layout test.
Comment 16 Joanmarie Diggs (irc: joanie) 2013-01-18 17:54:51 PST
(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.
Comment 17 Sergio Villar Senin 2013-01-20 03:37:47 PST
*** Bug 106431 has been marked as a duplicate of this bug. ***
Comment 18 chris fleizach 2013-01-21 08:54:31 PST
Comment on attachment 183509 [details]
Same fix; now with layout test.

r=me
Comment 19 WebKit Review Bot 2013-01-21 08:59:07 PST
Comment on attachment 183509 [details]
Same fix; now with layout test.

Clearing flags on attachment: 183509

Committed r140340: <http://trac.webkit.org/changeset/140340>
Comment 20 WebKit Review Bot 2013-01-21 08:59:12 PST
All reviewed patches have been landed.  Closing bug.