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

RESOLVED FIXED in mozilla1.9

Status

()

defect
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

({access, crash})

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

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment)

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

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

Comment 4

11 years ago
Posted patch patchSplinter Review
Attachment #316545 - Flags: review?(aaronleventhal)

Comment 5

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

Updated

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

Comment 6

11 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

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

Comment 7

11 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

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

Comment 8

11 years ago
Ok, thank you for looking at it anyway.
Possible to test?  re-request approval once addressed.
Attachment #316545 - Flags: approval1.9? → approval1.9-
(Assignee)

Comment 10

11 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

11 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

11 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: 11 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.