649604 - crash [@ nsAccessible::NativeState()]

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

This bug was filed from the Socorro interface and is 
report bp-e221982b-eede-4e8d-9ed1-48ab62110413 .
============================================================= 
This is another crash possibly regressed from bug 634218 which I currently can only reproduce in one specific textarea, but in that, I can repro it 100% of the time.

Comment 1

[Marco Zehe (:MarcoZ)]

This is a regression that was introduced in the code that went into Aurora.

Comment 2

[Trevor Saunders (:tbsaunde)]

So, looking at the stack mContent must be null which means the accessible is almost certainly defunct.  We got into NativeState() from nsHTMLTextFieldAccessible::GetValue()   .  So GetValue() is being called on a defunct accessible.  So the question is where should we check to see if the object is defunct.

Comment 3

[Trevor Saunders (:tbsaunde)]

Attachment: test patch

I suspect there are a couple other place we call NativeState() not using State() that we might not check if the  o object is defunct.  I'l try to check for these and have a better patch in a bit.  This patch should solve the reported issue though.

 We could change internal calls to use State(), but I'm not sure if there's a reason we didn't do that before.

Comment 4

[Marco Zehe (:MarcoZ)]

I can confirm that this try-server build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-dbee519b7b33/tryserver-win32/firefox-6.0a1.en-US.win32.zip
fixes the crash for me.

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 525656 [details] [diff] [review]
test patch

r=me since this definitely fixes the bug.

Comment 6

[Benjamin Smedberg]

Might it be simpler to back out the original patch for Aurora, since it apparently caused several crash regressions?

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #6)
> Might it be simpler to back out the original patch for Aurora, since it
> apparently caused several crash regressions?

I think generally yes, but in this case no.

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #3)
> Created attachment 525656 [details] [diff] [review]
> test patch
> 
> I suspect there are a couple other place we call NativeState() not using
> State() that we might not check if the  o object is defunct.  I'l try to check
> for these and have a better patch in a bit.

Trevor did you investigate this?

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

OK please ignore comment 7.

Given the spirit of our new development process, and a risk/reward analysis, I think we should resolve this and related fallout via backing out bug 634218 for FF5.

Aside: Trevor is aware and agrees via IRC

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 525656 [details] [diff] [review]
test patch

r=me, but why would you need so many reviews?

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to comment #8)
> (In reply to comment #3)
> > Created attachment 525656 [details] [diff] [review]
> > test patch
> > 
> > I suspect there are a couple other place we call NativeState() not using
> > State() that we might not check if the  o object is defunct.  I'l try to check
> > for these and have a better patch in a bit.
> 
> Trevor did you investigate this?

yes, so there are two other places that might have simlar issues the xul and html checkbox implementations of GetActionName() they currently use NativeState(), one fix for this would be to use State() instead of NativeState().  However I'm not sure if that would create a correctness issue where ARIA is used where it shouldn't be or if it fixes one using it where it previously wasn't but should have been.

Comment 12

[alexander :surkov (:asurkov)]

I'd say it's worth to discuss these in other bug since neither of these can lead to crash.

Comment 13

[alexander :surkov (:asurkov)]

landed on m-c - http://hg.mozilla.org/mozilla-central/rev/987aaff80567

Comment 14

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110425 Firefox/6.0a1

Comment 15

[Johnny Stenback (:jst)]

This is needed on aurora (or beta) 5, is the landed patch appropriate for Firefox 5 as well? If so, please request approval for the patch.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to comment #15)
> This is needed on aurora (or beta) 5, is the landed patch appropriate for
> Firefox 5 as well? If so, please request approval for the patch.

hu?  this seems really odd... We backed 634218 out on aurora so I'm not really sure what's going on here, can I see a stack or something?

If its actually this issue I could probably come up with a null check and assert patch quickly, but because of the backout of 634218 I doubt any current patch would be suitable for aurora.

thanks!
HU? THAT SOUNDS REALLY ODD SINCE

Comment 17

[Marco Zehe (:MarcoZ)]

Right, JST, this was a regression from bug 634218, and that was backed out from Aurora shortly after the initial merge, along with a few followups that had already landed to fix crashes introduced by that bug.

Comment 18

[Benjamin Smedberg]

Just mark it fixed and move on, then ;-)