Crash Crash [@ nsDocAccessible::CreateTextChangeEventForNode(nsIAccessible*, nsIDOMNode*, nsIAccessible*, int, int) ]

RESOLVED FIXED in mozilla1.9

Status

()

Core
Disability Access APIs
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

({access, crash})

Trunk
mozilla1.9
x86
Linux
access, crash
Points:
---
Bug Flags:
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
Found while triaging 3.0b5 crash reports. One crash report:
bp-08c7e926-0b1d-11dd-82e4-0013211cbf8a
The line indicated is:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsDocAccessible.cpp&rev=1.238&mark=1387#1387
Looks like *aContainerAccessible is being used without being checked for not NSNULL first. May there be situations where aContainerAccessible may be NULL? Should there be a check here?
Flags: blocking1.9?
Not enough info to block.  Would take a patch.
Flags: blocking1.9? → blocking1.9-
(Assignee)

Comment 2

10 years ago
Marco, let's put an assertion and not call CreateTextChangeEventForNode though it's suspicious GetAccessibleInParentChain return nsnull.
(Assignee)

Comment 4

10 years ago
Created attachment 316545 [details] [diff] [review]
patch
Attachment #316545 - Flags: review?(aaronleventhal)

Comment 5

10 years ago
Should we put that logic into GetAccessibleInParentChain() ... basically make sure it never returns null and returns the doc accessible if necessary?

Updated

10 years ago
Attachment #316545 - Flags: review?(aaronleventhal) → review+
(Assignee)

Comment 6

10 years ago
(In reply to comment #5)
> Should we put that logic into GetAccessibleInParentChain() ... basically make
> sure it never returns null and returns the doc accessible if necessary?
> 

good idea, I'll put new patch for this
(Assignee)

Updated

10 years ago
Assignee: ginn.chen → surkov.alexander
(Assignee)

Comment 7

10 years ago
(In reply to comment #6)
> (In reply to comment #5)
> > Should we put that logic into GetAccessibleInParentChain() ... basically make
> > sure it never returns null and returns the doc accessible if necessary?
> > 
> 
> good idea, I'll put new patch for this
> 

Aaron, GetAccessibleInParentChain() is used in many places and nsnull result is processed in different ways. I wouldn't risk to change this in the meantime. So if you fine with this then let's save the current approach.
(Assignee)

Updated

10 years ago
Attachment #316545 - Flags: approval1.9?

Comment 8

10 years ago
Ok, thank you for looking at it anyway.
Possible to test?  re-request approval once addressed.

Updated

10 years ago
Attachment #316545 - Flags: approval1.9? → approval1.9-
(Assignee)

Comment 10

10 years ago
Comment on attachment 316545 [details] [diff] [review]
patch

(In reply to comment #9)
> Possible to test?  re-request approval once addressed.
> 

We haven't a way to reproduce the bug. We just have stack trace. I tried to do small changes I can. If the test is major condition for the current status of the tree then let's wait for 1.9.x to land this trying of the the crash fix.

re-requesting approval
Attachment #316545 - Flags: approval1.9- → approval1.9?
Comment on attachment 316545 [details] [diff] [review]
patch

What's the regression risk?  Sorry to keep asking this, but we need to be extra extra careful right now.  Re-nom when answered.
Attachment #316545 - Flags: approval1.9? → approval1.9-
(Assignee)

Comment 12

10 years ago
Comment on attachment 316545 [details] [diff] [review]
patch

(In reply to comment #11)
> (From update of attachment 316545 [details] [diff] [review])
> What's the regression risk?  Sorry to keep asking this, but we need to be extra
> extra careful right now.  Re-nom when answered.
> 

Since we get a notification from layout module and we don't get which element has been changed then we must invalidate the document entirely. Here we do this by sending corresponding events to accessible for document node. I think the risk is minimal, it doesn't bring major changes.

re-requesting approval
Attachment #316545 - Flags: approval1.9- → approval1.9?
Comment on attachment 316545 [details] [diff] [review]
patch

a1.9=beltzner
Attachment #316545 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 14

10 years ago
/cvsroot/mozilla/accessible/src/base/nsDocAccessible.cpp,v  <--  nsDocAccessible.cpp
new revision: 1.241; previous revision: 1.240
done

Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsDocAccessible::CreateTextChangeEventForNode(nsIAccessible*, nsIDOMNode*, nsIAccessible*, int, int) ]
You need to log in before you can comment on or make changes to this bug.