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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

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?

Comment 1

[Damon Sicore (:damons)]

Not enough info to block.  Would take a patch.

Comment 2

[alexander :surkov (:asurkov)]

Marco, let's put an assertion and not call CreateTextChangeEventForNode though it's suspicious GetAccessibleInParentChain return nsnull.

Comment 3

[alexander :surkov (:asurkov)]

I think it makes sense to follow to http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsDocAccessible.cpp&rev=1.238&mark=1864#1864

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 5

[Aaron Leventhal]

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

Comment 6

[alexander :surkov (:asurkov)]

(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

Comment 7

[alexander :surkov (:asurkov)]

(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.

Comment 8

[Aaron Leventhal]

Ok, thank you for looking at it anyway.

Comment 9

[Damon Sicore (:damons)]

Possible to test?  re-request approval once addressed.

Comment 10

[alexander :surkov (:asurkov)]

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

Comment 11

[Damon Sicore (:damons)]

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.

Comment 12

[alexander :surkov (:asurkov)]

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

Comment 13

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

Comment on attachment 316545 [details] [diff] [review]
patch

a1.9=beltzner

Comment 14

[alexander :surkov (:asurkov)]

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