Last Comment Bug 716589 - crash nsDocAccessible::GetNativeWindow
: crash nsDocAccessible::GetNativeWindow
Status: RESOLVED FIXED
: crash
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: x86 Windows 7
: -- critical (vote)
: mozilla12
Assigned To: David Bolter [:davidb]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-09 10:09 PST by David Bolter [:davidb]
Modified: 2012-01-10 18:49 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
trivial fix (1.10 KB, patch)
2012-01-09 10:09 PST, David Bolter [:davidb]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description David Bolter [:davidb] 2012-01-09 10:09:07 PST
Created attachment 587030 [details] [diff] [review]
trivial fix

E.g.
report bp-e1a04182-b727-43d4-80cd-f4b8b2120107
Comment 1 Trevor Saunders (:tbsaunde) 2012-01-10 06:26:45 PST
Comment on attachment 587030 [details] [diff] [review]
trivial fix

r=me, but please change to an early bail out instead of adding another if wrapping the whole thing.
Comment 2 Trevor Saunders (:tbsaunde) 2012-01-10 06:30:46 PST
Do you know why the accessible itself has a pres shell, but the doc doesn't as seen from the NS_ENSURE_TRUE in FireEvent()?  I'm sort of worried there might be something deeper here.

it occurs to me the jaws hack checks the role of the target without making sure it isn't defunct, but the odds that will actually cause a crash seems pretty low since you'd have to focus the event and then shut it down in the same  refresh driver cycle.

also, the #ifdef DEBUG should cover a little more in that function than it already does for future cleanup.
Comment 3 David Bolter [:davidb] 2012-01-10 07:37:45 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Comment on attachment 587030 [details] [diff] [review]
> trivial fix
> 
> r=me, but please change to an early bail out instead of adding another if
> wrapping the whole thing.

We have examples of both. I looked before I did it this way. I usually do bail outs if the method is longer. /me shrugs.
Comment 4 David Bolter [:davidb] 2012-01-10 07:39:19 PST
BTW, thanks for the review and I can make that change.

I'm not sure what you mean about the jaws hack in comment 2 (yet).
Comment 6 David Bolter [:davidb] 2012-01-10 12:31:31 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Do you know why the accessible itself has a pres shell, but the doc doesn't
> as seen from the NS_ENSURE_TRUE in FireEvent()?  I'm sort of worried there
> might be something deeper here.
> 
> it occurs to me the jaws hack checks the role of the target without making
> sure it isn't defunct, but the odds that will actually cause a crash seems
> pretty low since you'd have to focus the event and then shut it down in the
> same  refresh driver cycle.
> 
> also, the #ifdef DEBUG should cover a little more in that function than it
> already does for future cleanup.

Please file spin-off bugs if you think we should take action on these concerns.
Comment 7 Ed Morley [:emorley] 2012-01-10 18:49:38 PST
https://hg.mozilla.org/mozilla-central/rev/1ed2eb54e546

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