Closed Bug 424073 Opened 12 years ago Closed 12 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: MarcoZ, Assigned: aaronlev)

References

Details

(Keywords: access, crash, Whiteboard: Just a null check -- extremely low risk & high impact)

Attachments

(1 file, 3 obsolete files)

Attached patch Add NULL check (obsolete) — Splinter Review
Protect against the ID being invalid and the labelElement being NULL.
Attachment #310729 - Flags: review?(surkov.alexander)
do_QueryInterface is safe to nsnull. Does this patch fix the problem?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Target Milestone: mozilla1.9beta5 → mozilla1.9
really I haven't idea what's wrong here, it's very strange stack
Comment on attachment 310729 [details] [diff] [review]
Add NULL check

It wouldn't make any sense if this patched changed anything.
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.
Attachment #310729 - Flags: review?(surkov.alexander) → review-
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.
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.
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.
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.
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:33-aaronleventhal@moonset.net-no-sfalse-strings/
It removes all cases where we return NULL for a string out parameter with S_FALSE in the HRESULT.
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.
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.
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.
Happens when the AT tries to get a description and the doc is still loading -- there is no body yet.
Assignee: marco.zehe → aaronleventhal
Attachment #311289 - Attachment is obsolete: true
Attachment #311313 - Attachment is obsolete: true
Attachment #311327 - Flags: review?(surkov.alexander)
Attachment #311289 - Flags: review?(surkov.alexander)
Attachment #311313 - Flags: review?(surkov.alexander)
Attachment #311327 - Flags: review?(surkov.alexander) → review+
This will also fix the second part of bug 424097.
Blocks: 424097
(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 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.
Attachment #311327 - Flags: approval1.9b5?
Attachment #311327 - Flags: approval1.9?
Whiteboard: Just a null check -- extremely low risk & high impact
Comment on attachment 311327 [details] [diff] [review]
The real fix -- found it by catching exceptions on my machine

a1.9b5=beltzner
Attachment #311327 - Flags: approval1.9b5?
Attachment #311327 - Flags: approval1.9b5+
Attachment #311327 - Flags: approval1.9?
Attachment #311327 - Flags: approval1.9+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.