377783 - Crash [@ table][@ nsQueryInterface::operator()] with accessibility and moving stuff

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

The testcase uses enhanced privileges, so you need to download it to your computer to get the crash.
Usually, it crashes for me after a few reloads.
It doesn't seem to crash on branch.

Talkback ID: TB31278004H
table

Talkback ID: TB31277659M
0x02706448
nsQueryInterface::operator()  [mozilla/xpcom/build/nscomptr.cpp, line 52]
nsCOMPtr<nsISupports>::operator=  [mozilla/dist/include/xpcom/nscomptr.h, line 1056]
XPCConvert::NativeInterface2JSObject  [mozilla/js/src/xpconnect/src/xpcconvert.cpp, line 1104]
XPCConvert::NativeData2JS  [mozilla/js/src/xpconnect/src/xpcconvert.cpp, line 480]
XPCWrappedNative::CallMethod  [mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2337]

Comment 1

[Aaron Leventhal]

I don't think we should file any more bugs on the Javascript-only testcases for accessibility until we fix bug 357583.

Comment 2

[Martijn Wargers (dead)]

So I guess this depends on a fix for bug 357583 then?

Comment 3

[Aaron Leventhal]

Okay, bug 357583 is checked in -- let's get back to these JS testcases! :)

Comment 4

[Martijn Wargers (dead)]

I'm still getting this crash with a 2007-05-06 build, which is after bug 357583 was fixed.

Comment 5

[Martijn Wargers (dead)]

Attachment: testcase

Aaron changed something in bug 268935 that makes the original testcase not work
anymore with current trunk builds.
This is the updated testcase, using nsIAccessibleRetrieval.

Btw, this type of crash I often get in my testing. But I'm not sure if it's related to this one, or something different.

Comment 6

[Mats Palmgren (inactive)]

FWIW, I can't reproduce this on Linux.

Comment 7

[Jesse Ruderman]

Aaron, this looks exploitable based on the stack in comment 0, so I hope you'll be able to fix it soon :)

Comment 8

[Aaron Leventhal]

Yikes, this testcase is difficult to understand.

It refers to a textarea element that is not even there.

Comment 9

[Martijn Wargers (dead)]

Maybe the testcase from bug 381010 is easier to understand?

Comment 10

[Aaron Leventhal]

If I completely remove nsHTMLTableAccessible::CacheChildren() the bug goes away. Martijn, can you verify that?

Comment 11

[Martijn Wargers (dead)]

Sorry, I don't have a build with accessibility enabled (mingw), so I can't verify.

Comment 12

[Aaron Leventhal]

Well, I wonder if this broke on 3/28 when the fix for bug 360184 went in.

Comment 13

[Martijn Wargers (dead)]

I don't crash with a 2007-03-25 build, but I do crash with a 2007-03-27 build.
Cause by bug 360184 (but that went into the branch) or bug 371594?

Comment 14

[Aaron Leventhal]

I don't think it's bug 371594.

Comment 15

[Aaron Leventhal]

I think it has to do with our code to try and deal with captions in a table. It assumes the caption accessible is a direct child of the table, but actually it can be anywhere.

That could happen on Windows more where we create an accessible object for thead, tbody and tfoot.

Comment 6 verifies that this bug doesn't occur on Linux.

It's a theory, anyway :)

Comment 16

[Aaron Leventhal]

Attachment: Same affect is fix for bug 360184 but simpler and doesn't cause crash

The inclusion of tbody/thead/tfoot accessibles doesn't affect this as I thought, so I'm not sure why the bug doesn't occur on Linux for Mats.

Comment 18

[Evan Yan]

Comment on attachment 270325 [details] [diff] [review]
Same affect is fix for bug 360184 but simpler and doesn't cause crash

when a html table has multiple <caption>, this patch doesn't work, because nsAccessible::CacheChildren() will create multiple captionAccessible.

Comment 19

[Aaron Leventhal]

Evan, what needs to happen when there are multiple captions? Do we need to remove the extras or use them for something?

Do you want to take the bug? It's happening in your code somehow.

Comment 20

[Martijn Wargers (dead)]

Maybe bug 243159, comment 20 is useful? It's talking about multiple table captions in layout code.

Comment 21

[Evan Yan]

(In reply to comment #19)
> Evan, what needs to happen when there are multiple captions? Do we need to
> remove the extras or use them for something?
> 
I think removing the extras is the right thing to do. However we can't get the extra captions from GetCaption(), that's why I added the condition in traversing DOM tree.

> Do you want to take the bug? It's happening in your code somehow.
> 
I'd like to but I don't have a windows environment.
Which part of the former code caused the crash?
I think it doesn't assume the caption accessible need to be a direct child of the table.

Comment 22

[Aaron Leventhal]

I don't know which part caused the crash to be honest, but when I removed or reimplemented the code it fixed the bug.

Comment 23

[Evan Yan]

Attachment: just a try, to move caption handling code after

does this help?

Comment 24

[Aaron Leventhal]

Comment on attachment 270585 [details] [diff] [review]
just a try, to move caption handling code after

No, the bug still occurs.

Comment 25

[Aaron Leventhal]

Attachment: Deal with multiple captions and other cleanup, see comment

Evan, sorry for the large patch. This is based on my earlier fix but I found more things to clean up, mostly related to the use of captions, after your comment about multiple captions. There was also the removal of SetCaption/SetSummary (see item #4).

1) Base the creation of caption accessibles on the HTML frame (created via display: table-caption)
2) Add nsHTMLCaptionAccessible class, which supports description_for relation (it describes the table)
3) Do not create the accessibles for non-visible captions
4) Remove the ability to set caption or summary from nsIAccessibleTable -- the API should not mutate the HTML document!
5) Reuse nsAccessible::CacheChildren() and move caption accessible to first child
6) Simplify GetCaption() to just use first child and check if it's a caption
7) Support description relation
8) Support summary=name, caption=description

Comment 26

[Evan Yan]

Comment on attachment 270901 [details] [diff] [review]
Deal with multiple captions and other cleanup, see comment

firefox crash at start, core stack:

#0  0x00002b5b34d3e1d1 in nanosleep () from /lib/libc.so.6
#1  0x00002b5b34d3dff4 in sleep () from /lib/libc.so.6
#2  0x00002b5b2edaea67 in ah_crap_handler (signum=11) at nsSigHandlers.cpp:135
#3  0x00002b5b2edc3aed in nsProfileLock::FatalSignalHandler (signo=11) at nsProfileLock.cpp:210
#4  <signal handler called>
#5  0x00002b5b33ddebbc in g_type_check_instance_cast () from /usr/lib/libgobject-2.0.so.0
#6  0x00002b5b2fc1e0bf in nsAccessibleWrap::GetAtkObject (acc=0x2aaab00ae310)
    at ../../../../mozilla/accessible/src/atk/nsAccessibleWrap.cpp:407
#7  0x00002b5b2fc24861 in nsApplicationAccessibleWrap::RemoveRootAccessible (this=0x11b70f0, aRootAccWrap=0x2aaab00ae310)
    at ../../../../mozilla/accessible/src/atk/nsAppRootAccessible.cpp:627
#8  0x00002b5b2fbdf496 in nsAccessibilityService::RemoveNativeRootAccessible (this=0x11db310, aRootAccessible=0x2aaab00ae310)
    at ../../../../mozilla/accessible/src/base/nsAccessibilityService.cpp:1752
#9  0x00002b5b2f08a589 in nsFrameManager::ReResolveStyleContext (this=0x2aaab00ae348, aPresContext=0x2aaab00a00c0, 
    aFrame=0x16d8160, aParentContent=0x16017e0, aChangeList=0x7fff7c5831e0, aMinChange=7)
    at ../../../mozilla/layout/base/nsFrameManager.cpp:1386


>Index: accessible/public/nsIAccessibleTable.idl
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/public/nsIAccessibleTable.idl,v

>-           attribute nsIAccessible       caption;
>-           attribute AString           summary;
>+  readonly attribute nsIAccessible       caption;
>+  readonly attribute AString             summary;

these change also affects xul tree table, please modify them also.

Comment 27

[Aaron Leventhal]

The startup crash is because your vtbl for a11y service is now out of whack. You need to rebuild anywhere that uses a11yservice.

Comment 28

[Aaron Leventhal]

Attachment: Remove summary/caption support for XUL trees

Comment 29

[Evan Yan]

Comment on attachment 271042 [details] [diff] [review]
Remove summary/caption support for XUL trees

r=me

Comment 30

[Aaron Leventhal]

Comment on attachment 271042 [details] [diff] [review]
Remove summary/caption support for XUL trees

Technically we need a layout peer to review the small changes to layout, even though they're
#ifdef ACCESSIBILITY

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 271042 [details] [diff] [review]
Remove summary/caption support for XUL trees

>Index: layout/tables/nsTableOuterFrame.cpp

Please remove the #include "nsIAccessibilityService.h" as well.

r+sr=mats for the layout changes

Comment 32

[Aaron Leventhal]

We still need that #include there, we're adding an addition user of the service in nsTableOuterFrame.

Or am I misunderstanding you?

Comment 33

[Mats Palmgren (inactive)]

Nevermind my previous comment, I made a mistake, the patch looks fine as is.

Comment 34

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007070604 Minefield/3.0a7pre

Thanks for fixing!