716589 - crash nsDocAccessible::GetNativeWindow

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

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

Attachment: trivial fix

E.g.
report bp-e1a04182-b727-43d4-80cd-f4b8b2120107

Comment 1

[Trevor Saunders (:tbsaunde)]

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)]

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] (NeedInfo me for attention)]

(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] (NeedInfo me for attention)]

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 5

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/1ed2eb54e546

Comment 6

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

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

https://hg.mozilla.org/mozilla-central/rev/1ed2eb54e546