424073 - Crash @ nsAccessible::GetTextFromRelationID(nsIAtom*, nsString)

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Marco Zehe (:MarcoZ)]

Crash reports are here: http://crash-stats.mozilla.com/report/list?range_unit=days&query_search=signature&query_type=contains&product=Firefox&platform=windows%2Cmac%2Clinux&version=Firefox%3A3.0b5pre&branch=1.9&signature=nsAccessible%3A%3AGetTextFromRelationID%28nsIAtom%2A%2C+nsString%26%29&query=Access&range_value=5.

I suspect we're not protecting against someone specifying an invalid id in an aria-labelledby attribute value.

Comment 1

[Marco Zehe (:MarcoZ)]

Attachment: Add NULL check

Protect against the ID being invalid and the labelElement being NULL.

Comment 2

[alexander :surkov (:asurkov)]

do_QueryInterface is safe to nsnull. Does this patch fix the problem?

Comment 3

[alexander :surkov (:asurkov)]

really I haven't idea what's wrong here, it's very strange stack

Comment 4

[Aaron Leventhal]

Comment on attachment 310729 [details] [diff] [review]
Add NULL check

It wouldn't make any sense if this patched changed anything.

Comment 5

[Aaron Leventhal]

Comment on attachment 310729 [details] [diff] [review]
Add NULL check

I don't think the crash is related to ARIA relations as the newsgroup message said. I think the crash is in our code to calculate positional descriptions for tree items, list items, menu items, radios etc.

I see get_accDescription() calling GetAccGroupAttrs(). It looks like the line numbers for the crash are off since GetAccGroupAttrs() is called on line 370 of nsAccessibleWrap.cpp  but the crash report is says that is on line 433.

So I don't think the final crash actually is in GetTextFromRelationID() necessarily but definitely get_accDescription()/GetAccGroupAttrs() seems like solid connection.

Comment 6

[Aaron Leventhal]

Actually I think it's that we return NULL with S_FALSE for accDescription since  bug 421650. This even matches the time frame for the crashes, which started a day after that checkin took effect.

Those S_FALSE with NULL burnt us before with get_title() as well. They're dangerous.

Comment 7

[Aaron Leventhal]

Attachment: Revert the two lines from bug 421650 that are an optional change and are probably causing the crash

Comment 8

[Aaron Leventhal]

We should really try to get approval 1.9b5 for this. It will cause huge numbers of crashes in nightly builds for JAWS and Window-Eyes users. There are often empty descriptions.

Comment 9

[James Teh [:Jamie]]

I am seeing these crashes frequently with both NVDA and JAWS 9. Interestingly enough, Mick doesn't seem to be seeing them.

One with JAWS 9:
0af70951-f873-11dc-892c-001a4bd43ef6
One with NVDA:
7c5f0da1-f7e1-11dc-adde-001a4bd43ef6
I haven't seen these crashes in the most recent build, but I always found them a little tricky to reproduce reliably.

Comment 10

[Aaron Leventhal]

Jamie, would NVDA potentially crash if any IAccessible methods return NULL for a string, with an HRESULT of S_FALSE? For example in accValue/accDescription/accName etc. etc.

Comment 11

[Aaron Leventhal]

Attachment: Alternative patch that catches more: don't use S_FALSE, and if no error always return a string even if it is empty.

Comment 12

[Aaron Leventhal]

Jamie, will you see if these special try server builds make the crash go away for you? https://build.mozilla.org/tryserver-builds/2008-03-23_19:[e-mail]-no-sfalse-strings/
It removes all cases where we return NULL for a string out parameter with S_FALSE in the HRESULT.

Comment 13

[alexander :surkov (:asurkov)]

With the patch we will be inconsistent with MSAA/IA2 spec. Do we really want this? If we want then we should discuss this with Pete to change IA2 spec.

Comment 14

[Aaron Leventhal]

We'll have to discuss with Pete, but not crashing is more important than that aspect of the spec. We need to find out if this is the cause, but so far it's the best fit.

Comment 15

[Aaron Leventhal]

Hmm, I'm getting the crash on my machine, which is good. It's happening in builds where I have this patch if I just run Inspect32 and accevent watcher.

Comment 16

[Aaron Leventhal]

Attachment: The real fix -- found it by catching exceptions on my machine

Happens when the AT tries to get a description and the doc is still loading -- there is no body yet.

Comment 17

[Marco Zehe (:MarcoZ)]

This will also fix the second part of bug 424097.

Comment 18

[Michael Curran]

(In reply to comment #10)
> Jamie, would NVDA potentially crash if any IAccessible methods return NULL for
> a string, with an HRESULT of S_FALSE? For example in
> accValue/accDescription/accName etc. etc.
NVDA should cope with this fine. In fact even if it was found not to, I would be happy to make sure it did, since the IA2 spec certainly says that this should happen.
Certainly in the virtualBuffer library (in-process) I can say that we only bother trying to use a bstr if the result was S_OK, and its not NULL.
Out-of-process in Python we do not get notified of the hResult, but Python does handle NULL ok.

Comment 19

[Marco Zehe (:MarcoZ)]

Comment on attachment 311327 [details] [diff] [review]
The real fix -- found it by catching exceptions on my machine

This is the cause for over 30 crashes over the last 3 days alone. Strongly agreeing with Aaron that we take this for B5.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311327 [details] [diff] [review]
The real fix -- found it by catching exceptions on my machine

a1.9b5=beltzner