Last Comment Bug 649604 - crash [@ nsAccessible::NativeState()]
: crash [@ nsAccessible::NativeState()]
Status: VERIFIED FIXED
: crash, regression
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: 5 Branch
: x86 Windows NT
: -- critical (vote)
: mozilla6
Assigned To: Trevor Saunders (:tbsaunde)
:
: alexander :surkov
Mentors:
Depends on:
Blocks: 634218
  Show dependency treegraph
 
Reported: 2011-04-13 03:07 PDT by Marco Zehe (:MarcoZ)
Modified: 2011-06-09 14:58 PDT (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
test patch (593 bytes, patch)
2011-04-13 04:12 PDT, Trevor Saunders (:tbsaunde)
mzehe: review+
surkov.alexander: review+
Details | Diff | Splinter Review

Description Marco Zehe (:MarcoZ) 2011-04-13 03:07:27 PDT
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) 2011-04-13 03:11:08 PDT
This is a regression that was introduced in the code that went into Aurora.
Comment 2 Trevor Saunders (:tbsaunde) 2011-04-13 04:07:28 PDT
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) 2011-04-13 04:12:37 PDT
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.  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) 2011-04-13 07:19:57 PDT
I can confirm that this try-server build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/trev.saunders@gmail.com-dbee519b7b33/tryserver-win32/firefox-6.0a1.en-US.win32.zip
fixes the crash for me.
Comment 5 Marco Zehe (:MarcoZ) 2011-04-13 07:48:55 PDT
Comment on attachment 525656 [details] [diff] [review]
test patch

r=me since this definitely fixes the bug.
Comment 6 Benjamin Smedberg [:bsmedberg] 2011-04-13 12:20:42 PDT
Might it be simpler to back out the original patch for Aurora, since it apparently caused several crash regressions?
Comment 7 David Bolter [:davidb] 2011-04-13 12:54:31 PDT
(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] 2011-04-13 12:55:44 PDT
(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] 2011-04-13 13:19:51 PDT
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 2011-04-13 19:17:01 PDT
Comment on attachment 525656 [details] [diff] [review]
test patch

r=me, but why would you need so many reviews?
Comment 11 Trevor Saunders (:tbsaunde) 2011-04-13 22:31:04 PDT
(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 2011-04-13 23:01:36 PDT
I'd say it's worth to discuss these in other bug since neither of these can lead to crash.
Comment 13 alexander :surkov 2011-04-14 04:54:23 PDT
landed on m-c - http://hg.mozilla.org/mozilla-central/rev/987aaff80567
Comment 14 Marco Zehe (:MarcoZ) 2011-04-26 04:42:48 PDT
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0a1) Gecko/20110425 Firefox/6.0a1
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2011-05-17 14:42:48 PDT
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) 2011-05-17 14:51:20 PDT
(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) 2011-05-17 22:34:56 PDT
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 [:bsmedberg] 2011-05-18 06:20:25 PDT
Just mark it fixed and move on, then ;-)

Note You need to log in before you can comment on or make changes to this bug.