583952 - Crash [@ nsAccUtils::GetHeaderCellsFor(nsIAccessibleTable*, nsIAccessibleTableCell*, int, nsIArray**) ] when using NVDA

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

User-Agent:       Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100802 Minefield/4.0b3pre
Build Identifier: Mozilla/5.0 (Windows; Windows NT 6.1; WOW64; rv:2.0b3pre) Gecko/20100802 Minefield/4.0b3pre

Minefield crashes when opening the specified URL while using a recent snapshot of NVDA.

Reproducible: Always

Steps to Reproduce:
1. Using a current snapshot of NVDA and Minefield, open the  URL specified in the bug.
Actual Results:  
Crash.

Expected Results:  
No crash!

Crash ID: bp-10ab03c8-b3ba-4d4f-84ba-99d602100802

This only happens with recent snapshots of NVDA because we started supporting table headers. We don't query for table headers in NVDA 2010.1.

Comment 1

[James Teh [:Jamie]]

Also happens in Firefox 3.6.x.

Crash ID: bp-9bfc5ca2-46c5-4364-9f10-30e6c2100802

Comment 2

[Marco Zehe (:MarcoZ)]

It would appear that this call to do_QueryInterface for the retrieved accessible to get the AccessibleTable interface fails:
http://hg.mozilla.org/mozilla-central/annotate/a4d86c3a3494/accessible/src/base/nsAccUtils.cpp#l745

The question is: Why?

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #0)
> This only happens with recent snapshots of NVDA because we started supporting
> table headers. We don't query for table headers in NVDA 2010.1.

Where are the recent snapshots?

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: patch (exporatory)

OK Marco pointed me to the snapshots (thanks).

Try server is closed right now... so here's an exploratory patch. It looks like GetCellAt could potentially return null for the outparam while returning the NS_OK return value, which is not correct.

Marco can you test this one locally?

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 462765 [details] [diff] [review]
patch (exporatory)

This patch fixes the crash. However, loading this page then causes a freeze that can only be unlocked by killing either the Firefox or NVDA processes. Seems like there is something else going wrong that gets uncovered once this crash is fixed.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Note that combined with the fix for bug 576838, we might be okay here.

Comment 7

[James Teh [:Jamie]]

For reference, bug filed against NVDA by another user: http://www.nvda-project.org/ticket/807
I can't reproduce it with the URL provided there, but perhaps the site appears differently on non-Russian systems?

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Jamie, a windows try build should show up here eventually:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-bd53bd51defd/

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #6)
> Note that combined with the fix for bug 576838, we might be okay here.

Oh hmmm, I took a closer look at Alexander's fix there and these will conflict, but in any event, we should probably not return NS_OK when the cell is null in GetCellAt.

Comment 10

[James Teh [:Jamie]]

(In reply to comment #8)
> Thanks Jamie, a windows try build should show up here eventually:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-bd53bd51defd/
Unfortunately, the win32 build never showed up. There's win32-debug, but I can't run this due to not having the required debug CRT. I suspect Mozilla builds use an older SDK than the one I have installed here. Any chance of a win32 (non-debug) build?

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Yeah it failed for some reason. I'll try again.

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Oops got distracted. Just pushed, and the build should show up here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-cf0d9be9621b/

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Third time lucky

Index of /pub/mozilla.org/firefox/tryserver-builds/[e-mail]-623ebb56d0ad

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-623ebb56d0ad/tryserver-win32/

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: updated to work with bug 576838 patch

Comment 16

[Marco Zehe (:MarcoZ)]

Comment on attachment 464063 [details] [diff] [review]
updated to work with bug 576838 patch

Fixes a crasher with latest NVDA snapshots which use IAccessibleTable2. needs to also be backported to 3.6.x ASAP.

Comment 17

[James Teh [:Jamie]]

Do you still need me to test for the freeze or is this fixed now?

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #17)
> Do you still need me to test for the freeze or is this fixed now?

If you have time...

A try build combined with bug 576838 fix will be here:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4ce59fa350ea/

Comment 19

[James Teh [:Jamie]]

(In reply to comment #18)
> A try build combined with bug 576838 fix will be here:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-4ce59fa350ea/
Heh, no win32 build yet, only win32-debug.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

http://hg.mozilla.org/mozilla-central/rev/24e5ac894021

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Jamie, if you like you can wait for a nightly or:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-2662404a03ef/tryserver-win32/

Comment 22

[David Bolter [:davidb] (NeedInfo me for attention)]

Pasted wrong changeset. Actual was:
http://hg.mozilla.org/mozilla-central/rev/2250a9d71055

Comment 23

[James Teh [:Jamie]]

Still seeing exactly the same crash in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100813 Minefield/4.0b4pre. Should this fix have made it into this build?
Crash report: bp-da293cf6-94db-4a19-ab9d-4e2942100813

Comment 24

[James Teh [:Jamie]]

Still crashes for me in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b4pre) Gecko/20100815 Minefield/4.0b4pre
Crash report: bp-1f7c2f38-a1ee-47e4-a6e7-62d202100815

Comment 25

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Jamie. Strange, since we had confirmation of the fix from the earlier version of the patch. I'll work up a bail out patch but it isn't ideal. I filed bug 587529 for the deeper problem.

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: new fix (skip non cells)

Alexander, I filed bug 587529 for the deeper problem. This fix should skip over non accessible table cells when constructing the array of header cells. I'm not sure if it correct because I'm not sure what assumptions are made elsewhere in our code about that collection.

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

Jamie, we also need to figure out why you get the crash and I don't (nor Marco?).

Comment 28

[James Teh [:Jamie]]

(In reply to comment #27)
> Jamie, we also need to figure out why you get the crash and I don't (nor
> Marco?).
Have you tried with a release/non-debug build? I have only been using release builds here. I can't run the Mozilla built debug builds due to using a different version of the Windows SDK.

Comment 29

[alexander :surkov (:asurkov)]

Comment on attachment 466191 [details] [diff] [review]
new fix (skip non cells)


>     nsCOMPtr<nsIAccessibleTableCell> tableCellAcc =
>       do_QueryInterface(cell);
> 
>+    // XXX Bug 587529
>+    if (!tableCellAcc) {

nit: you shouldn't use braces here

>+      // Should never happen, since GetCellAt should always return an
>+      // nsIAccessibleTableCell (right?)

absolutely

>+      continue;

that's unexpected, so NS_ENSURE_STATE should work here because this patch doesn't fix root problem but fixes crash.

mochitest is necessary.

Comment 30

[alexander :surkov (:asurkov)]

It crashes in debug build as well. When page is loaded then move focus there (for example double tab while search field is focused).

Comment 31

[Marco Zehe (:MarcoZ)]

With the regular nightly of August 15, I also still see a crash in a spot I saw it before the patch:bp-65657d06-5f24-4a58-86e1-5796e2100815

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

Marco, can you please try this build?
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-141be4dcc667/tryserver-win32/

Comment 33

[Marco Zehe (:MarcoZ)]

It still crashes like this on one of my pages:
http://crash-stats.mozilla.com/report/index/b69c3bd9-a3c6-4da9-bf83-f625f2100819

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: new fix 2

Carrying over Alex's r+ from the previous patch.

I recreated this crash in a downloaded release. I still haven't recreated via my local builds (debug and opt) but do get the NVDA silence problem. This patch definitely fixes the silence problem. A try build is coming:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-e9e22e1263c2/

Alexander, I'm not able to create a mochitest for this. I think once we know the deeper problem we can add one to that spin off bug.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #34)

> Alexander, I'm not able to create a mochitest for this. I think once we know
> the deeper problem we can add one to that spin off bug.

That's a point, we need it to fix bug 587529 you referred to in your patch. The mochitest should be extracted from that site and it can be done since all of us are able to reproduce the crash.

Comment 36

[Marco Zehe (:MarcoZ)]

Comment on attachment 467451 [details] [diff] [review]
new fix 2

This build no longer crashes for me with a NVDA snapshot, and also the NVDA hang no longer occurrs. r=me

Comment 37

[alexander :surkov (:asurkov)]

David, what's the status of the bug? We need to get this fix asap. Were you sucessfull to extract mochitest from testcase?

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

Landed on central: http://hg.mozilla.org/mozilla-central/rev/092ed1804da6

I'll try to extract a test case and attach it to bug 587529, given time and priorities. I do realize the importance.

Comment 39

[David Bolter [:davidb] (NeedInfo me for attention)]

Note: minimal test case posted to bug 587529.

Comment 40

[James Teh [:Jamie]]

Verified fixed in: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b5pre) Gecko/20100825 Minefield/4.0b5pre

Thanks guys. This one was driving me (and many other users) insane.

Comment 41

[Marco Zehe (:MarcoZ)]

David, can you roll up a patch merged against 1.9.2 and post it to the bug so we can ask drivers to get it into 1.9.2.10? This will impact Firefox 3.6 users as soon as NVDA versions using IAccessibleTable2 will become more widely spread.

Comment 42

[Marco Zehe (:MarcoZ)]

Requesting that this block 1.9.2.10 because as soon as NVDA 2010.2alpha or beta is released within the next couple of weeks, a lot of 3.6.x users will encounter this crash. We should make sure to get this into the next Firefox update if humanly possible.

Comment 43

[David Bolter [:davidb] (NeedInfo me for attention)]

The last patch should be sufficient to stop the bleeding on 1.9.2. Does it apply cleanly?

Comment 44

[Marco Zehe (:MarcoZ)]

Attachment: v1.9.2 version of the latest patch

This is the 1.9.2 version of "New fix 2", attachment 467451 [details] [diff] [review]. Manual merging was needed because the line offset was so great, but otherwise the patch is identical, and I've verified that it fixes the crasher.

Comment 45

[Daniel Veditz [:dveditz]]

What about 3.5.x? Does it crash there? Do we just not care anymore?

Comment 46

[LegNeato]

Comment on attachment 469902 [details] [diff] [review]
v1.9.2 version of the latest patch

a=LegNeato for 1.9.2.10. Please only land this on the default branch of 1.9.2, *not* the relbranch

Comment 47

[Marco Zehe (:MarcoZ)]

(In reply to comment #45)
> What about 3.5.x? Does it crash there? Do we just not care anymore?

3.5.x didn't have this enhanced table feature yet and is unaffected.

Comment 48

[Marco Zehe (:MarcoZ)]

Landed on the 1.9.2 default branch on David's behalf: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6dc5ac432cd9

Comment 49

[James Teh [:Jamie]]

An interesting observation:
Table cells that exhibit this problem have a different role and do not have the table-cell-index attribute. However, you can QueryInterface to IAccessibleTableCell. Why can you QI to IAccessibleTableCell when the table-cell-index attribute is missing? It seems that part of Gecko knows that this is not a real table cell but another part does not.

Comment 50

[alexander :surkov (:asurkov)]

Jamie, could you please file new bug for that?

Comment 51

[James Teh [:Jamie]]

(In reply to comment #49)
> Table cells that exhibit this problem have a different role and do not have the
> table-cell-index attribute. However, you can QueryInterface to
> IAccessibleTableCell.
Sorry. False alarm. If table-cell-index isn't present, you also can't QI to IAccessibleTableCell. My assumption was incorrect. These table cells reproduce the issue, but it's actually retrieving headers on other cells which caused the crash. In short, there's no additional bug.

Comment 52

[Al Billings [:abillings - ex-MoCo]]

Jamie or Marco, can you verify that this is fixed in the nightly 1.9.2 build as well since we'll be shipping it in the next 3.6 update.

Comment 53

[James Teh [:Jamie]]

Verified fixed in: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.10pre) Gecko/20100909 Namoroka/3.6.10pre

Comment 54

[Al Billings [:abillings - ex-MoCo]]

Thanks!