Closed Bug 583952 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Tracking Status
blocking1.9.2 --- -
status1.9.2 --- .11-fixed

People

(Reporter: Jamie, Assigned: davidb)

References

()

Details

(Keywords: access, crash, verified1.9.2)

Crash Data

Attachments

(2 files, 3 obsolete files)

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.
Keywords: access, crash
Version: unspecified → Trunk
Also happens in Firefox 3.6.x.

Crash ID: bp-9bfc5ca2-46c5-4364-9f10-30e6c2100802
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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?
Attached patch patch (exporatory) (obsolete) — Splinter Review
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?
Assignee: nobody → bolterbugz
Status: NEW → ASSIGNED
Attachment #462765 - Flags: feedback?(marco.zehe)
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.
Attachment #462765 - Flags: feedback?(marco.zehe) → feedback+
Note that combined with the fix for bug 576838, we might be okay here.
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?
(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.
(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/dbolter@mozilla.com-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?
Yeah it failed for some reason. I'll try again.
Third time lucky

Index of /pub/mozilla.org/firefox/tryserver-builds/dbolter@mozilla.com-623ebb56d0ad
Attachment #462765 - Attachment is obsolete: true
Attachment #464063 - Flags: review?(marco.zehe)
Attachment #464063 - Flags: review?(marco.zehe) → review+
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.
Attachment #464063 - Flags: approval2.0?
Attachment #464063 - Flags: approval2.0? → approval2.0+
Do you still need me to test for the freeze or is this fixed now?
(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/dbolter@mozilla.com-4ce59fa350ea/
(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/dbolter@mozilla.com-4ce59fa350ea/
Heh, no win32 build yet, only win32-debug.
http://hg.mozilla.org/mozilla-central/rev/24e5ac894021
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attached patch new fix (skip non cells) (obsolete) — Splinter Review
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.
Attachment #466191 - Flags: review?(surkov.alexander)
Jamie, we also need to figure out why you get the crash and I don't (nor Marco?).
(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 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.
Attachment #466191 - Flags: review?(surkov.alexander) → review+
It crashes in debug build as well. When page is loaded then move focus there (for example double tab while search field is focused).
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
Attached patch new fix 2Splinter Review
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/dbolter@mozilla.com-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.
Attachment #464063 - Attachment is obsolete: true
Attachment #466191 - Attachment is obsolete: true
Attachment #467451 - Flags: review?(marco.zehe)
(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 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
Attachment #467451 - Flags: review?(marco.zehe) → review+
David, what's the status of the bug? We need to get this fix asap. Were you sucessfull to extract mochitest from testcase?
Flags: in-testsuite?
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.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Note: minimal test case posted to bug 587529.
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.
Status: RESOLVED → VERIFIED
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.
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.
blocking1.9.2: --- → ?
The last patch should be sufficient to stop the bleeding on 1.9.2. Does it apply cleanly?
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.
Attachment #469902 - Flags: approval1.9.2.10?
What about 3.5.x? Does it crash there? Do we just not care anymore?
Summary: Crash [@ nsAccUtils::GetHeaderCellsFor(nsIAccessibleTable*, nsIAccessibleTableCell*, int, nsIArray**) ] → Crash [@ nsAccUtils::GetHeaderCellsFor(nsIAccessibleTable*, nsIAccessibleTableCell*, int, nsIArray**) ] when using NVDA
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
Attachment #469902 - Flags: approval1.9.2.10? → approval1.9.2.10+
blocking1.9.2: ? → -
(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.
Landed on the 1.9.2 default branch on David's behalf: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/6dc5ac432cd9
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.
Jamie, could you please file new bug for that?
(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.
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.
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
Thanks!
Keywords: verified1.9.2
Crash Signature: [@ nsAccUtils::GetHeaderCellsFor(nsIAccessibleTable*, nsIAccessibleTableCell*, int, nsIArray**) ]
You need to log in before you can comment on or make changes to this bug.