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

RESOLVED FIXED in mozilla1.9

Status

()

Core
Disability Access APIs
P2
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: MarcoZ, Assigned: Aaron Leventhal)

Tracking

({access, crash})

Trunk
mozilla1.9
x86
Windows XP
access, crash
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: Just a null check -- extremely low risk & high impact)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Comment 1

10 years ago
Created attachment 310729 [details] [diff] [review]
Add NULL check

Protect against the ID being invalid and the labelElement being NULL.
Attachment #310729 - Flags: review?(surkov.alexander)

Comment 2

10 years ago
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

Comment 3

10 years ago
really I haven't idea what's wrong here, it's very strange stack
(Assignee)

Comment 4

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

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

Comment 5

10 years ago
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-
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
Created attachment 311289 [details] [diff] [review]
Revert the two lines from bug 421650 that are an optional change and are probably causing the crash
Attachment #311289 - Flags: review?(surkov.alexander)
(Assignee)

Comment 8

10 years ago
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

10 years ago
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.
(Assignee)

Comment 10

10 years ago
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.
(Assignee)

Comment 11

10 years ago
Created attachment 311313 [details] [diff] [review]
Alternative patch that catches more: don't use S_FALSE, and if no error always return a string even if it is empty.
Attachment #310729 - Attachment is obsolete: true
Attachment #311313 - Flags: review?(surkov.alexander)
(Assignee)

Comment 12

10 years ago
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.

Comment 13

10 years ago
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.
(Assignee)

Comment 14

10 years ago
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.
(Assignee)

Comment 15

10 years ago
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.
(Assignee)

Comment 16

10 years ago
Created attachment 311327 [details] [diff] [review]
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.
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)

Updated

10 years ago
Attachment #311327 - Flags: review?(surkov.alexander) → review+
(Reporter)

Comment 17

10 years ago
This will also fix the second part of bug 424097.
(Reporter)

Updated

10 years ago
Blocks: 424097

Comment 18

10 years ago
(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.

(Reporter)

Comment 19

10 years ago
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?
(Assignee)

Updated

10 years ago
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+
(Assignee)

Updated

10 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.