900835 - crash in nsEventStateManager::PreHandleEvent(nsPresContext*, nsEvent*, nsIFrame*, nsEventStatus*)

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

I've had quite a few of these crashes while programmatically clicking accessibles in the recent/saved searches area of twitter.com.

Steps to reproduce:
1. Ensure you have at least one recent or saved search on twitter.com.
2. Open twitter.com.
3. Press / to search.
4. Due to a bug, you'll need to press shift+tab and then tab to make recent/saved searches appear.
To reproduce generally:
5. Retrieve the accessible for a link to one of the searches.
6. Programmatically click the accessible.
To reproduce with NVDA:
5. Use object navigation to move to one of the search links.
6. Press NVDA+numpadEnter (laptop layout: NVDA+enter) to activate it.

bp-b53251e3-1b29-4f9e-a3d6-6ac082130801 .
bp-37e53cb1-6eae-4687-9f69-175212130802

Could this be a regression from the first patch of bug 894485? I doubt it, but thought it worth mentioning. Also, this may not be strictly a11y related, so my apologies if I've filed this in the wrong place.

Comment 1

[alexander :surkov (:asurkov)]

Olli, ideas? It looks like mPresContext->EventStateManager() returns null

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Hmm, why you think mPresContext->EventStateManager() returns null?

However, EventStateManager() is wrongly named. It may return null after unlink. But we're
not very consistent with naming stuff which may return null after unlink.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #2)
> Hmm, why you think mPresContext->EventStateManager() returns null?

probably I misread it but it looked like manager must be a null:
http://hg.mozilla.org/mozilla-central/annotate/05d3797276d3/layout/base/nsPresShell.cpp#l6810

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Yeah, possibly. Though the crash address is somewhat odd then. I'd expect something closer
to 0x0 crash address (some offset from null).

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #2)

> However, EventStateManager() is wrongly named. It may return null after
> unlink. But we're
> not very consistent with naming stuff which may return null after unlink.

Btw, Accessible::DoCommand starts a runnable so I guess when runnable triggers the unlink may happen

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch

patch per irc chat with Olli

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 785007 [details] [diff] [review]
patch

It is not clear to me whether we want all those events if frame dies, but that
issue is not about this bug.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #7)
> Comment on attachment 785007 [details] [diff] [review]
> patch
> 
> It is not clear to me whether we want all those events if frame dies, but
> that
> issue is not about this bug.

presumably they wont' be dispatched so result should be the same in the end, and we shouldn't run into this case often

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 785007 [details] [diff] [review]
patch

>+  nsCOMPtr<nsPresContext> presContext = presShell->GetPresContext();

shouldn't that be nsRefPtr?

>-  nsPresContext* presContext = presShell->GetPresContext();
>+  nsCOMPtr<nsPresContext> presContext = presShell->GetPresContext();

same, its not entirely clear to me we care if its alive after events, but might as well be safe

Comment 10

[alexander :surkov (:asurkov)]

ok, no smart pointers for prescontext

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

You should keep prescontext alive, so smart pointer please.
But yeah, refptr is technically correct with it.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #11)
> You should keep prescontext alive, so smart pointer please.
> But yeah, refptr is technically correct with it.

ok then :)

Comment 13

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/4f36a65d85e7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f36a65d85e7

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 785007 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not sure but the bug was introduced in Firefox 25 frame
User impact if declined: crashes (ping smaug for more info)
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): not risky
String or IDL/UUID changes made by this patch: no

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 785007 [details] [diff] [review]
patch

Very early in the Aurora cycle, might as well fix.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b62f2b0fdb9d

Comment 18

[Paul Silaghi, QA [:pauly]]

(In reply to James Teh [:Jamie] from comment #0)
> 5. Retrieve the accessible for a link to one of the searches.
> 6. Programmatically click the accessible.
I don't really understand these steps. Can you please explain a bit ?
I'm trying to reproduce the problem first, and then to verify it on fixed branches.

Comment 19

[James Teh [:Jamie]]

Verified fixed in Firefox 26.0a1 (2013-09-12).

I don't know how to test this without use of NVDA (a screen reader). I guess you could do it with a straight Python shell and NVDA's oleacc bindings, but the steps to find the required accessible would be ridiculously complicated. I can try to explain further how i tested it with NVDA, but you'd probably need to spend some time learning how to use NVDA to test it effectively.

Alex, is there a way to test this within Firefox (perhaps via the Web Console) that doesn't require an external AT?

Comment 20

[u279076]

(In reply to James Teh [:Jamie] from comment #19)
> Verified fixed in Firefox 26.0a1 (2013-09-12).

Thanks for your help James. Could you please also verify this is fixed with the latest Firefox 25 Beta:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/25.0b2-candidates/build1/

Comment 21

[James Teh [:Jamie]]

Verified fixed in Firefox 25b2 candidate build 1.

It's worth noting that I've just realised Twitter have changed their interface a bit, so it's possible this isn't triggering exactly the same code path any more. However, it hasn't changed *that* drastically, so it's very likely it does.

Comment 22

[u279076]

Thanks again, James.