547692 - Crash [@ nsCoreUtils::DispatchMouseEvent(unsigned int, nsIPresShell*, nsIContent*) ]

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Reproducible crash for me when doing the following with Firefox 3.7a2pre and NVDA running:
1. Receiving a nudge from some friend on Facebook.
2. Going to the Facebook main page where these nudges show up.
3. Pressing SPACE (clicking) "nudge back".
4. Pressing SPACE (clicking) "send nudge".
5. Pressing SPACE (clicking) on "OK".

Not happening in Firefox 3.6.

Comment 1

[Marco Zehe (:MarcoZ)]

The actual crash happens in a line that was added/changed in bug 500882. Therefore CC'ing Boris.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Which line?  Can you point me to a crash incident?

Comment 3

[Marco Zehe (:MarcoZ)]

Sorry 'bout that! Not my charming self today! :) Here are two reports I submitted:
bp-49acefdc-5e7a-4618-bb0f-6e71d2100222
bp-617ea70d-c0d1-4ec3-acd5-993ae2100221

Comment 4

[Boris Zbarsky [:bzbarsky]]

Perfect, thanks.  The line is:

  nsIFrame *frame = aContent->GetPrimaryFrame();

at the top of nsCoreUtils::DispatchMouseEvent.  The crash address is 0x14, which means aContent is null.

Should this function just have a null-check up front, or does this indicate a deeper problem somewhere?  Presumably someone passed a null aContent to nsAccessible::DoCommand _and_ mDOMNode was not an nsIContent (was the document or something?), right?

Comment 5

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

I think we used to use a timer with an nsCommandClosure which probably held onto content (assuming a true 'closure')? ... and now we don't, see: http://hg.mozilla.org/mozilla-central/rev/b8a1a64e36e0

A null check up front does make some sense.

Comment 6

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

Oh could be the document I suppose... hmmm.

Comment 7

[Boris Zbarsky [:bzbarsky]]

The code cited in comment 5 seems to stick the nsIContent into an nsCOMPtr in the event (it better do that if it plans to point to it at all, so that's good!).

Comment 8

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

Yes.

I think maybe we want to add a DoAction to nsDocAccessible that does nothing (or just maybe calls TakeFocus()).

Comment 9

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

Attachment: an idea

Untested (may try later, but need to update my windows tree and today is heavy on meetings)

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> Created an attachment (id=428243) [details]
> an idea
> 
> Untested (may try later, but need to update my windows tree and today is heavy
> on meetings)

nope. the document inherits an action from either root element or body. The right fix should be a fix in DoCommand() to call GetRoleContent(mDOMNode) instead of do_QueryInterface(mDOMNode)

Comment 11

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

Yeah, my way would ignore legitimate actions on documents (probably rare but just as well to cover those cases).

Comment 12

[Boris Zbarsky [:bzbarsky]]

Oh, as a note.... the old code before bug 500882 effectively had a null-check of aContent and an early return of PR_FALSE if aContent is null in nsCoreUtils::DispatchMouseEvent.  It was just not obvious that it did that.

Comment 13

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

Attachment: getrolecontent approach

Trivial patch; let's see if it works. Marco can you apply this locally? This approach is better because we will support body/root element actions (e.g. for iframe based ARIA widgets).

Comment 14

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

(In reply to comment #12)
> Oh, as a note.... the old code before bug 500882 effectively had a null-check
> of aContent and an early return of PR_FALSE if aContent is null in
> nsCoreUtils::DispatchMouseEvent.  It was just not obvious that it did that.

Oh I missed that. Good to know thanks.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 428249 [details] [diff] [review]
getrolecontent approach

r=me event if it doesn't fix the Marco's case.

Comment 16

[Marco Zehe (:MarcoZ)]

This patch fixes the problem!

Comment 17

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

Thanks Marco.

Note I filed bug 547895 while writing a test for this bug.

Comment 18

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

Landed: http://hg.mozilla.org/mozilla-central/rev/f3687d491693

Thanks guys.

Comment 19

[Marco Zehe (:MarcoZ)]

Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.3a2pre) Gecko/20100224 Minefield/3.7a2pre (.NET CLR 3.5.30729)